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?

[DaanHoogland]

this comment is cluttering. can you move it to javadoc , possibly on a new method or remove it completely?

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