Repository navigation
Fix duplicate public IP allocated to system VMs at zone startup - #14348
nagaboinaramgopal wants to merge 3 commits into
Conversation
assignIpAddressWithLock guarded the Free to Allocating transition with an op_lock application lock plus a plain, non-locking read, and released that lock before the enclosing transaction committed. Two system VM scanner threads (console proxy and secondary storage) starting at zone bring-up could both read the same pool row as Free and both allocate it, leaving two nics with the same public IP and a VM console that never connects. Re-read the candidate row with a FOR UPDATE row lock held for the whole allocation transaction and re-check the state under it. A second thread now blocks until the first commits and then sees the row is no longer Free, so it backs off instead of allocating the same address. This is the same row lock the following markPublicIpAsAllocated step already relies on.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14348 +/- ##
============================================
- Coverage 19.91% 19.90% -0.01%
+ Complexity 20200 20197 -3
============================================
Files 6373 6373
Lines 577230 577230
Branches 70696 70696
============================================
- Hits 114958 114926 -32
- Misses 449703 449740 +37
+ Partials 12569 12564 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@Damans227 Could you pls take a look |
| logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid()); | ||
| if (userIp.getState() != State.Free) { | ||
| logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState()); | ||
| return null; |
There was a problem hiding this comment.
the list passed in here only has one ip, so does the losing system vm now fail with a no free ip error even when other ips are free? could we try the next free ip instead?
There was a problem hiding this comment.
Fixed in 85aba61. assignAndAllocateIpAddressEntry now returns null instead of failing when its candidate was taken concurrently, and fetchNewPublicIp re-selects a different free IP and retries (bounded to a few attempts). listAvailablePublicIps filters on the Free state, so the retry skips the address the winner already moved to Allocating and picks another; the no-free-IP error is now only raised once the pool is genuinely exhausted.
assignAndAllocateIpAddressEntry failed by throwing when its single candidate was locked and found no longer Free (taken by a concurrent allocation), so the losing system VM reported no free IP even when other addresses were free. Return null in that case and have fetchNewPublicIp re-select a different free IP and retry, bounded to a few attempts. listAvailablePublicIps filters on the Free state, so the retry skips the address the winner already moved to Allocating and picks another; InsufficientAddressCapacityException is thrown only once the attempts are exhausted.
| // and re-checks it is still Free, so the loser gets back null; re-select a different free IP and retry | ||
| // rather than failing with no-free-IP while free addresses still exist. | ||
| IPAddressVO addr = null; | ||
| for (int attempt = 1; attempt <= MAX_PUBLIC_IP_ALLOCATION_ATTEMPTS; attempt++) { |
There was a problem hiding this comment.
can we add a test where the first pick is taken and the second try gets another ip? the new tests only cover the lock part
There was a problem hiding this comment.
Added in 43384a5: testFetchNewPublicIpRetriesWhenTheFirstPickIsTakenConcurrently drives the first candidate to be taken (assignAndAllocateIpAddressEntry returns null) and asserts fetchNewPublicIp re-selects and allocates a different free IP instead of failing, verifying it attempts allocation twice. 4 tests pass in IpAddressManagerImplTest.
|
thanks @nagaboinaramgopal, fix looks right. LGTM. |
Add a unit test proving fetchNewPublicIp re-selects a different free IP when the first candidate is taken by a concurrent allocation, instead of failing with no-free-IP. Extract the final PublicIp build into buildPublicIp and make assignAndAllocateIpAddressEntry package-visible so the retry loop can be exercised without a live database.
This branch has not been deployed
2 participants
Description
At zone bring-up the console proxy and secondary storage VMs are brought up by two independent
capacity scanner threads. Both resolve their public NIC through the same path
(PublicNetworkGuru.getIp with forSystemVms, then IpAddressManagerImpl.fetchNewPublicIp), and on
a fresh zone they can hit it within a few hundred milliseconds of each other.
assignIpAddressWithLock guarded the Free to Allocating transition with an op_lock application
lock and a plain, non-locking read, and released that lock before the surrounding transaction
committed. In that window the second thread took the lock, still read the row as Free from its
own snapshot, and allocated the same pool row. The result was two nics on the public network
with the same address, an ARP conflict, and VM consoles stuck on "Connecting to VM console"
because the console proxy and the SSVM were fighting over one IP.
The allocation UPDATEs an existing pool row rather than inserting, so the unique key on
user_ip_address never fires, and nics has no unique index on (network_id, ip4_address), so
nothing at the DB layer refused it either.
The fix re-reads the candidate row under a FOR UPDATE row lock held for the whole allocation
transaction and re-checks the state under that lock
so a second thread blocks until the first commits, then sees the row is no longer Free and backs
off. This is the same row lock the markPublicIpAsAllocated step right after it already relies on.
#9234 locked the user facing public IP APIs but not this system VM allocation path.
On a lost race the losing scanner gets no free address for that attempt and retries on its next
scan cycle rather than allocating a duplicate, so it is self healing with no duplicate and no
manual recovery.
Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
How Has This Been Tested?
Added IpAddressManagerImplTest for assignIpAddressWithLock: it allocates when the locked read is
Free, backs off and does not update when the locked read comes back Allocating (the losing
thread), and returns null when the row is gone. The tests assert the read goes through
lockRow(id, true), the FOR UPDATE read the fix adds, so they fail before the change (the method
used acquireInLockTable with a plain read) and pass after. server module build and the new tests
are green.
How did you try to break this feature and the system with this change?
assignIpAddressWithLock has a single caller and always runs inside the assignAndAllocateIpAddressEntry
transaction, so the FOR UPDATE lock is held to commit rather than per statement. One row is locked
per transaction, so there is no lock ordering deadlock. The sub-second allocation race is timing
dependent and not deterministically reproducible, so the guarantee rests on the FOR UPDATE row lock
being held to commit, the same mechanism the existing markPublicIpAsAllocated step uses.