You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "nagaboinaramgopal (via GitHub)" <gi...@apache.org> on 2026/10/07 21:02:46 UTC
[PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4223206729
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -420,22 +420,21 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final V
}
private IPAddressVO assignIpAddressWithLock(IPAddressVO possibleAddr) {
- IPAddressVO finalAddress = null;
- IPAddressVO userIp = _ipAddressDao.acquireInLockTable(possibleAddr.getId());
- if (userIp != null) {
- logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid());
- if (userIp.getState() == State.Free) {
- possibleAddr.setState(State.Allocating);
- if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) {
- logger.info("successfully allocated ip address {}", possibleAddr.getAddress());
- finalAddress = possibleAddr;
- }
- } else {
- logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState());
- }
- _ipAddressDao.releaseFromLockTable(possibleAddr.getId());
+ IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true);
+ if (userIp == null) {
+ return null;
+ }
+ 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;
Review 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?
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "nagaboinaramgopal (via GitHub)" <gi...@apache.org>.
nagaboinaramgopal commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4224213123
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -950,11 +955,30 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<L
public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<Long> vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId,
final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final String requestedGateway, final boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean forSystemVms)
throws InsufficientAddressCapacityException {
- List<IPAddressVO> addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true);
- IPAddressVO addr = addrs.get(0);
- if (assign) {
+ // Two allocations (for example the console proxy and secondary storage system VMs started at the same
+ // time) can be handed the same free address because listAvailablePublicIps selects a candidate in a
+ // separate transaction from the one that marks it Allocating. assignIpAddressWithLock now locks the row
+ // 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++) {
Review Comment:
Added in 43384a56a6: 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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4223813292
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -420,22 +420,21 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final V
}
private IPAddressVO assignIpAddressWithLock(IPAddressVO possibleAddr) {
- IPAddressVO finalAddress = null;
- IPAddressVO userIp = _ipAddressDao.acquireInLockTable(possibleAddr.getId());
- if (userIp != null) {
- logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid());
- if (userIp.getState() == State.Free) {
- possibleAddr.setState(State.Allocating);
- if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) {
- logger.info("successfully allocated ip address {}", possibleAddr.getAddress());
- finalAddress = possibleAddr;
- }
- } else {
- logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState());
- }
- _ipAddressDao.releaseFromLockTable(possibleAddr.getId());
+ IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true);
+ if (userIp == null) {
+ return null;
+ }
+ 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;
Review Comment:
that covers it, thanks
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -950,11 +955,30 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<L
public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<Long> vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId,
final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final String requestedGateway, final boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean forSystemVms)
throws InsufficientAddressCapacityException {
- List<IPAddressVO> addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true);
- IPAddressVO addr = addrs.get(0);
- if (assign) {
+ // Two allocations (for example the console proxy and secondary storage system VMs started at the same
+ // time) can be handed the same free address because listAvailablePublicIps selects a candidate in a
+ // separate transaction from the one that marks it Allocating. assignIpAddressWithLock now locks the row
+ // 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++) {
Review 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
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4226508758
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -950,11 +955,30 @@ public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<L
public PublicIp fetchNewPublicIp(final long dcId, final Long podId, final List<Long> vlanDbIds, final Account owner, final VlanType vlanUse, final Long guestNetworkId,
final boolean sourceNat, final boolean assign, final boolean allocate, final String requestedIp, final String requestedGateway, final boolean isSystem, final Long vpcId, final Boolean displayIp, final boolean forSystemVms)
throws InsufficientAddressCapacityException {
- List<IPAddressVO> addrs = listAvailablePublicIps(dcId, podId, vlanDbIds, owner, vlanUse, guestNetworkId, sourceNat, assign, allocate, requestedIp, requestedGateway, isSystem, vpcId, displayIp, forSystemVms, true);
- IPAddressVO addr = addrs.get(0);
- if (assign) {
+ // Two allocations (for example the console proxy and secondary storage system VMs started at the same
+ // time) can be handed the same free address because listAvailablePublicIps selects a candidate in a
+ // separate transaction from the one that marks it Allocating. assignIpAddressWithLock now locks the row
+ // 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++) {
Review Comment:
that covers it, thanks
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "nagaboinaramgopal (via GitHub)" <gi...@apache.org>.
nagaboinaramgopal commented on code in PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#discussion_r4223263653
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -420,22 +420,21 @@ private IPAddressVO assignAndAllocateIpAddressEntry(final Account owner, final V
}
private IPAddressVO assignIpAddressWithLock(IPAddressVO possibleAddr) {
- IPAddressVO finalAddress = null;
- IPAddressVO userIp = _ipAddressDao.acquireInLockTable(possibleAddr.getId());
- if (userIp != null) {
- logger.debug("locked row for ip address {} (id: {})", possibleAddr.getAddress(), possibleAddr.getUuid());
- if (userIp.getState() == State.Free) {
- possibleAddr.setState(State.Allocating);
- if (_ipAddressDao.update(possibleAddr.getId(), possibleAddr)) {
- logger.info("successfully allocated ip address {}", possibleAddr.getAddress());
- finalAddress = possibleAddr;
- }
- } else {
- logger.debug("locked ip address {} is not free {}", possibleAddr.getAddress(), userIp.getState());
- }
- _ipAddressDao.releaseFromLockTable(possibleAddr.getId());
+ IPAddressVO userIp = _ipAddressDao.lockRow(possibleAddr.getId(), true);
+ if (userIp == null) {
+ return null;
+ }
+ 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;
Review Comment:
Fixed in 85aba61c24. 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.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "Damans227 (via GitHub)" <gi...@apache.org>.
Damans227 commented on PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#issuecomment-6068469842
thanks @nagaboinaramgopal, fix looks right. LGTM.
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#issuecomment-6064302658
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/14348?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 3.71%. Comparing base ([`ed1db53`](https://app.codecov.io/gh/apache/cloudstack/commit/ed1db53815cf243eb87c10d2f135e1f10895f282?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`f3b4181`](https://app.codecov.io/gh/apache/cloudstack/commit/f3b418105d5fb5a4291d7d5d7a5f0e3b118cfab3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on main.
> :exclamation: There is a different number of reports uploaded between BASE (ed1db53) and HEAD (f3b4181). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (ed1db53) | HEAD (f3b4181) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## main #14348 +/- ##
=============================================
- Coverage 19.91% 3.71% -16.21%
=============================================
Files 6373 487 -5886
Lines 577230 41992 -535238
Branches 70696 7942 -62754
=============================================
- Hits 114958 1558 -113400
+ Misses 449703 40208 -409495
+ Partials 12569 226 -12343
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/14348/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/14348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `3.71% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/14348/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Harness](https://app.codecov.io/gh/apache/cloudstack/pull/14348?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix duplicate public IP allocated to system VMs at zone startup [cloudstack]
Posted by "nagaboinaramgopal (via GitHub)" <gi...@apache.org>.
nagaboinaramgopal commented on PR #14348:
URL: https://github.com/apache/cloudstack/pull/14348#issuecomment-6066861548
@Damans227 Could you pls take a look
--
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: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org