engine: retry NIC IP allocation instead of NPE when the allocation race 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.

Author: nagaboinaramgopal

engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java

@@ -1058,7 +1058,7 @@ public void saveExtraDhcpOptions(final String networkUuid, final Long nicId, fin
         }
     }
 
-    private NicVO persistNicAfterRaceCheck(final NicVO nic, final Long networkId, final NicProfile profile, int deviceId) {
+    protected NicVO persistNicAfterRaceCheck(final NicVO nic, final Long networkId, final NicProfile profile, int deviceId) {
         return Transaction.execute(new TransactionCallback<NicVO>() {
             @Override
             public NicVO doInTransaction(TransactionStatus status) {
@@ -1074,7 +1074,7 @@ public NicVO doInTransaction(TransactionStatus status) {
         });
     }
 
-    private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Network network, final Boolean isDefaultNic, int deviceId, final VirtualMachineProfile vm)
+    protected NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Network network, final Boolean isDefaultNic, int deviceId, final VirtualMachineProfile vm)
             throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException {
         final NetworkVO ntwkVO = _networksDao.findById(network.getId());
         logger.debug("Allocating NIC for Instance {} in Network {} with requested profile {}", vm.getVirtualMachine(), network, requested);
@@ -1120,9 +1120,9 @@ private NicVO checkForRaceAndAllocateNic(final NicProfile requested, final Netwo
             }
 
             if (vo == null) {
-                if (requested.getRequestedIPv4() != null) {
+                if (requested != null && requested.getRequestedIPv4() != null) {
                     throw new InsufficientVirtualNetworkCapacityException("Unable to acquire requested Guest IP address " + requested.getRequestedIPv4() + " for network " + network, DataCenter.class, dcVo.getId());
-                } else {
+                } else if (requested != null) {
                     requested.setIPv4Address(null);
                 }
                 retryIpAllocation = true;

engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorNicRaceTest.java

@@ -0,0 +1,98 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements.  See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership.  The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License.  You may obtain a copy of the License at
+//
+//   http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied.  See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.cloudstack.engine.orchestration;
+
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
+import static org.mockito.ArgumentMatchers.eq;
+import static org.mockito.ArgumentMatchers.isNull;
+
+import java.util.Collections;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+import org.mockito.Mockito;
+import org.mockito.Spy;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.dc.DataCenter.NetworkType;
+import com.cloud.dc.DataCenterVO;
+import com.cloud.dc.dao.DataCenterDao;
+import com.cloud.network.Network;
+import com.cloud.network.Networks.TrafficType;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkVO;
+import com.cloud.network.guru.NetworkGuru;
+import com.cloud.vm.NicProfile;
+import com.cloud.vm.NicVO;
+import com.cloud.vm.VirtualMachine.Type;
+import com.cloud.vm.VirtualMachineProfile;
+
+@RunWith(MockitoJUnitRunner.Silent.class)
+public class NetworkOrchestratorNicRaceTest {
+
+    @Mock
+    NetworkDao _networksDao;
+    @Mock
+    DataCenterDao _dcDao;
+
+    @Spy
+    @InjectMocks
+    NetworkOrchestrator orchestrator = new NetworkOrchestrator();
+
+    @Test
+    public void checkForRaceAndAllocateNicRetriesInsteadOfNpeWhenNoIpRequested() throws Exception {
+        Network network = Mockito.mock(Network.class);
+        Mockito.when(network.getId()).thenReturn(1L);
+        Mockito.when(network.getDataCenterId()).thenReturn(1L);
+        Mockito.when(network.getTrafficType()).thenReturn(TrafficType.Guest);
+
+        NetworkVO ntwkVO = Mockito.mock(NetworkVO.class);
+        Mockito.when(ntwkVO.getGuruName()).thenReturn("TestGuru");
+        Mockito.when(_networksDao.findById(1L)).thenReturn(ntwkVO);
+
+        DataCenterVO dcVo = Mockito.mock(DataCenterVO.class);
+        Mockito.when(dcVo.getNetworkType()).thenReturn(NetworkType.Advanced);
+        Mockito.when(_dcDao.findById(1L)).thenReturn(dcVo);
+
+        VirtualMachineProfile vm = Mockito.mock(VirtualMachineProfile.class);
+        Mockito.when(vm.getId()).thenReturn(1L);
+        Mockito.when(vm.getType()).thenReturn(Type.User);
+
+        NicProfile profile = Mockito.mock(NicProfile.class);
+        Mockito.when(profile.getIpv4AllocationRaceCheck()).thenReturn(true);
+
+        NetworkGuru guru = Mockito.mock(NetworkGuru.class);
+        Mockito.when(guru.getName()).thenReturn("TestGuru");
+        Mockito.when(guru.allocate(eq(network), isNull(), eq(vm))).thenReturn(profile);
+        orchestrator.setNetworkGurus(Collections.singletonList(guru));
+
+        NicVO persisted = Mockito.mock(NicVO.class);
+        // First attempt loses the IP-allocation race (persist returns null); the retry wins (non-null).
+        Mockito.doReturn(null).doReturn(persisted)
+                .when(orchestrator).persistNicAfterRaceCheck(any(NicVO.class), eq(1L), eq(profile), anyInt());
+
+        // requested == null is the common "no explicit IP" deploy path. Before the fix, losing the race
+        // dereferenced the null requested profile and threw NPE instead of retrying the allocation.
+        NicVO result = orchestrator.checkForRaceAndAllocateNic(null, network, null, 0, vm);
+
+        Assert.assertSame("losing the race should retry and return the persisted NIC, not NPE", persisted, result);
+    }
+}