Copilot commented on code in PR #13538:
URL: https://github.com/apache/cloudstack/pull/13538#discussion_r3520700125
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -108,6 +108,21 @@ get_linstor_uuid_from_path() {
echo "$volUuid"
}
+get_linstor_uuid_from_device() {
+ local fullpath="$1"
+ # VMs started before the /dev/drbd/by-res/ change still reference the raw
DRBD
+ # device node (e.g. /dev/drbd1098) in their live libvirt XML. Ask udev for
the
+ # device's symlinks and map it back to the volume UUID via the by-res
symlink.
+ local link
+ for link in $(udevadm info --query=symlink --name="$fullpath" 2>/dev/null ||
true); do
+ if [[ "$link" == drbd/by-res/cs-* ]]; then
+ get_linstor_uuid_from_path "/dev/$link"
+ return 0
+ fi
+ done
+ echo "${fullpath##*/}"
+}
Review Comment:
If udev doesn’t return a /dev/drbd/by-res/cs-<uuid> symlink, this function
falls back to the basename (e.g. drbd1098), which recreates the same
non-restorable backup filename problem the PR is trying to fix. It’s safer to
treat this as a hard error and return non-zero so callers can abort the backup
early.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -117,6 +132,8 @@ backup_running_vm() {
while read -r disk fullpath; do
if [[ "$fullpath" == /dev/drbd/by-res/* ]]; then
volUuid=$(get_linstor_uuid_from_path "$fullpath")
+ elif [[ "$fullpath" == /dev/drbd[0-9]* ]]; then
+ volUuid=$(get_linstor_uuid_from_device "$fullpath")
Review Comment:
This call assumes get_linstor_uuid_from_device always succeeds. With the
suggested change to return non-zero on failure, handle the error here (and
clean up) to avoid producing unusable backup filenames.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -174,10 +191,14 @@ backup_running_vm() {
# Use qemu-img convert to sparsify linstor backups which get bloated due to
virsh backup-begin.
name="root"
while read -r disk fullpath; do
- if [[ "$fullpath" != /dev/drbd/by-res/* ]]; then
+ if [[ "$fullpath" == /dev/drbd/by-res/* ]]; then
+ volUuid=$(get_linstor_uuid_from_path "$fullpath")
+ elif [[ "$fullpath" == /dev/drbd[0-9]* ]]; then
+ volUuid=$(get_linstor_uuid_from_device "$fullpath")
Review Comment:
Same as above: if resolving the UUID from a raw /dev/drbdNNN path fails,
abort instead of continuing and leaving a backup with an unexpected filename
that restore won’t find.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -213,6 +234,8 @@ backup_stopped_vm() {
volUuid=$(get_ceph_uuid_from_path "$disk")
elif [[ "$disk" == /dev/drbd/by-res/* ]]; then
volUuid=$(get_linstor_uuid_from_path "$disk")
+ elif [[ "$disk" == /dev/drbd[0-9]* ]]; then
+ volUuid=$(get_linstor_uuid_from_device "$disk")
Review Comment:
Same issue in the stopped-VM path: if the UUID can’t be resolved from a raw
/dev/drbdNNN device, fail fast (and cleanup) so the operation doesn’t generate
a backup that cannot be restored.
--
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]
