From 2c7ea81155926bbed4b9fb6e7d05300f6fc6273a Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Wed, 26 Aug 2026 13:00:08 +0300 Subject: [PATCH 1/4] Backup: support StorPool volumes in NAS backup provider on KVM Volume pool/path info is now sent to the agent whenever a VM has a StorPool volume, not just when the VM looks Stopped, since nasbackup.sh re-checks the VM's actual liveness itself right before acting and needs the StorPool path to clone a backup source disk if it finds the VM already stopped. - StorPoolStorageAdaptor.createPhysicalDisk() now creates and attaches a StorPool volume (previously a no-op returning null), used when restoring a backup provisions a new volume. - nasbackup.sh clones the live StorPool volume into a point-in-time volume before reading from it for a cold backup, and cleans it up afterwards (with an EXIT trap as a safety net). - Backup size is now reported via an explicit BACKUP_SIZE_TOTAL= marker on stdout instead of being inferred from output position/shape, which broke down once StorPool added a third code shape; take-backup errors are now always returned as a BackupAnswer rather than letting an uncaught exception surface as a plain Answer. - Restore reports back the volume path StorPool actually assigned (BackupAnswer.restoredVolumePath) instead of the path CloudStack guessed, since StorPool controls the device path itself. - Backup file identifiers are normalized to the qcow2 basename so StorPool's full device path matches what nasbackup.sh expects. --- .../cloudstack/backup/BackupAnswer.java | 12 + .../cloudstack/backup/NASBackupProvider.java | 60 +- .../backup/NASBackupProviderTest.java | 159 +++++ .../LibvirtRestoreBackupCommandWrapper.java | 68 +- .../LibvirtTakeBackupCommandWrapper.java | 136 ++-- .../kvm/storage/StorPoolStorageAdaptor.java | 60 +- scripts/vm/hypervisor/kvm/nasbackup.sh | 173 ++++- .../plugins/storpool/TestNasBackupStorPool.py | 650 ++++++++++++++++++ test/integration/plugins/storpool/sp_util.py | 70 +- 9 files changed, 1270 insertions(+), 118 deletions(-) create mode 100644 test/integration/plugins/storpool/TestNasBackupStorPool.py diff --git a/core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java b/core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java index abe78ee5553d..9ffc4dd04462 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java +++ b/core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java @@ -35,6 +35,10 @@ public class BackupAnswer extends Answer { // Set when an incremental was requested but the agent had to fall back to a full // (e.g. VM was stopped). Provider should record this backup as type=full. private Boolean incrementalFallback; + // Set when restore provisions a volume on storage that assigns its own path/ID (e.g. + // StorPool), so the provider doesn't have to guess it from the CloudStack volume UUID. + // Null when the guessed path is already correct (e.g. RBD/Linstor/NFS). + private String restoredVolumePath; public BackupAnswer(final Command command, final boolean success, final String details) { super(command, success, details); @@ -91,4 +95,12 @@ public void setIncrementalFallback(Boolean incrementalFallback) { this.incrementalFallback = incrementalFallback; } + public String getRestoredVolumePath() { + return restoredVolumePath; + } + + public void setRestoredVolumePath(String restoredVolumePath) { + this.restoredVolumePath = restoredVolumePath; + } + } diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index 57244305ebba..20a7c5087cc5 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -396,6 +396,24 @@ protected boolean allVolumesOnCheckpointCapableStorage(VirtualMachine vm) { return true; } + /** + * True when any of the VM's volumes sits on a StorPool pool. Used to decide whether volume + * pool/path info must be sent to the agent even though the VM currently looks Running — see + * the caller in {@link #takeBackup}. + */ + private boolean hasStorPoolVolume(List volumes) { + if (volumes == null) { + return false; + } + for (VolumeVO volume : volumes) { + StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId()); + if (pool != null && Storage.StoragePoolType.StorPool.equals(pool.getPoolType())) { + return true; + } + } + return false; + } + /** * Read the {@code nas.active_checkpoint_id} VM detail. Returns {@code null} when no detail * exists (post-restore, first backup, or after explicit reset). @@ -602,8 +620,20 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce command.setBitmapParent(decision.bitmapParent); command.setParentPaths(decision.parentPaths); - if (VirtualMachine.State.Stopped.equals(vm.getState())) { - List vmVolumes = volumeDao.findByInstance(vm.getId()); + // Always sent (not just for a Stopped VM) when any volume is on StorPool: nasbackup.sh + // re-checks the VM's actual liveness itself right before acting, and that fresh check can + // disagree with this state read taken here. If it does and the VM turns out to be + // stopped, nasbackup.sh needs the StorPool volume path to clone a backup source disk from + // — without it there is nothing to derive that from. + boolean vmStopped = VirtualMachine.State.Stopped.equals(vm.getState()); + List vmVolumes = volumeDao.findByInstance(vm.getId()); + boolean hasStorPoolVolume = hasStorPoolVolume(vmVolumes); + if (vmStopped || hasStorPoolVolume) { + if (!vmStopped) { + logger.debug("VM {} looks {} but has a StorPool volume — sending volume pool/path info to the agent anyway, " + + "in case nasbackup.sh's own liveness check finds it already stopped by execution time", + vm.getInstanceName(), vm.getState()); + } vmVolumes.sort(Comparator.comparing(Volume::getDeviceId)); Pair, List> volumePoolsAndPaths = getVolumePoolsAndPaths(vmVolumes); command.setVolumePools(volumePoolsAndPaths.first()); @@ -773,11 +803,18 @@ private Pair restoreVMBackup(VirtualMachine vm, Backup backup) private List getBackupFiles(List backedVolumes) { List backupFiles = new ArrayList<>(); for (Backup.VolumeInfo backedVolume : backedVolumes) { - backupFiles.add(backedVolume.getPath()); + backupFiles.add(getBackupFileIdentifier(backedVolume.getPath())); } return backupFiles; } + // nasbackup.sh names each qcow2 after the basename of the volume's disk path. No-op for + // most drivers (bare volume path already); reduces StorPool's full device path to match. + private String getBackupFileIdentifier(String volumePath) { + int idx = volumePath.lastIndexOf('/'); + return idx >= 0 ? volumePath.substring(idx + 1) : volumePath; + } + private Pair, List> getVolumePoolsAndPaths(List volumes) { List volumePools = new ArrayList<>(); List volumePaths = new ArrayList<>(); @@ -790,9 +827,13 @@ private Pair, List> getVolumePoolsAndPaths(List DataStore dataStore = dataStoreMgr.getDataStore(storagePool.getId(), DataStoreRole.Primary); volumePools.add(dataStore != null ? (PrimaryDataStoreTO)dataStore.getTO() : null); - String volumePathPrefix = getVolumePathPrefix(storagePool); - String volumePathSuffix = getVolumePathSuffix(storagePool); - volumePaths.add(String.format("%s%s%s", volumePathPrefix, volume.getPath(), volumePathSuffix)); + if (Storage.StoragePoolType.StorPool.equals(storagePool.getPoolType())) { + volumePaths.add(volume.getPath()); + } else { + String volumePathPrefix = getVolumePathPrefix(storagePool); + String volumePathSuffix = getVolumePathSuffix(storagePool); + volumePaths.add(String.format("%s%s%s", volumePathPrefix, volume.getPath(), volumePathSuffix)); + } } return new Pair<>(volumePools, volumePaths); } @@ -805,6 +846,8 @@ private String getVolumePathPrefix(StoragePoolVO storagePool) { volumePathPrefix = storagePool.getPath() + "/"; } else if (Storage.StoragePoolType.Linstor.equals(storagePool.getPoolType())) { volumePathPrefix = "/dev/drbd/by-res/cs-"; + } else if (Storage.StoragePoolType.StorPool.equals(storagePool.getPoolType())) { + volumePathPrefix = storagePool.getPath(); } else { // Should be Storage.StoragePoolType.NetworkFilesystem volumePathPrefix = String.format("/mnt/%s/", storagePool.getUuid()); @@ -875,7 +918,7 @@ public Pair restoreBackedUpVolume(Backup backup, Backup.VolumeI restoreCommand.setVmExists(null); restoreCommand.setVmState(vmNameAndState.second()); restoreCommand.setMountTimeout(NASBackupRestoreMountTimeout.value()); - restoreCommand.setBackupFiles(Collections.singletonList(matchingVolume.getPath())); + restoreCommand.setBackupFiles(Collections.singletonList(getBackupFileIdentifier(matchingVolume.getPath()))); BackupAnswer answer; try { @@ -887,6 +930,9 @@ public Pair restoreBackedUpVolume(Backup backup, Backup.VolumeI } if (answer.getResult()) { + if (answer.getRestoredVolumePath() != null) { + restoredVolume.setPath(answer.getRestoredVolumePath()); + } try { volumeDao.persist(restoredVolume); } catch (Exception e) { diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index 9e3d7b77c152..ce3ac5ea8db0 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.Objects; import java.util.Optional; +import java.util.UUID; import com.cloud.vm.snapshot.dao.VMSnapshotDao; import org.junit.Assert; @@ -633,6 +634,164 @@ public void restoreClearsActiveCheckpointDetail() throws AgentUnavailableExcepti Mockito.verify(vmInstanceDetailsDao).removeDetail(vmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID); } + private VMInstanceVO mockActiveVm(Long vmId, Long hostId, String name) { + VMInstanceVO vm = mock(VMInstanceVO.class); + Mockito.when(vm.getId()).thenReturn(vmId); + Mockito.when(vm.getLastHostId()).thenReturn(hostId); + Mockito.when(vm.getRemoved()).thenReturn(null); + Mockito.when(vm.getName()).thenReturn(name); + return vm; + } + + private void mockActiveHostById(Long hostId) { + HostVO host = mock(HostVO.class); + Mockito.when(host.getStatus()).thenReturn(Status.Up); + Mockito.when(host.getId()).thenReturn(hostId); + Mockito.when(hostDao.findById(hostId)).thenReturn(host); + } + + private void mockHostByIp(String hostIp, Long hostId) { + HostVO host = mock(HostVO.class); + Mockito.when(host.getId()).thenReturn(hostId); + Mockito.when(hostDao.findByIp(hostIp)).thenReturn(host); + } + + private BackupVO mockBackup(Long vmId, Long backupOfferingId, String externalId, Long id) { + BackupVO backup = new BackupVO(); + backup.setVmId(vmId); + backup.setBackupOfferingId(backupOfferingId); + backup.setExternalId(externalId); + ReflectionTestUtils.setField(backup, "id", id); + return backup; + } + + private BackupVO mockBackupWithVolume(Long vmId, Long backupOfferingId, String externalId, + long size, Backup.VolumeInfo backedUp, Long id) { + BackupVO backup = mockBackup(vmId, backupOfferingId, externalId, id); + backup.setSize(size); + backup.setBackedUpVolumes(new Gson().toJson(Collections.singletonList(backedUp))); + return backup; + } + + private void mockNasRepository(Long backupOfferingId) { + BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas", "test-repo", + "nfs", "address", "sync", 1024L, null); + Mockito.when(backupRepositoryDao.findByBackupOfferingId(backupOfferingId)).thenReturn(repo); + } + + private void mockStorPoolRootVolume(Long vmId, Long poolId, String devicePath) { + StoragePoolVO pool = mock(StoragePoolVO.class); + Mockito.when(pool.getId()).thenReturn(poolId); + Mockito.when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.StorPool); + Mockito.when(storagePoolDao.findById(poolId)).thenReturn(pool); + + VolumeVO rootVolume = mock(VolumeVO.class); + Mockito.when(rootVolume.getPoolId()).thenReturn(poolId); + Mockito.when(rootVolume.getPath()).thenReturn(devicePath); + Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(Collections.singletonList(rootVolume)); + } + + private void mockSourceVolume(String volUuid, String name) { + VolumeVO srcVolume = mock(VolumeVO.class); + Mockito.when(srcVolume.getUuid()).thenReturn(volUuid); + Mockito.when(srcVolume.getName()).thenReturn(name); + Mockito.when(volumeDao.findByUuid(volUuid)).thenReturn(srcVolume); + } + + private void mockThinDiskOffering(Long diskOfferingId) { + DiskOfferingVO diskOffering = mock(DiskOfferingVO.class); + Mockito.when(diskOffering.getId()).thenReturn(diskOfferingId); + Mockito.when(diskOffering.getProvisioningType()).thenReturn(Storage.ProvisioningType.THIN); + Mockito.when(diskOfferingDao.findByUuid(Mockito.anyString())).thenReturn(diskOffering); + } + + private void mockStorPoolPoolByUuid(String dsUuid, Long poolId) { + StoragePoolVO pool = mock(StoragePoolVO.class); + Mockito.when(pool.getId()).thenReturn(poolId); + Mockito.when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.StorPool); + Mockito.when(storagePoolDao.findByUuid(dsUuid)).thenReturn(pool); + } + + private BackupAnswer mockSuccessfulAgentSend() throws AgentUnavailableException, OperationTimedoutException { + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(true); + Mockito.when(agentManager.send(Mockito.anyLong(), Mockito.any(RestoreBackupCommand.class))).thenReturn(answer); + return answer; + } + + private void assertRestoreVolumePaths(List expectedPaths) throws AgentUnavailableException, OperationTimedoutException { + ArgumentCaptor captor = ArgumentCaptor.forClass(RestoreBackupCommand.class); + Mockito.verify(agentManager).send(Mockito.anyLong(), captor.capture()); + Assert.assertEquals(expectedPaths, captor.getValue().getRestoreVolumePaths()); + } + + private void assertPersistedVolume(Storage.ImageFormat expectedFormat, String expectedPath) { + ArgumentCaptor volumeCaptor = ArgumentCaptor.forClass(VolumeVO.class); + Mockito.verify(volumeDao).persist(volumeCaptor.capture()); + Assert.assertEquals(expectedFormat, volumeCaptor.getValue().getFormat()); + Assert.assertEquals(expectedPath, volumeCaptor.getValue().getPath()); + } + + /** + * StorPool stores the full device path in the volume's path column, so it must be sent as-is. + */ + @Test + public void restoreVMBackupDoesNotDoublePrefixStorPoolVolumePath() + throws AgentUnavailableException, OperationTimedoutException { + Long vmId = 21L; + Long hostId = 22L; + Long backupOfferingId = 23L; + Long poolId = 24L; + String storPoolDevicePath = "/dev/storpool-byid/t.t.t"; + + VMInstanceVO vm = mockActiveVm(vmId, hostId, "vm21"); + mockActiveHostById(hostId); + BackupVO backup = mockBackup(vmId, backupOfferingId, "i-2-21-VM/2026.06.01.10.00.00", 210L); + mockNasRepository(backupOfferingId); + mockStorPoolRootVolume(vmId, poolId, storPoolDevicePath); + mockSuccessfulAgentSend(); + + boolean ok = nasBackupProvider.restoreVMFromBackup(vm, backup, false, null); + Assert.assertTrue(ok); + + assertRestoreVolumePaths(Collections.singletonList(storPoolDevicePath)); + } + + /** + * StorPool provisions/names new volumes itself (a globalId), so the persisted path must + * come from BackupAnswer#getRestoredVolumePath, not the guessed UUID-based path. + */ + @Test + public void restoreBackedUpVolumeUsesQcow2FormatAndRealPathForStorPool() + throws AgentUnavailableException, OperationTimedoutException { + Long backupOfferingId = 33L; + Long poolId = 34L; + String volUuid = UUID.randomUUID().toString(); + String hostIp = "10.0.0.9"; + String dsUuid = UUID.randomUUID().toString(); + String realStorPoolPath = "/dev/storpool-byid/t.t.t"; + + mockSourceVolume(volUuid, "data-sp"); + mockThinDiskOffering(6L); + mockStorPoolPoolByUuid(dsUuid, poolId); + mockHostByIp(hostIp, 8L); + mockNasRepository(backupOfferingId); + + Backup.VolumeInfo backedUp = new Backup.VolumeInfo(volUuid, "i-2-99-VM/2026/data-sp.qcow2", + Volume.Type.DATADISK, 2048L, 1L, "disk-offering-uuid", null, null); + BackupVO backup = mockBackupWithVolume(99L, backupOfferingId, "i-2-99-VM/2026.06.22.10.00.00", + 2048L, backedUp, 330L); + + BackupAnswer answer = mockSuccessfulAgentSend(); + Mockito.when(answer.getRestoredVolumePath()).thenReturn(realStorPoolPath); + + Pair result = nasBackupProvider.restoreBackedUpVolume( + backup, backedUp, hostIp, dsUuid, new Pair<>("i-2-42-VM", VirtualMachine.State.Stopped), null, false); + Assert.assertTrue(result.first()); + + assertPersistedVolume(Storage.ImageFormat.QCOW2, realStorPoolPath); + } + /** * Single-volume restore (restoreBackedUpVolume) must also clear the target VM's * active_checkpoint_id, so the next backup of that VM is a fresh full — the restored diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java index ccd0ec634525..171d396f8efa 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java @@ -95,6 +95,7 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser List backupFiles = command.getBackupFiles(); String newVolumeId = null; + String actualRestoredVolumePath = null; try { String mountDirectory = mountBackupDirectory(backupRepoAddress, backupRepoType, mountOptions, mountTimeout); if (Objects.isNull(vmExists)) { @@ -103,8 +104,11 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser String backupFile = backupFiles.get(0); newVolumeId = getVolumeUuidFromPath(volumePath, volumePool); Long size = command.getRestoreVolumeSizes().get(0); - restoreVolume(storagePoolMgr, backupPath, volumePool, volumePath, diskType, backupFile, size, + String resolvedVolumePath = restoreVolume(storagePoolMgr, backupPath, volumePool, volumePath, diskType, backupFile, size, new Pair<>(vmName, command.getVmState()), mountDirectory, timeout, mountTimeout); + if (Storage.StoragePoolType.StorPool.equals(volumePool.getPoolType())) { + actualRestoredVolumePath = resolvedVolumePath; + } } else if (Boolean.TRUE.equals(vmExists)) { restoreVolumesOfExistingVM(storagePoolMgr, restoreVolumePools, restoreVolumePaths, backedVolumeUUIDs, backupPath, backupFiles, mountDirectory, timeout, mountTimeout); } else { @@ -115,7 +119,9 @@ public Answer execute(RestoreBackupCommand command, LibvirtComputingResource ser return new BackupAnswer(command, false, errorMessage); } - return new BackupAnswer(command, true, newVolumeId); + BackupAnswer answer = new BackupAnswer(command, true, newVolumeId); + answer.setRestoredVolumePath(actualRestoredVolumePath); + return answer; } private void verifyBackupFile(String backupPath, String volUuid) { @@ -173,7 +179,12 @@ private void restoreVolumesOfDestroyedVMs(KVMStoragePoolManager storagePoolMgr, } } - private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType, String backupFile, + /** + * @return the volume path the backup was actually written to. Equal to {@code volumePath} + * except when the pool assigns the created volume's identity itself (StorPool), in which + * case it reflects the real device path. + */ + private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType, String backupFile, Long size, Pair vmNameAndState, String mountDirectory, int timeout, Integer mountTimeout) { String bkpPath; String volumeUuid; @@ -181,15 +192,17 @@ private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String backupPa bkpPath = getBackupPath(mountDirectory, backupPath, backupFile, diskType); volumeUuid = getVolumeUuidFromPath(volumePath, volumePool); verifyBackupFile(bkpPath, volumeUuid); - if (!replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath, timeout, true, size)) { + Pair restoreResult = replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath, timeout, true, size); + if (!restoreResult.first()) { throw new CloudRuntimeException(String.format("Unable to restore contents from the backup volume [%s].", volumeUuid)); - } + volumePath = restoreResult.second(); if (VirtualMachine.State.Running.equals(vmNameAndState.second())) { if (!attachVolumeToVm(storagePoolMgr, vmNameAndState.first(), volumePool, volumePath)) { throw new CloudRuntimeException(String.format("Failed to attach volume to VM: %s", vmNameAndState.first())); } } + return volumePath; } finally { unmountBackupDirectory(mountDirectory, mountTimeout); deleteTemporaryDirectory(mountDirectory); @@ -293,11 +306,16 @@ private boolean checkBackupPathExists(String backupPath) { } private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout) { - return replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, backupPath, timeout, false, null); + return replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, backupPath, timeout, false, null).first(); } - private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout, boolean createTargetVolume, Long size) { - if (List.of(Storage.StoragePoolType.RBD, Storage.StoragePoolType.Linstor).contains(volumePool.getPoolType())) { + /** + * @return (success, the volume path the backup was actually written to). The path only + * differs from the input {@code volumePath} for pools that assign the created volume's + * identity themselves instead of accepting the one the caller proposed (StorPool). + */ + private Pair replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout, boolean createTargetVolume, Long size) { + if (List.of(Storage.StoragePoolType.RBD, Storage.StoragePoolType.Linstor, Storage.StoragePoolType.StorPool).contains(volumePool.getPoolType())) { return replaceBlockDeviceWithBackup(storagePoolMgr, volumePool, volumePath, backupPath, timeout, createTargetVolume, size); } @@ -310,12 +328,12 @@ private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, Pr if (hasBackingChain(backupPath)) { String[] qemuImgCmd = new String[] { Script.getExecutableAbsolutePath("qemu-img"), "convert", "-O", "qcow2", backupPath, volumePath }; int flattenExit = Script.executeCommandForExitValue(qemuImgCmd); - return flattenExit == 0; + return new Pair<>(flattenExit == 0, volumePath); } String[] rsyncCmd = new String[] { Script.getExecutableAbsolutePath("rsync"), "-az", backupPath, volumePath }; int exitValue = Script.executeCommandForExitValue(timeout, rsyncCmd); - return exitValue == 0; + return new Pair<>(exitValue == 0, volumePath); } private boolean hasBackingChain(String qcow2Path) { @@ -323,23 +341,32 @@ private boolean hasBackingChain(String qcow2Path) { String.format(QEMU_IMG_HAS_BACKING_COMMAND, qcow2Path)) == 0; } - private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout, boolean createTargetVolume, Long size) { + private Pair replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout, boolean createTargetVolume, Long size) { KVMStoragePool volumeStoragePool = storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid()); + Storage.StoragePoolType poolType = volumePool.getPoolType(); QemuImg qemu; try { qemu = new QemuImg(timeout, true, false); String volumeUuid = getVolumeUuidFromPath(volumePath, volumePool); KVMPhysicalDisk disk = null; if (createTargetVolume) { - if (Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) { + if (Storage.StoragePoolType.Linstor.equals(poolType) || Storage.StoragePoolType.StorPool.equals(poolType)) { if (size == null) { - throw new CloudRuntimeException("Restore volume size is required for Linstor pool when creating target volume"); + throw new CloudRuntimeException(String.format("Restore volume size is required for %s pool when creating target volume", poolType)); } disk = volumeStoragePool.createPhysicalDisk(volumeUuid, QemuImg.PhysicalDiskFormat.RAW, Storage.ProvisioningType.THIN, size, null); + if (disk == null) { + throw new CloudRuntimeException(String.format("Failed to provision a %s volume for restore [%s]", poolType, volumeUuid)); + } + if (Storage.StoragePoolType.StorPool.equals(poolType)) { + volumePath = disk.getPath(); + } } } else { - if (Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) { - storagePoolMgr.connectPhysicalDisk(volumePool.getPoolType(), volumePool.getUuid(), volumeUuid, null); + if (Storage.StoragePoolType.Linstor.equals(poolType)) { + storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumeUuid, null); + } else if (Storage.StoragePoolType.StorPool.equals(poolType)) { + storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumePath, null); } else { disk = volumeStoragePool.getPhysicalDisk(volumePath); } @@ -349,7 +376,7 @@ private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMg logger.debug("Restoring volume: {}", disk.toString()); } } catch (LibvirtException ex) { - throw new CloudRuntimeException(String.format("Failed to create qemu-img command to restore %s volume with backup", volumePool.getPoolType()), ex); + throw new CloudRuntimeException(String.format("Failed to create qemu-img command to restore %s volume with backup", poolType), ex); } QemuImgFile srcBackupFile = null; @@ -357,15 +384,16 @@ private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMg try { srcBackupFile = new QemuImgFile(backupPath, QemuImg.PhysicalDiskFormat.QCOW2); String destVolume; - switch(volumePool.getPoolType()) { + switch(poolType) { case Linstor: + case StorPool: destVolume = volumePath; break; case RBD: destVolume = KVMPhysicalDisk.RBDStringBuilder(volumeStoragePool, volumePath); break; default: - throw new CloudRuntimeException(String.format("Unsupported storage pool type [%s] for block device restore with backup.", volumePool.getPoolType())); + throw new CloudRuntimeException(String.format("Unsupported storage pool type [%s] for block device restore with backup.", poolType)); } destVolumeFile = new QemuImgFile(destVolume, QemuImg.PhysicalDiskFormat.RAW); logger.debug("Starting convert backup {} to volume {}", backupPath, volumePath); @@ -375,10 +403,10 @@ private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMg String srcFilename = srcBackupFile != null ? srcBackupFile.getFileName() : null; String destFilename = destVolumeFile != null ? destVolumeFile.getFileName() : null; logger.error("Failed to convert backup {} to volume {}, the error was: {}", srcFilename, destFilename, e.getMessage()); - return false; + return new Pair<>(false, volumePath); } - return true; + return new Pair<>(true, volumePath); } private boolean attachVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vmName, PrimaryDataStoreTO volumePool, String volumePath) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java index b7e609587650..540f5fd0e316 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java @@ -46,6 +46,10 @@ public class LibvirtTakeBackupCommandWrapper extends CommandWrapper diskPaths = new ArrayList<>(); - if (Objects.nonNull(volumePaths)) { - for (int idx = 0; idx < volumePaths.size(); idx++) { - PrimaryDataStoreTO volumePool = volumePools.get(idx); - String volumePath = volumePaths.get(idx); - if (volumePool.getPoolType() != Storage.StoragePoolType.RBD) { + try { + if (Objects.nonNull(volumePaths)) { + for (int idx = 0; idx < volumePaths.size(); idx++) { + PrimaryDataStoreTO volumePool = volumePools.get(idx); + String volumePath = volumePaths.get(idx); + if (volumePool.getPoolType() == Storage.StoragePoolType.RBD) { + KVMStoragePool volumeStoragePool = storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid()); + String rbdDestVolumeFile = KVMPhysicalDisk.RBDStringBuilder(volumeStoragePool, volumePath); + diskPaths.add(rbdDestVolumeFile); + continue; + } + // StorPool (among others) is passed through as-is: nasbackup.sh checks the + // VM's actual liveness itself right before acting, and only then — if the VM + // turns out to be stopped — clones this into a point-in-time backup source + // volume. Doing that here instead would rely on the same stale state read + // nasbackup.sh's own check exists to correct for. diskPaths.add(volumePath); - } else { - KVMStoragePool volumeStoragePool = storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid()); - String rbdDestVolumeFile = KVMPhysicalDisk.RBDStringBuilder(volumeStoragePool, volumePath); - diskPaths.add(rbdDestVolumeFile); } } - } - Pair result = runBackupScript(libvirtComputingResource, command, vmName, backupRepoType, backupRepoAddress, - mountOptions, backupPath, diskPaths, command.getMode(), - command.getBitmapNew(), command.getBitmapParent(), command.getParentPaths(), timeout); + Pair result = runBackupScript(libvirtComputingResource, command, vmName, backupRepoType, backupRepoAddress, + mountOptions, backupPath, diskPaths, command.getMode(), + command.getBitmapNew(), command.getBitmapParent(), command.getParentPaths(), timeout); - if (result.first() != 0) { - logger.debug("Failed to take VM backup: " + result.second()); - BackupAnswer answer = new BackupAnswer(command, false, StringUtils.trimToEmpty(result.second())); - if (EXIT_CLEANUP_FAILED.equals(result.first())) { - logger.debug("Backup cleanup failed"); - answer.setNeedsCleanup(true); + if (result.first() != 0) { + logger.debug("Failed to take VM backup: " + result.second()); + BackupAnswer answer = new BackupAnswer(command, false, StringUtils.trimToEmpty(result.second())); + if (EXIT_CLEANUP_FAILED.equals(result.first())) { + logger.debug("Backup cleanup failed"); + answer.setNeedsCleanup(true); + } + return answer; } + + // The script self-heals to a full backup when an incremental can't proceed (e.g. the + // parent checkpoint can't be re-registered) and signals it with INCREMENTAL_FALLBACK + // on stdout. Detect it and the reported size from the raw output, then strip both + // marker lines before using stdout as the answer's human-facing details. + String rawStdout = result.second(); + boolean incrementalFallback = StringUtils.contains(rawStdout, INCREMENTAL_FALLBACK_MARKER); + long backupSize = extractBackupSize(rawStdout); + String stdout = stripMarkerLines(rawStdout).trim(); + + BackupAnswer answer = new BackupAnswer(command, true, stdout); + answer.setSize(backupSize); + // A successful run always created command.getBitmapNew() (full and incremental both do; + // it is null for legacy-full, which the orchestrator treats as "no bitmap"). + answer.setBitmapCreated(command.getBitmapNew()); + answer.setIncrementalFallback(incrementalFallback); return answer; + } catch (RuntimeException e) { + logger.error("Failed to take VM backup: " + e.getMessage(), e); + return new BackupAnswer(command, false, e.getMessage()); } - - // The script self-heals to a full backup when an incremental can't proceed (e.g. the - // parent checkpoint can't be re-registered) and signals it with INCREMENTAL_FALLBACK - // on stdout. Detect it, then strip the marker line before parsing the backup size. - String rawStdout = result.second(); - boolean incrementalFallback = StringUtils.contains(rawStdout, INCREMENTAL_FALLBACK_MARKER); - String stdout = stripMarkerLines(rawStdout).trim(); - long backupSize = parseBackupSize(stdout, diskPaths); - - BackupAnswer answer = new BackupAnswer(command, true, stdout); - answer.setSize(backupSize); - // A successful run always created command.getBitmapNew() (full and incremental both do; - // it is null for legacy-full, which the orchestrator treats as "no bitmap"). - answer.setBitmapCreated(command.getBitmapNew()); - answer.setIncrementalFallback(incrementalFallback); - return answer; } - /** Remove nasbackup.sh's stdout signalling marker lines so they don't pollute size parsing. */ + /** Remove nasbackup.sh's stdout signalling marker lines so they don't pollute the answer details. */ private String stripMarkerLines(String stdout) { if (StringUtils.isBlank(stdout)) { return ""; } StringBuilder sb = new StringBuilder(); for (String line : stdout.split("\n", -1)) { - if (line.contains(INCREMENTAL_FALLBACK_MARKER)) { + if (line.contains(INCREMENTAL_FALLBACK_MARKER) || line.startsWith(BACKUP_SIZE_MARKER_PREFIX)) { continue; } if (sb.length() > 0) { @@ -136,6 +151,33 @@ private String stripMarkerLines(String stdout) { return sb.toString(); } + /** + * Find nasbackup.sh's {@code BACKUP_SIZE_TOTAL=} marker line. Unlike the old + * position/shape-based parsing this replaced, it doesn't need to know or guess which of + * nasbackup.sh's code paths actually ran. + *

+ * The marker is only metadata: a missing/unparseable marker does not mean the backup (which + * already exited 0) failed, so this logs a warning and reports an unknown size (0) rather + * than throwing — the caller's generic RuntimeException handler would otherwise turn an + * on-disk-successful backup into a reported failure. + */ + private long extractBackupSize(String rawStdout) { + if (rawStdout != null) { + for (String line : rawStdout.split("\n", -1)) { + if (line.startsWith(BACKUP_SIZE_MARKER_PREFIX)) { + try { + return Long.parseLong(line.substring(BACKUP_SIZE_MARKER_PREFIX.length()).trim()); + } catch (NumberFormatException e) { + logger.warn("nasbackup.sh reported an unparseable {} marker: {}", BACKUP_SIZE_MARKER_PREFIX, line); + return 0L; + } + } + } + } + logger.warn("nasbackup.sh did not report a {} marker in its output; backup succeeded but its size is unknown", BACKUP_SIZE_MARKER_PREFIX); + return 0L; + } + /** * Run nasbackup.sh once with the given args. Returns the exit code + captured stdout. */ @@ -214,24 +256,4 @@ private String validateBackupArgs(TakeBackupCommand command) { } return "Unknown backup mode: " + mode; } - - /** - * Sum the per-disk size lines emitted by nasbackup.sh. Single-volume mode emits one - * line containing just the byte count; multi-volume mode emits one line per disk - * whose first whitespace-separated token is the byte count. - */ - private long parseBackupSize(String stdout, List diskPaths) { - long backupSize = 0L; - if (CollectionUtils.isEmpty(diskPaths)) { - List outputLines = Arrays.asList(stdout.split("\n")); - if (!outputLines.isEmpty()) { - backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim()); - } - } else { - for (String line : stdout.split("\n")) { - backupSize = backupSize + Long.parseLong(line.split(" ")[0].trim()); - } - } - return backupSize; - } } diff --git a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java index 545f7b33c5fd..0d471323c297 100644 --- a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java +++ b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java @@ -454,7 +454,47 @@ public boolean createFolder(String uuid, String path, String localPath) { @Override public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, PhysicalDiskFormat format, ProvisioningType provisioningType, long size, byte[] passphrase) { - return null; + SP_LOG("StorPoolStorageAdaptor.createPhysicalDisk: name=%s, pool=%s, size=%d", name, pool.getUuid(), size); + + if (passphrase != null) { + throw new CloudRuntimeException("Encrypted StorPool volumes are not supported"); + } + + if (size <= 0) { + throw new CloudRuntimeException("Size must be greater than 0 to create a StorPool volume"); + } + + String[] uuidParts = pool.getUuid() != null ? pool.getUuid().split(";") : new String[0]; + if (uuidParts.length == 0 || StringUtils.isBlank(uuidParts[0])) { + throw new CloudRuntimeException("Unable to resolve the StorPool template for pool " + pool.getUuid()); + } + String template = uuidParts[0]; + + Map tags = new HashMap<>(); + tags.put("cs", "volume"); + tags.put("uuid", name); + + OutputInterpreter.AllLinesParser parser = createStorPoolVolume(template, size, tags, + String.format(" for %s", name)); + + String globalId = getNameFromResponse(parser.getLines(), false, false); + if (StringUtils.isBlank(globalId)) { + LOGGER.warn(String.format("StorPool volume for %s was created but its globalId could not be parsed from the response; " + + "it must be found (tag uuid=%s) and deleted manually to avoid an orphaned volume", name, name)); + throw new CloudRuntimeException(String.format("StorPool did not return a volume name/globalId when creating %s", name)); + } + + String volumePath = StorPoolUtil.devPath(globalId); + if (!attachOrDetachVolume("attach", "volume", volumePath)) { + volumeDelete(globalId); + throw new CloudRuntimeException(String.format("Could not attach newly created StorPool volume %s", volumePath)); + } + + KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool); + disk.setFormat(PhysicalDiskFormat.RAW); + disk.setSize(size); + disk.setVirtualSize(size); + return disk; } @Override @@ -466,21 +506,25 @@ public KVMPhysicalDisk createDiskFromTemplate(KVMPhysicalDisk template, String n private OutputInterpreter.AllLinesParser createStorPoolVolume(KVMStoragePool destPool, QemuImgFile srcFile, QemuImg qemu, String templateUuid) throws QemuImgException, LibvirtException { Map info = qemu.info(srcFile); - Map reqParams = new HashMap<>(); - reqParams.put("template", templateUuid); - reqParams.put("size", info.get("virtual_size")); Map tags = new HashMap<>(); tags.put("cs", "template"); + return createStorPoolVolume(templateUuid, info.get("virtual_size"), tags, ""); + } + + private OutputInterpreter.AllLinesParser createStorPoolVolume(String templateUuid, Object size, + Map tags, String errorContext) { + Map reqParams = new HashMap<>(); + reqParams.put("template", templateUuid); + reqParams.put("size", size); reqParams.put("tags", tags); - Gson gson = new Gson(); - String js = gson.toJson(reqParams); + String js = new Gson().toJson(reqParams); - Script sc = createStorPoolRequest(js, "VolumeCreate", null,true); + Script sc = createStorPoolRequest(js, "VolumeCreate", null, true); OutputInterpreter.AllLinesParser parser = new OutputInterpreter.AllLinesParser(); String res = sc.execute(parser); if (res != null) { - throw new CloudRuntimeException("Could not create volume due to: " + res); + throw new CloudRuntimeException(String.format("Could not create StorPool volume%s due to: %s", errorContext, res)); } return parser; } diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index 5869885c44c1..fcb24c96bdd8 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -34,6 +34,9 @@ BACKUP_DIR="" DISK_PATHS="" QUIESCE="" QUIESCE_TIMEOUT="" # Seconds to wait for the guest agent freeze/thaw; empty => libvirt default +# StorPool volumes cloned via sp_create_backup_source_disk during this run, so cleanup() can +# always release them even if the script exits before their normal per-disk cleanup runs. +SP_CLEANUP_VOLUMES=() # Incremental backup parameters (all optional; legacy callers omit them) MODE="" # "full" or "incremental"; empty => legacy full-only behavior (no checkpoint created) BITMAP_NEW="" # Bitmap/checkpoint name to create with this backup (e.g. "backup-1711586400") @@ -184,6 +187,129 @@ thaw_guest() { return 1 } +# StorPool volume name (with the "~globalId" form the storpool/storpool_req CLIs expect) from +# /dev/storpool-byid/. +sp_volume_name_from_path() { + local fullpath="$1" + local name + if [[ "$fullpath" == /dev/storpool-byid/* ]]; then + name="${fullpath#/dev/storpool-byid/}" + name="~${name%%/*}" + else + return 1 + fi + echo "$name" +} + +# Poll for the device node to actually appear after a successful attach: udev creates the +# symlink asynchronously, so a successful "attach" CLI call doesn't guarantee the device path +# is usable yet. +sp_wait_for_device_symlink() { + local name="$1" + local devpath="/dev/storpool-byid/${name#\~}" + local tries=10 i + for ((i = 0; i < tries; i++)); do + if [[ -e "$devpath" ]] && [[ "$(blockdev --getsize64 "$devpath" 2>/dev/null || echo 0)" -gt 0 ]]; then + return 0 + fi + sleep 0.1 + done + return 1 +} + +# detach is retried (a live volume's detach can transiently fail while still in use), attach is +# not. A successful attach also waits for the device symlink. +sp_attach_detach_volume() { + local cmd="$1" name="$2" + local tries=10 i + for ((i = 0; i < tries; i++)); do + if [[ "$cmd" == "attach" ]]; then + storpool -M -B attach volume "$name" here onRemoteAttached export >>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; } + break + else + storpool -M -B detach volume "$name" here >>"$logFile" 2>&1 && return 0 + sleep 1 + fi + done + return 1 +} + +# Clones $1 (a live StorPool volume path) into a new, point-in-time StorPool volume via the +# VolumeCreate API's baseOn option, attaches it, and echoes the clone's device path. Only called +# once this script's own (fresh, right-here) VM liveness check has already routed to the cold +# path — see backup_stopped_vm. +sp_create_backup_source_disk() { + local volume_path="$1" + local base_on + if ! base_on=$(sp_volume_name_from_path "$volume_path"); then + echo "Could not resolve a StorPool volume name from path $volume_path to create a backup source volume" >&2 + return 1 + fi + + log -ne "StorPool: creating backup source volume based on $base_on (from $volume_path)" + + local resp + if ! resp=$(storpool_req -P -M --json "{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}" VolumeCreate 2>>"$logFile"); then + log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above for storpool_req output" + echo "Could not create a backup source volume based on $base_on" >&2 + return 1 + fi + + local global_id + global_id=$(python3 -c ' +import sys, json +try: + data = json.load(sys.stdin) + gid = data.get("globalId") +except Exception: + gid = None +if not gid: + sys.exit(1) +print(gid) +' <<< "$resp" 2>>"$logFile") || { + log -ne "StorPool: VolumeCreate baseOn=$base_on returned no globalId, response: $resp -- the volume was created but its name could not be parsed; it must be found (tag cs=backup, baseOn=$base_on) and deleted manually to avoid an orphaned volume" + echo "StorPool did not return a volume name when cloning $base_on for backup" >&2 + return 1 + } + + local clone_path="/dev/storpool-byid/$global_id" + local clone_name="~$global_id" + log -ne "StorPool: created backup source volume $clone_name ($clone_path), attaching" + if ! sp_attach_detach_volume "attach" "$clone_name"; then + log -ne "StorPool: failed to attach backup source volume $clone_name, deleting it" + storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || true + echo "Could not attach backup source volume $clone_path cloned from $base_on" >&2 + return 1 + fi + + SP_CLEANUP_VOLUMES+=("$clone_name") + log -ne "StorPool: backup source volume $clone_name attached at $clone_path, reading from it for this backup" + echo "$clone_path" +} + +# Detach + delete a volume previously returned by sp_create_backup_source_disk. Best-effort: +# logs on failure rather than aborting the backup, matching the wrapper's old cleanup semantics. +sp_delete_backup_source_disk() { + local clone_name="$1" + log -ne "StorPool: releasing backup source volume $clone_name" + sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name" + storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || log -ne "Failed to delete StorPool backup source volume $clone_name" +} + +# Safety net for any clone not already released by its own per-disk cleanup (e.g. the script +# exiting between creating it and reaching that point). A cleanup here means an earlier step +# didn't run its normal release — worth noticing, so it's logged even though it isn't fatal. +sp_cleanup_backup_source_disks() { + local clone_name + for clone_name in "${SP_CLEANUP_VOLUMES[@]:-}"; do + [[ -z "$clone_name" ]] && continue + log -ne "StorPool: cleaning up backup source volume $clone_name left over on script exit" + sp_delete_backup_source_disk "$clone_name" + done + SP_CLEANUP_VOLUMES=() +} +trap sp_cleanup_backup_source_disks EXIT + backup_running_vm() { mount_operation mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; } @@ -482,12 +608,12 @@ for dev in data.get("return", []) or []: ) fi - # Print statistics - virsh -c qemu:///system domjobinfo $VM --completed + # Print statistics (informational only — logged, not parsed; see BACKUP_SIZE_TOTAL below) + virsh -c qemu:///system domjobinfo $VM --completed >>"$logFile" 2>&1 backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING: du failed for $dest, reporting size as 0"; backup_size=0; } timeout "$UNMOUNT_TIMEOUT" umount "$mount_point" 2>>"$logFile" || { log "WARNING: umount of $mount_point failed or timed out"; true; } rmdir "$mount_point" 2>>"$logFile" || { log "WARNING: rmdir of $mount_point failed"; true; } - echo "$backup_size" + echo "BACKUP_SIZE_TOTAL=$backup_size" } backup_stopped_vm() { @@ -496,10 +622,13 @@ backup_stopped_vm() { mount_operation mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; } - IFS="," + local -a disk_arr=() + IFS=',' read -ra disk_arr <<< "$DISK_PATHS" name="root" - for disk in $DISK_PATHS; do + for disk in "${disk_arr[@]}"; do + local read_disk="$disk" + local sp_clone_name="" if [[ "$disk" == rbd:* ]]; then volUuid=$(get_ceph_uuid_from_path "$disk") elif [[ "$disk" == /dev/drbd/by-res/* ]]; then @@ -510,25 +639,48 @@ backup_stopped_vm() { cleanup exit 1 fi + elif [[ "$disk" == /dev/storpool-byid/* ]]; then + volUuid="${disk##*/}" + # Clone before reading, so this backup never depends on (or interferes with) the live + # volume's attach state — safe even if the VM is started again before this finishes. + if ! read_disk=$(sp_create_backup_source_disk "$disk"); then + log -ne "Failed to create a StorPool backup source volume for $disk" + echo "Failed to create a StorPool backup source volume for $disk" + cleanup + exit 1 + fi + sp_clone_name=$(sp_volume_name_from_path "$read_disk") else volUuid="${disk##*/}" fi output="$dest/$name.$volUuid.qcow2" - if ! qemu-img convert -O qcow2 "$disk" "$output" >> "$logFile" 2> >(cat >&2); then - echo "qemu-img convert failed for $disk $output" + if ! qemu-img convert -O qcow2 "$read_disk" "$output" >> "$logFile" 2> >(cat >&2); then + log -ne "qemu-img convert failed for $read_disk $output" + echo "qemu-img convert failed for $read_disk $output" cleanup exit 1 fi + log -ne "Wrote $output from $read_disk" + + if [[ -n "$sp_clone_name" ]]; then + sp_delete_backup_source_disk "$sp_clone_name" + local -a remaining_clones=() + local tracked_clone + for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do + [[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone") + done + SP_CLEANUP_VOLUMES=("${remaining_clones[@]}") + fi # Pre-seed a persistent bitmap on the source disk so the NEXT backup (taken # after this VM is started again) can be incremental against the qcow2 we # just wrote. Without this, every backup after a stopped-VM backup would # fall back to full because no parent bitmap exists on the host yet. - # Only applies to file-backed qcow2 sources — RBD/LINSTOR have their own + # Only applies to file-backed qcow2 sources — RBD/LINSTOR/StorPool have their own # snapshot mechanisms and qemu-img bitmap is not the right primitive there. # bitmap --add should not fail on a file-backed qcow2; if it does, fail the backup so the # underlying problem is surfaced rather than silently degrading future backups to full. - if [[ -n "$BITMAP_NEW" && "$disk" != rbd:* && "$disk" != /dev/drbd/by-res/* ]]; then + if [[ -n "$BITMAP_NEW" && "$disk" != rbd:* && "$disk" != /dev/drbd/by-res/* && "$disk" != /dev/storpool-byid/* ]]; then if ! qemu-img bitmap --add "$disk" "$BITMAP_NEW" 2>>"$logFile"; then echo "Failed to pre-seed bitmap $BITMAP_NEW on $disk" cleanup @@ -540,7 +692,8 @@ backup_stopped_vm() { done sync - find "$dest" -type f -exec stat -c '%s' {} + + backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING: du failed for $dest, reporting size as 0"; backup_size=0; } + echo "BACKUP_SIZE_TOTAL=$backup_size" } delete_backup() { diff --git a/test/integration/plugins/storpool/TestNasBackupStorPool.py b/test/integration/plugins/storpool/TestNasBackupStorPool.py new file mode 100644 index 000000000000..4d171cea3b8f --- /dev/null +++ b/test/integration/plugins/storpool/TestNasBackupStorPool.py @@ -0,0 +1,650 @@ +# 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. + +""" +Smoke tests for NAS backup and recovery on StorPool primary storage. + +Assumes the environment already has NAS backup configured for the zone +(a NAS backup repository and an imported backup offering). +""" + +import math +import time +import uuid + +from marvin.cloudstackTestCase import cloudstackTestCase +from marvin.codes import FAILED, KVM +from marvin.lib.base import (Account, + Backup, + BackupOffering, + BackupRepository, + DiskOffering, + SecurityGroup, + ServiceOffering, + StoragePool, + VirtualMachine, + Volume) +from marvin.lib.common import (get_domain, + get_template, + list_disk_offering, + list_networks, + list_service_offering, + list_storage_pools, + list_zones) +from marvin.lib.utils import cleanup_resources, get_hypervisor_type +from nose.plugins.attrib import attr +from storpool import spapi + +from sp_util import StorPoolHelper, TestData + + +def _template_size_gb(template): + size_bytes = getattr(template, "size", None) + if not size_bytes: + return 1 + return max(1, int(math.ceil(float(size_bytes) / (1024 ** 3)))) + + +class TestNasBackupStorPool(cloudstackTestCase): + + @classmethod + def setUpClass(cls): + super(TestNasBackupStorPool, cls).setUpClass() + try: + cls.setUpCloudStack() + except Exception: + cls.cleanUpCloudStack() + raise + + @classmethod + def setUpCloudStack(cls): + cls._setup_common() + if cls.unsupportedHypervisor: + return + + cls.vm = cls._create_test_vm("SP-NasBkp-%s" % uuid.uuid4(), cls.disk_offering.id) + cls._verify_setup_volumes(cls.vm) + + @classmethod + def _setup_common(cls): + """Build everything shared by the StorPool-only and mixed-storage NAS + backup suites: account, template, network, backup offering, StorPool + primary storage/service/disk offering. Does not create the test VM, + so subclasses can plug in a different disk offering for it.""" + config = cls.getClsConfig() + StorPoolHelper.logger = cls + + zone_cfg = config.zones[0] + assert zone_cfg is not None + cls.zone_cfg = zone_cfg + + testClient = super(TestNasBackupStorPool, cls).getClsTestClient() + cls.apiclient = testClient.getApiClient() + cls._cleanup = [] + cls.unsupportedHypervisor = False + cls.skip_reason = None + cls.nfs_storage_pool = None + cls.nfs_tag = None + + td = TestData() + cls.testdata = td.testdata + cls.helper = StorPoolHelper() + + cls.services = testClient.getParsedTestDataConfig() + cls.domain = get_domain(cls.apiclient) + cls.zone = list_zones(cls.apiclient, name=zone_cfg.name)[0] + assert cls.zone is not None + cls.services["mode"] = cls.zone.networktype + + cls.hypervisor = get_hypervisor_type(cls.apiclient) + if str(cls.hypervisor).lower() != KVM.lower(): + cls.unsupportedHypervisor = True + cls.skip_reason = "NAS backup on StorPool requires KVM" + return + + sp_pools = cls.helper.get_pool(zone_cfg) + if not sp_pools: + cls.unsupportedHypervisor = True + cls.skip_reason = "No StorPool primary storage configured in zone" + return + + cls.spapi = spapi.Api(host=zone_cfg.spEndpoint, port=zone_cfg.spEndpointPort, + auth=zone_cfg.spAuthToken, multiCluster=True) + + template = get_template(cls.apiclient, cls.zone.id, account="system") + if template == FAILED: + assert False, "get_template() failed to return a system template" + cls.template = template + + offerings = BackupOffering.listByZone(cls.apiclient, cls.zone.id) or [] + nas_offerings = [o for o in offerings + if getattr(o, "provider", None) and "nas" in str(o.provider).lower()] + if not nas_offerings: + cls.unsupportedHypervisor = True + cls.skip_reason = "No imported NAS backup offering found in the zone" + return + cls.backup_offering = BackupOffering(nas_offerings[0].__dict__) + + primarystorage = sp_pools[0] + storage_pools = list_storage_pools(cls.apiclient, name=primarystorage["name"]) + if not storage_pools: + cls.unsupportedHypervisor = True + cls.skip_reason = "StorPool primary storage pool %s not found" % primarystorage["name"] + return + cls.primary_storage = storage_pools[0] + + sp_tag = primarystorage["tags"] if primarystorage["tags"] else primarystorage["name"] + cls.helper.updateStoragePoolTags(cls.apiclient, cls.primary_storage.id, sp_tag) + cls.sp_tag = sp_tag + + cls.template_size_gb = _template_size_gb(cls.template) + cls.service_offering = ServiceOffering.create( + cls.apiclient, + { + "name": "sp-nas-so-%s" % uuid.uuid4(), + "displaytext": "StorPool NAS offering (template %s GiB)" % cls.template_size_gb, + "cpunumber": 1, + "cpuspeed": 500, + "memory": 512, + "storagetype": "shared", + "tags": sp_tag, + }) + cls._cleanup.append(cls.service_offering) + + cls.disk_offering = cls._get_or_create_disk_offering( + sp_tag, "StorPool NAS backup disk offering") + + cls.account = cls.helper.create_account( + cls.apiclient, + cls.services["account"], + accounttype=1, + domainid=cls.domain.id, + roleid=1) + cls._cleanup.append(cls.account) + + if cls.zone.securitygroupsenabled: + securitygroup = SecurityGroup.list( + cls.apiclient, account=cls.account.name, domainid=cls.account.domainid)[0] + cls.helper.set_securityGroups( + cls.apiclient, + account=cls.account.name, + domainid=cls.account.domainid, + id=securitygroup.id) + + cls.network_ids = None + if str(cls.zone.networktype).lower() == "advanced": + networks = list_networks(cls.apiclient, zoneid=cls.zone.id, listall=True) or [] + guest_networks = [n for n in networks + if str(getattr(n, "traffictype", "")).lower() == "guest"] + shared = [n for n in guest_networks + if str(getattr(n, "type", "")).lower() == "shared"] + picked = (shared or guest_networks)[0] if (shared or guest_networks) else None + if picked is None: + cls.unsupportedHypervisor = True + cls.skip_reason = "Advanced zone has no existing guest network to deploy on" + return + cls.network_ids = [picked.id] + + @classmethod + def _get_or_create_disk_offering(cls, tag, displaytext): + disk_offerings = list_disk_offering(cls.apiclient, name=tag) + if disk_offerings is None: + disk_offering = DiskOffering.create( + cls.apiclient, + { + "name": tag, + "displaytext": displaytext, + "disksize": 5, + "storagetype": "shared", + "tags": tag, + }) + cls._cleanup.append(disk_offering) + return disk_offering + return disk_offerings[0] + + @classmethod + def _create_test_vm(cls, name, diskofferingid): + vm = VirtualMachine.create( + cls.apiclient, + {"name": name}, + zoneid=cls.zone.id, + templateid=cls.template.id, + accountid=cls.account.name, + domainid=cls.account.domainid, + serviceofferingid=cls.service_offering.id, + diskofferingid=diskofferingid, + hypervisor=cls.hypervisor, + networkids=cls.network_ids, + rootdisksize=cls.template_size_gb, + mode=cls.services["mode"]) + cls._cleanup.insert(0, vm) + return vm + + @classmethod + def _verify_setup_volumes(cls, vm): + volumes = Volume.list(cls.apiclient, virtualmachineid=vm.id, listall=True) + assert isinstance(volumes, list) and len(volumes) >= 1 + for vol in volumes: + if cls.nfs_storage_pool and vol.storageid == cls.nfs_storage_pool.id: + continue + cls.helper.verify_storpool_volume(cls.spapi, vol) + return volumes + + @classmethod + def tearDownClass(cls): + cls.cleanUpCloudStack() + + @classmethod + def cleanUpCloudStack(cls): + try: + if hasattr(cls, "vm") and cls.vm is not None: + cls._remove_vm_backups_and_offering(cls.apiclient, cls.vm) + if hasattr(cls, "_cleanup") and cls._cleanup: + cleanup_resources(cls.apiclient, cls._cleanup) + except Exception as e: + raise Exception("Warning: Exception during cleanup : %s" % e) + + @classmethod + def _remove_vm_backups_and_offering(cls, apiclient, vm): + try: + backups = Backup.list(apiclient, vm.id) or [] + for backup in backups: + try: + Backup.delete(apiclient, backup.id, forced=True) + except Exception: + Backup.delete(apiclient, backup.id) + except Exception: + pass + try: + if hasattr(cls, "backup_offering") and cls.backup_offering is not None: + cls.backup_offering.removeOffering(apiclient, vm.id, forced=True) + except Exception: + pass + + def setUp(self): + self.apiclient = self.testClient.getApiClient() + self.cleanup = [] + self.cleanup_backups = [] + if self.unsupportedHypervisor: + self.skipTest(self.skip_reason or "Skipping unsupported environment") + self._remove_vm_backups_and_offering(self.apiclient, self.vm) + + def tearDown(self): + try: + for obj in list(self.cleanup_backups): + try: + Backup.delete(self.apiclient, obj.id, forced=True) + except Exception: + pass + for obj in self.cleanup: + if isinstance(obj, VirtualMachine): + self._remove_vm_backups_and_offering(self.apiclient, obj) + cleanup_resources(self.apiclient, self.cleanup) + except Exception as e: + raise Exception("Warning: Exception during cleanup : %s" % e) + + def _verify_vm_volumes_on_storpool(self, vm, spapi_client=None): + """Verify every volume of vm. A volume that lives on the NFS primary + storage pool (only possible in the mixed-storage suite, where + self.nfs_storage_pool is set) is checked against NFS instead of + StorPool, so the NAS backup lifecycle tests can run unmodified + against either an all-StorPool VM or a mixed StorPool+NFS one.""" + volumes = Volume.list(self.apiclient, virtualmachineid=vm.id, listall=True) + self.assertTrue(isinstance(volumes, list) and len(volumes) >= 1, + "VM should have at least one volume") + for vol in volumes: + if self.nfs_storage_pool and vol.storageid == self.nfs_storage_pool.id: + self.assertFalse(vol.path and vol.path.startswith("/dev/storpool-byid/"), + "Volume %s is on NFS storage %s, should not have a StorPool path, got %s" % + (vol.id, self.nfs_storage_pool.id, vol.path)) + continue + self.assertTrue(vol.path and vol.path.startswith("/dev/storpool-byid/"), + "Volume %s path should be /dev/storpool-byid/..., got %s" % + (vol.id, vol.path)) + self.helper.verify_storpool_volume(spapi_client or self.spapi, vol) + spvolume = self.helper.get_storpool_volume_by_path(spapi_client or self.spapi, vol.path) + sp_uuid_tag = (spvolume.tags or {}).get("uuid") + self.assertEqual(sp_uuid_tag, vol.id, + "StorPool volume uuid tag %s should match CloudStack volume id %s for %s" % + (sp_uuid_tag, vol.id, vol.path)) + return volumes + + def _write_guest_marker(self, vm, marker, ipaddress=None): + """Create a marker file in the guest and sync it to disk before backup.""" + try: + ssh = vm.get_ssh_client(ipaddress=ipaddress, reconnect=True) if ipaddress \ + else vm.get_ssh_client(reconnect=True) + ssh.execute("touch %s; sync" % marker) + except Exception as err: + self.fail("SSH failed writing marker on VM %s: %s" % + (ipaddress or getattr(vm, "ipaddress", vm.id), err)) + + def _assert_guest_reachable(self, vm, marker=None, ipaddress=None, msg=None): + if ipaddress is None: + listed = VirtualMachine.list( + self.apiclient, id=vm.id, listall=True) or [] + self.assertTrue(listed and listed[0].state == "Running", + "VM %s should be Running for SSH check" % vm.id) + ipaddress = listed[0].ipaddress + try: + ssh = vm.get_ssh_client(ipaddress=ipaddress, reconnect=True) + if marker: + result = ssh.execute("ls %s" % marker) + self.assertEqual(result[0], marker, + msg or ("Guest should retain marker %s" % marker)) + else: + result = ssh.execute("echo ok") + self.assertTrue(result and result[0].strip() == "ok", + msg or "Guest should answer SSH after restore") + except Exception as err: + self.fail("SSH failed for VM %s (%s): %s" % + (vm.id, ipaddress, err)) + + def _create_backup(self, vmid, name=None): + Backup.create(self.apiclient, vmid, name) + backups = Backup.list(self.apiclient, vmid) or [] + self.assertTrue(backups, "Backup.create should produce a listable backup") + return backups[0] + + def _backup_type(self, backup): + return getattr(backup, 'type', 'FULL') or 'FULL' + + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_01_vm_backup_lifecycle(self): + """Assign offering, create and delete an ad-hoc NAS backup on StorPool VM.""" + backups = Backup.list(self.apiclient, self.vm.id) + self.assertEqual(backups, None, "There should not exist any backup for the VM") + + self.backup_offering.assignOffering(self.apiclient, self.vm.id) + backup = self._create_backup(self.vm.id) + self.cleanup_backups.append(backup) + + self._verify_vm_volumes_on_storpool(self.vm) + + Backup.delete(self.apiclient, backup.id) + backups = Backup.list(self.apiclient, self.vm.id) + self.assertEqual(backups, None, "There should not exist any backup for the VM") + + self.backup_offering.removeOffering(self.apiclient, self.vm.id) + + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_02_create_vm_from_backup(self): + """Create a new VM from NAS backup and verify volumes via StorPool API. + """ + self.backup_offering.assignOffering(self.apiclient, self.vm.id) + + marker = "storpool_nas_backup_%s.txt" % int(time.time()) + self._write_guest_marker(self.vm, marker) + + time.sleep(5) + backup = self._create_backup(self.vm.id, "sp-nas-backup1") + self.cleanup_backups.append(backup) + + self.backup_offering.removeOffering(self.apiclient, self.vm.id) + + new_vm_name = "vm-from-sp-nas-%s" % int(time.time()) + vm_services = self.services.get("small", self.services) + new_vm = Backup.createVMFromBackup( + self.apiclient, + vm_services, + mode=self.services["mode"], + backupid=backup.id, + vmname=new_vm_name, + accountname=self.account.name, + domainid=self.account.domainid, + zoneid=self.zone.id, + networkids=self.network_ids) + self.cleanup.append(new_vm) + + self.assertEqual(new_vm.name, new_vm_name) + self.assertEqual(new_vm.state, "Running") + self.assertEqual(new_vm.zoneid, self.zone.id) + + volumes = self._verify_vm_volumes_on_storpool(new_vm) + self.assertEqual(2, len(volumes), + "New VM should have ROOT + DATADISK volumes on StorPool") + + self._assert_guest_reachable( + new_vm, marker=marker, ipaddress=new_vm.ipaddress, + msg="Instance created from backup should retain guest file") + + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_03_restore_vm_and_volume_from_backup(self): + """Restore VM in place and restore a volume attach; verify StorPool volumes.""" + self.backup_offering.assignOffering(self.apiclient, self.vm.id) + + marker = "storpool_nas_restore_%s.txt" % int(time.time()) + self._write_guest_marker(self.vm, marker) + time.sleep(5) + backup = self._create_backup(self.vm.id) + self.cleanup_backups.append(backup) + + self.vm.stop(self.apiclient, forced=True) + Backup.restoreVM(self.apiclient, backup.id) + self.vm.start(self.apiclient) + self._verify_vm_volumes_on_storpool(self.vm) + self._assert_guest_reachable( + self.vm, marker=marker, + msg="In-place restore should boot and retain guest file") + + target_vm = VirtualMachine.create( + self.apiclient, + {"name": "SP-NasAttach-%s" % uuid.uuid4()}, + zoneid=self.zone.id, + templateid=self.template.id, + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + hypervisor=self.hypervisor, + networkids=self.network_ids, + rootdisksize=self.template_size_gb, + mode=self.services["mode"], + startvm=False) + self.cleanup.append(target_vm) + + volumes = Volume.list(self.apiclient, virtualmachineid=self.vm.id, listall=True) + data_disk = next((v for v in volumes if v.type == "DATADISK"), None) + self.assertIsNotNone(data_disk, "Source VM should have a data disk") + + Backup.restoreVolumeFromBackupAndAttachToVM( + self.apiclient, + backupid=backup.id, + volumeid=data_disk.id, + virtualmachineid=target_vm.id) + target_vm.start(self.apiclient) + + target_volumes = self._verify_vm_volumes_on_storpool(target_vm) + self.assertEqual(2, len(target_volumes), + "Target VM should have ROOT + restored DATADISK volumes") + self._assert_guest_reachable( + target_vm, msg="VM with restored volume attached should boot and answer SSH") + + self.backup_offering.removeOffering(self.apiclient, self.vm.id) + + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_04_restore_destroyed_vm_from_backup(self): + """Destroy (no expunge) then restoreBackup — exercises restoreVolumesOfDestroyedVMs. + + Uses a ROOT-only VM: destroyVirtualMachine detaches data disks, which would + make volume-count validation fail against a ROOT+DATA backup. + """ + destroyed_vm = VirtualMachine.create( + self.apiclient, + {"name": "SP-NasDestroyed-%s" % uuid.uuid4()}, + zoneid=self.zone.id, + templateid=self.template.id, + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + hypervisor=self.hypervisor, + networkids=self.network_ids, + rootdisksize=self.template_size_gb, + mode=self.services["mode"]) + self.cleanup.append(destroyed_vm) + + self.backup_offering.assignOffering(self.apiclient, destroyed_vm.id) + + marker = "storpool_nas_destroyed_%s.txt" % int(time.time()) + self._write_guest_marker(destroyed_vm, marker) + time.sleep(5) + backup = self._create_backup(destroyed_vm.id) + self.cleanup_backups.append(backup) + + destroyed_vm.delete(self.apiclient, expunge=False) + + listed = VirtualMachine.list( + self.apiclient, id=destroyed_vm.id, listall=True) or [] + self.assertTrue(listed, "Destroyed VM should still be listable") + self.assertEqual(listed[0].state, "Destroyed") + + Backup.restoreVM(self.apiclient, backup.id) + + restored = VirtualMachine.list( + self.apiclient, id=destroyed_vm.id, listall=True) or [] + self.assertTrue(restored) + self.assertEqual(restored[0].state, "Stopped", + "restoreBackup should leave the Instance Stopped") + + destroyed_vm.start(self.apiclient) + volumes = self._verify_vm_volumes_on_storpool(destroyed_vm) + self.assertEqual(1, len(volumes), + "ROOT-only destroyed VM restore should have one StorPool volume") + self._assert_guest_reachable( + destroyed_vm, marker=marker, + msg="Restored destroyed VM should boot and retain guest file") + + self.backup_offering.removeOffering(self.apiclient, destroyed_vm.id) + + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_05_backup_stopped_vm(self): + """Restore a NAS backup that was taken while the StorPool VM was stopped. + """ + self.backup_offering.assignOffering(self.apiclient, self.vm.id) + + marker = "storpool_nas_stopped_restore_%s.txt" % int(time.time()) + self._write_guest_marker(self.vm, marker) + time.sleep(5) + + self.vm.stop(self.apiclient, forced=True) + + backup = self._create_backup(self.vm.id, "sp-nas-stopped-restore-backup") + self.cleanup_backups.append(backup) + self.assertEqual(self._backup_type(backup).upper(), "FULL", + "A stopped-VM NAS backup on StorPool should always be a FULL backup") + + Backup.restoreVM(self.apiclient, backup.id) + + self.vm.start(self.apiclient) + self._verify_vm_volumes_on_storpool(self.vm) + self._assert_guest_reachable( + self.vm, marker=marker, + msg="Restore of a stopped-VM StorPool backup should boot and retain the guest file") + + self.backup_offering.removeOffering(self.apiclient, self.vm.id) + + +class TestNasBackupMixedStorage(TestNasBackupStorPool): + """Repeats the NAS backup smoke tests (test_01 .. test_05, inherited + unchanged from TestNasBackupStorPool) against a VM with mixed-provider + volumes: ROOT on StorPool, DATADISK on NFS primary storage. + + The NFS primary storage pool, its name and storage tag are taken from + the Marvin cfg file (the "nfs://" primaryStorages entry for the zone). + Other suites assume that pool is left in Maintenance, so it is always + put into Maintenance before this suite touches it, taken out of + Maintenance only for the duration of creating volumes/VMs on it, and + put back into Maintenance once every test in this class has finished + (including when setup itself fails). + """ + + @classmethod + def setUpCloudStack(cls): + cls._setup_common() + if cls.unsupportedHypervisor: + return + + cls._setup_nfs_environment() + if cls.unsupportedHypervisor: + return + + cls.nfs_storage_pool = cls._ensure_maintenance(cls.nfs_storage_pool.id, enable=False) + + cls.vm = cls._create_test_vm("SP-NasMixed-%s" % uuid.uuid4(), cls.nfs_disk_offering.id) + cls._verify_setup_volumes(cls.vm) + + @classmethod + def _setup_nfs_environment(cls): + nfs_pools = cls.helper.get_nfs_pool(cls.zone_cfg) + if not nfs_pools: + cls.unsupportedHypervisor = True + cls.skip_reason = "No NFS primary storage configured in zone" + return + nfs_cfg = nfs_pools[0] + + storage_pools = list_storage_pools(cls.apiclient, name=nfs_cfg["name"]) + if not storage_pools: + cls.unsupportedHypervisor = True + cls.skip_reason = "NFS primary storage pool %s not found" % nfs_cfg["name"] + return + cls.nfs_storage_pool = storage_pools[0] + + cls.nfs_tag = nfs_cfg["tags"] if nfs_cfg["tags"] else nfs_cfg["name"] + cls.helper.updateStoragePoolTags(cls.apiclient, cls.nfs_storage_pool.id, cls.nfs_tag) + + # Other suites assume the NFS pool is unavailable; make sure that + # invariant holds before this suite starts using it. + cls.nfs_storage_pool = cls._ensure_maintenance(cls.nfs_storage_pool.id, enable=True) + + cls.nfs_disk_offering = cls._get_or_create_disk_offering( + cls.nfs_tag, "NFS NAS backup disk offering") + cls.nfs_service_offering = cls._get_or_create_nfs_service_offering() + + @classmethod + def _get_or_create_nfs_service_offering(cls): + service_offerings = list_service_offering(cls.apiclient, name=cls.nfs_tag) + if service_offerings is None: + service_offering = ServiceOffering.create( + cls.apiclient, + { + "name": cls.nfs_tag, + "displaytext": "NFS NAS offering (%s)" % cls.nfs_tag, + "cpunumber": 1, + "cpuspeed": 500, + "memory": 512, + "storagetype": "shared", + "tags": cls.nfs_tag, + }) + cls._cleanup.append(service_offering) + return service_offering + return service_offerings[0] + + @classmethod + def _ensure_maintenance(cls, pool_id, enable): + pool = list_storage_pools(cls.apiclient, id=pool_id)[0] + if enable and pool.state != "Maintenance": + pool = StoragePool.enableMaintenance(cls.apiclient, pool_id) + elif not enable and pool.state == "Maintenance": + pool = StoragePool.cancelMaintenance(cls.apiclient, pool_id) + return pool + + @classmethod + def cleanUpCloudStack(cls): + try: + super(TestNasBackupMixedStorage, cls).cleanUpCloudStack() + finally: + if getattr(cls, "nfs_storage_pool", None) is not None: + cls._ensure_maintenance(cls.nfs_storage_pool.id, enable=True) diff --git a/test/integration/plugins/storpool/sp_util.py b/test/integration/plugins/storpool/sp_util.py index 084a57ee954e..280a7797386f 100644 --- a/test/integration/plugins/storpool/sp_util.py +++ b/test/integration/plugins/storpool/sp_util.py @@ -21,6 +21,7 @@ from marvin.lib.base import (Account, Cluster, Configurations, + ImageStore, ServiceOffering, Snapshot, StoragePool, @@ -682,26 +683,50 @@ def destroy_vm(self, apiclient, virtualmachineid): apiclient.destroyVirtualMachine(cmd) @classmethod - def check_storpool_volume_size(cls, volume, spapi): - name = volume.path.split("/")[3] + def get_storpool_volume_by_path(cls, spapi, path): + """ + Resolve a CloudStack volume path (/dev/storpool-byid/) to a + StorPool volume. + """ + if not path: + raise Exception("Volume path is empty") + parts = path.rstrip("/").split("/") + if len(parts) < 4: + raise Exception("Unexpected StorPool volume path: %s" % path) + name = "~" + parts[3] try: - spvolume = spapi.volumeList(volumeName = "~" + name) - if spvolume[0].size != volume.size: - raise Exception("Storpool volume size is not the same as CloudStack db size") - except spapi.ApiError as err: - raise Exception(err) + volumes = spapi.volumeList(volumeName=name) + except Exception as err: + raise Exception("StorPool volume not found for path %s: %s" % (path, err)) + if not volumes: + raise Exception("StorPool volume not found for path %s" % path) + return volumes[0] + + @classmethod + def check_storpool_volume_size(cls, volume, spapi): + spvolume = cls.get_storpool_volume_by_path(spapi, volume.path) + if spvolume.size != volume.size: + raise Exception("Storpool volume size is not the same as CloudStack db size") @classmethod def check_storpool_volume_iops(cls, spapi, volume,): - name = volume.path.split("/")[3] - try: - spvolume = spapi.volumeList(volumeName = "~" + name) - logging.debug(spvolume[0].iops) - logging.debug(volume.maxiops) - if spvolume[0].iops != volume.maxiops: - raise Exception("Storpool volume size is not the same as CloudStack db size") - except spapi.ApiError as err: - raise Exception(err) + spvolume = cls.get_storpool_volume_by_path(spapi, volume.path) + logging.debug(spvolume.iops) + logging.debug(volume.maxiops) + if spvolume.iops != volume.maxiops: + raise Exception("Storpool volume size is not the same as CloudStack db size") + + @classmethod + def verify_storpool_volume(cls, spapi, volume, check_size=True): + """Assert the CloudStack volume exists on StorPool; optionally compare size.""" + if not volume.path or not volume.path.startswith("/dev/storpool"): + raise Exception("Volume %s does not have a StorPool device path: %s" % + (volume.id, volume.path)) + spvolume = cls.get_storpool_volume_by_path(spapi, volume.path) + if check_size and volume.size is not None and spvolume.size != volume.size: + raise Exception("StorPool volume size %s does not match CloudStack size %s for %s" % + (spvolume.size, volume.size, volume.path)) + return spvolume @classmethod def create_custom_disk(cls, apiclient, services, size = None, miniops = None, maxiops =None, diskofferingid=None, zoneid=None, account=None, domainid=None, snapshotid=None): @@ -926,6 +951,19 @@ def get_pool(cls, zone): return return sp_pools + @classmethod + def get_nfs_pool(cls, zone): + """Return the NFS primary storage entries configured for this zone in + the Marvin cfg file. NFS entries don't set "provider" (unlike + StorPool/RBD), so they're identified by their "nfs://" url instead.""" + storage_pools = zone.primaryStorages + nfs_pools = [] + for storage in storage_pools: + url = storage['url'] or "" + if not storage['provider'] and str(url).lower().startswith("nfs://"): + nfs_pools.append(storage) + return nfs_pools + @classmethod def create_snapshot_template(cls, apiclient, services, snapshot_id, zone_id): cmd = createTemplate.createTemplateCmd() From 361c806743363c39fbab05cc4c16c8a3de6e1edf Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Tue, 29 Sep 2026 13:26:28 +0300 Subject: [PATCH 2/4] addressed reviews --- .../cloudstack/backup/NASBackupProvider.java | 10 ++- .../LibvirtRestoreBackupCommandWrapper.java | 17 ++--- ...ibvirtRestoreBackupCommandWrapperTest.java | 11 ++++ .../kvm/storage/StorPoolStorageAdaptor.java | 6 +- scripts/vm/hypervisor/kvm/nasbackup.sh | 30 ++++++--- .../plugins/storpool/TestNasBackupStorPool.py | 62 +++++++++++++++++++ 6 files changed, 112 insertions(+), 24 deletions(-) diff --git a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java index 20a7c5087cc5..2fa9c44090d6 100644 --- a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java +++ b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java @@ -397,17 +397,15 @@ protected boolean allVolumesOnCheckpointCapableStorage(VirtualMachine vm) { } /** - * True when any of the VM's volumes sits on a StorPool pool. Used to decide whether volume - * pool/path info must be sent to the agent even though the VM currently looks Running — see - * the caller in {@link #takeBackup}. + * True when any of the given volumes sits on a pool of the given type. */ - private boolean hasStorPoolVolume(List volumes) { + private boolean hasVolumeOnPoolType(List volumes, Storage.StoragePoolType poolType) { if (volumes == null) { return false; } for (VolumeVO volume : volumes) { StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId()); - if (pool != null && Storage.StoragePoolType.StorPool.equals(pool.getPoolType())) { + if (pool != null && poolType.equals(pool.getPoolType())) { return true; } } @@ -627,7 +625,7 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce // — without it there is nothing to derive that from. boolean vmStopped = VirtualMachine.State.Stopped.equals(vm.getState()); List vmVolumes = volumeDao.findByInstance(vm.getId()); - boolean hasStorPoolVolume = hasStorPoolVolume(vmVolumes); + boolean hasStorPoolVolume = hasVolumeOnPoolType(vmVolumes, Storage.StoragePoolType.StorPool); if (vmStopped || hasStorPoolVolume) { if (!vmStopped) { logger.debug("VM {} looks {} but has a StorPool volume — sending volume pool/path info to the agent anyway, " diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java index 171d396f8efa..17c164d027cf 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java @@ -363,12 +363,15 @@ private Pair replaceBlockDeviceWithBackup(KVMStoragePoolManager } } } else { - if (Storage.StoragePoolType.Linstor.equals(poolType)) { - storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumeUuid, null); - } else if (Storage.StoragePoolType.StorPool.equals(poolType)) { - storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumePath, null); - } else { - disk = volumeStoragePool.getPhysicalDisk(volumePath); + switch (poolType) { + case Linstor: + storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumeUuid, null); + break; + case StorPool: + storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumePath, null); + break; + default: + disk = volumeStoragePool.getPhysicalDisk(volumePath); } qemu.setSkipTargetVolumeCreation(true); } @@ -422,7 +425,7 @@ private boolean attachVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vm virshCmd.add(deviceToAttachDiskTo); virshCmd.add("--driver"); virshCmd.add("qemu"); - if (!Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) { + if (!List.of(Storage.StoragePoolType.Linstor, Storage.StoragePoolType.StorPool).contains(volumePool.getPoolType())) { virshCmd.add("--subdriver"); virshCmd.add("qcow2"); } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java index 3bdc23d27a15..21499a45c8ff 100644 --- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java @@ -695,6 +695,17 @@ public void testAttachVolumeOmitsQcow2SubdriverForLinstor() throws Exception { Assert.assertFalse(args.contains("--subdriver")); } + @Test + public void testAttachVolumeOmitsQcow2SubdriverForStorPool() throws Exception { + String[] cmd = captureAttachCommand(Storage.StoragePoolType.StorPool); + List args = Arrays.asList(cmd); + + // StorPoolStorageAdaptor#createPhysicalDisk always provisions RAW block devices, + // declaring qcow2 makes libvirt try to parse the raw data as a qcow2 image. + Assert.assertTrue(args.contains("attach-disk")); + Assert.assertFalse(args.contains("--subdriver")); + } + @Test public void testAttachVolumePassesRbdXmlThroughAFile() throws Exception { String[] cmd = captureAttachCommand(Storage.StoragePoolType.RBD); diff --git a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java index 0d471323c297..e1cc9bbfdca7 100644 --- a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java +++ b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java @@ -485,9 +485,11 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys } String volumePath = StorPoolUtil.devPath(globalId); - if (!attachOrDetachVolume("attach", "volume", volumePath)) { + try { + attachOrDetachVolume("attach", "volume", volumePath); + } catch (CloudRuntimeException e) { volumeDelete(globalId); - throw new CloudRuntimeException(String.format("Could not attach newly created StorPool volume %s", volumePath)); + throw e; } KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool); diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index fcb24c96bdd8..aded790bdc41 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -282,7 +282,10 @@ print(gid) return 1 fi - SP_CLEANUP_VOLUMES+=("$clone_name") + # Not registered in SP_CLEANUP_VOLUMES here: the caller always invokes this function via + # command substitution ($(...)), which runs it in a subshell — any append to the array here + # would be a mutation of that subshell's own copy and vanish once it exits. The caller must + # register $clone_name itself right after reading it back from stdout below. log -ne "StorPool: backup source volume $clone_name attached at $clone_path, reading from it for this backup" echo "$clone_path" } @@ -293,7 +296,10 @@ sp_delete_backup_source_disk() { local clone_name="$1" log -ne "StorPool: releasing backup source volume $clone_name" sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name" - storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || log -ne "Failed to delete StorPool backup source volume $clone_name" + if ! storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1; then + log -ne "Failed to delete StorPool backup source volume $clone_name" + return 1 + fi } # Safety net for any clone not already released by its own per-disk cleanup (e.g. the script @@ -650,6 +656,9 @@ backup_stopped_vm() { exit 1 fi sp_clone_name=$(sp_volume_name_from_path "$read_disk") + # Register the clone here, in the parent shell, right after the command substitution + # above returns — sp_create_backup_source_disk cannot do this itself (see its comment). + SP_CLEANUP_VOLUMES+=("$sp_clone_name") else volUuid="${disk##*/}" fi @@ -663,13 +672,16 @@ backup_stopped_vm() { log -ne "Wrote $output from $read_disk" if [[ -n "$sp_clone_name" ]]; then - sp_delete_backup_source_disk "$sp_clone_name" - local -a remaining_clones=() - local tracked_clone - for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do - [[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone") - done - SP_CLEANUP_VOLUMES=("${remaining_clones[@]}") + # Only drop the clone from SP_CLEANUP_VOLUMES once it's actually deleted — otherwise the + # EXIT trap's safety net would never see (and retry) a clone left behind by a failed delete. + if sp_delete_backup_source_disk "$sp_clone_name"; then + local -a remaining_clones=() + local tracked_clone + for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do + [[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone") + done + SP_CLEANUP_VOLUMES=("${remaining_clones[@]}") + fi fi # Pre-seed a persistent bitmap on the source disk so the NEXT backup (taken diff --git a/test/integration/plugins/storpool/TestNasBackupStorPool.py b/test/integration/plugins/storpool/TestNasBackupStorPool.py index 4d171cea3b8f..feb4cae415dc 100644 --- a/test/integration/plugins/storpool/TestNasBackupStorPool.py +++ b/test/integration/plugins/storpool/TestNasBackupStorPool.py @@ -556,6 +556,68 @@ def test_05_backup_stopped_vm(self): self.backup_offering.removeOffering(self.apiclient, self.vm.id) + @attr(tags=["advanced", "backup", "storpool"], required_hardware="true") + def test_06_restore_volume_attach_to_running_vm(self): + """Restore a volume from backup and hot-attach it to an already-Running VM. + + Regression test for LibvirtRestoreBackupCommandWrapper.attachVolumeToVm only + excluding LINSTOR from the qcow2 --subdriver flag: a restored StorPool volume + is a RAW block device (like LINSTOR), so attaching it with --subdriver qcow2 + makes virsh try to open the raw device as a qcow2 image. test_03 never caught + this because it attaches to a target VM that is still Stopped at attach time. + """ + self.backup_offering.assignOffering(self.apiclient, self.vm.id) + + marker = "storpool_nas_running_attach_%s.txt" % int(time.time()) + self._write_guest_marker(self.vm, marker) + time.sleep(5) + backup = self._create_backup(self.vm.id) + self.cleanup_backups.append(backup) + + volumes = Volume.list(self.apiclient, virtualmachineid=self.vm.id, listall=True) + data_disk = next((v for v in volumes if v.type == "DATADISK"), None) + self.assertIsNotNone(data_disk, "Source VM should have a data disk") + + # No startvm=False here: the target VM must already be Running when + # restoreVolumeFromBackupAndAttachToVM is called, to exercise the + # live virsh attach-disk path instead of an offline volume write. + target_vm = VirtualMachine.create( + self.apiclient, + {"name": "SP-NasRunAttach-%s" % uuid.uuid4()}, + zoneid=self.zone.id, + templateid=self.template.id, + accountid=self.account.name, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + hypervisor=self.hypervisor, + networkids=self.network_ids, + rootdisksize=self.template_size_gb, + mode=self.services["mode"]) + self.cleanup.append(target_vm) + + listed = VirtualMachine.list(self.apiclient, id=target_vm.id, listall=True) or [] + self.assertTrue(listed and listed[0].state == "Running", + "Target VM should be Running before the volume is hot-attached") + + Backup.restoreVolumeFromBackupAndAttachToVM( + self.apiclient, + backupid=backup.id, + volumeid=data_disk.id, + virtualmachineid=target_vm.id) + + target_volumes = self._verify_vm_volumes_on_storpool(target_vm) + self.assertEqual(2, len(target_volumes), + "Target VM should have ROOT + hot-attached restored DATADISK volumes") + self._assert_guest_reachable( + target_vm, msg="VM should stay reachable after a volume is hot-attached from backup") + + ssh = target_vm.get_ssh_client(reconnect=True) + result = ssh.execute("lsblk -d -n -o NAME | grep -c .") + self.assertTrue(result and int(result[0].strip()) >= 2, + "Guest should see the hot-attached disk after restore") + + self.backup_offering.removeOffering(self.apiclient, self.vm.id) + class TestNasBackupMixedStorage(TestNasBackupStorPool): """Repeats the NAS backup smoke tests (test_01 .. test_05, inherited From c5bed9a383cf8ea4f91ea885f7f92acdc7931f0d Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Mon, 19 Oct 2026 13:44:50 +0300 Subject: [PATCH 3/4] StorPool: clean up volumes left behind by failed NAS backup/restore Restore (KVM agent): - When restoring a backed-up volume to a new StorPool volume fails, either while copying the backup data or while attaching the volume to the running VM, the created volume was leaked. The management server persists the restored volume only on success, and StorPool assigns the volume's identity itself, so nothing else could find and delete it. - Add StorageAdaptor#cleanupCreatedPhysicalDisk (default: no-op) and call it from LibvirtRestoreBackupCommandWrapper on both failure paths. The StorPool implementation detaches the volume from this host and deletes it. The restore still fails; a cleanup failure is only logged. - Fix the delete when the attach in StorPoolStorageAdaptor #createPhysicalDisk fails: the volume was deleted by its bare globalId instead of "~". Backup (nasbackup.sh) - Delete the clone with onAttached=detachForce, which removes it even if the attach succeeded but the device symlink never appeared. This replaces the separate detach-with-retries step. --- .../LibvirtRestoreBackupCommandWrapper.java | 20 ++++- .../kvm/storage/KVMStoragePoolManager.java | 7 ++ .../kvm/storage/StorageAdaptor.java | 14 ++++ .../kvm/storage/StorPoolStorageAdaptor.java | 22 ++++- scripts/vm/hypervisor/kvm/nasbackup.sh | 82 +++++++++++++------ 5 files changed, 119 insertions(+), 26 deletions(-) diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java index 17c164d027cf..45c9b7bd9867 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java @@ -193,12 +193,14 @@ private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String backup volumeUuid = getVolumeUuidFromPath(volumePath, volumePool); verifyBackupFile(bkpPath, volumeUuid); Pair restoreResult = replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath, timeout, true, size); + volumePath = restoreResult.second(); if (!restoreResult.first()) { + cleanupCreatedVolume(storagePoolMgr, volumePool, volumePath); throw new CloudRuntimeException(String.format("Unable to restore contents from the backup volume [%s].", volumeUuid)); } - volumePath = restoreResult.second(); if (VirtualMachine.State.Running.equals(vmNameAndState.second())) { if (!attachVolumeToVm(storagePoolMgr, vmNameAndState.first(), volumePool, volumePath)) { + cleanupCreatedVolume(storagePoolMgr, volumePool, volumePath); throw new CloudRuntimeException(String.format("Failed to attach volume to VM: %s", vmNameAndState.first())); } } @@ -209,6 +211,22 @@ private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String backup } } + /** + * Best-effort release of a volume created for a restore that then failed. A failed restore + * returns no volume path to the management server, so for storages that assign the created + * volume's identity themselves (StorPool) nobody else could find and delete it. + */ + private void cleanupCreatedVolume(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath) { + try { + if (!storagePoolMgr.cleanupCreatedPhysicalDisk(volumePool.getPoolType(), volumePool.getUuid(), volumePath)) { + logger.warn("Volume {} created for the failed restore was not cleaned up by the {} storage adaptor; it may need to be deleted manually", + volumePath, volumePool.getPoolType()); + } + } catch (Exception e) { + logger.warn("Failed to clean up volume {} created for the failed restore; it may need to be deleted manually", volumePath, e); + } + } + private String mountBackupDirectory(String backupRepoAddress, String backupRepoType, String mountOptions, Integer mountTimeout) { String randomChars = RandomStringUtils.random(5, true, false); diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java index d99847fd921a..892ac872004a 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStoragePoolManager.java @@ -443,6 +443,13 @@ public boolean disconnectPhysicalDisk(StoragePoolType type, String poolUuid, Str return adaptor.disconnectPhysicalDisk(volPath, pool); } + public boolean cleanupCreatedPhysicalDisk(StoragePoolType type, String poolUuid, String volPath) { + StorageAdaptor adaptor = getStorageAdaptor(type); + KVMStoragePool pool = adaptor.getStoragePool(poolUuid); + + return adaptor.cleanupCreatedPhysicalDisk(volPath, pool); + } + public synchronized boolean deleteStoragePool(StoragePoolType type, String uuid) { StorageAdaptor adaptor = getStorageAdaptor(type); if (type == StoragePoolType.NetworkFilesystem) { diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java index fb474a6bc7d3..899739eee827 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/StorageAdaptor.java @@ -152,4 +152,18 @@ default Pair unprepareStorageClient(String uuid, Map listPhysicalDisks(String storagePoolUuid, KVMStoragePool pool) { SP_LOG("StorPoolStorageAdaptor.listPhysicalDisks: uuid=%s, pool=%s", storagePoolUuid, pool); @@ -488,7 +508,7 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys try { attachOrDetachVolume("attach", "volume", volumePath); } catch (CloudRuntimeException e) { - volumeDelete(globalId); + volumeDelete("~" + globalId); throw e; } diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index aded790bdc41..ad719cd92f50 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -217,23 +217,61 @@ sp_wait_for_device_symlink() { return 1 } -# detach is retried (a live volume's detach can transiently fail while still in use), attach is -# not. A successful attach also waits for the device symlink. -sp_attach_detach_volume() { - local cmd="$1" name="$2" - local tries=10 i - for ((i = 0; i < tries; i++)); do - if [[ "$cmd" == "attach" ]]; then - storpool -M -B attach volume "$name" here onRemoteAttached export >>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; } - break - else - storpool -M -B detach volume "$name" here >>"$logFile" 2>&1 && return 0 - sleep 1 - fi - done +# A successful attach also waits for the device symlink. +sp_attach_volume() { + local name="$1" + storpool -M -B attach volume "$name" here onRemoteAttached export >>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; } return 1 } +sp_api_post() { + local query="$1" json="$2" + local key value host="" port="81" token="" + while IFS='=' read -r key value; do + case "$key" in + SP_API_HTTP_HOST) host="$value" ;; + SP_API_HTTP_PORT) port="$value" ;; + SP_AUTH_TOKEN) token="$value" ;; + esac + done < <(storpool_confget 2>>"$logFile") + if [[ -z "$host" || -z "$token" ]]; then + log -ne "StorPool: SP_API_HTTP_HOST or SP_AUTH_TOKEN missing from storpool_confget, cannot call $query" + return 1 + fi + + local resp + if ! resp=$(curl -sS --connect-timeout 10 -X POST \ + -H @<(printf 'Authorization: Storpool v1:%s\n' "$token") \ + -H "Content-Type: application/json" -d "$json" \ + "http://$host:$port/ctrl/1.0/$query" 2>>"$logFile"); then + log -ne "StorPool: API call $query failed, see above for curl output" + return 1 + fi + python3 -c ' +import sys, json +try: + resp = json.load(sys.stdin) +except Exception: + sys.exit(1) +if "error" in resp or "data" not in resp: + sys.exit(1) +print(json.dumps(resp["data"])) +' <<< "$resp" || { + log -ne "StorPool: API call $query returned an error: $resp" + return 1 + } +} + +# Deletes $1, force-detaching it first wherever it is still attached (StorPool refuses to delete +# an attached volume). Returns 1, after logging, if the delete failed. +sp_delete_volume() { + local volume_name="$1" + if ! sp_api_post "MultiCluster/VolumeDelete/$volume_name" '{"onAttached":"detachForce"}' >/dev/null; then + log -ne "StorPool: failed to delete volume $volume_name, it must be deleted manually" + return 1 + fi +} + # Clones $1 (a live StorPool volume path) into a new, point-in-time StorPool volume via the # VolumeCreate API's baseOn option, attaches it, and echoes the clone's device path. Only called # once this script's own (fresh, right-here) VM liveness check has already routed to the cold @@ -249,8 +287,8 @@ sp_create_backup_source_disk() { log -ne "StorPool: creating backup source volume based on $base_on (from $volume_path)" local resp - if ! resp=$(storpool_req -P -M --json "{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}" VolumeCreate 2>>"$logFile"); then - log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above for storpool_req output" + if ! resp=$(sp_api_post "MultiCluster/VolumeCreate" "{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}"); then + log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above" echo "Could not create a backup source volume based on $base_on" >&2 return 1 fi @@ -275,9 +313,9 @@ print(gid) local clone_path="/dev/storpool-byid/$global_id" local clone_name="~$global_id" log -ne "StorPool: created backup source volume $clone_name ($clone_path), attaching" - if ! sp_attach_detach_volume "attach" "$clone_name"; then + if ! sp_attach_volume "$clone_name"; then log -ne "StorPool: failed to attach backup source volume $clone_name, deleting it" - storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || true + sp_delete_volume "$clone_name" || true echo "Could not attach backup source volume $clone_path cloned from $base_on" >&2 return 1 fi @@ -290,16 +328,12 @@ print(gid) echo "$clone_path" } -# Detach + delete a volume previously returned by sp_create_backup_source_disk. Best-effort: +# Deletes a volume previously returned by sp_create_backup_source_disk. Best-effort: # logs on failure rather than aborting the backup, matching the wrapper's old cleanup semantics. sp_delete_backup_source_disk() { local clone_name="$1" log -ne "StorPool: releasing backup source volume $clone_name" - sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name" - if ! storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1; then - log -ne "Failed to delete StorPool backup source volume $clone_name" - return 1 - fi + sp_delete_volume "$clone_name" } # Safety net for any clone not already released by its own per-disk cleanup (e.g. the script From a18fb4ab3056c9eac7d8c1a2dfd180c594c2553a Mon Sep 17 00:00:00 2001 From: Slavka Peleva Date: Tue, 20 Oct 2026 08:49:44 +0300 Subject: [PATCH 4/4] addressed Copilot reviews --- .../backup/NASBackupProviderTest.java | 6 +++--- .../kvm/storage/StorPoolStorageAdaptor.java | 4 ++++ scripts/vm/hypervisor/kvm/nasbackup.sh | 7 ++++++- .../plugins/storpool/TestNasBackupStorPool.py | 18 +++++++++++------- test/integration/plugins/storpool/sp_util.py | 16 ++++++++++------ 5 files changed, 34 insertions(+), 17 deletions(-) diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java index ce3ac5ea8db0..10343bae41c5 100644 --- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -698,11 +698,11 @@ private void mockSourceVolume(String volUuid, String name) { Mockito.when(volumeDao.findByUuid(volUuid)).thenReturn(srcVolume); } - private void mockThinDiskOffering(Long diskOfferingId) { + private void mockThinDiskOffering(String diskOfferingUuid, Long diskOfferingId) { DiskOfferingVO diskOffering = mock(DiskOfferingVO.class); Mockito.when(diskOffering.getId()).thenReturn(diskOfferingId); Mockito.when(diskOffering.getProvisioningType()).thenReturn(Storage.ProvisioningType.THIN); - Mockito.when(diskOfferingDao.findByUuid(Mockito.anyString())).thenReturn(diskOffering); + Mockito.when(diskOfferingDao.findByUuid(diskOfferingUuid)).thenReturn(diskOffering); } private void mockStorPoolPoolByUuid(String dsUuid, Long poolId) { @@ -772,7 +772,7 @@ public void restoreBackedUpVolumeUsesQcow2FormatAndRealPathForStorPool() String realStorPoolPath = "/dev/storpool-byid/t.t.t"; mockSourceVolume(volUuid, "data-sp"); - mockThinDiskOffering(6L); + mockThinDiskOffering("disk-offering-uuid", 6L); mockStorPoolPoolByUuid(dsUuid, poolId); mockHostByIp(hostIp, 8L); mockNasRepository(backupOfferingId); diff --git a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java index 54534d9244ef..1817dd12fc53 100644 --- a/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java +++ b/plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java @@ -480,6 +480,10 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys throw new CloudRuntimeException("Encrypted StorPool volumes are not supported"); } + if (!PhysicalDiskFormat.RAW.equals(format)) { + throw new CloudRuntimeException(String.format("StorPool volumes can only be created as RAW, %s was requested", format)); + } + if (size <= 0) { throw new CloudRuntimeException("Size must be greater than 0 to create a StorPool volume"); } diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh index ad719cd92f50..faff7546c4cd 100755 --- a/scripts/vm/hypervisor/kvm/nasbackup.sh +++ b/scripts/vm/hypervisor/kvm/nasbackup.sh @@ -689,7 +689,12 @@ backup_stopped_vm() { cleanup exit 1 fi - sp_clone_name=$(sp_volume_name_from_path "$read_disk") + if ! sp_clone_name=$(sp_volume_name_from_path "$read_disk"); then + log -ne "Failed to resolve the StorPool volume name of backup source volume $read_disk, it must be deleted manually" + echo "Failed to resolve the StorPool volume name of backup source volume $read_disk" + cleanup + exit 1 + fi # Register the clone here, in the parent shell, right after the command substitution # above returns — sp_create_backup_source_disk cannot do this itself (see its comment). SP_CLEANUP_VOLUMES+=("$sp_clone_name") diff --git a/test/integration/plugins/storpool/TestNasBackupStorPool.py b/test/integration/plugins/storpool/TestNasBackupStorPool.py index feb4cae415dc..c922d346a851 100644 --- a/test/integration/plugins/storpool/TestNasBackupStorPool.py +++ b/test/integration/plugins/storpool/TestNasBackupStorPool.py @@ -89,7 +89,8 @@ def _setup_common(cls): StorPoolHelper.logger = cls zone_cfg = config.zones[0] - assert zone_cfg is not None + if zone_cfg is None: + raise Exception("No zone configured in the Marvin config") cls.zone_cfg = zone_cfg testClient = super(TestNasBackupStorPool, cls).getClsTestClient() @@ -106,8 +107,10 @@ def _setup_common(cls): cls.services = testClient.getParsedTestDataConfig() cls.domain = get_domain(cls.apiclient) - cls.zone = list_zones(cls.apiclient, name=zone_cfg.name)[0] - assert cls.zone is not None + zones = list_zones(cls.apiclient, name=zone_cfg.name) + if not zones: + raise Exception("Zone %s not found" % zone_cfg.name) + cls.zone = zones[0] cls.services["mode"] = cls.zone.networktype cls.hypervisor = get_hypervisor_type(cls.apiclient) @@ -127,7 +130,7 @@ def _setup_common(cls): template = get_template(cls.apiclient, cls.zone.id, account="system") if template == FAILED: - assert False, "get_template() failed to return a system template" + raise Exception("get_template() failed to return a system template") cls.template = template offerings = BackupOffering.listByZone(cls.apiclient, cls.zone.id) or [] @@ -202,7 +205,7 @@ def _setup_common(cls): @classmethod def _get_or_create_disk_offering(cls, tag, displaytext): disk_offerings = list_disk_offering(cls.apiclient, name=tag) - if disk_offerings is None: + if not disk_offerings: disk_offering = DiskOffering.create( cls.apiclient, { @@ -237,7 +240,8 @@ def _create_test_vm(cls, name, diskofferingid): @classmethod def _verify_setup_volumes(cls, vm): volumes = Volume.list(cls.apiclient, virtualmachineid=vm.id, listall=True) - assert isinstance(volumes, list) and len(volumes) >= 1 + if not isinstance(volumes, list) or not volumes: + raise Exception("No volumes found for VM %s" % vm.id) for vol in volumes: if cls.nfs_storage_pool and vol.storageid == cls.nfs_storage_pool.id: continue @@ -678,7 +682,7 @@ def _setup_nfs_environment(cls): @classmethod def _get_or_create_nfs_service_offering(cls): service_offerings = list_service_offering(cls.apiclient, name=cls.nfs_tag) - if service_offerings is None: + if not service_offerings: service_offering = ServiceOffering.create( cls.apiclient, { diff --git a/test/integration/plugins/storpool/sp_util.py b/test/integration/plugins/storpool/sp_util.py index 280a7797386f..33b18155c6fa 100644 --- a/test/integration/plugins/storpool/sp_util.py +++ b/test/integration/plugins/storpool/sp_util.py @@ -710,16 +710,20 @@ def check_storpool_volume_size(cls, volume, spapi): @classmethod def check_storpool_volume_iops(cls, spapi, volume,): - spvolume = cls.get_storpool_volume_by_path(spapi, volume.path) - logging.debug(spvolume.iops) - logging.debug(volume.maxiops) - if spvolume.iops != volume.maxiops: - raise Exception("Storpool volume size is not the same as CloudStack db size") + name = volume.path.split("/")[3] + try: + spvolume = spapi.volumeList(volumeName = "~" + name) + logging.debug(spvolume[0].iops) + logging.debug(volume.maxiops) + if spvolume[0].iops != volume.maxiops: + raise Exception("Storpool volume size is not the same as CloudStack db size") + except spapi.ApiError as err: + raise Exception(err) @classmethod def verify_storpool_volume(cls, spapi, volume, check_size=True): """Assert the CloudStack volume exists on StorPool; optionally compare size.""" - if not volume.path or not volume.path.startswith("/dev/storpool"): + if not volume.path or not volume.path.startswith("/dev/storpool-byid/"): raise Exception("Volume %s does not have a StorPool device path: %s" % (volume.id, volume.path)) spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)