Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4377051172
##########
plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java:
##########
@@ -505,6 +505,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 pattern (one `findById` per volume) in the backup
decision path. Mandatory: change to a bulk lookup (e.g., fetch all pools for
the involved pool IDs in one query and check types in-memory), or adjust the
volume query to include pool type so the loop doesn’t hit the DB repeatedly.
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -455,7 +475,53 @@ public boolean createFolder(String uuid, String path,
String localPath) {
@Override
public KVMPhysicalDisk createPhysicalDisk(String name, KVMStoragePool
pool, PhysicalDiskFormat format,
ProvisioningType provisioningType, long size, byte[] passphrase) {
- return null;
+ SP_LOG("StorPoolStorageAdaptor.createPhysicalDisk: name=%s, pool=%s,
size=%d", name, pool.getUuid(), size);
+
+ if (passphrase != null) {
+ throw new CloudRuntimeException("Encrypted StorPool volumes are
not supported");
+ }
+
+ if (!PhysicalDiskFormat.RAW.equals(format)) {
+ throw new CloudRuntimeException(String.format("StorPool volumes
can only be created as RAW, %s was requested", format));
+ }
+
+ if (size <= 0) {
+ throw new CloudRuntimeException("Size must be greater than 0 to
create a StorPool volume");
+ }
+
+ String[] uuidParts = pool.getUuid() != null ?
pool.getUuid().split(";") : new String[0];
+ if (uuidParts.length == 0 || StringUtils.isBlank(uuidParts[0])) {
+ throw new CloudRuntimeException("Unable to resolve the StorPool
template for pool " + pool.getUuid());
+ }
+ String template = uuidParts[0];
+
+ Map<String, String> tags = new HashMap<>();
+ tags.put("cs", "volume");
+ tags.put("uuid", name);
+
+ OutputInterpreter.AllLinesParser parser =
createStorPoolVolume(template, size, tags,
+ String.format(" for %s", name));
+
+ String globalId = getNameFromResponse(parser.getLines(), false, false);
+ if (StringUtils.isBlank(globalId)) {
+ LOGGER.warn(String.format("StorPool volume for %s was created but
its globalId could not be parsed from the response; " +
+ "it must be found (tag uuid=%s) and deleted manually to
avoid an orphaned volume", name, name));
+ throw new CloudRuntimeException(String.format("StorPool did not
return a volume name/globalId when creating %s", name));
+ }
+
+ String volumePath = StorPoolUtil.devPath(globalId);
+ try {
+ attachOrDetachVolume("attach", "volume", volumePath);
+ } catch (CloudRuntimeException e) {
+ volumeDelete("~" + globalId);
+ throw e;
+ }
+
+ KVMPhysicalDisk disk = new KVMPhysicalDisk(volumePath, name, pool);
+ disk.setFormat(PhysicalDiskFormat.RAW);
+ disk.setSize(size);
+ disk.setVirtualSize(size);
+ return disk;
Review Comment:
After the attach call returns, the `/dev/storpool-byid/<globalId>`
symlink/device node may not be usable immediately (udev is asynchronous). This
can cause intermittent restore failures if subsequent steps access `volumePath`
right away. Mandatory for reliability: add a bounded wait/poll after attach
(existence + non-zero size/`blockdev --getsize64` equivalent) before returning
the disk; if the wait times out, detach+delete and throw.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -682,14 +683,30 @@ 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")
Review Comment:
Capitalize the product name consistently in the error message: change
'Storpool' to 'StorPool'.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -406,21 +410,184 @@ backup_running_vm_pull() {
# End the pull job. Aborting the active job is how a pull backup finishes
(there is no
# backup-end command): libvirt tears down the NBD server and releases the
scratch devices,
# which the agent then deletes. The data already pulled is a complete
backup, so from here
- # on a failure must no longer discard it.
- trap - EXIT INT TERM HUP
+ # on a failure must no longer discard it. Hand EXIT back to the StorPool
safety net.
+ trap - INT TERM HUP
+ trap sp_cleanup_backup_source_disks EXIT
if ! abort_backup_job; then
log -e "warning: failed to end (domjobabort) backup job for $VM"
fi
rm -rf "$work_dir"
sync
- # Statistics -- the size on the last stdout line is parsed by the Java
wrapper.
- du -sb $dest | cut -f1
+ backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING:
du failed for $dest, reporting size as 0"; backup_size=0; }
+ timeout "$UNMOUNT_TIMEOUT" umount "$mount_point" 2>>"$logFile" || { log
"WARNING: umount of $mount_point failed or timed out"; true; }
+ rmdir "$mount_point" 2>>"$logFile" || { log "WARNING: rmdir of $mount_point
failed"; true; }
+ echo "BACKUP_SIZE_TOTAL=$backup_size"
+}
- umount $mount_point
- rmdir $mount_point
+# StorPool volume name (with the "~globalId" form the storpool/storpool_req
CLIs expect) from
+# /dev/storpool-byid/<globalId>.
+sp_volume_name_from_path() {
+ local fullpath="$1"
+ local name
+ if [[ "$fullpath" == /dev/storpool-byid/* ]]; then
+ name="${fullpath#/dev/storpool-byid/}"
+ name="~${name%%/*}"
+ else
+ return 1
+ fi
+ echo "$name"
+}
+
+# Poll for the device node to actually appear after a successful attach: udev
creates the
+# symlink asynchronously, so a successful "attach" CLI call doesn't guarantee
the device path
+# is usable yet.
+sp_wait_for_device_symlink() {
+ local name="$1"
+ local devpath="/dev/storpool-byid/${name#\~}"
+ local tries=10 i
+ for ((i = 0; i < tries; i++)); do
+ if [[ -e "$devpath" ]] && [[ "$(blockdev --getsize64 "$devpath"
2>/dev/null || echo 0)" -gt 0 ]]; then
+ return 0
+ fi
+ sleep 0.1
+ done
+ return 1
+}
+
+# A successful attach also waits for the device symlink.
+sp_attach_volume() {
+ local name="$1"
+ storpool -M -B attach volume "$name" here onRemoteAttached export
>>"$logFile" 2>&1 && { sp_wait_for_device_symlink "$name"; return $?; }
+ return 1
+}
+
+sp_api_post() {
+ local query="$1" json="$2"
+ local key value host="" port="81" token=""
+ while IFS='=' read -r key value; do
+ case "$key" in
+ SP_API_HTTP_HOST) host="$value" ;;
+ SP_API_HTTP_PORT) port="$value" ;;
+ SP_AUTH_TOKEN) token="$value" ;;
+ esac
+ done < <(storpool_confget 2>>"$logFile")
+ if [[ -z "$host" || -z "$token" ]]; then
+ log -ne "StorPool: SP_API_HTTP_HOST or SP_AUTH_TOKEN missing from
storpool_confget, cannot call $query"
+ return 1
+ fi
+
+ local resp
+ if ! resp=$(curl -sS --connect-timeout 10 -X POST \
+ -H @<(printf 'Authorization: Storpool v1:%s\n' "$token") \
+ -H "Content-Type: application/json" -d "$json" \
+ "http://$host:$port/ctrl/1.0/$query"; 2>>"$logFile"); then
+ log -ne "StorPool: API call $query failed, see above for curl output"
+ return 1
+ fi
+ python3 -c '
+import sys, json
+try:
+ resp = json.load(sys.stdin)
+except Exception:
+ sys.exit(1)
+if "error" in resp or "data" not in resp:
+ sys.exit(1)
+print(json.dumps(resp["data"]))
+' <<< "$resp" || {
+ log -ne "StorPool: API call $query returned an error: $resp"
+ return 1
+ }
+}
+
+# Deletes $1, force-detaching it first wherever it is still attached (StorPool
refuses to delete
+# an attached volume). Returns 1, after logging, if the delete failed.
+sp_delete_volume() {
+ local volume_name="$1"
+ if ! sp_api_post "MultiCluster/VolumeDelete/$volume_name"
'{"onAttached":"detachForce"}' >/dev/null; then
+ log -ne "StorPool: failed to delete volume $volume_name, it must be
deleted manually"
+ return 1
+ fi
}
+# Clones $1 (a live StorPool volume path) into a new, point-in-time StorPool
volume via the
+# VolumeCreate API's baseOn option, attaches it, and echoes the clone's device
path. Only called
+# once this script's own (fresh, right-here) VM liveness check has already
routed to the cold
+# path — see backup_stopped_vm.
+sp_create_backup_source_disk() {
+ local volume_path="$1"
+ local base_on
+ if ! base_on=$(sp_volume_name_from_path "$volume_path"); then
+ echo "Could not resolve a StorPool volume name from path $volume_path to
create a backup source volume" >&2
+ return 1
+ fi
+
+ log -ne "StorPool: creating backup source volume based on $base_on (from
$volume_path)"
+
+ local resp
+ if ! resp=$(sp_api_post "MultiCluster/VolumeCreate"
"{\"baseOn\":\"$base_on\",\"tags\":{\"cs\":\"backup\"}}"); then
+ log -ne "StorPool: VolumeCreate baseOn=$base_on failed, see above"
+ echo "Could not create a backup source volume based on $base_on" >&2
+ return 1
+ fi
+
+ local global_id
+ global_id=$(python3 -c '
+import sys, json
+try:
+ data = json.load(sys.stdin)
+ gid = data.get("globalId")
+except Exception:
+ gid = None
+if not gid:
+ sys.exit(1)
+print(gid)
+' <<< "$resp" 2>>"$logFile") || {
+ log -ne "StorPool: VolumeCreate baseOn=$base_on returned no globalId,
response: $resp -- the volume was created but its name could not be parsed; it
must be found (tag cs=backup, baseOn=$base_on) and deleted manually to avoid an
orphaned volume"
+ echo "StorPool did not return a volume name when cloning $base_on for
backup" >&2
+ return 1
+ }
+
+ local clone_path="/dev/storpool-byid/$global_id"
+ local clone_name="~$global_id"
+ log -ne "StorPool: created backup source volume $clone_name ($clone_path),
attaching"
+ if ! sp_attach_volume "$clone_name"; then
+ log -ne "StorPool: failed to attach backup source volume $clone_name,
deleting it"
+ sp_delete_volume "$clone_name" || true
+ echo "Could not attach backup source volume $clone_path cloned from
$base_on" >&2
+ return 1
+ fi
+
+ # Not registered in SP_CLEANUP_VOLUMES here: the caller always invokes this
function via
+ # command substitution ($(...)), which runs it in a subshell — any append to
the array here
+ # would be a mutation of that subshell's own copy and vanish once it exits.
The caller must
+ # register $clone_name itself right after reading it back from stdout below.
+ log -ne "StorPool: backup source volume $clone_name attached at $clone_path,
reading from it for this backup"
+ echo "$clone_path"
+}
+
+# Deletes a volume previously returned by sp_create_backup_source_disk.
Best-effort:
+# logs on failure rather than aborting the backup, matching the wrapper's old
cleanup semantics.
+sp_delete_backup_source_disk() {
+ local clone_name="$1"
+ log -ne "StorPool: releasing backup source volume $clone_name"
+ sp_delete_volume "$clone_name"
+}
+
+# Safety net for any clone not already released by its own per-disk cleanup
(e.g. the script
+# exiting between creating it and reaching that point). A cleanup here means
an earlier step
+# didn't run its normal release — worth noticing, so it's logged even though
it isn't fatal.
+sp_cleanup_backup_source_disks() {
+ local clone_name
+ for clone_name in "${SP_CLEANUP_VOLUMES[@]:-}"; do
+ [[ -z "$clone_name" ]] && continue
+ log -ne "StorPool: cleaning up backup source volume $clone_name left over
on script exit"
+ sp_delete_backup_source_disk "$clone_name"
+ done
+ SP_CLEANUP_VOLUMES=()
+}
+trap sp_cleanup_backup_source_disks EXIT
+
Review Comment:
Setting a global `trap ... EXIT` here replaces any previously configured
EXIT handler (and will itself be replaced by later `trap EXIT` calls). This can
either break existing cleanup logic (e.g., unmount/removing scratch dirs) or
prevent the StorPool safety-net from running, depending on trap assignment
order. Mandatory: avoid installing a standalone global EXIT trap; instead chain
it into the script’s existing exit handlers (e.g., make the existing on-exit
handler call `sp_cleanup_backup_source_disks` first, or set `trap
'sp_cleanup_backup_source_disks; <existing-exit-handler>' EXIT` in the same
place the script currently establishes its EXIT trap).
--
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]
