Skip to content

Commit 93238b0

Browse files
slavkapslavkap
committed
addressed reviews
1 parent 2c92601 commit 93238b0

6 files changed

Lines changed: 112 additions & 24 deletions

File tree

‎plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java‎

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -384,17 +384,15 @@ protected boolean allVolumesOnCheckpointCapableStorage(VirtualMachine vm) {
384384
}
385385

386386
/**
387-
* True when any of the VM's volumes sits on a StorPool pool. Used to decide whether volume
388-
* pool/path info must be sent to the agent even though the VM currently looks Running — see
389-
* the caller in {@link #takeBackup}.
387+
* True when any of the given volumes sits on a pool of the given type.
390388
*/
391-
private boolean hasStorPoolVolume(List<VolumeVO> volumes) {
389+
private boolean hasVolumeOnPoolType(List<VolumeVO> volumes, Storage.StoragePoolType poolType) {
392390
if (volumes == null) {
393391
return false;
394392
}
395393
for (VolumeVO volume : volumes) {
396394
StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId());
397-
if (pool != null && Storage.StoragePoolType.StorPool.equals(pool.getPoolType())) {
395+
if (pool != null && poolType.equals(pool.getPoolType())) {
398396
return true;
399397
}
400398
}
@@ -613,7 +611,7 @@ public Pair<Boolean, Backup> takeBackup(final VirtualMachine vm, Boolean quiesce
613611
// — without it there is nothing to derive that from.
614612
boolean vmStopped = VirtualMachine.State.Stopped.equals(vm.getState());
615613
List<VolumeVO> vmVolumes = volumeDao.findByInstance(vm.getId());
616-
boolean hasStorPoolVolume = hasStorPoolVolume(vmVolumes);
614+
boolean hasStorPoolVolume = hasVolumeOnPoolType(vmVolumes, Storage.StoragePoolType.StorPool);
617615
if (vmStopped || hasStorPoolVolume) {
618616
if (!vmStopped) {
619617
logger.debug("VM {} looks {} but has a StorPool volume — sending volume pool/path info to the agent anyway, "

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

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -363,12 +363,15 @@ private Pair<Boolean, String> replaceBlockDeviceWithBackup(KVMStoragePoolManager
363363
}
364364
}
365365
} else {
366-
if (Storage.StoragePoolType.Linstor.equals(poolType)) {
367-
storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumeUuid, null);
368-
} else if (Storage.StoragePoolType.StorPool.equals(poolType)) {
369-
storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumePath, null);
370-
} else {
371-
disk = volumeStoragePool.getPhysicalDisk(volumePath);
366+
switch (poolType) {
367+
case Linstor:
368+
storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumeUuid, null);
369+
break;
370+
case StorPool:
371+
storagePoolMgr.connectPhysicalDisk(poolType, volumePool.getUuid(), volumePath, null);
372+
break;
373+
default:
374+
disk = volumeStoragePool.getPhysicalDisk(volumePath);
372375
}
373376
qemu.setSkipTargetVolumeCreation(true);
374377
}
@@ -422,7 +425,7 @@ private boolean attachVolumeToVm(KVMStoragePoolManager storagePoolMgr, String vm
422425
virshCmd.add(deviceToAttachDiskTo);
423426
virshCmd.add("--driver");
424427
virshCmd.add("qemu");
425-
if (!Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
428+
if (!List.of(Storage.StoragePoolType.Linstor, Storage.StoragePoolType.StorPool).contains(volumePool.getPoolType())) {
426429
virshCmd.add("--subdriver");
427430
virshCmd.add("qcow2");
428431
}

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -695,6 +695,17 @@ public void testAttachVolumeOmitsQcow2SubdriverForLinstor() throws Exception {
695695
Assert.assertFalse(args.contains("--subdriver"));
696696
}
697697

698+
@Test
699+
public void testAttachVolumeOmitsQcow2SubdriverForStorPool() throws Exception {
700+
String[] cmd = captureAttachCommand(Storage.StoragePoolType.StorPool);
701+
List<String> args = Arrays.asList(cmd);
702+
703+
// StorPoolStorageAdaptor#createPhysicalDisk always provisions RAW block devices,
704+
// declaring qcow2 makes libvirt try to parse the raw data as a qcow2 image.
705+
Assert.assertTrue(args.contains("attach-disk"));
706+
Assert.assertFalse(args.contains("--subdriver"));
707+
}
708+
698709
@Test
699710
public void testAttachVolumePassesRbdXmlThroughAFile() throws Exception {
700711
String[] cmd = captureAttachCommand(Storage.StoragePoolType.RBD);

‎plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -485,9 +485,11 @@ public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool pool, Phys
485485
}
486486

487487
String volumePath = StorPoolUtil.devPath(globalId);
488-
if (!attachOrDetachVolume("attach", "volume", volumePath)) {
488+
try {
489+
attachOrDetachVolume("attach", "volume", volumePath);
490+
} catch (CloudRuntimeException e) {
489491
volumeDelete(globalId);
490-
throw new CloudRuntimeException(String.format("Could not attach newly created StorPool volume %s", volumePath));
492+
throw e;
491493
}
492494

493495
KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool);

‎scripts/vm/hypervisor/kvm/nasbackup.sh‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,10 @@ print(gid)
233233
return 1
234234
fi
235235

236-
SP_CLEANUP_VOLUMES+=("$clone_name")
236+
# Not registered in SP_CLEANUP_VOLUMES here: the caller always invokes this function via
237+
# command substitution ($(...)), which runs it in a subshell — any append to the array here
238+
# would be a mutation of that subshell's own copy and vanish once it exits. The caller must
239+
# register $clone_name itself right after reading it back from stdout below.
237240
log -ne "StorPool: backup source volume $clone_name attached at $clone_path, reading from it for this backup"
238241
echo "$clone_path"
239242
}
@@ -244,7 +247,10 @@ sp_delete_backup_source_disk() {
244247
local clone_name="$1"
245248
log -ne "StorPool: releasing backup source volume $clone_name"
246249
sp_attach_detach_volume "detach" "$clone_name" || log -ne "Failed to detach StorPool backup source volume $clone_name"
247-
storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1 || log -ne "Failed to delete StorPool backup source volume $clone_name"
250+
if ! storpool_req -P -M VolumeDelete "$clone_name" >>"$logFile" 2>&1; then
251+
log -ne "Failed to delete StorPool backup source volume $clone_name"
252+
return 1
253+
fi
248254
}
249255

250256
# Safety net for any clone not already released by its own per-disk cleanup (e.g. the script
@@ -580,6 +586,9 @@ backup_stopped_vm() {
580586
exit 1
581587
fi
582588
sp_clone_name=$(sp_volume_name_from_path "$read_disk")
589+
# Register the clone here, in the parent shell, right after the command substitution
590+
# above returns — sp_create_backup_source_disk cannot do this itself (see its comment).
591+
SP_CLEANUP_VOLUMES+=("$sp_clone_name")
583592
else
584593
volUuid="${disk##*/}"
585594
fi
@@ -593,13 +602,16 @@ backup_stopped_vm() {
593602
log -ne "Wrote $output from $read_disk"
594603

595604
if [[ -n "$sp_clone_name" ]]; then
596-
sp_delete_backup_source_disk "$sp_clone_name"
597-
local -a remaining_clones=()
598-
local tracked_clone
599-
for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do
600-
[[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone")
601-
done
602-
SP_CLEANUP_VOLUMES=("${remaining_clones[@]}")
605+
# Only drop the clone from SP_CLEANUP_VOLUMES once it's actually deleted — otherwise the
606+
# EXIT trap's safety net would never see (and retry) a clone left behind by a failed delete.
607+
if sp_delete_backup_source_disk "$sp_clone_name"; then
608+
local -a remaining_clones=()
609+
local tracked_clone
610+
for tracked_clone in "${SP_CLEANUP_VOLUMES[@]}"; do
611+
[[ "$tracked_clone" == "$sp_clone_name" ]] || remaining_clones+=("$tracked_clone")
612+
done
613+
SP_CLEANUP_VOLUMES=("${remaining_clones[@]}")
614+
fi
603615
fi
604616

605617
# Pre-seed a persistent bitmap on the source disk so the NEXT backup (taken

‎test/integration/plugins/storpool/TestNasBackupStorPool.py‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -556,6 +556,68 @@ def test_05_backup_stopped_vm(self):
556556

557557
self.backup_offering.removeOffering(self.apiclient, self.vm.id)
558558

559+
@attr(tags=["advanced", "backup", "storpool"], required_hardware="true")
560+
def test_06_restore_volume_attach_to_running_vm(self):
561+
"""Restore a volume from backup and hot-attach it to an already-Running VM.
562+
563+
Regression test for LibvirtRestoreBackupCommandWrapper.attachVolumeToVm only
564+
excluding LINSTOR from the qcow2 --subdriver flag: a restored StorPool volume
565+
is a RAW block device (like LINSTOR), so attaching it with --subdriver qcow2
566+
makes virsh try to open the raw device as a qcow2 image. test_03 never caught
567+
this because it attaches to a target VM that is still Stopped at attach time.
568+
"""
569+
self.backup_offering.assignOffering(self.apiclient, self.vm.id)
570+
571+
marker = "storpool_nas_running_attach_%s.txt" % int(time.time())
572+
self._write_guest_marker(self.vm, marker)
573+
time.sleep(5)
574+
backup = self._create_backup(self.vm.id)
575+
self.cleanup_backups.append(backup)
576+
577+
volumes = Volume.list(self.apiclient, virtualmachineid=self.vm.id, listall=True)
578+
data_disk = next((v for v in volumes if v.type == "DATADISK"), None)
579+
self.assertIsNotNone(data_disk, "Source VM should have a data disk")
580+
581+
# No startvm=False here: the target VM must already be Running when
582+
# restoreVolumeFromBackupAndAttachToVM is called, to exercise the
583+
# live virsh attach-disk path instead of an offline volume write.
584+
target_vm = VirtualMachine.create(
585+
self.apiclient,
586+
{"name": "SP-NasRunAttach-%s" % uuid.uuid4()},
587+
zoneid=self.zone.id,
588+
templateid=self.template.id,
589+
accountid=self.account.name,
590+
domainid=self.account.domainid,
591+
serviceofferingid=self.service_offering.id,
592+
hypervisor=self.hypervisor,
593+
networkids=self.network_ids,
594+
rootdisksize=self.template_size_gb,
595+
mode=self.services["mode"])
596+
self.cleanup.append(target_vm)
597+
598+
listed = VirtualMachine.list(self.apiclient, id=target_vm.id, listall=True) or []
599+
self.assertTrue(listed and listed[0].state == "Running",
600+
"Target VM should be Running before the volume is hot-attached")
601+
602+
Backup.restoreVolumeFromBackupAndAttachToVM(
603+
self.apiclient,
604+
backupid=backup.id,
605+
volumeid=data_disk.id,
606+
virtualmachineid=target_vm.id)
607+
608+
target_volumes = self._verify_vm_volumes_on_storpool(target_vm)
609+
self.assertEqual(2, len(target_volumes),
610+
"Target VM should have ROOT + hot-attached restored DATADISK volumes")
611+
self._assert_guest_reachable(
612+
target_vm, msg="VM should stay reachable after a volume is hot-attached from backup")
613+
614+
ssh = target_vm.get_ssh_client(reconnect=True)
615+
result = ssh.execute("lsblk -d -n -o NAME | grep -c .")
616+
self.assertTrue(result and int(result[0].strip()) >= 2,
617+
"Guest should see the hot-attached disk after restore")
618+
619+
self.backup_offering.removeOffering(self.apiclient, self.vm.id)
620+
559621

560622
class TestNasBackupMixedStorage(TestNasBackupStorPool):
561623
"""Repeats the NAS backup smoke tests (test_01 .. test_05, inherited

0 commit comments

Comments
 (0)