Repository navigation
Fix duplicate public IP allocated to system VMs at zone startup - #14348
Open
nagaboinaramgopal wants to merge 1 commit into
Open
nagaboinaramgopal wants to merge 1 commit into
nagaboinaramgopal wants to merge 1 commit 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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.