retry NIC IP allocation instead of NPE when the allocating by rootnomad · Pull Request #14038 · apache/cloudstack

rootnomad · 2026-09-02T14:46:52Z

Description

checkForRaceAndAllocateNic dereferenced the requested NicProfile when a
concurrent deploy had already taken the IP (persistNicAfterRaceCheck returned
null). When the deploy passes no requested profile, requested is null, so the
losing thread threw a NullPointerException instead of nulling the IP and
retrying.

The null-requested path is the internal deploys that allocate a NIC without a
profile: Basic zone reallocate, and the control and management NICs on routers
and system VMs. User VM deploys build a NicProfile even when no IP is asked, so
they already retried correctly.

Null-guard the requested profile so a lost race retries the allocation.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Feature/Enhancement Scale or Bug Severity

Bug Severity

Minor

How Has This Been Tested?

Added a unit test that simulates losing the allocation race with no explicit IP
requested and asserts the allocation is retried rather than throwing a
NullPointerException. Also built the standard packages and deployed on a KVM
advanced zone.

DaanHoogland

@nagaboinaramgopal can you see if this is applicable to older LTS branches?

rootnomad · 2026-09-02T23:13:43Z

Thanks @DaanHoogland . I removed the inline comment, the null-guard reads clearly on its own. And on the LTS question: the same deref of a possibly-null requested profile in checkForRaceAndAllocateNic is present on 4.19, 4.20 and 4.22, so this applies to all of them. Happy to open backport PRs once this merges, or to retarget to the oldest branch and let it merge forward, whichever you prefer.

DaanHoogland · 2026-09-03T07:55:06Z

Thanks @DaanHoogland . I removed the inline comment, the null-guard reads clearly on its own. And on the LTS question: the same deref of a possibly-null requested profile in checkForRaceAndAllocateNic is present on 4.19, 4.20 and 4.22, so this applies to all of them. Happy to open backport PRs once this merges, or to retarget to the oldest branch and let it merge forward, whichever you prefer.

We have the custom to put fixes on the oldest open LTS branch (until january that will be 4.20) and than merge it forwards to newer LTS branches and main. So rebasing this (and some of your other contribution) on 4.20 would be great, thanks.

rootnomad · 2026-09-03T15:14:26Z

Thanks @DaanHoogland . I removed the inline comment, the null-guard reads clearly on its own. And on the LTS question: the same deref of a possibly-null requested profile in checkForRaceAndAllocateNic is present on 4.19, 4.20 and 4.22, so this applies to all of them. Happy to open backport PRs once this merges, or to retarget to the oldest branch and let it merge forward, whichever you prefer.

We have the custom to put fixes on the oldest open LTS branch (until january that will be 4.20) and than merge it forwards to newer LTS branches and main. So rebasing this (and some of your other contribution) on 4.20 would be great, thanks.

Sounds good, thanks for explaining the workflow. I'll rebase this onto 4.20 and update the base, and do the same for my other bug-fix PRs so they merge forward. The two feature PRs (HA capacity reserve, event-driven DRS) I'll leave targeting main.

DaanHoogland · 2026-09-03T16:11:46Z

Thanks @DaanHoogland . I removed the inline comment, the null-guard reads clearly on its own. And on the LTS question: the same deref of a possibly-null requested profile in checkForRaceAndAllocateNic is present on 4.19, 4.20 and 4.22, so this applies to all of them. Happy to open backport PRs once this merges, or to retarget to the oldest branch and let it merge forward, whichever you prefer.

We have the custom to put fixes on the oldest open LTS branch (until january that will be 4.20) and than merge it forwards to newer LTS branches and main. So rebasing this (and some of your other contribution) on 4.20 would be great, thanks.

Sounds good, thanks for explaining the workflow. I'll rebase this onto 4.20 and update the base, and do the same for my other bug-fix PRs so they merge forward. The two feature PRs (HA capacity reserve, event-driven DRS) I'll leave targeting main.

thanks, and welcome to the community ;)

…ce is lost checkForRaceAndAllocateNic dereferenced the requested NicProfile when a concurrent deploy had already taken the IP (persistNicAfterRaceCheck returned null). On the common path the user requests no explicit IP, so requested is null and the loser threw a NullPointerException instead of nulling the IP and retrying. This defeats the ipv4AllocationRaceCheck retry for exactly the case it exists for (bulk/autoscale/CKS deploys onto one network). Null-guard the requested profile so the allocation is retried.

