Conversation
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=<bytes> 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.
boring-cyborg
Bot
added
component:backup
component:integration-test
component:kvm
component:primary-storage
component:storage
Python
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Restore attachment compatibility and StorPool volume cleanup failures remain unresolved.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds StorPool support to KVM NAS backup and restore workflows.
Changes:
- Adds StorPool volume provisioning and restored-path reporting.
- Adds point-in-time cloning for cold backups.
- Improves backup size reporting, cleanup, and error handling.
- Expands integration and provider tests.
File summaries
| File | Summary |
|---|---|
test/integration/plugins/storpool/TestNasBackupStorPool.py |
StorPool NAS backup/restore tests |
test/integration/plugins/storpool/sp_util.py |
StorPool test helpers |
scripts/vm/hypervisor/kvm/nasbackup.sh |
StorPool cloning and cleanup |
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java |
StorPool volume provisioning |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java |
Backup output and size parsing |
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java |
StorPool restore handling |
plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java |
Provider restore-path tests |
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java |
Backup and restore path propagation |
core/src/main/java/org/apache/cloudstack/backup/BackupAnswer.java |
Restored-volume path response |
Review details
Suppressed comments (3)
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:362
- If
qemu-img convertfails after this StorPool volume has been created and attached,replaceBlockDeviceWithBackuponly returns a failed answer; it never detaches or deletes the newly provisioned volume. Because the restore does not persist a CloudStack volume on this path, every failed restore can leave an attached, orphaned StorPool volume. Track the created disk and clean it up on all subsequent failure paths (or add an explicit rollback around the conversion).
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();
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:490
attachOrDetachVolume()throwsCloudRuntimeExceptionwhen the StorPool attach command fails, so theif (!...)branch is not entered in that failure mode. The newly created volume is therefore left behind even though restore reports failure, leaking an unattached StorPool volume on every attach exception; wrap the attach in cleanup logic that deletesglobalIdfor both a false return and an exception.
if (!attachOrDetachVolume("attach", "volume", volumePath)) {
volumeDelete(globalId);
throw new CloudRuntimeException(String.format("Could not attach newly created StorPool volume %s", volumePath));
scripts/vm/hypervisor/kvm/nasbackup.sh:582
- Because
read_disk=$(sp_create_backup_source_disk ...)executes the function in a command-substitution subshell, the function's append toSP_CLEANUP_VOLUMESis lost when it returns. Ifqemu-img convertor a later per-disk step fails, the parent EXIT trap therefore has no clone name to release, leaving the attached backup-source StorPool volume orphaned. Return the path through a variable/file without command substitution (or otherwise register the clone in the parent shell) before proceeding.
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")
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+318
to
319
| if (List.of(Storage.StoragePoolType.RBD, Storage.StoragePoolType.Linstor, Storage.StoragePoolType.StorPool).contains(volumePool.getPoolType())) { | ||
| return replaceBlockDeviceWithBackup(storagePoolMgr, volumePool, volumePath, backupPath, timeout, createTargetVolume, size); |
Comment on lines
+595
to
+602
| 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[@]}") |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14203 +/- ##
============================================
+ Coverage 19.90% 19.92% +0.01%
- Complexity 20171 20207 +36
============================================
Files 6372 6373 +1
Lines 577180 577311 +131
Branches 70693 70713 +20
============================================
+ Hits 114869 115004 +135
+ Misses 449755 449736 -19
- Partials 12556 12571 +15
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| * 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<VolumeVO> volumes) { |
Member
There was a problem hiding this comment.
I would suggest to pass pooltype as a parameter of the method
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
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.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
How Has This Been Tested?
Manual testing and smoke tests with StorPool as a primary storage, KVM hypervisors