network: restore released NIC state when a VM re-syncs Stopped/Stopping state

engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java

@@ -265,6 +265,12 @@ NicProfile prepareNic(VirtualMachineProfile vmProfile, DeployDestination dest, R
         throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException, ConcurrentOperationException, InsufficientCapacityException,
         ResourceUnavailableException;
 
+    /**
+     * Re-reserves the Allocated nics of a VM that is Running, e.g. after its nics were released on a power report
+     * that was later reverted, and restores the active nics count of their networks.
+     */
+    void reconcileNicsForRunningVm(VirtualMachineProfile vmProfile, DeployDestination dest, ReservationContext context);
+
     /**
      * Removes the provided nic from the given vm
      * @param vm

engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java

@@ -5432,6 +5432,7 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
             ActionEventUtils.onActionEvent(User.UID_SYSTEM, Account.ACCOUNT_ID_SYSTEM, vm.getDomainId(),
                 EventTypes.EVENT_VM_START, "Out of band VM power on", vm.getId(), getApiCommandResourceTypeForVm(vm).toString());
             logger.info("VM {} is sync-ed to at Running state according to power-on report from hypervisor.", vm.getInstanceName());
+            reconcileNicsForRunningVm(vm);
             break;
 
         case Destroyed:
@@ -5456,6 +5457,20 @@ private void handlePowerOnReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
         }
     }
 
+    private void reconcileNicsForRunningVm(final VMInstanceVO vm) {
+        final VMInstanceVO runningVm = _vmDao.findById(vm.getId());
+        if (runningVm == null || runningVm.getType() != VirtualMachine.Type.User || runningVm.getState() != State.Running || runningVm.getHostId() == null
+                || _nicsDao.listByVmId(runningVm.getId()).stream().noneMatch(nic -> nic.getState() == Nic.State.Allocated)) {
+            return;
+        }
+        final Host host = _hostDao.findById(runningVm.getHostId());
+        final DataCenter dc = _entityMgr.findById(DataCenter.class, runningVm.getDataCenterId());
+        final User user = _entityMgr.findById(User.class, User.UID_SYSTEM);
+        final Account account = _entityMgr.findById(Account.class, Account.ACCOUNT_ID_SYSTEM);
+        final ReservationContext context = new ReservationContextImpl(null, null, user, account);
+        _networkMgr.reconcileNicsForRunningVm(new VirtualMachineProfileImpl(runningVm), new DeployDestination(dc, null, null, host), context);
+    }
+
     private void handlePowerOffReportWithNoPendingJobsOnVM(final VMInstanceVO vm) {
         switch (vm.getState()) {
         case Starting:

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

@@ -2187,6 +2187,26 @@
         }
     }
 
+    @Override
+    public void reconcileNicsForRunningVm(final VirtualMachineProfile vmProfile, final DeployDestination dest, final ReservationContext context) {
+        for (final NicVO nic : _nicDao.listByVmId(vmProfile.getId())) {
+            if (nic.getState() != Nic.State.Allocated) {
+                continue;
+            }
+            try {
+                final Pair<NetworkGuru, NetworkVO> implemented = implementNetwork(nic.getNetworkId(), dest, context);
+                if (implemented == null || implemented.first() == null) {
+                    logger.warn("Unable to implement Network ID={} to reconcile NIC {} of Running Instance {}", nic.getNetworkId(), nic, vmProfile);
+                    continue;
+                }
+                prepareNic(vmProfile, dest, context, nic.getId(), implemented.second());
+                logger.info("Re-reserved NIC {} of Running Instance {}", nic, vmProfile);
+            } catch (final Exception e) {
+                logger.warn("Failed to reconcile NIC {} of Running Instance {}", nic, vmProfile, e);
+            }
+        }
+    }
+
     @Override
     public NicProfile prepareNic(final VirtualMachineProfile vmProfile, final DeployDestination dest, final ReservationContext context, final long nicId, final Network network)
             throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException, ConcurrentOperationException, InsufficientCapacityException,

engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java

@@ -62,6 +62,7 @@
 import com.cloud.resource.ResourceManager;
 import org.apache.cloudstack.api.ApiConstants;
 import org.apache.cloudstack.context.CallContext;
+import org.apache.cloudstack.engine.orchestration.service.NetworkOrchestrationService;
 import org.apache.cloudstack.engine.subsystem.api.storage.StoragePoolAllocator;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeDataFactory;
 import org.apache.cloudstack.engine.subsystem.api.storage.VolumeInfo;
@@ -282,6 +283,8 @@
     @Mock
     NicDao _nicsDao;
     @Mock
+    NetworkOrchestrationService _networkMgr;
+    @Mock
     NetworkService networkService;
     @Mock
     NetworkModel networkModel;
@@ -2049,4 +2052,63 @@
         }
     }
 
+
+    private NicVO mockNicInState(Nic.State state) {
+        NicVO nic = mock(NicVO.class);
+        when(nic.getState()).thenReturn(state);
+        return nic;
+    }
+
+    private void mockVmForNicReconciliation(VirtualMachine.Type type, State state, Nic.State... nicStates) {
+        when(vmInstanceDaoMock.findById(vmInstanceVoMockId)).thenReturn(vmInstanceMock);
+        Mockito.lenient().when(vmInstanceMock.getType()).thenReturn(type);
+        Mockito.lenient().when(vmInstanceMock.getState()).thenReturn(state);
+        Mockito.lenient().when(vmInstanceMock.getHostId()).thenReturn(hostMockId);
+        Mockito.lenient().when(vmInstanceMock.getDataCenterId()).thenReturn(zoneMockId);
+        List<NicVO> nics = new ArrayList<>();
+        for (Nic.State nicState : nicStates) {
+            nics.add(mockNicInState(nicState));
+        }
+        Mockito.lenient().when(_nicsDao.listByVmId(vmInstanceVoMockId)).thenReturn(nics);
+    }
+
+    @Test
+    public void reconcileNicsForRunningVmReReservesReleasedNicsOnTheVmHost() {
+        mockVmForNicReconciliation(VirtualMachine.Type.User, State.Running, Nic.State.Reserved, Nic.State.Allocated);
+
+        ReflectionTestUtils.invokeMethod(virtualMachineManagerImpl, "reconcileNicsForRunningVm", vmInstanceMock);
+
+        ArgumentCaptor<VirtualMachineProfile> profileCaptor = ArgumentCaptor.forClass(VirtualMachineProfile.class);
+        ArgumentCaptor<DeployDestination> destCaptor = ArgumentCaptor.forClass(DeployDestination.class);
+        verify(_networkMgr).reconcileNicsForRunningVm(profileCaptor.capture(), destCaptor.capture(), any());
+        assertEquals(vmInstanceMock, profileCaptor.getValue().getVirtualMachine());
+        assertEquals(hostMock, destCaptor.getValue().getHost());
+    }
+
+    @Test
+    public void reconcileNicsForRunningVmSkipsVmWithoutReleasedNics() {
+        mockVmForNicReconciliation(VirtualMachine.Type.User, State.Running, Nic.State.Reserved);
+
+        ReflectionTestUtils.invokeMethod(virtualMachineManagerImpl, "reconcileNicsForRunningVm", vmInstanceMock);
+
+        verify(_networkMgr, never()).reconcileNicsForRunningVm(any(), any(), any());
+    }
+
+    @Test
+    public void reconcileNicsForRunningVmSkipsSystemVms() {
+        mockVmForNicReconciliation(VirtualMachine.Type.DomainRouter, State.Running, Nic.State.Allocated);
+
+        ReflectionTestUtils.invokeMethod(virtualMachineManagerImpl, "reconcileNicsForRunningVm", vmInstanceMock);
+
+        verify(_networkMgr, never()).reconcileNicsForRunningVm(any(), any(), any());
+    }
+
+    @Test
+    public void reconcileNicsForRunningVmSkipsVmThatDidNotReachRunning() {
+        mockVmForNicReconciliation(VirtualMachine.Type.User, State.Stopped, Nic.State.Allocated);
+
+        ReflectionTestUtils.invokeMethod(virtualMachineManagerImpl, "reconcileNicsForRunningVm", vmInstanceMock);
+
+        verify(_networkMgr, never()).reconcileNicsForRunningVm(any(), any(), any());
+    }
 }

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

@@ -1010,4 +1010,36 @@
             assertEquals("testtag", nicProfile.getName());
         }
     }
+
+    private NicVO mockNic(long id, long networkId, Nic.State state) {
+        NicVO nic = mock(NicVO.class);
+        Mockito.lenient().when(nic.getId()).thenReturn(id);
+        Mockito.lenient().when(nic.getNetworkId()).thenReturn(networkId);
+        when(nic.getState()).thenReturn(state);
+        return nic;
+    }
+
+    @Test
+    public void reconcileNicsForRunningVmPreparesOnlyAllocatedNicsAndContinuesOnFailure() throws Exception {
+        VirtualMachineProfile vmProfile = mock(VirtualMachineProfile.class);
+        when(vmProfile.getId()).thenReturn(1L);
+        DeployDestination dest = mock(DeployDestination.class);
+        ReservationContext context = mock(ReservationContext.class);
+        NetworkVO network = mock(NetworkVO.class);
+        NicVO failingNic = mockNic(11L, 101L, Nic.State.Allocated);
+        NicVO unimplementedNic = mockNic(12L, 102L, Nic.State.Allocated);
+        NicVO releasedNic = mockNic(13L, 103L, Nic.State.Allocated);
+        NicVO reservedNic = mockNic(14L, 104L, Nic.State.Reserved);
+        when(testOrchestrator._nicDao.listByVmId(1L)).thenReturn(List.of(failingNic, unimplementedNic, releasedNic, reservedNic));
+        Mockito.doThrow(new CloudRuntimeException("implement failed")).when(testOrchestrator).implementNetwork(101L, dest, context);
+        Mockito.doReturn(new Pair<>(null, null)).when(testOrchestrator).implementNetwork(102L, dest, context);
+        Mockito.doReturn(new Pair<>(guru, network)).when(testOrchestrator).implementNetwork(103L, dest, context);
+        Mockito.doReturn(null).when(testOrchestrator).prepareNic(vmProfile, dest, context, 13L, network);
+
+        testOrchestrator.reconcileNicsForRunningVm(vmProfile, dest, context);
+
+        verify(testOrchestrator).prepareNic(vmProfile, dest, context, 13L, network);
+        verify(testOrchestrator, times(1)).prepareNic(any(), any(), any(), ArgumentMatchers.anyLong(), any());
+        verify(testOrchestrator, never()).implementNetwork(104L, dest, context);
+    }
 }

server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java

@@ -758,6 +758,10 @@
         return null;
     }
 
+    @Override
+    public void reconcileNicsForRunningVm(VirtualMachineProfile vmProfile, DeployDestination dest, ReservationContext context) {
+    }
+
     @Override
     public NicProfile prepareNic(VirtualMachineProfile vmProfile, DeployDestination dest, ReservationContext context, long nicId, Network network)
         throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException, ConcurrentOperationException, InsufficientCapacityException,

test/integration/smoke/test_nic.py

@@ -18,14 +18,17 @@
 from marvin.codes import FAILED
 from marvin.cloudstackTestCase import cloudstackTestCase
 from marvin.lib.base import (Account,
+                             Configurations,
+                             Host,
                              ServiceOffering,
                              Network,
                              VirtualMachine,
                              NetworkOffering)
 from marvin.lib.common import (get_zone,
                                get_suitable_test_template,
                                get_domain)
-from marvin.lib.utils import validateList
+from marvin.lib.utils import validateList, wait_until
+from marvin.sshClient import SshClient
 from marvin.codes import PASS
 from nose.plugins.attrib import attr
 
@@ -146,8 +149,9 @@ def signal_handler(signal, frame):
                 zoneid=self.services["network"]["zoneid"]
             )
             self.cleanup.insert(0, self.test_network2)
-        except Exception as ex:
-            self.debug("Exception during NIC test SETUP!: " + str(ex))
+        except Exception:
+            self.tearDown()
+            raise
 
     @attr(
         tags=[
@@ -456,7 +460,85 @@ def run(self):
             self.assertTrue(found, "Nic not found for the specific network")
 
 
+    def get_vm_nic(self, vm_uuid):
+        return self.dbclient.execute(
+            "SELECT n.state, n.broadcast_uri, n.ip4_address, n.network_id FROM nics n JOIN vm_instance v ON n.instance_id = v.id "
+            "WHERE v.uuid = '%s' AND n.removed IS NULL" % vm_uuid)[0]
+
+    def get_nics_count(self, network_id):
+        return self.dbclient.execute("SELECT nics_count FROM op_networks WHERE id = %d" % network_id)[0][0]
+
+    def wait_for_vm_and_nic_state(self, vm, vm_state, nic_state, timeout):
+        def check_state():
+            vms = VirtualMachine.list(self.apiclient, id=vm.id, listall=True)
+            nic = self.get_vm_nic(vm.id)
+            return vms is not None and vms[0].state == vm_state and nic[0] == nic_state, nic
+
+        result, nic = wait_until(5, timeout // 5, check_state)
+        self.assertTrue(result, "VM %s did not reach state %s with nic state %s, nic is %s" % (vm.name, vm_state, nic_state, nic))
+        return nic
+
+    @attr(tags=["advanced", "advancedns", "smoke"], required_hardware="true")
+    def test_04_nic_reserved_again_after_out_of_band_power_on(self):
+        """Test that a NIC released on a missing VM is reserved again and nics_count restored once the VM runs again
+
+        1. Deploy a VM in an isolated network, record its nic and the network's nics_count
+        2. Destroy the domain with virsh, so the VM is reported missing and synced to Stopped
+        3. Verify the nic is released (Allocated, no broadcast uri) and nics_count went down by one
+        4. Recreate the domain with virsh, so the VM is synced to Running out of band
+        5. Verify the nic is Reserved again with the same broadcast uri and ip, and nics_count is restored
+        """
+        if self.hypervisor.lower() != "kvm":
+            self.skipTest("Out of band power on through virsh is only tested on KVM")
+
+        host_config = self.config.__dict__["zones"][0].__dict__["pods"][0].__dict__["clusters"][0].__dict__["hosts"][0].__dict__
+        ping_interval = int(Configurations.list(self.apiclient, name="ping.interval")[0].value)
+
+        vm = VirtualMachine.create(
+            self.apiclient,
+            self.services["small"],
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            serviceofferingid=self.service_offering.id,
+            networkids=[self.test_network.id]
+        )
+        self.cleanup.insert(0, vm)
+
+        # 1 - record the reserved nic and nics_count
+        nic_state, broadcast_uri, ip_address, network_id = self.get_vm_nic(vm.id)
+        self.assertEqual(nic_state, "Reserved", "Nic of a running VM should be Reserved")
+        self.assertIsNotNone(broadcast_uri, "Nic of a running VM should have a broadcast uri")
+        nics_count = self.get_nics_count(network_id)
+
+        vm_response = VirtualMachine.list(self.apiclient, id=vm.id, listall=True)[0]
+        host = Host.list(self.apiclient, id=vm_response.hostid)[0]
+        ssh_host = SshClient(host.ipaddress, 22, host_config["username"], host_config["password"], 10)
+        domain_xml = "/tmp/%s.xml" % vm_response.instancename
+        self.cleanup_domain_xml = lambda: ssh_host.execute("rm -f %s" % domain_xml)
+
+        # 2 - make the VM go missing
+        ssh_host.execute("virsh dumpxml %s > %s" % (vm_response.instancename, domain_xml))
+        ssh_host.execute("virsh destroy %s" % vm_response.instancename)
+
+        # 3 - nic released and nics_count decremented
+        # a missing VM is only synced after a graceful period of two ping intervals, noticed on the next ping
+        released_nic = self.wait_for_vm_and_nic_state(vm, "Stopped", "Allocated", 3 * ping_interval + 30)
+        self.assertIsNone(released_nic[1], "Released nic should not have a broadcast uri")
+        self.assertEqual(self.get_nics_count(network_id), nics_count - 1, "nics_count should drop by one when the nic is released")
+
+        # 4 - bring the domain back out of band, a persistent domain is still defined and only needs a start
+        ssh_host.execute("virsh create %s || virsh start %s" % (domain_xml, vm_response.instancename))
+
+        # 5 - nic reserved again and nics_count restored
+        # the power on is noticed on the next ping
+        reconciled_nic = self.wait_for_vm_and_nic_state(vm, "Running", "Reserved", ping_interval + 30)
+        self.assertEqual(reconciled_nic[1], broadcast_uri, "Reconciled nic should get the network's broadcast uri back")
+        self.assertEqual(reconciled_nic[2], ip_address, "Reconciled nic should keep its ip address")
+        self.assertEqual(self.get_nics_count(network_id), nics_count, "nics_count should be restored when the nic is reserved again")
+
     def tearDown(self):
+        if hasattr(self, "cleanup_domain_xml"):
+            self.cleanup_domain_xml()
         try:
             for obj in self.cleanup:
                 try: