Nas backup: Fix mount/unmount error handling and timeout in LibvirtRestoreBackupCommandWrapper - #14006
Conversation
…up restore Script.executeCommand returns null when the command fails, it does not throw, so the try/catch around the mount and umount of the backup repository could never fire and the return value was discarded. A repository that fails to mount was therefore treated as mounted, and the restore carried on against an empty directory until it failed later with a misleading "backup file not found". A failed umount was ignored the same way, leaking the mount. Both now go through executeCommandForExitValue and check the exit value. The same refactor also dropped the timeouts. mountTimeout was still passed into mountBackupDirectory but never used, and the rsync of the volume lost the command timeout, so both fell back to the one hour default in Script instead of the configured values. An unresponsive repository could hold a restore up for an hour rather than failing after nas.backup.restore.mount.timeout seconds.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #14006 +/- ##
=========================================
Coverage 17.86% 17.86%
- Complexity 16037 16039 +2
=========================================
Files 5928 5928
Lines 534479 534496 +17
Branches 65410 65412 +2
=========================================
+ Hits 95468 95479 +11
- Misses 428173 428179 +6
Partials 10838 10838
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:
|
|
@blueorangutan package |
|
@abh1sar a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
abh1sar
changed the title
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19015 |
There was a problem hiding this comment.
Pull request overview
This PR fixes restore-from-backup behavior for KVM/libvirt by correctly treating failed mount/umount operations as failures (via exit codes instead of relying on exceptions) and by restoring the intended command timeouts so restores don’t hang for Script’s 1-hour default.
Changes:
- Switch mount to
Script.executeCommandForExitValue(timeout, ...)and fail fast on non-zero exit codes. - Switch umount to
executeCommandForExitValue(...)and fail on non-zero exit codes (instead of silently ignoring failure). - Ensure rsync uses the configured restore command timeout; extend unit tests and add a new timeout-focused test.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java | Validates mount/umount exit codes and restores rsync timeout usage during restore operations. |
| plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java | Updates mocks for timeout-aware executeCommandForExitValue and adds a test asserting the configured mount timeout is used. |
Suppressed comments (1)
plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java:605
- This test attempts to capture the timeout used for the mount command, but the "mount" detection has the same issue as other varargs stubs: invocation.getArguments() contains (timeout, String[] cmd), and String.valueOf(cmd) won't equal "mount". As a result, mountTimeout[0] is never set and the assertion can fail (or the test can pass without actually checking the mount invocation, depending on defaults).
if (Arrays.stream(invocation.getArguments()).map(String::valueOf).anyMatch("mount"::equals)) {
mountTimeout[0] = invocation.getArgument(0);
return 1; // stop the restore right after the mount
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…repository The directory created for the mount is removed by the caller in a finally block, but that block is only reached once the mount has succeeded, so a repository that cannot be mounted left an empty directory behind on every attempt. It is now removed before the failure is reported, best effort and logged if it cannot be. The unmount ran without a timeout and so fell back to the one hour default in Script. Unmounting a repository that has become unreachable blocks as easily as mounting one, and this runs in the cleanup path of a restore, so it is now bounded by the configured mount timeout like the mount itself.
abh1sar
changed the title
|
Hi @shwstppr can you please take a look |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@shwstppr a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19173 |
shwstppr
left a comment
shwstppr
left a comment
There was a problem hiding this comment.
Tested LGTM,
Environment: Advanced zone 2x KVM hosts.
Success path — PASS
- Took a real backup, stopped the VM, restored it, booted it again
- Mount → file verify → rsync → unmount all completed cleanly with exit-value checks; no regression from the fix
Failure path — PASS
- Repointed the repo at a host with no NFS service, with a scoped iptables DROP on port 2049 (isolated to that decoy host — never touched the real shared storage server), nas.backup.restore.mount.timeout=15
- Mount hung, then was force-killed at exactly 15.003s, correctly detected as a failure via exit value (not silently ignored)
- Error reported was "Failed to mount the backup repository on the KVM host" — accurate, not the old misleading "backup file not found"
- Temp mount directory was cleaned up, no leak
- Whole restore call completed in ~17–21s total, versus the old code's fallback to a 1-hour default
|
@blueorangutan package |
|
@weizhouapache a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19239 |
|
@blueorangutan test |
|
@weizhouapache a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16986)
|
|
merging based on approvals and Abhishek's manual verification |
Conflicts: - LibvirtRestoreBackupCommandWrapperTest.java: apache#14006 appended three mount/unmount timeout tests at the end of the class, where the volume mapping tests end too; kept both.
…eouts apache#14006 runs the mount, the umount and the rsync of a restore through Script.executeCommandForExitValue(long, String...) so that they honour the configured timeouts. The volume mapping tests still stubbed and verified the overloads without a timeout. The two tests asserting where a backup is written therefore failed, and the checks that a backup is never written into the wrong volume passed without verifying anything, as nothing called that overload any more. Stub and verify the timeout variants instead, as apache#14006 does for the existing tests.
Description
Script.executeCommand returns null when the command fails, it does not throw, so the try/catch around the mount and umount of the backup repository could never fire and the return value was discarded.
A repository that fails to mount was therefore treated as mounted, and the restore carried on against an empty directory until it failed later with a misleading "backup file not found". A failed umount was ignored the same way, leaking the mount. Both now go through executeCommandForExitValue and check the exit value.
The same refactor also dropped the timeouts. mountTimeout was still passed into mountBackupDirectory but never used, and the rsync of the volume lost the command timeout, so both fell back to the one hour default in Script instead of the configured values. An unresponsive repository could hold a restore up for an hour rather than failing after nas.backup.restore.mount.timeout seconds.
This is a regression from 56ad044
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
Verified that restore fails after the specified nas.backup.restore.mount.timeout with a dead address.
How did you try to break this feature and the system with this change?