veeam-kvm: fix issues; improvements

api/src/main/java/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java

@@ -67,6 +67,14 @@ public Long getNicId() {
         return nicId;
     }
 
+    public void setVmId(Long vmId) {
+        this.vmId = vmId;
+    }
+
+    public void setNicId(Long nicId) {
+        this.nicId = nicId;
+    }
+
     /////////////////////////////////////////////////////
     /////////////// API Implementation///////////////////
     /////////////////////////////////////////////////////

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java

@@ -20,6 +20,8 @@
 import java.io.File;
 import java.io.FileWriter;
 import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Paths;
 import java.util.HashMap;
 import java.util.Map;
 
@@ -32,6 +34,7 @@
 import org.json.JSONArray;
 import org.json.JSONObject;
 import org.libvirt.Domain;
+import org.libvirt.DomainJobInfo;
 import org.libvirt.LibvirtException;
 import com.cloud.agent.api.Answer;
 import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
@@ -44,6 +47,9 @@
 public class LibvirtStartBackupCommandWrapper extends CommandWrapper<StartBackupCommand, Answer, LibvirtComputingResource> {
     protected Logger logger = LogManager.getLogger(getClass());
 
+    /** virDomainJobType VIR_DOMAIN_JOB_NONE. */
+    static final int NO_JOB = 0;
+
     @Override
     public Answer execute(StartBackupCommand cmd, LibvirtComputingResource resource) {
         if (cmd.isStoppedVM()) {
@@ -72,8 +78,15 @@ public Answer handleRunningVmBackup(StartBackupCommand cmd, LibvirtComputingReso
                 dir.mkdirs();
             }
 
+            Map<String, String> diskPathLabelMap = resource.getDiskPathLabelMap(vmName);
+            Map<String, String> diskPathScratchFileMap = getScratchFiles(cmd.getDiskPathUuidMap(), diskPathLabelMap);
+            String scratchError = prepareScratchFiles(vmName, diskPathScratchFileMap, resource);
+            if (scratchError != null) {
+                return new StartBackupAnswer(cmd, false, scratchError);
+            }
+
             // Create backup XML
-            String backupXml = createBackupXml(cmd, fromCheckpointId, socket, resource);
+            String backupXml = createBackupXml(cmd, fromCheckpointId, socket, diskPathLabelMap, diskPathScratchFileMap, resource);
             String checkpointXml = createCheckpointXml(toCheckpointId);
 
             // Write XMLs to temp files
@@ -158,7 +171,8 @@ private String createCheckpointXmlForRedefine(String checkpointName, Long create
         return xml.toString();
     }
 
-    private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId, String socket, LibvirtComputingResource resource) throws LibvirtException {
+    private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId, String socket, Map<String, String> diskPathLabelMap,
+                                   Map<String, String> diskPathScratchFileMap, LibvirtComputingResource resource) throws LibvirtException {
         StringBuilder xml = new StringBuilder();
         xml.append("<domainbackup mode=\"pull\">\n");
 
@@ -167,7 +181,6 @@ private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId,
         xml.append("  <disks>\n");
 
         Map<String, String> diskPathUuidMap = cmd.getDiskPathUuidMap();
-        Map<String, String> diskPathLabelMap = resource.getDiskPathLabelMap(cmd.getVmName());
         Map<String, Boolean> diskPathHasFromCheckpointMap = new HashMap<>();
         if (StringUtils.isNotBlank(fromCheckpointId)) {
             Domain vm = null;
@@ -193,7 +206,7 @@ private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId,
             }
             String diskName = entry.getValue();
             String export = diskPathUuidMap.get(diskPath);
-            String scratchFile = "/var/tmp/scratch-" + export + ".qcow2";
+            String scratchFile = diskPathScratchFileMap.get(diskPath);
             xml.append("    <disk name=\"").append(diskName).append("\" type=\"file\" exportname=\"").append(export);
             if (StringUtils.isNotBlank(fromCheckpointId) && Boolean.TRUE.equals(diskPathHasFromCheckpointMap.get(diskPath))) {
                 xml.append("\" backupmode=\"incremental\"")
@@ -211,6 +224,79 @@ private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId,
         return xml.toString();
     }
 
+     // Scratch file of each disk in the backup next to the disk on its primary storage
+    static Map<String, String> getScratchFiles(Map<String, String> diskPathUuidMap, Map<String, String> diskPathLabelMap) {
+        Map<String, String> scratchFiles = new HashMap<>();
+        for (Map.Entry<String, String> entry : diskPathUuidMap.entrySet()) {
+            if (!diskPathLabelMap.containsKey(entry.getKey())) {
+                continue;
+            }
+            String directory = new File(entry.getKey()).getParent();
+            scratchFiles.put(entry.getKey(), new File(directory, "backup-scratch-" + entry.getValue() + ".qcow2").getPath());
+        }
+        return scratchFiles;
+    }
+
+    String prepareScratchFiles(String vmName, Map<String, String> diskPathScratchFileMap, LibvirtComputingResource resource) {
+        Domain dm = null;
+        try {
+            dm = resource.getDomain(resource.getLibvirtUtilitiesHelper().getConnection(), vmName);
+            return prepareScratchFiles(dm, diskPathScratchFileMap);
+        } catch (LibvirtException e) {
+            return "Failed to prepare the backup scratch files: " + e.getMessage();
+        } finally {
+            if (dm != null) {
+                try {
+                    dm.free();
+                } catch (LibvirtException e) {
+                    logger.debug("Failed to free domain {}", vmName, e);
+                }
+            }
+        }
+    }
+
+    String prepareScratchFiles(Domain dm, Map<String, String> diskPathScratchFileMap) throws LibvirtException {
+        DomainJobInfo job = dm.getJobInfo();
+        if (job != null && job.getType() != NO_JOB) {
+            // libvirt refuses a backup anyway; never touch the scratch files of a running one.
+            return "The VM has an active job, a backup cannot start";
+        }
+        Map<String, Long> requiredSpace = new HashMap<>();
+        for (Map.Entry<String, String> entry : diskPathScratchFileMap.entrySet()) {
+            String scratchFile = entry.getValue();
+            if (!deleteStaleScratchFile(scratchFile)) {
+                return "Failed to remove the stale backup scratch file " + scratchFile;
+            }
+            long capacity = dm.blockInfo(entry.getKey()).getCapacity();
+            // 1% on top of the virtual size covers the qcow2 metadata of the scratch file.
+            requiredSpace.merge(new File(scratchFile).getParent(), capacity + capacity / 100, Long::sum);
+        }
+        for (Map.Entry<String, Long> entry : requiredSpace.entrySet()) {
+            long usable = getUsableSpace(entry.getKey());
+            if (usable < entry.getValue()) {
+                return String.format("Not enough free space for the backup scratch files in %s: %d bytes needed, %d available",
+                        entry.getKey(), entry.getValue(), usable);
+            }
+        }
+        return null;
+    }
+
+    boolean deleteStaleScratchFile(String scratchFile) {
+        try {
+            if (Files.deleteIfExists(Paths.get(scratchFile))) {
+                logger.warn("Removed the stale backup scratch file {}", scratchFile);
+            }
+            return true;
+        } catch (IOException e) {
+            logger.error("Failed to remove the stale backup scratch file {}", scratchFile, e);
+            return false;
+        }
+    }
+
+    long getUsableSpace(String directory) {
+        return new File(directory).getUsableSpace();
+    }
+
     private String createCheckpointXml(String checkpointId) {
         return "<domaincheckpoint>\n" +
                "  <name>" + checkpointId + "</name>\n" +

plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapperTest.java

@@ -16,14 +16,25 @@
 // 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.Comparator;
+import java.util.List;
 import java.util.Map;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 import org.apache.cloudstack.backup.StartBackupAnswer;
 import org.apache.cloudstack.backup.StartBackupCommand;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
+import org.libvirt.Domain;
+import org.libvirt.DomainBlockInfo;
+import org.libvirt.DomainJobInfo;
+import org.libvirt.LibvirtException;
 import org.mockito.MockedConstruction;
 import org.mockito.Mockito;
 import org.mockito.junit.MockitoJUnitRunner;
@@ -41,7 +52,8 @@ public class LibvirtStartBackupCommandWrapperTest {
 
     @Before
     public void setUp() {
-        wrapper = new LibvirtStartBackupCommandWrapper();
+        wrapper = Mockito.spy(new LibvirtStartBackupCommandWrapper());
+        Mockito.lenient().doReturn(null).when(wrapper).prepareScratchFiles(Mockito.any(), Mockito.any(), Mockito.any(LibvirtComputingResource.class));
         command = Mockito.mock(StartBackupCommand.class);
         resource = Mockito.mock(LibvirtComputingResource.class);
     }
@@ -171,12 +183,134 @@ public void testExecuteRunningVmCreateBackupXmlExceptionReturnsFailure() {
         Mockito.when(command.getToCheckpointId()).thenReturn("cp-running-5");
         Mockito.when(command.getFromCheckpointId()).thenReturn(null);
         Mockito.when(command.getSocket()).thenReturn("sock-5");
-        Mockito.when(command.getDiskPathUuidMap()).thenReturn(Map.of("/path/disk1.qcow2", "vol-1"));
         Mockito.when(resource.getDiskPathLabelMap("i-2-3-VM")).thenThrow(new RuntimeException("disk labels unavailable"));
 
         Answer answer = wrapper.execute(command, resource);
 
         Assert.assertFalse(answer.getResult());
         Assert.assertTrue(answer.getDetails().contains("Error starting backup: disk labels unavailable"));
     }
+
+    @Test
+    public void testScratchFilesGoNextToTheDisk() {
+        Map<String, String> scratchFiles = LibvirtStartBackupCommandWrapper.getScratchFiles(
+                Map.of("/mnt/pool-1/vol-path-1", "vol-uuid-1", "/var/lib/libvirt/images/vol-path-2", "vol-uuid-2"),
+                Map.of("/mnt/pool-1/vol-path-1", "vda", "/var/lib/libvirt/images/vol-path-2", "vdb"));
+
+        Assert.assertEquals("/mnt/pool-1/backup-scratch-vol-uuid-1.qcow2", scratchFiles.get("/mnt/pool-1/vol-path-1"));
+        Assert.assertEquals("/var/lib/libvirt/images/backup-scratch-vol-uuid-2.qcow2", scratchFiles.get("/var/lib/libvirt/images/vol-path-2"));
+    }
+
+    @Test
+    public void testScratchFilesOnlyForDisksInTheBackup() {
+        Map<String, String> scratchFiles = LibvirtStartBackupCommandWrapper.getScratchFiles(
+                Map.of("/mnt/pool-1/vol-path-1", "vol-uuid-1", "/mnt/pool-1/vol-path-2", "vol-uuid-2"),
+                Map.of("/mnt/pool-1/vol-path-1", "vda"));
+
+        Assert.assertEquals(List.of("/mnt/pool-1/vol-path-1"), List.copyOf(scratchFiles.keySet()));
+    }
+
+    private static Domain domainWithDisks(int jobType, Map<String, Long> capacities) throws LibvirtException {
+        Domain dm = Mockito.mock(Domain.class);
+        DomainJobInfo job = Mockito.mock(DomainJobInfo.class);
+        Mockito.when(job.getType()).thenReturn(jobType);
+        Mockito.when(dm.getJobInfo()).thenReturn(job);
+        for (Map.Entry<String, Long> entry : capacities.entrySet()) {
+            DomainBlockInfo info = Mockito.mock(DomainBlockInfo.class);
+            Mockito.lenient().when(info.getCapacity()).thenReturn(entry.getValue());
+            Mockito.lenient().when(dm.blockInfo(entry.getKey())).thenReturn(info);
+        }
+        return dm;
+    }
+
+    private static Map<String, String> scratchFilesFor(Map<String, String> diskPathUuidMap) {
+        return LibvirtStartBackupCommandWrapper.getScratchFiles(diskPathUuidMap,
+                diskPathUuidMap.keySet().stream().collect(Collectors.toMap(path -> path, path -> "vd")));
+    }
+
+    @Test
+    public void testPrepareScratchFilesRemovesStaleFile() throws Exception {
+        Path pool = Files.createTempDirectory("pool");
+        try {
+            String disk = pool.resolve("vol-path-1").toString();
+            Map<String, String> scratchFiles = scratchFilesFor(Map.of(disk, "vol-uuid-1"));
+            Path stale = Path.of(scratchFiles.get(disk));
+            Files.writeString(stale, "left by a crashed backup");
+            Mockito.doReturn(10L * 1024 * 1024 * 1024).when(wrapper).getUsableSpace(pool.toString());
+
+            String error = wrapper.prepareScratchFiles(domainWithDisks(LibvirtStartBackupCommandWrapper.NO_JOB, Map.of(disk, 1024L * 1024 * 1024)), scratchFiles);
+
+            Assert.assertNull(error);
+            Assert.assertFalse(Files.exists(stale));
+        } finally {
+            deleteRecursively(pool);
+        }
+    }
+
+    @Test
+    public void testPrepareScratchFilesNeedsRoomForAllDisksInTheDirectory() throws Exception {
+        Path pool = Files.createTempDirectory("pool");
+        try {
+            String disk1 = pool.resolve("vol-path-1").toString();
+            String disk2 = pool.resolve("vol-path-2").toString();
+            long tenGiB = 10L * 1024 * 1024 * 1024;
+            Map<String, String> scratchFiles = scratchFilesFor(Map.of(disk1, "vol-uuid-1", disk2, "vol-uuid-2"));
+            // Room for one disk's worst case, not for both.
+            Mockito.doReturn(15L * 1024 * 1024 * 1024).when(wrapper).getUsableSpace(pool.toString());
+
+            String error = wrapper.prepareScratchFiles(domainWithDisks(LibvirtStartBackupCommandWrapper.NO_JOB, Map.of(disk1, tenGiB, disk2, tenGiB)), scratchFiles);
+
+            Assert.assertNotNull(error);
+            Assert.assertTrue(error.contains("Not enough free space"));
+            Assert.assertTrue(error.contains(pool.toString()));
+        } finally {
+            deleteRecursively(pool);
+        }
+    }
+
+    @Test
+    public void testPrepareScratchFilesLeavesFilesOfAnActiveJobAlone() throws Exception {
+        Path pool = Files.createTempDirectory("pool");
+        try {
+            String disk = pool.resolve("vol-path-1").toString();
+            Map<String, String> scratchFiles = scratchFilesFor(Map.of(disk, "vol-uuid-1"));
+            Path inUse = Path.of(scratchFiles.get(disk));
+            Files.writeString(inUse, "scratch of a running backup");
+            int unboundedJob = 2;
+
+            String error = wrapper.prepareScratchFiles(domainWithDisks(unboundedJob, Map.of(disk, 1024L)), scratchFiles);
+
+            Assert.assertNotNull(error);
+            Assert.assertTrue(Files.exists(inUse));
+        } finally {
+            deleteRecursively(pool);
+        }
+    }
+
+    @Test
+    public void testExecuteRunningVmScratchFailureDoesNotBeginBackup() {
+        Mockito.when(command.isStoppedVM()).thenReturn(false);
+        Mockito.when(command.getVmName()).thenReturn("i-2-3-VM");
+        Mockito.when(command.getFromCheckpointId()).thenReturn(null);
+        Mockito.when(command.getDiskPathUuidMap()).thenReturn(Map.of("/path/disk1.qcow2", "vol-1"));
+        Mockito.when(resource.getDiskPathLabelMap("i-2-3-VM")).thenReturn(Map.of("/path/disk1.qcow2", "vda"));
+        Mockito.doReturn("Not enough free space for the backup scratch files in /path").when(wrapper)
+                .prepareScratchFiles(Mockito.eq("i-2-3-VM"), Mockito.any(), Mockito.any(LibvirtComputingResource.class));
+
+        try (MockedConstruction<Script> scripts = Mockito.mockConstruction(Script.class)) {
+            Answer answer = wrapper.execute(command, resource);
+
+            Assert.assertFalse(answer.getResult());
+            Assert.assertTrue(answer.getDetails().contains("Not enough free space"));
+            Assert.assertTrue(scripts.constructed().isEmpty());
+        }
+    }
+
+    private static void deleteRecursively(Path dir) throws IOException {
+        try (Stream<Path> paths = Files.walk(dir)) {
+            for (Path path : paths.sorted(Comparator.reverseOrder()).collect(Collectors.toList())) {
+                Files.deleteIfExists(path);
+            }
+        }
+    }
 }