diff --git a/core/src/main/java/com/cloud/agent/api/DeleteUefiNvramCommand.java b/core/src/main/java/com/cloud/agent/api/DeleteUefiNvramCommand.java new file mode 100644 index 000000000000..6f82bae15820 --- /dev/null +++ b/core/src/main/java/com/cloud/agent/api/DeleteUefiNvramCommand.java @@ -0,0 +1,49 @@ +// +// 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 com.cloud.agent.api; + +/** + * Asks a KVM host to delete the per-VM UEFI NVRAM file of an expunged VM. + */ +public class DeleteUefiNvramCommand extends Command { + private String vmName; + private String vmUuid; + + protected DeleteUefiNvramCommand() { + } + + public DeleteUefiNvramCommand(String vmName, String vmUuid) { + this.vmName = vmName; + this.vmUuid = vmUuid; + } + + public String getVmName() { + return vmName; + } + + public String getVmUuid() { + return vmUuid; + } + + @Override + public boolean executeInSequence() { + return false; + } +} diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapper.java new file mode 100644 index 000000000000..d8afd3298215 --- /dev/null +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapper.java @@ -0,0 +1,113 @@ +// +// 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 com.cloud.hypervisor.kvm.resource.wrapper; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.UUID; + +import org.apache.commons.lang3.StringUtils; +import org.libvirt.Connect; +import org.libvirt.Domain; +import org.libvirt.Error; +import org.libvirt.LibvirtException; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteUefiNvramCommand; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; +import com.cloud.resource.CommandWrapper; +import com.cloud.resource.ResourceWrapper; + +@ResourceWrapper(handles = DeleteUefiNvramCommand.class) +public class LibvirtDeleteUefiNvramCommandWrapper extends CommandWrapper { + + @Override + public Answer execute(DeleteUefiNvramCommand command, LibvirtComputingResource resource) { + String vmUuid = command.getVmUuid(); + if (!isValidUuid(vmUuid)) { + return new Answer(command, false, String.format("Invalid VM UUID [%s]; not deleting any UEFI NVRAM file", vmUuid)); + } + + String nvramPath = resource.getUefiNvramPath(vmUuid); + if (StringUtils.isBlank(nvramPath)) { + return new Answer(command, true, "No UEFI NVRAM path is configured on this host; nothing to delete"); + } + + try { + if (isDomainDefined(resource, vmUuid)) { + String msg = String.format("VM [%s] with UUID [%s] is still defined on this host; keeping its UEFI NVRAM file [%s]", + command.getVmName(), vmUuid, nvramPath); + logger.warn(msg); + return new Answer(command, false, msg); + } + } catch (LibvirtException e) { + String msg = String.format("Unable to check whether VM [%s] is still defined on this host; keeping its UEFI NVRAM file [%s]: %s", + command.getVmName(), nvramPath, e.getMessage()); + logger.warn(msg); + return new Answer(command, false, msg); + } + + try { + if (Files.deleteIfExists(Path.of(nvramPath))) { + logger.info("Deleted UEFI NVRAM file [{}] of expunged VM [{}]", nvramPath, command.getVmName()); + return new Answer(command, true, String.format("Deleted UEFI NVRAM file [%s]", nvramPath)); + } + return new Answer(command, true, String.format("UEFI NVRAM file [%s] does not exist on this host", nvramPath)); + } catch (IOException e) { + String msg = String.format("Unable to delete UEFI NVRAM file [%s] of VM [%s]: %s", nvramPath, command.getVmName(), e.getMessage()); + logger.warn(msg); + return new Answer(command, false, msg); + } + } + + protected boolean isValidUuid(String vmUuid) { + if (StringUtils.isBlank(vmUuid)) { + return false; + } + try { + return UUID.fromString(vmUuid).toString().equalsIgnoreCase(vmUuid); + } catch (IllegalArgumentException e) { + return false; + } + } + + protected boolean isDomainDefined(LibvirtComputingResource resource, String vmUuid) throws LibvirtException { + Connect conn = resource.getLibvirtUtilitiesHelper().getConnection(); + Domain domain = null; + try { + domain = conn.domainLookupByUUIDString(vmUuid); + return domain != null; + } catch (LibvirtException e) { + if (e.getError() != null && e.getError().getCode() == Error.ErrorNumber.VIR_ERR_NO_DOMAIN) { + return false; + } + throw e; + } finally { + if (domain != null) { + try { + domain.free(); + } catch (LibvirtException e) { + logger.trace("Ignoring libvirt error while freeing domain", e); + } + } + } + } +} diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapper.java index fb432cd71e66..b5fda64556f2 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapper.java @@ -18,6 +18,9 @@ // package com.cloud.hypervisor.kvm.resource.wrapper; +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; import java.util.List; import org.apache.cloudstack.storage.to.PrimaryDataStoreTO; @@ -89,6 +92,10 @@ public Answer execute(ImportConvertedInstanceCommand cmd, LibvirtComputingResour UnmanagedInstanceTO convertedInstanceTO = getConvertedUnmanagedInstance(temporaryConvertUuid, disks, xmlParser); + if (forceConvertToPool) { + // The converted disks are adopted where they are; only the domain XML written by virt-v2v is left over + cleanupConvertedDomainXml(convertedBasePath); + } return new ImportConvertedInstanceAnswer(cmd, convertedInstanceTO); } catch (Exception e) { String error = String.format("Error converting instance %s from %s, due to: %s", @@ -103,6 +110,17 @@ public Answer execute(ImportConvertedInstanceCommand cmd, LibvirtComputingResour } } + protected void cleanupConvertedDomainXml(String convertedBasePath) { + Path xmlPath = Path.of(String.format("%s.xml", convertedBasePath)); + try { + if (Files.deleteIfExists(xmlPath)) { + logger.info("Removed the converted domain XML {} after importing the converted disks in place", xmlPath); + } + } catch (IOException e) { + logger.warn("Unable to remove the converted domain XML {}: {}", xmlPath, e.getMessage()); + } + } + private boolean isForcedBlockStorageConversion(DataStoreTO conversionTemporaryLocation, boolean forceConvertToPool) { if (!forceConvertToPool || !(conversionTemporaryLocation instanceof PrimaryDataStoreTO)) { return false; diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapperTest.java new file mode 100644 index 000000000000..de52c44f4799 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapperTest.java @@ -0,0 +1,179 @@ +// +// 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 com.cloud.hypervisor.kvm.resource.wrapper; + +import java.io.File; +import java.io.IOException; +import java.util.UUID; + +import org.junit.Assert; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TemporaryFolder; +import org.junit.runner.RunWith; +import org.libvirt.Connect; +import org.libvirt.Domain; +import org.libvirt.Error; +import org.libvirt.LibvirtException; +import org.mockito.Mock; +import org.mockito.Mockito; +import org.mockito.Spy; +import org.mockito.junit.MockitoJUnitRunner; + +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteUefiNvramCommand; +import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource; + +@RunWith(MockitoJUnitRunner.class) +public class LibvirtDeleteUefiNvramCommandWrapperTest { + + @Spy + private LibvirtDeleteUefiNvramCommandWrapper wrapper = new LibvirtDeleteUefiNvramCommandWrapper(); + + @Mock + private LibvirtComputingResource resource; + @Mock + private LibvirtUtilitiesHelper libvirtUtilitiesHelper; + @Mock + private Connect conn; + + @Rule + public TemporaryFolder nvramFolder = new TemporaryFolder(); + + private final String vmUuid = UUID.randomUUID().toString(); + + private File createNvramFile(String uuid) throws IOException { + File nvramFile = nvramFolder.newFile(uuid + ".fd"); + Mockito.lenient().when(resource.getUefiNvramPath(uuid)).thenReturn(nvramFile.getAbsolutePath()); + return nvramFile; + } + + private LibvirtException libvirtException(Error.ErrorNumber errorNumber) { + Error error = Mockito.mock(Error.class); + Mockito.when(error.getCode()).thenReturn(errorNumber); + LibvirtException exception = Mockito.mock(LibvirtException.class); + Mockito.when(exception.getError()).thenReturn(error); + return exception; + } + + @Test + public void testExecuteDeletesOnlyTheNvramFileOfTheExpungedVm() throws Exception { + File nvramFile = createNvramFile(vmUuid); + File otherNvramFile = nvramFolder.newFile(UUID.randomUUID() + ".fd"); + Mockito.doReturn(false).when(wrapper).isDomainDefined(resource, vmUuid); + + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", vmUuid), resource); + + Assert.assertTrue(answer.getResult()); + Assert.assertFalse(nvramFile.exists()); + Assert.assertTrue(otherNvramFile.exists()); + } + + @Test + public void testExecuteSucceedsWhenNvramFileDoesNotExist() throws Exception { + String missingPath = new File(nvramFolder.getRoot(), vmUuid + ".fd").getAbsolutePath(); + Mockito.when(resource.getUefiNvramPath(vmUuid)).thenReturn(missingPath); + Mockito.doReturn(false).when(wrapper).isDomainDefined(resource, vmUuid); + + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", vmUuid), resource); + + Assert.assertTrue(answer.getResult()); + } + + @Test + public void testExecuteKeepsNvramFileWhenDomainIsStillDefined() throws Exception { + File nvramFile = createNvramFile(vmUuid); + Mockito.doReturn(true).when(wrapper).isDomainDefined(resource, vmUuid); + + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", vmUuid), resource); + + Assert.assertFalse(answer.getResult()); + Assert.assertTrue(nvramFile.exists()); + } + + @Test + public void testExecuteKeepsNvramFileWhenDomainCheckFails() throws Exception { + File nvramFile = createNvramFile(vmUuid); + Mockito.doThrow(LibvirtException.class).when(wrapper).isDomainDefined(resource, vmUuid); + + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", vmUuid), resource); + + Assert.assertFalse(answer.getResult()); + Assert.assertTrue(nvramFile.exists()); + } + + @Test + public void testExecuteDoesNothingWhenNoNvramPathIsConfigured() throws Exception { + Mockito.when(resource.getUefiNvramPath(vmUuid)).thenReturn(null); + + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", vmUuid), resource); + + Assert.assertTrue(answer.getResult()); + Mockito.verify(wrapper, Mockito.never()).isDomainDefined(Mockito.any(), Mockito.anyString()); + } + + @Test + public void testExecuteRejectsAnInvalidUuid() throws Exception { + Answer answer = wrapper.execute(new DeleteUefiNvramCommand("i-2-3-VM", "../" + vmUuid), resource); + + Assert.assertFalse(answer.getResult()); + Mockito.verify(resource, Mockito.never()).getUefiNvramPath(Mockito.anyString()); + Mockito.verify(wrapper, Mockito.never()).isDomainDefined(Mockito.any(), Mockito.anyString()); + } + + @Test + public void testIsValidUuid() { + Assert.assertTrue(wrapper.isValidUuid(vmUuid)); + Assert.assertFalse(wrapper.isValidUuid(null)); + Assert.assertFalse(wrapper.isValidUuid("")); + Assert.assertFalse(wrapper.isValidUuid("not-a-uuid")); + Assert.assertFalse(wrapper.isValidUuid(vmUuid + "/../x")); + } + + @Test + public void testIsDomainDefinedReturnsTrueAndFreesTheDomain() throws Exception { + Domain domain = Mockito.mock(Domain.class); + Mockito.when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + Mockito.when(libvirtUtilitiesHelper.getConnection()).thenReturn(conn); + Mockito.when(conn.domainLookupByUUIDString(vmUuid)).thenReturn(domain); + + Assert.assertTrue(wrapper.isDomainDefined(resource, vmUuid)); + Mockito.verify(domain).free(); + } + + @Test + public void testIsDomainDefinedReturnsFalseWhenTheDomainDoesNotExist() throws Exception { + Mockito.when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + Mockito.when(libvirtUtilitiesHelper.getConnection()).thenReturn(conn); + LibvirtException noDomain = libvirtException(Error.ErrorNumber.VIR_ERR_NO_DOMAIN); + Mockito.when(conn.domainLookupByUUIDString(vmUuid)).thenThrow(noDomain); + + Assert.assertFalse(wrapper.isDomainDefined(resource, vmUuid)); + } + + @Test(expected = LibvirtException.class) + public void testIsDomainDefinedRethrowsLibvirtErrorsOtherThanMissingDomain() throws Exception { + Mockito.when(resource.getLibvirtUtilitiesHelper()).thenReturn(libvirtUtilitiesHelper); + Mockito.when(libvirtUtilitiesHelper.getConnection()).thenReturn(conn); + Mockito.when(conn.domainLookupByUUIDString(vmUuid)).thenThrow(LibvirtException.class); + + wrapper.isDomainDefined(resource, vmUuid); + } +} diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapperTest.java index 9635243a21de..6c18d97bfab4 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtImportConvertedInstanceCommandWrapperTest.java @@ -36,7 +36,9 @@ import org.apache.cloudstack.vm.UnmanagedInstanceTO; import org.junit.Assert; import org.junit.Before; +import org.junit.Rule; import org.junit.Test; +import org.junit.rules.TemporaryFolder; import org.junit.runner.RunWith; import org.mockito.Mock; import org.mockito.MockedConstruction; @@ -45,6 +47,9 @@ import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import java.io.File; +import java.io.IOException; +import java.util.ArrayList; import java.util.Arrays; import java.util.List; import java.util.UUID; @@ -70,6 +75,9 @@ public class LibvirtImportConvertedInstanceCommandWrapperTest { @Mock private KVMStoragePoolManager storagePoolManager; + @Rule + public TemporaryFolder conversionPoolFolder = new TemporaryFolder(); + private static final String secondaryPoolUrl = "nfs://192.168.1.1/secondary"; private static final String vmName = "VmToImport"; @@ -352,4 +360,72 @@ public void testExecuteConvertUnsupportedHypervisors() { Answer answer = importInstanceCommandWrapper.execute(cmd, libvirtComputingResourceMock); Assert.assertFalse(answer.getResult()); } + + private ImportConvertedInstanceCommand getForcedFileBasedImportCommand(String convertUuid) { + String poolUuid = UUID.randomUUID().toString(); + Mockito.when(primaryDataStore.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem); + Mockito.when(primaryDataStore.getUuid()).thenReturn(poolUuid); + Mockito.when(storagePoolManager.getStoragePool(Storage.StoragePoolType.NetworkFilesystem, poolUuid)).thenReturn(destinationPool); + Mockito.when(destinationPool.getLocalPath()).thenReturn(conversionPoolFolder.getRoot().getAbsolutePath()); + + RemoteInstanceTO remoteInstanceTO = getRemoteInstanceTO(Hypervisor.HypervisorType.VMware); + ImportConvertedInstanceCommand cmd = Mockito.mock(ImportConvertedInstanceCommand.class); + Mockito.when(cmd.getSourceInstance()).thenReturn(remoteInstanceTO); + Mockito.when(cmd.getConversionTemporaryLocation()).thenReturn(primaryDataStore); + Mockito.when(cmd.getTemporaryConvertUuid()).thenReturn(convertUuid); + Mockito.when(cmd.isForceConvertToPool()).thenReturn(true); + return cmd; + } + + @Test + public void testExecuteForceConvertToFilePoolRemovesOnlyConvertedDomainXml() throws IOException { + String convertUuid = UUID.randomUUID().toString(); + File convertedXml = conversionPoolFolder.newFile(convertUuid + ".xml"); + File convertedDisk = conversionPoolFolder.newFile(convertUuid + "-sda"); + File unrelatedXml = conversionPoolFolder.newFile(UUID.randomUUID() + ".xml"); + ImportConvertedInstanceCommand cmd = getForcedFileBasedImportCommand(convertUuid); + + LibvirtDomainXMLParser parser = Mockito.mock(LibvirtDomainXMLParser.class); + KVMPhysicalDisk disk = Mockito.mock(KVMPhysicalDisk.class); + Mockito.doReturn(parser).when(importInstanceCommandWrapper).parseMigratedVMXmlDomain(Mockito.anyString()); + Mockito.doReturn(List.of(disk)).when(importInstanceCommandWrapper).getTemporaryDisksFromParsedXml(Mockito.eq(destinationPool), + Mockito.eq(parser), Mockito.anyString(), Mockito.anyString(), Mockito.eq(convertUuid)); + Mockito.doReturn(Mockito.mock(UnmanagedInstanceTO.class)).when(importInstanceCommandWrapper) + .getConvertedUnmanagedInstance(convertUuid, List.of(disk), parser); + + Answer answer = importInstanceCommandWrapper.execute(cmd, libvirtComputingResourceMock); + + Assert.assertTrue(answer.getResult()); + Assert.assertFalse(convertedXml.exists()); + Assert.assertTrue(convertedDisk.exists()); + Assert.assertTrue(unrelatedXml.exists()); + Mockito.verify(importInstanceCommandWrapper, Mockito.never()).moveTemporaryDisksToDestination(Mockito.anyList(), + Mockito.anyList(), Mockito.anyList(), Mockito.any()); + } + + @Test + public void testExecuteForceConvertToFilePoolKeepsConvertedDomainXmlWhenNoDiskIsFound() throws IOException { + String convertUuid = UUID.randomUUID().toString(); + File convertedXml = conversionPoolFolder.newFile(convertUuid + ".xml"); + ImportConvertedInstanceCommand cmd = getForcedFileBasedImportCommand(convertUuid); + + LibvirtDomainXMLParser parser = Mockito.mock(LibvirtDomainXMLParser.class); + Mockito.doReturn(parser).when(importInstanceCommandWrapper).parseMigratedVMXmlDomain(Mockito.anyString()); + Mockito.doReturn(new ArrayList()).when(importInstanceCommandWrapper).getTemporaryDisksFromParsedXml(Mockito.eq(destinationPool), + Mockito.eq(parser), Mockito.anyString(), Mockito.anyString(), Mockito.eq(convertUuid)); + + Answer answer = importInstanceCommandWrapper.execute(cmd, libvirtComputingResourceMock); + + Assert.assertFalse(answer.getResult()); + Assert.assertTrue(convertedXml.exists()); + } + + @Test + public void testCleanupConvertedDomainXmlIgnoresMissingFile() { + String convertedBasePath = new File(conversionPoolFolder.getRoot(), UUID.randomUUID().toString()).getAbsolutePath(); + + importInstanceCommandWrapper.cleanupConvertedDomainXml(convertedBasePath); + + Assert.assertFalse(new File(convertedBasePath + ".xml").exists()); + } } diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java index 5e3beed769a6..ef867c46404a 100644 --- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java +++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java @@ -16,7 +16,10 @@ // under the License. package com.cloud.hypervisor; +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; import com.cloud.agent.api.Command; +import com.cloud.agent.api.DeleteUefiNvramCommand; import com.cloud.agent.api.to.DataObjectType; import com.cloud.agent.api.to.NicTO; import com.cloud.agent.api.to.VirtualMachineTO; @@ -44,6 +47,8 @@ import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; +import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.backup.Backup; import org.apache.cloudstack.backup.BackupManagerImpl; import org.apache.cloudstack.backup.BackupProvider; @@ -77,7 +82,12 @@ public class KVMGuru extends HypervisorGuruBase implements HypervisorGuru { @Inject private VolumeApiService volumeApiService; + @Inject + AgentManager agentMgr; + @Inject + VMInstanceDetailsDao vmInstanceDetailsDao; + protected static final int UEFI_NVRAM_CLEANUP_TIMEOUT_SECONDS = 60; @Override public HypervisorType getHypervisorType() { @@ -349,6 +359,41 @@ public boolean trackVmHostChange() { return false; } + @Override + public List finalizeExpunge(VirtualMachine vm) { + deleteUefiNvramIfNeeded(vm); + return super.finalizeExpunge(vm); + } + + /** + * Best-effort removal of the per-VM UEFI NVRAM file from the host the VM last ran on. + * It never fails the expunge: if the host is unreachable or the deletion fails, the file is left behind as before. + */ + protected void deleteUefiNvramIfNeeded(VirtualMachine vm) { + try { + if (vm.getType() != VirtualMachine.Type.User + || vmInstanceDetailsDao.findDetail(vm.getId(), ApiConstants.BootType.UEFI.toString()) == null) { + return; + } + Long hostId = vm.getHostId() != null ? vm.getHostId() : vm.getLastHostId(); + if (hostId == null) { + logger.debug("No host recorded for UEFI VM [{}]; skipping UEFI NVRAM cleanup.", vm); + return; + } + DeleteUefiNvramCommand command = new DeleteUefiNvramCommand(vm.getInstanceName(), vm.getUuid()); + command.setWait(UEFI_NVRAM_CLEANUP_TIMEOUT_SECONDS); + Answer answer = agentMgr.easySend(hostId, command); + if (answer == null || !answer.getResult()) { + logger.warn("Unable to delete the UEFI NVRAM file of expunged VM [{}] on host [{}]; it has to be removed manually. Details: {}", + vm, hostId, answer == null ? "host unavailable" : answer.getDetails()); + return; + } + logger.debug("UEFI NVRAM cleanup for expunged VM [{}] on host [{}]: {}", vm, hostId, answer.getDetails()); + } catch (RuntimeException e) { + logger.warn("Unexpected error while cleaning up the UEFI NVRAM file of expunged VM [{}]; it has to be removed manually.", vm, e); + } + } + @Override public Map getClusterSettings(long vmId) { return null; diff --git a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java index e42acd90e884..d5b711832429 100644 --- a/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java +++ b/server/src/test/java/com/cloud/hypervisor/KVMGuruTest.java @@ -43,6 +43,9 @@ import org.mockito.Spy; import org.mockito.junit.MockitoJUnitRunner; +import com.cloud.agent.AgentManager; +import com.cloud.agent.api.Answer; +import com.cloud.agent.api.DeleteUefiNvramCommand; import com.cloud.agent.api.to.NicTO; import com.cloud.agent.api.to.VirtualMachineTO; import com.cloud.configuration.ConfigurationManagerImpl; @@ -66,6 +69,8 @@ import com.cloud.storage.dao.GuestOSHypervisorDao; import com.cloud.utils.Pair; import com.cloud.vm.dao.VMInstanceDao; +import com.cloud.vm.dao.VMInstanceDetailsDao; +import com.cloud.vm.VMInstanceDetailVO; import com.cloud.vm.VMInstanceVO; import com.cloud.vm.VirtualMachine; import com.cloud.vm.VirtualMachineProfile; @@ -85,6 +90,10 @@ public class KVMGuruTest { VMInstanceDao _instanceDao; @Mock VolumeDao _volumeDao; + @Mock + AgentManager agentMgr; + @Mock + VMInstanceDetailsDao vmInstanceDetailsDao; @Spy @InjectMocks @@ -639,4 +648,77 @@ public void testImportVirtualMachineFromBackupDoesNotReinstateReservedDeviceIds( // device id 4 is reserved, so the volume gets the next free one Mockito.verify(_volumeDao).attachVolume(11L, 42L, 5L); } + + private VirtualMachine mockExpungedVm(VirtualMachine.Type type, boolean uefi, Long lastHostId) { + VirtualMachine expungedVm = Mockito.mock(VirtualMachine.class); + Mockito.lenient().when(expungedVm.getId()).thenReturn(42L); + Mockito.lenient().when(expungedVm.getType()).thenReturn(type); + Mockito.lenient().when(expungedVm.getHostId()).thenReturn(null); + Mockito.lenient().when(expungedVm.getLastHostId()).thenReturn(lastHostId); + Mockito.lenient().when(expungedVm.getUuid()).thenReturn("5f1c3d2e-0b7a-4c55-9f4e-2a6b8d9c1e3f"); + Mockito.lenient().when(expungedVm.getInstanceName()).thenReturn("i-2-42-VM"); + Mockito.lenient().when(vmInstanceDetailsDao.findDetail(42L, ApiConstants.BootType.UEFI.toString())) + .thenReturn(uefi ? Mockito.mock(VMInstanceDetailVO.class) : null); + return expungedVm; + } + + @Test + public void testFinalizeExpungeDeletesUefiNvramOnLastHostAndReturnsNoCommands() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.User, true, 7L); + Answer answer = Mockito.mock(Answer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(agentMgr.easySend(Mockito.eq(7L), Mockito.any(DeleteUefiNvramCommand.class))).thenReturn(answer); + + Assert.assertNull(guru.finalizeExpunge(expungedVm)); + + Mockito.verify(agentMgr).easySend(Mockito.eq(7L), Mockito.argThat(command -> command instanceof DeleteUefiNvramCommand + && "5f1c3d2e-0b7a-4c55-9f4e-2a6b8d9c1e3f".equals(((DeleteUefiNvramCommand) command).getVmUuid()) + && "i-2-42-VM".equals(((DeleteUefiNvramCommand) command).getVmName()) + && command.getWait() == KVMGuru.UEFI_NVRAM_CLEANUP_TIMEOUT_SECONDS)); + } + + @Test + public void testDeleteUefiNvramIfNeededSkipsNonUefiVm() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.User, false, 7L); + + guru.deleteUefiNvramIfNeeded(expungedVm); + + Mockito.verify(agentMgr, Mockito.never()).easySend(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testDeleteUefiNvramIfNeededSkipsSystemVm() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.DomainRouter, true, 7L); + + guru.deleteUefiNvramIfNeeded(expungedVm); + + Mockito.verify(vmInstanceDetailsDao, Mockito.never()).findDetail(Mockito.anyLong(), Mockito.anyString()); + Mockito.verify(agentMgr, Mockito.never()).easySend(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testDeleteUefiNvramIfNeededSkipsVmWithoutHost() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.User, true, null); + + guru.deleteUefiNvramIfNeeded(expungedVm); + + Mockito.verify(agentMgr, Mockito.never()).easySend(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void testDeleteUefiNvramIfNeededDoesNotFailWhenHostIsUnavailable() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.User, true, 7L); + Mockito.when(agentMgr.easySend(Mockito.eq(7L), Mockito.any(DeleteUefiNvramCommand.class))).thenReturn(null); + + Assert.assertNull(guru.finalizeExpunge(expungedVm)); + } + + @Test + public void testDeleteUefiNvramIfNeededDoesNotFailOnUnexpectedError() { + VirtualMachine expungedVm = mockExpungedVm(VirtualMachine.Type.User, true, 7L); + Mockito.when(vmInstanceDetailsDao.findDetail(42L, ApiConstants.BootType.UEFI.toString())).thenThrow(new RuntimeException("db down")); + + Assert.assertNull(guru.finalizeExpunge(expungedVm)); + Mockito.verify(agentMgr, Mockito.never()).easySend(Mockito.anyLong(), Mockito.any()); + } }