Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4458771827
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -455,7 +475,63 @@ 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));
Review Comment:
The no-format `KVMStoragePool.createPhysicalDisk(name, provisioningType,
...)` overload is how normal KVM volume creation reaches this adaptor, and
`StorPoolStoragePool` currently forwards that call with `format == null` even
though its default format is RAW. Rejecting every non-RAW value here therefore
rejects `null` too, so ordinary StorPool volume creation still fails before the
new disk can be provisioned. Treat a null format as the pool default (RAW), or
make `StorPoolStoragePool` pass RAW.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -406,21 +410,182 @@ 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:
Review Comment:
The `/ctrl/1.0/MultiCluster/VolumeCreate` response exposes the created
volume under `data.name` (the existing `StorPoolUtil.getNameFromResponse` reads
that field), not `data.globalId`. As written, every cold StorPool backup will
reject a successful clone as having no global ID, leaving the created clone
orphaned and failing the backup. Read `name` from the response (and normalize
any leading `~` if the API returns one) before building the device path.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -406,21 +410,182 @@ 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
Review Comment:
The new `VolumeCreate` request only sends `baseOn` and `tags`, but the
StorPool integration's existing `volumeCopy` implementation sends the
configured template together with `baseOn`
(`plugins/storage/volume/storpool/src/main/java/org/apache/cloudstack/storage/datastore/util/StorPoolUtil.java:557-567`).
This script never reads `SP_TEMPLATE` from `storpool_confget`, so StorPool
deployments that require the template for a multi-cluster clone will reject
every stopped-VM backup before `qemu-img` runs. Include and validate the
configured template in this request, matching the existing StorPool volume-copy
path.
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java:
##########
@@ -176,29 +182,54 @@ private void
restoreVolumesOfDestroyedVMs(KVMStoragePoolManager storagePoolMgr,
}
}
- private void restoreVolume(KVMStoragePoolManager storagePoolMgr, String
backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType,
String backupFile,
+ /**
+ * @return the volume path the backup was actually written to. Equal to
{@code volumePath}
+ * except when the pool assigns the created volume's identity itself
(StorPool), in which
+ * case it reflects the real device path.
+ */
+ private String restoreVolume(KVMStoragePoolManager storagePoolMgr, String
backupPath, PrimaryDataStoreTO volumePool, String volumePath, String diskType,
String backupFile,
Long size, Pair<String, VirtualMachine.State>
vmNameAndState, String mountDirectory, int timeout, Integer mountTimeout) {
String bkpPath;
String volumeUuid;
try {
bkpPath = getBackupPath(mountDirectory, backupPath, backupFile,
diskType);
volumeUuid = getVolumeUuidFromPath(volumePath, volumePool);
verifyBackupFile(bkpPath, volumeUuid);
- if (!replaceVolumeWithBackup(storagePoolMgr, volumePool,
volumePath, bkpPath, timeout, true, size)) {
+ Pair<Boolean, String> restoreResult =
replaceVolumeWithBackup(storagePoolMgr, volumePool, volumePath, bkpPath,
timeout, true, size);
+ volumePath = restoreResult.second();
+ if (!restoreResult.first()) {
+ cleanupCreatedVolume(storagePoolMgr, volumePool, volumePath);
throw new CloudRuntimeException(String.format("Unable to
restore contents from the backup volume [%s].", volumeUuid));
Review Comment:
The cleanup added here only runs when `replaceVolumeWithBackup` returns a
`Pair` with `first()==false`. `replaceBlockDeviceWithBackup` can instead throw
after `createPhysicalDisk` has already provisioned the StorPool volume (for
example, its `QemuImg` setup catches `LibvirtException` and rethrows
`CloudRuntimeException`), so this call exits through `restoreVolume`'s
`finally` without deleting the newly created disk. Handle exceptions from the
replacement while retaining the actual created path, or move the cleanup into
the creation/replacement method's failure path, so failed restores cannot
orphan StorPool volumes.
--
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]