github-actions · 2026-09-03T17:34:37Z

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

DaanHoogland · 2026-09-03T18:51:36Z

@blueorangutan package

blueorangutan · 2026-09-03T18:52:05Z

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

blueorangutan · 2026-09-03T20:12:05Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19094

blueorangutan · 2026-09-07T08:49:20Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 19122

blueorangutan · 2026-09-07T11:03:13Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19127

Fold the requested-IPv4 throw and the address reset under a single requested != null guard instead of repeating the check on both branches.

codecov · 2026-09-08T07:33:43Z

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.35%. Comparing base (2cd8c5e) to head (75e8a0f).
⚠️ Report is 23 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...tack/engine/orchestration/NetworkOrchestrator.java	0.00%	4 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #14038   +/-   ##
=========================================
  Coverage     16.34%   16.35%           
- Complexity    13574    13579    +5     
=========================================
  Files          5669     5669           
  Lines        501368   501406   +38     
  Branches      60903    60908    +5     
=========================================
+ Hits          81964    81996   +32     
+ Misses       410219   410214    -5     
- Partials       9185     9196   +11

Flag	Coverage Δ
uitests	`4.14% <ø> (-0.01%)`	⬇️
unittests	`17.21% <0.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

blueorangutan · 2026-09-08T08:06:14Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✖️ debian ✔️ suse15. SL-JID 19145

DaanHoogland · 2026-09-08T10:04:21Z

@blueorangutan test

DaanHoogland

clgtm

DaanHoogland · 2026-09-08T10:05:24Z

generic hardening, I think smoke tests should be enough for this one, cc @sureshanaparti , can you do a quick review?

nvazquez · 2026-09-08T14:28:25Z

@blueorangutan package

blueorangutan · 2026-09-08T14:30:05Z

@nvazquez a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

nvazquez

Code LGTM

blueorangutan · 2026-09-08T15:32:29Z

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19152

nvazquez · 2026-09-08T16:04:37Z

@blueorangutan test

blueorangutan · 2026-09-08T16:06:05Z

@nvazquez a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

blueorangutan · 2026-09-09T08:17:00Z

[SF] Trillian test result (tid-16931)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 54600 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr14038-t16931-kvm-ol8.zip
Smoke tests completed. 140 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
test_02_list_cpvm_vm	`Failure`	0.05	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.05	test_ssvm.py

sudo87

lgtm

rootnomad · 2026-09-15T14:57:55Z

Thank you @DaanHoogland, @nvazquez and @sudo87 for the reviews, and for running the smoke tests. Please let me know if anything else is needed from my side for 4.20.4.

DaanHoogland · 2026-10-01T14:25:49Z

lgtm

@sudo87 , am i correct to understand, you did test this? (can you expand a little?)

sudo87 · 2026-10-05T04:59:57Z

@DaanHoogland my bad, should have used "clgtm". I didn't test it.

Damans227 · 2026-10-14T15:23:37Z

-                    throw new InsufficientVirtualNetworkCapacityException("Unable to acquire requested Guest IP address " + requested.getRequestedIPv4() + " for network " + network, DataCenter.class, dcVo.getId());
-                } else {
-                    requested.setIPv4Address(null);
+                if (requested != null) {


which deploy path sends no nic profile here? user vm deploys always build one even when no ip is asked, so i think its routers or system vms and not bulk or cks deploys

Good question, thanks @Damans227. The null requested comes from allocate(): when a network is handed in with an empty profile list, the loop adds a single null entry and calls allocateNic(null, ...). You're right that normal user-vm deploys build a NicProfile and don't reach it; the path that does is reallocate() in Basic zones and the other internal add-network flows that pass no profile. Before the fix, losing the allocation race there dereferenced the null requested and threw NPE; the guard makes it retry the same as the non-null case.

makes sense, so its the basic zone reallocate plus the control and management nics on routers and system vms. can we update the description too? it still says bulk deploys, autoscale and cks hit this

Good point, thanks. Updated the description to drop the bulk/autoscale/cks examples, since those are user VMs that build a profile. It now says the null-requested path is the internal deploys that pass no profile: basic-zone reallocate and the control and management NICs on routers and system VMs.

that covers it, thanks

boring-cyborg Bot added the component:orchestration label Sep 2, 2026

DaanHoogland reviewed Sep 2, 2026

View reviewed changes

Comment thread ...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java Outdated

rootnomad force-pushed the fix/nic-allocation-race-npe branch from 82d4eb1 to d0fc3c2 Compare September 2, 2026 23:10

DaanHoogland added this to CloudStack Testing Sep 3, 2026

github-project-automation Bot moved this to Backlog in CloudStack Testing Sep 3, 2026

DaanHoogland self-assigned this Sep 3, 2026

rootnomad force-pushed the fix/nic-allocation-race-npe branch from d0fc3c2 to 0035c61 Compare September 3, 2026 17:31

github-actions Bot added the status:has-conflicts label Sep 3, 2026

rootnomad changed the base branch from main to 4.20 September 3, 2026 17:54

github-actions Bot removed the status:has-conflicts label Sep 3, 2026

DaanHoogland added this to the 4.20.4 milestone Sep 4, 2026

DaanHoogland reviewed Sep 7, 2026

View reviewed changes

Comment thread ...hestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java Outdated

engine: nest the requested null-check in the NIC allocation retry

75e8a0f

Fold the requested-IPv4 throw and the address reset under a single requested != null guard instead of repeating the check on both branches.

DaanHoogland changed the title ~~engine: retry NIC IP allocation instead of NPE when the allocation ra…~~ retry NIC IP allocation instead of NPE when the allocation ra… Sep 8, 2026

DaanHoogland approved these changes Sep 8, 2026

View reviewed changes

DaanHoogland requested a review from sureshanaparti September 8, 2026 10:05

DaanHoogland moved this from Backlog to In progress in CloudStack Testing Sep 8, 2026

DaanHoogland changed the title ~~retry NIC IP allocation instead of NPE when the allocation ra…~~ retry NIC IP allocation instead of NPE when the allocating Sep 8, 2026

nvazquez approved these changes Sep 8, 2026

View reviewed changes

DaanHoogland assigned sudo87 and unassigned DaanHoogland Sep 10, 2026

sudo87 approved these changes Sep 11, 2026

View reviewed changes

sudo87 moved this from In progress to In review in CloudStack Testing Sep 11, 2026

rootnomad mentioned this pull request Sep 15, 2026

consoleproxy: fix race where the capacity scanner restarts a console proxy while it is being destroyed #14175

Open

2 of 14 tasks

DaanHoogland added the needs-testing label Oct 5, 2026

Damans227 reviewed Oct 14, 2026

View reviewed changes

Conversation

rootnomad commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

How Has This Been Tested?

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rootnomad commented Sep 2, 2026

Uh oh!

DaanHoogland commented Sep 3, 2026

Uh oh!

rootnomad commented Sep 3, 2026

Uh oh!

DaanHoogland commented Sep 3, 2026

Uh oh!

github-actions Bot commented Sep 3, 2026

Uh oh!

DaanHoogland commented Sep 3, 2026

Uh oh!

blueorangutan commented Sep 3, 2026

Uh oh!

blueorangutan commented Sep 3, 2026

Uh oh!

Uh oh!

blueorangutan commented Sep 7, 2026

Uh oh!

blueorangutan commented Sep 7, 2026

Uh oh!

codecov Bot commented Sep 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

blueorangutan commented Sep 8, 2026

Uh oh!

DaanHoogland commented Sep 8, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Sep 8, 2026

Uh oh!

nvazquez commented Sep 8, 2026

Uh oh!

blueorangutan commented Sep 8, 2026

Uh oh!

nvazquez left a comment

Choose a reason for hiding this comment

Uh oh!

blueorangutan commented Sep 8, 2026

Uh oh!

nvazquez commented Sep 8, 2026

Uh oh!

blueorangutan commented Sep 8, 2026

Uh oh!

blueorangutan commented Sep 9, 2026

Uh oh!

sudo87 left a comment

Choose a reason for hiding this comment

Uh oh!

rootnomad commented Sep 15, 2026

Uh oh!

DaanHoogland commented Oct 1, 2026

Uh oh!

sudo87 commented Oct 5, 2026

Uh oh!

Damans227 Oct 14, 2026

Choose a reason for hiding this comment

Uh oh!

rootnomad Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

Damans227 Oct 20, 2026

Choose a reason for hiding this comment

Uh oh!

rootnomad commented Sep 2, 2026 •

edited

Loading

codecov Bot commented Sep 8, 2026 •

edited

Loading