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

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

Description

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 losing thread threw a NullPointerException instead of nulling the IP and
retrying. This defeats the ipv4AllocationRaceCheck retry for exactly the case it
was written for: many VMs deploying onto one guest network at once (bulk deploy,
autoscale groups, Kubernetes/CKS node provisioning).

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?

nagaboinaramgopal · 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.

nagaboinaramgopal · 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 4 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

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

nagaboinaramgopal 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

nagaboinaramgopal 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

nagaboinaramgopal 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

Uh oh!

Conversation

nagaboinaramgopal commented Sep 2, 2026

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!

nagaboinaramgopal commented Sep 2, 2026

Uh oh!

DaanHoogland commented Sep 3, 2026

Uh oh!

nagaboinaramgopal 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Sep 8, 2026 •

edited

Loading