[jmsperu]

Summary

• Add BACKUP_TIMEOUT (default 6 hours) to prevent indefinitely stuck backup jobs; aborts via domjobabort when exceeded
• Add EXIT trap with cleanup() that resumes paused VMs, removes temp dirs, and unmounts NFS on any exit (error, signal, or normal) — prevents orphan mounts from accumulating
• Add check_free_space() pre-flight check (default 1 GB minimum) before writing backup data
• Add -q/--quiesce flag for optional fsfreeze/thaw via qemu-guest-agent for application-consistent backups
• Use set -eo pipefail for stricter error handling
• Fix mount_operation(): proper if mount instead of broken $? check after pipe
• Quote all variable expansions to prevent word splitting
• Remove manual umount/rmdir from functions (now handled by trap)

Motivation

The current nasbackup.sh has several reliability issues observed in production:

1. No timeout — backup polling loop (until domjobinfo --completed) runs forever if backup stalls, blocking the agent
2. No cleanup on failure — failed backups leave orphan NFS mounts (/tmp/csbackup.XXXXX) and paused VMs that never resume (related: KVM NAS backup: VM remains paused indefinitely when backup job fails #12821)
3. No space check — backups silently fail or produce corrupt output when the NAS is full
4. No quiesce — running VM backups are crash-consistent only, even when qemu-guest-agent is available
5. Broken error handling — $? after a pipe always returns the pipe's exit status, not the command's

Test plan

• Backup a running VM with sufficient space — verify backup completes normally
• Backup a running VM with BACKUP_TIMEOUT=30 — verify timeout and domjobabort
• Backup with NAS at <1GB free — verify pre-flight rejection
• Kill backup mid-run — verify cleanup resumes VM and unmounts NFS
• Backup with -q flag and qemu-guest-agent installed — verify fsfreeze/thaw in logs
• Backup with -q flag without qemu-guest-agent — verify graceful fallback
• Delete backup — verify cleanup trap handles unmount
• Backup a stopped VM — verify space check and error handling on disk convert

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 20.45%. Comparing base (c76c01e) to head (84c227c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #12843   +/-   ##
=========================================
  Coverage     20.45%   20.45%           
- Complexity    21417    21419    +2     
=========================================
  Files          6487     6487           
  Lines        589329   589329           
  Branches      72246    72246           
=========================================
+ Hits         120554   120567   +13     
+ Misses       455642   455630   -12     
+ Partials      13133    13132    -1     

Flag	Coverage Δ
uitests	3.70% <ø> (ø)
unittests	21.76% <ø> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[abh1sar]

@jmsperu can you please check on 4.21?
There were lots of improvement done to the nasbackup.sh script
Quiesce support was also added (which can also be opt out of)

[jmsperu]

@jmsperu can you please check on 4.21? There were lots of improvement done to the nasbackup.sh script Quiesce support was also added (which can also be opt out of)

Thanks for the heads up! I see 4.22 already has quiesce and improved error handling. I'll rebase onto 4.22 and update the PR to only include what's still missing: backup timeout, trap handler with VM resume, and free space check. The other PRs (compression, verification, bandwidth throttle, encryption, parallel execution) are still new features on top of 4.22.

[jmsperu]

Belated reply @abh1sar — you were right, there's significant overlap with what's already landed on main. The quiesce support and EXIT_CLEANUP_FAILED handling from this PR have since been merged upstream (likely via the broader nasbackup.sh improvements in #12822 and others), so those bits are now redundant.

What's still uniquely unaddressed by what landed:

• BACKUP_TIMEOUT env var (default 21600s = 6h) wrapping the long-running virsh backup-begin poll loop so a stuck backup eventually fails rather than holding the agent forever
• MIN_FREE_SPACE check against the mounted NAS before qemu-img convert runs, with a fast-fail when the NAS doesn't have enough free space — currently we discover this mid-write
• trap cleanup EXIT so cleanup() runs even on SIGTERM / shell death (today cleanup() only runs on graceful exits)

I'll rebase against main to drop the overlapping changes and keep only those three, then push again. If you'd rather close this and open a fresh narrower PR with just those bits, also happy to do that — your call.

[jmsperu]

Rebased. Force-pushed 0deb4c4 and changed the base from 4.20 → main.

What landed in main since this PR was opened (via other PRs):

• QUIESCE arg + freeze/thaw
• EXIT_CLEANUP_FAILED exit code
• Basic cleanup() function (called explicitly at 6 sites)

So this PR is now reduced to just the three improvements those didn't cover:

1. BACKUP_TIMEOUT env var (default 6h) bounds the domjobinfo wait loop in backup_running_vm so a stuck QEMU backup hits domjobabort and frees the agent's command slot rather than holding it open until the orchestrator-level timeout.

2. MIN_FREE_SPACE env var (default 1 GiB) + check_free_space() runs after mount and before any qemu-img convert in both backup paths — fail-fast on a near-full NAS instead of failing mid-write.

3. trap cleanup EXIT replaces the explicit calls as the primary cleanup mechanism so orphan NFS mounts no longer accumulate on SIGTERM/SIGINT or set -e failures between the explicit call sites. cleanup() is guarded by a CLEANUP_DONE flag so the trap doesn't re-run an already-completed cleanup from an explicit call. cleanup() also now resumes the VM if it's still paused — a backup that dies mid-pause currently leaves the guest stuck paused until an operator intervenes.

Total: 71 +/3 - on one file. Ready for another look @abh1sar.

[jmsperu]

Friendly nudge — the new push is sitting on GitHub Actions action_required, so no CI has actually run yet against the rebase. Could a committer (@abh1sar @DaanHoogland) click Approve and run when you have a moment? Thanks!

[DaanHoogland]

Friendly nudge — the new push is sitting on GitHub Actions action_required, so no CI has actually run yet against the rebase. Could a committer (@abh1sar @DaanHoogland) click Approve and run when you have a moment? Thanks!

I’ve added you to the collaborators list @jmsperu . With great privileges comes great responsibility… ;)

[jmsperu]

Friendly bump — this adds defensive hardening around nasbackup.sh:

• Timeout on the long-running operations (so a stuck mount/backup doesn't pin the agent thread)
• Free-space pre-flight on the backup target (fail early with a clear message vs. partial-write garbage)
• Trap-based cleanup (always unmount, never leak the temp dir or the mount even on signal/error)
• Resume the VM if backup failed mid-flight (paired with NAS backup: resume paused VM on backup failure and fix missing exit #12822)
• Quiesce support fixes

Latest CI had a single shard failure in component/test_affinity_groups_projects — unrelated to this script (patch only touches nasbackup.sh). Just retriggered. @DaanHoogland @sureshanaparti — review welcome.

[DaanHoogland]

I’ll look at this and your other PRs tomorrow, bandwith permitting @jmsperu .

[Copilot]

Pull request overview

This PR updates the KVM NAS backup helper script (nasbackup.sh) to improve reliability by adding a backup polling timeout, introducing an EXIT cleanup trap to avoid orphaned mounts/paused VMs, adding a pre-flight free-space check, and supporting optional guest filesystem quiescing via qemu-guest-agent.

Changes:

• Add BACKUP_TIMEOUT and enforce it in the domjobinfo polling loop (abort via domjobabort on timeout).
• Add check_free_space() and call it before writing backup data.
• Add an idempotent cleanup() and install trap cleanup EXIT to ensure cleanup on all exit paths.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[DaanHoogland]

co-pilot has some smarter comments than I ;)

[jmsperu]

Friendly nudge — all CI checks are green and the PR is MERGEABLE/CLEAN. Whenever you have bandwidth, would appreciate a fresh look 🙏

Recap of what this adds to nasbackup.sh:

• -t / --timeout flag with per-step kill-switch + cleanup trap so a hung qemu-img / rsync can't tie up a libvirt host indefinitely.
• Pre-flight free-space check on the NAS target — refuse to start if there isn't headroom for a full VM image.
• Quiesce / qemu-guest-agent integration before snapshotting so the guest filesystem is consistent.
• Defensive cleanup trap that removes partial files on any error path.

Backwards-compatible: invoking the script without -t / --quiesce behaves exactly as 4.22 did.

[DaanHoogland]

@abh1sar , can you have a look at this one, please?

[jmsperu]

Thanks @DaanHoogland / Copilot. Pushed d64286b addressing the open points:

• Failed job didn't break the wait loop: now exits after cleanup instead of spinning until BACKUP_TIMEOUT.
• backup_stopped_vm qemu-img failure: exits after cleanup (no partial-treated-as-success) and appends to the agent log instead of truncating it.
• mount | tee under set -eo pipefail: mount's own exit is tested in an if, so a mount failure is reported instead of being masked by tee / aborting before the check.
• cleanup umount: only unmounts when the path is actually a mount, so a non-mount umount no longer reports EXIT_CLEANUP_FAILED via the EXIT trap.

While testing this I also found that backup_stopped_vm didn't unmount on success — so trap cleanup EXIT ran rm -rf "$dest" and deleted the just-completed backup. It now unmounts on success like backup_running_vm/delete_backup. Verified with a stubbed self-test: a successful backup persists, and a failed convert exits non-zero with the partial cleaned up.

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[weizhouapache]

@jmsperu
can you fix the conflicts ? #12822 has been merged

[jmsperu]

@weizhouapache apologies for the slow reply here. The conflicts with #12822 were resolved on 28 Jul (merge of main into this branch) and the PR has been mergeable since. I've just pushed af4fbb8 for the remaining open comments:

• @DaanHoogland's question on CLEANUP_DONE: the flag deliberately goes at the top of cleanup(). It marks "cleanup has been entered": cleanup() can itself exit $EXIT_CLEANUP_FAILED, and that exit fires the EXIT trap, which calls cleanup() again while the first call is still on the stack. Renamed to CLEANUP_STARTED with the comment spelling this out.
• The three Copilot points (umount only if it is a mountpoint, mount tested directly in if rather than through | tee, backup_stopped_vm unmounting on success and exiting after cleanup) are already in the branch; replied on each thread.

Only Sonar coverage is red, and this PR is a shell script, so there is nothing for JaCoCo to measure. Could someone approve-and-run the workflows on the new SHA?

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19378

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-17067)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 59124 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12843-t17067-kvm-ol8.zip
Smoke tests completed. 156 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[jmsperu]

Thanks @DaanHoogland for running Trillian: tid-17067 on 3 Oct came back 156/156 OK, with no errors. The branch is mergeable with 4.22. If it looks good to you, could a committer review and merge it?

[DaanHoogland]

Thanks @DaanHoogland for running Trillian: tid-17067 on 3 Oct came back 156/156 OK, with no errors. The branch is mergeable with 4.22. If it looks good to you, could a committer review and merge it?

we do ask for testing. @abh1sar can you?

[github-actions]

This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch.

[jmsperu]

Rebased on the latest main (27c7dc6), so the conflicts are gone. I folded the branch into one commit because its old merge from main no longer replayed cleanly. The conflict was with #14305 (quiesce fixes) in nasbackup.sh: I kept its abort_backup_job and the domjobinfo query retries, and moved the backup timeout into a deadline at the top of the polling loop, so it also covers the retries and goes through cleanup (which now aborts the job before removing the destination) instead of calling domjobabort itself. cleanup keeps both the re-entry guard and the job abort. bash -n passes, and shellcheck shows no warnings beyond the ones already on main.

[Damans227]

@jmsperu if the last unmount of a running vm backup fails or times out, wont this trap delete the finished backup? the script carries on, so it can still report success

[jmsperu]

Good catch, you are right, and it was worse than one path. Once the share was still mounted, the EXIT trap ran rm -rf on $dest, which at that point is the finished backup on the NAS. For a running VM, if the final umount timed out and the trap's own umount retry then succeeded, the script deleted the backup and still exited 0. A stopped VM backup lost its backup the same way, but at least reported a failure. In stats, $dest can be the root of the backup store, so a failed umount there could delete existing backups.

Fixed in 84c227c:

1. A BACKUP_COMPLETE flag is set just before the final unmount. cleanup() removes $dest only for a backup that did not complete. For delete and stats the trap now only unmounts and removes the temporary mount directory.
2. Both backup paths unmount through one helper. It tries umount with a time limit twice, then umount -l. If the share still lingers it logs a WARNING and keeps the success status, because the backup is intact.
3. If timeout is not installed, plain umount is used.
4. An interrupted backup is still aborted and removed as before.

I checked this with a stubbed harness (fake virsh, mount, umount, df, du and a fake NAS directory). Running VM backup with an umount that times out once: before, exit 0 with no files left on the NAS; after, exit 0 with all backup files present. With every umount failing, including the lazy one, the files are kept and the run succeeds with warnings. A failed backup job still exits 1 and its partial files are removed. Stats with a failing umount no longer deletes anything. Shellcheck reports nothing new.