Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4047447537
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -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<Boolean, String>
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);
Review Comment:
Adding StorPool to the block-device restore path can reach
`attachVolumeToVm` for a running target, but that method currently adds
`--subdriver qcow2` for every pool except LINSTOR.
`StorPoolStorageAdaptor.createPhysicalDisk` returns a RAW
`/dev/storpool-byid/...` device, so attaching it as qcow2 can fail to open the
restored volume (or interpret its contents incorrectly). StorPool needs the
raw/block-device attach handling used for LINSTOR rather than the qcow2
subdriver.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -440,25 +569,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[@]}")
Review Comment:
When the per-disk release cannot detach or delete the clone, this loop
removes `sp_clone_name` from `SP_CLEANUP_VOLUMES` unconditionally. That defeats
the EXIT trap's safety net and can leave an attached/orphaned StorPool backup
volume after a transient cleanup failure. Only remove the entry after
confirming both operations succeeded, or retain it so the EXIT cleanup can
retry and report the failure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
