engine: retry NIC IP allocation instead of NPE when the allocation ra…

[nagaboinaramgopal]

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]

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]

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]

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]

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 ;)

[github-actions]

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

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@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]

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