diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java index cfbc64339909..356293859646 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vm/RemoveNicFromVMCmd.java +++ b/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/////////////////// ///////////////////////////////////////////////////// diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java index 629fd179afb2..9835786296eb 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapper.java +++ b/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 { 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 diskPathLabelMap = resource.getDiskPathLabelMap(vmName); + Map 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 diskPathLabelMap, + Map diskPathScratchFileMap, LibvirtComputingResource resource) throws LibvirtException { StringBuilder xml = new StringBuilder(); xml.append("\n"); @@ -167,7 +181,6 @@ private String createBackupXml(StartBackupCommand cmd, String fromCheckpointId, xml.append(" \n"); Map diskPathUuidMap = cmd.getDiskPathUuidMap(); - Map diskPathLabelMap = resource.getDiskPathLabelMap(cmd.getVmName()); Map 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(" getScratchFiles(Map diskPathUuidMap, Map diskPathLabelMap) { + Map scratchFiles = new HashMap<>(); + for (Map.Entry 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 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 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 requiredSpace = new HashMap<>(); + for (Map.Entry 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 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 "\n" + " " + checkpointId + "\n" + diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapperTest.java index 7932e90f47a0..c9df5665448f 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtStartBackupCommandWrapperTest.java @@ -16,7 +16,14 @@ // 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; @@ -24,6 +31,10 @@ 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,7 +183,6 @@ 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); @@ -179,4 +190,127 @@ public void testExecuteRunningVmCreateBackupXmlExceptionReturnsFailure() { Assert.assertFalse(answer.getResult()); Assert.assertTrue(answer.getDetails().contains("Error starting backup: disk labels unavailable")); } + + @Test + public void testScratchFilesGoNextToTheDisk() { + Map 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 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 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 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 scratchFilesFor(Map 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 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 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 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