nagaboinaramgopal opened a new pull request, #14348:
URL: https://github.com/apache/cloudstack/pull/14348
### 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
IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true);
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
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] Bug fix (non-breaking change which fixes an issue)
- [ ] Enhancement (improves an existing feature and functionality)
- [ ] Cleanup (Code refactoring and cleanup, that may add test cases)
- [ ] Build/CI
- [ ] Test (unit or integration test code)
### Feature/Enhancement Scale or Bug Severity
#### Bug Severity
- [x] Major
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
