Copilot commented on code in PR #14203:
URL: https://github.com/apache/cloudstack/pull/14203#discussion_r4399329448
##########
plugins/storage/volume/storpool/src/main/java/com/cloud/hypervisor/kvm/storage/StorPoolStorageAdaptor.java:
##########
@@ -455,7 +475,57 @@ 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 {
+ if (!attachOrDetachVolume("attach", "volume", volumePath)) {
+ // The attach succeeded but the device never showed up;
StorPool refuses to delete an attached volume.
+ attachOrDetachVolume("detach", "volume", volumePath);
+ throw new CloudRuntimeException(String.format("StorPool volume
%s for %s was attached but its device did not appear", volumePath, name));
+ }
+ } catch (CloudRuntimeException e) {
+ volumeDelete("~" + globalId);
+ throw e;
+ }
Review Comment:
If `volumeDelete(...)` fails in this catch block, its exception replaces the
original error `e`, and no log line says the created volume still exists. Wrap
the cleanup in its own try, log a manual-delete warning on failure, then
rethrow `e`.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -757,10 +924,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"
Review Comment:
If `DISK_PATHS` is empty, the array is empty, the loop never runs, and the
script still prints `BACKUP_SIZE_TOTAL=0` and exits 0. This happens when
libvirt reports the VM as shut off but CloudStack sent no disk paths. Before
this change, the empty output made the Java size parse fail; now an empty
backup is recorded as a success. Fail the stopped-VM path when there are no
disks to read.
##########
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"
Review Comment:
If `sp_delete_backup_source_disk` fails, its non-zero status makes `set -e`
(line 20) exit the shell inside this EXIT trap. The remaining clones in
`SP_CLEANUP_VOLUMES` are never attempted, and on the pull path `cleanup` is
skipped. Make the call best-effort, as the create-failure path in
`sp_create_backup_source_disk` already does with `|| true`.
##########
test/integration/plugins/storpool/TestNasBackupStorPool.py:
##########
@@ -0,0 +1,716 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""
+Smoke tests for NAS backup and recovery on StorPool primary storage.
+
+Assumes the environment already has NAS backup configured for the zone
+(a NAS backup repository and an imported backup offering).
+"""
+
+import math
+import time
+import uuid
+
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.codes import FAILED, KVM
+from marvin.lib.base import (Account,
+ Backup,
+ BackupOffering,
+ BackupRepository,
+ DiskOffering,
+ SecurityGroup,
+ ServiceOffering,
+ StoragePool,
+ VirtualMachine,
+ Volume)
Review Comment:
`Account` and `BackupRepository` are imported in this module but never used.
Remove them from the import list to avoid lint failures.
##########
test/integration/plugins/storpool/sp_util.py:
##########
@@ -21,6 +21,7 @@
from marvin.lib.base import (Account,
Cluster,
Configurations,
+ ImageStore,
ServiceOffering,
Review Comment:
The `ImageStore` import is added but never used in sp_util.py. Only the
import line references it, so this will be reported as an unused import by the
Python lint checks.
--
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]
