KVM: remove leftover conversion XML and UEFI NVRAM files

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;
+    }
+}

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<DeleteUefiNvramCommand, Answer, LibvirtComputingResource> {
+
+    @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);
+                }
+            }
+        }
+    }
+}

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;

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);
+    }
+}