Copilot commented on code in PR #14305:
URL: https://github.com/apache/cloudstack/pull/14305#discussion_r4182172445
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -276,12 +292,20 @@ print(len(files))
backup_begin=1;
fi
fi
+ if [[ $backup_begin -eq 1 ]]; then
+ BACKUP_JOB_ACTIVE=1
+ fi
if [[ $thaw -eq 1 ]]; then
- if ! response=$(virsh -c qemu:///system qemu-agent-command "$VM"
'{"execute":"guest-fsfreeze-thaw"}' 2>&1); then
- echo "Failed to thaw the filesystem for vm $VM: $response"
- cleanup
- exit 1
+ if ! response=$(qemu_agent_command '{"execute":"guest-fsfreeze-thaw"}'
2>&1); then
+ if [[ $freeze_ok -eq 1 ]]; then
+ echo "Failed to thaw the filesystem for vm $VM: $response"
+ cleanup
+ exit 1
Review Comment:
`freeze_ok == 0` does not establish that the guest is unquiesced: as the
comment above notes, a timed-out freeze may still complete in the guest. If the
following thaw genuinely fails, this branch continues and can report a
successful backup while leaving the VM filesystem frozen. Continue only when
the freeze failure proves the command was never delivered (for example, no
guest agent); for timeout/ambiguous failures, retry or query
`guest-fsfreeze-status` and fail safely unless thawing is confirmed.
##########
server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java:
##########
@@ -826,18 +826,15 @@ private void createCheckedBackup(CreateBackupCmd cmd,
Account owner, boolean isS
true, 0);
Pair<Boolean, Backup> result = backupProvider.takeBackup(vm,
cmd.getQuiesceVM());
+ Backup backup = result.second();
if (!result.first()) {
+ if (backup != null) {
+ updateBackupFromCmd(backup.getId(), cmd, backupScheduleId);
+ }
Review Comment:
This records the schedule linkage, but it does not resolve the linked
issue's retention failure for these Error rows. The exception prevents the only
retention call at `BackupManagerImpl.java:809-810`, and
`BackupDaoImpl.java:250-254` explicitly filters `listBySchedule` to `BackedUp`,
so repeated cleanup failures still accumulate indefinitely despite
`maxbackups`. Please add a retention path that includes these rows and handles
their resource accounting correctly.
##########
scripts/vm/hypervisor/kvm/nasbackup.sh:
##########
@@ -503,9 +530,33 @@ mount_operation() {
fi
}
+abort_backup_job() {
+ [[ $BACKUP_JOB_ACTIVE -eq 1 ]] || return 0
+
+ # The push backup job writes into $dest until it ends, so it must be gone
before the
+ # destination is removed or unmounted.
+ virsh -c qemu:///system domjobabort "$VM" > /dev/null 2>>"$logFile" || true
+ local i job
+ for ((i = 0; i < 60; i++)); do
+ job=$(virsh -c qemu:///system domjobinfo "$VM" 2>/dev/null | awk '/Job
type:/ {print $3}')
+ if [[ -z "$job" || "$job" == "None" ]]; then
+ BACKUP_JOB_ACTIVE=0
+ return 0
+ fi
+ sleep 1
+ done
Review Comment:
A failed `domjobinfo` call also produces an empty `job`, so this branch
treats “unable to verify” as “job ended” and immediately allows `cleanup()` to
delete/unmount the destination. That recreates the data-loss condition this
guard is intended to prevent whenever libvirt is temporarily unavailable or
returns an error. Only clear `BACKUP_JOB_ACTIVE` after a successful query
explicitly reports `None`; query failures should keep polling and ultimately
leave the destination mounted.
--
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]
