diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java index d851867ef136..022679dc3920 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/orchestration/service/NetworkOrchestrationService.java +++ b/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 diff --git a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java index c98391a654db..996e71dd0602 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/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: diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 9b1609c669a5..219fb5525f6d 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -2187,6 +2187,26 @@ public int compare(final NicVO nic1, final NicVO nic2) { } } + @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 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, diff --git a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java index c9a404f9c89c..d919451ed9b7 100644 --- a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java +++ b/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 @@ public class VirtualMachineManagerImplTest { @Mock NicDao _nicsDao; @Mock + NetworkOrchestrationService _networkMgr; + @Mock NetworkService networkService; @Mock NetworkModel networkModel; @@ -2049,4 +2052,63 @@ public void testUnmanageSuccessKvm() throws Exception { } } + + 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 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 profileCaptor = ArgumentCaptor.forClass(VirtualMachineProfile.class); + ArgumentCaptor 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()); + } } diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index e3989737112d..4de2b19657e8 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -1010,4 +1010,36 @@ public void testImportNicWithIP4Address() throws Exception { 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); + } } diff --git a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java index aa27b24e60f5..76db7173d87c 100644 --- a/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java +++ b/server/src/test/java/com/cloud/vpc/MockNetworkManagerImpl.java @@ -758,6 +758,10 @@ public Pair allocateNic(NicProfile requested, Network netwo 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, diff --git a/test/integration/smoke/test_nic.py b/test/integration/smoke/test_nic.py index ec7af5498d6e..0916f4481186 100644 --- a/test/integration/smoke/test_nic.py +++ b/test/integration/smoke/test_nic.py @@ -18,6 +18,8 @@ from marvin.codes import FAILED from marvin.cloudstackTestCase import cloudstackTestCase from marvin.lib.base import (Account, + Configurations, + Host, ServiceOffering, Network, VirtualMachine, @@ -25,7 +27,8 @@ 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: