From 8a5e630532bc75febb282f73d84c1e264be3a581 Mon Sep 17 00:00:00 2001 From: Andrija Panic Date: Fri, 30 Oct 2026 08:27:18 +0100 Subject: [PATCH 1/2] kvm: remove the converted domain XML after an in-place import When a VMware VM is converted straight into file-based primary storage (forceconverttopool), the converted disks are used where they are, but the .xml file written by virt-v2v was never removed. It stayed in the storage pool even after the VM was expunged. Delete that one file once the converted instance has been read successfully. On failure it is kept, because the existing cleanup command reads it to find the converted disks. --- ...ImportConvertedInstanceCommandWrapper.java | 18 +++++ ...rtConvertedInstanceCommandWrapperTest.java | 76 +++++++++++++++++++ 2 files changed, 94 insertions(+) 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/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()); + } } From f30fdd1cda94b437ecda3ec30936dcc452441fef Mon Sep 17 00:00:00 2001 From: Andrija Panic Date: Fri, 30 Oct 2026 08:27:19 +0100 Subject: [PATCH 2/2] kvm: delete the UEFI NVRAM file when a VM is expunged Every UEFI VM has a .fd NVRAM file on its KVM host. It was never removed, so it stayed on the host after the VM was expunged. On expunge, ask the host the VM last ran on to delete it. This is best effort: it never fails the expunge and waits at most 60 seconds. The host only deletes the file of a valid VM UUID, and keeps it if a domain with that UUID is still defined there. --- .../agent/api/DeleteUefiNvramCommand.java | 49 +++++ .../LibvirtDeleteUefiNvramCommandWrapper.java | 113 +++++++++++ ...virtDeleteUefiNvramCommandWrapperTest.java | 179 ++++++++++++++++++ .../java/com/cloud/hypervisor/KVMGuru.java | 45 +++++ .../com/cloud/hypervisor/KVMGuruTest.java | 82 ++++++++ 5 files changed, 468 insertions(+) create mode 100644 core/src/main/java/com/cloud/agent/api/DeleteUefiNvramCommand.java create mode 100644 plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapper.java create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtDeleteUefiNvramCommandWrapperTest.java 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/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/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()); + } }