Repository navigation
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% 20.21% +0.31%
- Complexity 20171 20755 +584
============================================
Files 6372 6426 +54
Lines 577180 580269 +3089
Branches 70693 71051 +358
============================================
+ Hits 114869 117310 +2441
- Misses 449755 450242 +487
- Partials 12556 12717 +161
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
There was a problem hiding this comment.
for example
hasVolumeWithStorageType(volumes, StorPool)
Contributor
Author
There was a problem hiding this comment.
thank you @weizhouapache, for the clarification! I already did that as you suggested, but I didn't have time to test the changes and push them.
Member
Contributor
|
Just tested this against #14194 And nothing breaks here against Linstor (didn't test anything else). just that whatever patch lands first, the final merge will need this: |
| 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); |
Contributor
There was a problem hiding this comment.
if-else can be optimized with switch as you did in below changes
1 of 14 tasks
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
9 open findings
attachVolumeToVmfor a running target,…primaryDataStoreDao.findById(...)to look up aStoragePoolVO, while nearby… · NewcreatePhysicalDisk(...)receives a requestedformatbut currently ignores it and always… · NewcreatePhysicalDisk(...)receives a requestedformatbut currently ignores it and always… · Newlist_disk_offering(...)can return an empty list (not justNone). In that case… · Newlist_service_offering(...)may return[], which would… · Newsp_clone_namefrom…mockThinDiskOffering(Long diskOfferingId)accepts an ID and sets it on the mocked… · New
🧠 Review effort: Lite
Comment on lines
+389
to
+400
| private boolean hasVolumeOnPoolType(List<VolumeVO> volumes, Storage.StoragePoolType poolType) { | ||
| if (volumes == null) { | ||
| return false; | ||
| } | ||
| for (VolumeVO volume : volumes) { | ||
| StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId()); | ||
| if (pool != null && poolType.equals(pool.getPoolType())) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
| if (passphrase != null) { | ||
| throw new CloudRuntimeException("Encrypted StorPool volumes are not supported"); | ||
| } | ||
|
|
Comment on lines
+495
to
+499
| KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool); | ||
| disk.setFormat(PhysicalDiskFormat.RAW); | ||
| disk.setSize(size); | ||
| disk.setVirtualSize(size); | ||
| return disk; |
Comment on lines
+203
to
+217
| 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] |
Comment on lines
+679
to
+695
| 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] |
| 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); |
| 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") |
Copilot stopped reviewing on behalf of
slavkap due to an error
October 12, 2026 17:33
slavkap
force-pushed
the
sp-nas-backup
branch
from
October 13, 2026 11:32
6149044 to
93238b0
Compare
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
16 open findings
sp_volume_name_from_path "$read_disk"can fail and return a non-zero exit code (or an empty… · NewattachVolumeToVmfor a running target,…findByIdper volume) on the hot path oftakeBackup().… · NewBACKUP_SIZE_TOTAL=stdout… · NewIFSin the function scope and doesn’t restore it, which can subtly break later… · Newcheck_storpool_volume_iops()reports a size mismatch even though the… · Newverify_storpool_volume()accepts any path starting with/dev/storpool, but… · Newstorage['url']/storage['provider']will raiseKeyErrorif a Marvin cfg entry… · Newlist_service_offering(...)may return[], which would…list_disk_offering(...)can return an empty list (not justNone). In that case…createPhysicalDisk(...)receives a requestedformatbut currently ignores it and always…createPhysicalDisk(...)receives a requestedformatbut currently ignores it and always…primaryDataStoreDao.findById(...)to look up aStoragePoolVO, while nearby…sp_clone_namefrom…mockThinDiskOffering(Long diskOfferingId)accepts an ID and sets it on the mocked…
🧠 Review effort: Lite
| cleanup | ||
| exit 1 | ||
| fi | ||
| sp_clone_name=$(sp_volume_name_from_path "$read_disk") |
Comment on lines
+389
to
+400
| private boolean hasVolumeOnPoolType(List<VolumeVO> volumes, Storage.StoragePoolType poolType) { | ||
| if (volumes == null) { | ||
| return false; | ||
| } | ||
| for (VolumeVO volume : volumes) { | ||
| StoragePoolVO pool = primaryDataStoreDao.findById(volume.getPoolId()); | ||
| if (pool != null && poolType.equals(pool.getPoolType())) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
Comment on lines
+114
to
+124
| // 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); |
Comment on lines
+561
to
+566
| 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" |
| 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"): |
Comment on lines
+959
to
+964
| 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) |
Damans227
reviewed
Oct 13, 2026
| 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 |
Collaborator
There was a problem hiding this comment.
if the attach works but the device doesnt show up in time, wont this delete fail on a still attached clone? it isnt in the cleanup list yet so it stays behind
Damans227
reviewed
Oct 13, 2026
| if (!replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath, timeout, true, size)) { | ||
| Pair<Boolean, String> 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)); |
Collaborator
There was a problem hiding this comment.
if the copy fails here, does anything delete the new storpool volume? cloudstack never gets its id back so it looks like it stays behind
Copilot stopped reviewing on behalf of
slavkap due to an error
October 13, 2026 16:53
This branch has not been deployed
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