Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4258233644
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -440,25 +575,54 @@ 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")
Review Comment:
`sp_volume_name_from_path "$read_disk"` can fail and return a non-zero exit
code (or an empty string), but the code currently ignores that and still
appends `$sp_clone_name` to `SP_CLEANUP_VOLUMES`. This can cause missed cleanup
(no valid name to delete) and/or confusing log output. Make this mandatory by
checking the return status and failing the backup (or at least not registering
an empty name) if the clone name cannot be derived.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java:
##########
@@ -73,59 +77,70 @@ public Answer execute(TakeBackupCommand command,
LibvirtComputingResource libvir
}
List<String> diskPaths = new ArrayList<>();
- if (Objects.nonNull(volumePaths)) {
- for (int idx = 0; idx < volumePaths.size(); idx++) {
- PrimaryDataStoreTO volumePool = volumePools.get(idx);
- String volumePath = volumePaths.get(idx);
- if (volumePool.getPoolType() != Storage.StoragePoolType.RBD) {
+ try {
+ if (Objects.nonNull(volumePaths)) {
+ for (int idx = 0; idx < volumePaths.size(); idx++) {
+ PrimaryDataStoreTO volumePool = volumePools.get(idx);
+ String volumePath = volumePaths.get(idx);
+ if (volumePool.getPoolType() ==
Storage.StoragePoolType.RBD) {
+ KVMStoragePool volumeStoragePool =
storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid());
+ String rbdDestVolumeFile =
KVMPhysicalDisk.RBDStringBuilder(volumeStoragePool, volumePath);
+ diskPaths.add(rbdDestVolumeFile);
+ continue;
+ }
+ // StorPool (among others) is passed through as-is:
nasbackup.sh checks the
+ // VM's actual liveness itself right before acting, and
only then — if the VM
+ // turns out to be stopped — clones this into a
point-in-time backup source
+ // volume. Doing that here instead would rely on the same
stale state read
+ // nasbackup.sh's own check exists to correct for.
diskPaths.add(volumePath);
- } else {
- KVMStoragePool volumeStoragePool =
storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid());
- String rbdDestVolumeFile =
KVMPhysicalDisk.RBDStringBuilder(volumeStoragePool, volumePath);
- diskPaths.add(rbdDestVolumeFile);
}
}
- }
- Pair<Integer, String> result =
runBackupScript(libvirtComputingResource, command, vmName, backupRepoType,
backupRepoAddress,
- mountOptions, backupPath, diskPaths, command.getMode(),
- command.getBitmapNew(), command.getBitmapParent(),
command.getParentPaths(), timeout);
+ Pair<Integer, String> result =
runBackupScript(libvirtComputingResource, command, vmName, backupRepoType,
backupRepoAddress,
+ mountOptions, backupPath, diskPaths, command.getMode(),
+ command.getBitmapNew(), command.getBitmapParent(),
command.getParentPaths(), timeout);
- if (result.first() != 0) {
- logger.debug("Failed to take VM backup: " + result.second());
- BackupAnswer answer = new BackupAnswer(command, false,
StringUtils.trimToEmpty(result.second()));
- if (EXIT_CLEANUP_FAILED.equals(result.first())) {
- logger.debug("Backup cleanup failed");
- answer.setNeedsCleanup(true);
+ if (result.first() != 0) {
+ logger.debug("Failed to take VM backup: " + result.second());
+ BackupAnswer answer = new BackupAnswer(command, false,
StringUtils.trimToEmpty(result.second()));
+ if (EXIT_CLEANUP_FAILED.equals(result.first())) {
+ logger.debug("Backup cleanup failed");
+ answer.setNeedsCleanup(true);
+ }
+ return answer;
}
+
+ // 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);
Review Comment:
The success-path behavior now depends on parsing and stripping the new
`BACKUP_SIZE_TOTAL=` stdout marker; without a unit test, regressions could
silently report size=0 or leak marker lines into the details. Add a dedicated
test covering: (1) correct size extraction, (2) handling of missing/unparseable
marker (logs warning, size=0, still success), and (3) marker removal from
`details` while preserving other output.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -426,10 +558,13 @@ backup_stopped_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
- IFS=","
+ 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"
Review Comment:
This changes `IFS` in the function scope and doesn’t restore it, which can
subtly break later word-splitting behavior in the script. Set `IFS` only for
the `read` invocation (e.g., `local IFS=','`) or save/restore the prior value
to avoid side effects.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,26 +683,50 @@ def destroy_vm(self, apiclient, virtualmachineid):
apiclient.destroyVirtualMachine(cmd)
@classmethod
- def check_storpool_volume_size(cls, volume, spapi):
- name = volume.path.split("/")[3]
+ def get_storpool_volume_by_path(cls, spapi, path):
+ """
+ Resolve a CloudStack volume path (/dev/storpool-byid/<globalId>) to a
+ StorPool volume.
+ """
+ if not path:
+ raise Exception("Volume path is empty")
+ parts = path.rstrip("/").split("/")
+ if len(parts) < 4:
+ raise Exception("Unexpected StorPool volume path: %s" % path)
+ name = "~" + parts[3]
try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- if spvolume[0].size != volume.size:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ volumes = spapi.volumeList(volumeName=name)
+ except Exception as err:
+ raise Exception("StorPool volume not found for path %s: %s" %
(path, err))
+ if not volumes:
+ raise Exception("StorPool volume not found for path %s" % path)
+ return volumes[0]
+
+ @classmethod
+ def check_storpool_volume_size(cls, volume, spapi):
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ if spvolume.size != volume.size:
+ raise Exception("Storpool volume size is not the same as
CloudStack db size")
@classmethod
def check_storpool_volume_iops(cls, spapi, volume,):
- name = volume.path.split("/")[3]
- try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- logging.debug(spvolume[0].iops)
- logging.debug(volume.maxiops)
- if spvolume[0].iops != volume.maxiops:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ 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")
Review Comment:
The exception message in `check_storpool_volume_iops()` reports a size
mismatch even though the check is for IOPS. Update the message to indicate an
IOPS mismatch and include the expected/actual values to make failures
actionable.
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -383,6 +383,22 @@ protected boolean
allVolumesOnCheckpointCapableStorage(VirtualMachine vm) {
return true;
}
+ /**
+ * True when any of the given volumes sits on a pool of the given type.
+ */
+ 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;
+ }
Review Comment:
This introduces an N+1 DAO lookup (one `findById` per volume) on the hot
path of `takeBackup()`. Consider fetching all referenced pools in one call
(e.g., query by IDs and build a map) or using an existing join-based DAO method
if available to avoid repeated DB round-trips.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,26 +683,50 @@ def destroy_vm(self, apiclient, virtualmachineid):
apiclient.destroyVirtualMachine(cmd)
@classmethod
- def check_storpool_volume_size(cls, volume, spapi):
- name = volume.path.split("/")[3]
+ def get_storpool_volume_by_path(cls, spapi, path):
+ """
+ Resolve a CloudStack volume path (/dev/storpool-byid/<globalId>) to a
+ StorPool volume.
+ """
+ if not path:
+ raise Exception("Volume path is empty")
+ parts = path.rstrip("/").split("/")
+ if len(parts) < 4:
+ raise Exception("Unexpected StorPool volume path: %s" % path)
+ name = "~" + parts[3]
try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- if spvolume[0].size != volume.size:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ volumes = spapi.volumeList(volumeName=name)
+ except Exception as err:
+ raise Exception("StorPool volume not found for path %s: %s" %
(path, err))
+ if not volumes:
+ raise Exception("StorPool volume not found for path %s" % path)
+ return volumes[0]
+
+ @classmethod
+ def check_storpool_volume_size(cls, volume, spapi):
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ if spvolume.size != volume.size:
+ raise Exception("Storpool volume size is not the same as
CloudStack db size")
@classmethod
def check_storpool_volume_iops(cls, spapi, volume,):
- name = volume.path.split("/")[3]
- try:
- spvolume = spapi.volumeList(volumeName = "~" + name)
- logging.debug(spvolume[0].iops)
- logging.debug(volume.maxiops)
- if spvolume[0].iops != volume.maxiops:
- raise Exception("Storpool volume size is not the same as
CloudStack db size")
- except spapi.ApiError as err:
- raise Exception(err)
+ spvolume = cls.get_storpool_volume_by_path(spapi, volume.path)
+ 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"):
Review Comment:
`verify_storpool_volume()` accepts any path starting with `/dev/storpool`,
but `get_storpool_volume_by_path()` assumes a `/dev/storpool-byid/<globalId>`
layout (it indexes `parts[3]`). If a path like `/dev/storpool/...` is ever
present, this will raise "Unexpected StorPool volume path" even though
`verify_storpool_volume()` allowed it. Tighten the prefix check to
`/dev/storpool-byid/` (or extend `get_storpool_volume_by_path()` to handle both
formats) so the validation and parsing rules are consistent.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -926,6 +951,19 @@ def get_pool(cls, zone):
return
return sp_pools
+ @classmethod
+ def get_nfs_pool(cls, zone):
+ """Return the NFS primary storage entries configured for this zone in
+ the Marvin cfg file. NFS entries don't set "provider" (unlike
+ StorPool/RBD), so they're identified by their "nfs://" url instead."""
+ 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)
Review Comment:
Accessing `storage['url']` / `storage['provider']` will raise `KeyError` if
a Marvin cfg entry omits either key (which can happen across different
configs). Prefer `storage.get('url')` / `storage.get('provider')` with safe
defaults to keep the helper robust across environments.
--
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]
