Nasbackup quiesce fixes - #14305
Nasbackup quiesce fixes#14305MitchDrage wants to merge 4 commits into
Conversation
|
@MitchDrage thanks for the PR. I'll review/test it in the next few days. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Ambiguous freeze/thaw and job-status failures can still leave guests frozen or delete destinations under active jobs, while failed-row retention remains incomplete.
Review effort: Balanced
Findings: 2
Open (3)
What changed in this PR
Fixes NAS backup quiescing, cleanup, and failed scheduled-backup handling.
Changes:
- Adds configurable guest-agent freeze/thaw timeouts.
- Aborts active libvirt jobs before cleanup.
- Preserves metadata for failed scheduled backups and expands tests.
| File | Description |
|---|---|
BackupManagerTest.java |
Tests failed scheduled-backup linkage. |
BackupManagerImpl.java |
Persists metadata on failed backups. |
nasbackup.sh |
Handles quiescing, aborts, and cleanup. |
LibvirtTakeBackupCommandWrapperTest.java |
Tests timeout argument forwarding. |
LibvirtTakeBackupCommandWrapper.java |
Passes timeout to the script. |
NASBackupProviderTest.java |
Tests failure and cleanup behavior. |
NASBackupProvider.java |
Adds timeout configuration and returns Error backups. |
TakeBackupCommand.java |
Carries the quiesce timeout. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| 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 |
| 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 |
| if (!result.first()) { | ||
| if (backup != null) { | ||
| updateBackupFromCmd(backup.getId(), cmd, backupScheduleId); | ||
| } |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14305 +/- ##
============================================
+ Coverage 18.00% 19.09% +1.08%
- Complexity 16219 16234 +15
============================================
Files 5936 5487 -449
Lines 535716 497497 -38219
Branches 65596 58516 -7080
============================================
- Hits 96459 94994 -1465
+ Misses 428268 391708 -36560
+ Partials 10989 10795 -194
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| local status=0 | ||
|
|
||
| if ! abort_backup_job; then | ||
| echo "Backup job for vm $VM is still running after abort, leaving $dest mounted at $mount_point" |
There was a problem hiding this comment.
if the abort never finishes, what unmounts the share on the host later? looks like the mount stays there for good
There was a problem hiding this comment.
Yep, that's where the old code ended up too: cleanup() deleted the destination under the live job, then umount failed busy and left the mount behind (we had a significant number of those in production).
With this PR the thaw path aborts the job first, and in testing the abort completes in about 1.4s, so the 60s timeout should only be reached if the NAS itself has hung.
That's why I kept the old behaviour of leaving the mount in place when this timeout is reached; I've just made it explicit, with a message naming the mount point.
If you'd like something else here, what would you recommend for that case? I was thinking a umount -f when the abort times out. The issue is that concurrent backups to the same repository on that host share the NFS superblock, so their I/O would fail too. Again though, that should only happen if the NAS has hung, so an alert would be a good thing to add here regardless.
There was a problem hiding this comment.
makes sense to keep the mount then. can we raise an alert when cleanup fails, with the host and the mount point in it? that way an admin sees it without digging through the logs
Description
Fixes #14295, one commit per issue:
BackupManagerImplrecords the schedule id, name and description on it.nas.backup.quiesce.agent.timeout(default 30 seconds, 0 keeps libvirt's default of 5), passed tonasbackup.shas--quiesce-timeoutand on tovirsh --timeoutfor freeze and thaw. The per-VM override from the issue is left out.cleanup()now runsvirsh domjobabortand waits up to 60 seconds for the job to end. If it is still running, the files and mount are left in place and the backup goes to Error state. Also fixed theFailedbranch of the job polling loop, which calledcleanupwithout exiting.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
I've listed this as major as I'm having backups continuously fail. I've applied the timeout to nasbackup.sh by hand as a workaround and I'm waiting on the next scheduled run to confirm it. I'll add a comment tomorrow as to the result, or add some changes to fix it.
How Has This Been Tested?
BackupManagerTest,NASBackupProviderTestand a newLibvirtTakeBackupCommandWrapperTest. These and the NAS plugin suite pass.nasbackup.shagainst stubvirsh/mountcommands for each failure path, including an abort that never completes.--timeout 30on freeze and thaw completed. Separately, aborting a running push backup ended the job about 1.4 seconds afterdomjobabortreturned, after which the destination could be removed and unmounted.How did you try to break this feature and the system with this change?