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✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #14203 +/- ##
=============================================
- Coverage 19.90% 3.70% -16.20%
=============================================
Files 6372 493 -5879
Lines 577180 42544 -534636
Branches 70693 8065 -62628
=============================================
- Hits 114869 1577 -113292
+ Misses 449755 40741 -409014
+ Partials 12556 226 -12330
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
Contributor
Author
There was a problem hiding this comment.
@Damans227, it is almost impossible this to happen but I added in the VolumeDelete request to detach it force before deleting it
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
Contributor
Author
There was a problem hiding this comment.
thank you @Damans227, good catch here! I've added to delete the StorPool's volume on fail, but I think the rest of the supported storage pools will have the same issue that you pointed
Copilot stopped reviewing on behalf of
slavkap due to an error
October 13, 2026 16:53
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 "~<globalId>". 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.
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
22 open issues
sp_volume_name_from_path "$read_disk"can fail and return a non-zero exit code (or an empty…attachVolumeToVmfor a running target,…findById) which is an N+1 pattern and runs on every… · Newnasbackup.sh(or any environment where… · Newassertin setup. In Python,assertstatements are skipped when… · Newassertin setup. In Python,assertstatements are skipped when… · Newcheck_storpool_volume_iopson an IOPS mismatch, but it… · Newget_nfs_pool()assumesstorage['provider'](andstorage['url']) keys always exist, but the… · Newstorage['url']/storage['provider']will raiseKeyErrorif a Marvin cfg entry…verify_storpool_volume()accepts any path starting with/dev/storpool, but…check_storpool_volume_iops()reports a size mismatch even though the…IFSin the function scope and doesn’t restore it, which can subtly break later…BACKUP_SIZE_TOTAL=stdout…findByIdper volume) on the hot path oftakeBackup().…list_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…
And 2 more that still need to be addressed.
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; | ||
| } |
Comment on lines
+164
to
+179
| 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; | ||
| } |
Comment on lines
+91
to
+92
| zone_cfg = config.zones[0] | ||
| assert zone_cfg is not None |
Comment on lines
+109
to
+110
| cls.zone = list_zones(cls.apiclient, name=zone_cfg.name)[0] | ||
| assert cls.zone is not None |
| 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") |
Comment on lines
+962
to
+963
| url = storage['url'] or "" | ||
| if not storage['provider'] and str(url).lower().startswith("nfs://"): |
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