kvm: do not fail a NAS backup on non-numeric output from nasbackup.sh - #9
Open
calvix wants to merge 1 commit into
Open
kvm: do not fail a NAS backup on non-numeric output from nasbackup.sh#9calvix wants to merge 1 commit into
calvix wants to merge 1 commit into
Conversation
Copy link
Copy Markdown
Owner
Sorry, something went wrong.
parseBackupSize() parsed every line of the backup script's stdout as a byte count. The script is not the only writer on that stream: mount helpers and storage clients emit warnings of their own, and one such line was enough to throw NumberFormatException out of the wrapper, e.g. java.lang.NumberFormatException: For input string: "2026-08-25T17:38:38.403+0000" emitted by a Ceph client as "unable to find a keyring on /etc/ceph/ceph.keyring". The cost of that is out of proportion to the cause. The exception escapes after the backup has already been written, so the data is on the repository but the record stays in BackingUp - there is no timeout and no transition to Failed - and it blocks every later restore of that instance until the row is corrected by hand. Ignore lines that do not begin with a byte count instead, logging them at debug. Only the multi-volume branch was reachable in practice, which is the branch taken for a stopped instance, so running instances backed up normally and the failure went unnoticed. Adds LibvirtTakeBackupCommandWrapperTest; four of its seven cases fail without this change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
LibvirtTakeBackupCommandWrapper.parseBackupSize() parsed every line of nasbackup.sh's stdout as a byte count. The script is not the only writer on that stream — mount helpers and storage clients emit warnings of their own — and a single such line threw out of the wrapper:
That line came from the Ceph client:
The cost is out of proportion to the cause. The exception escapes after the backup has already been written, so the data sits on the repository, but the record stays in BackingUp — there is no timeout and no transition to Failed — and it blocks every later restore of that instance until the row is fixed by hand.
This ignores lines that do not begin with a byte count, logging them at debug, instead of parsing them.
Why it went unnoticed
Only the multi-volume branch (diskPaths non-empty) was reachable in practice, and that is the branch taken for a stopped instance. Running instances back up through the single-volume branch and were unaffected, so the bug only surfaces on the stopped-instance path.
Reproduction
On a KVM host with Ceph RBD primary storage and a NAS backup repository, where the agent's environment has no readable /etc/ceph/ceph.keyring: back up a stopped instance. The backup lands on the repository; the record never leaves BackingUp.
Fix
parseSizeLine() returns the leading byte count of a line, or null when the line is not a size line:
Trailing tokens after the byte count are still honoured, and blank lines are ignored.
Relationship to other PRs
Neither apache#12843 nor apache#12898 touches parseBackupSize. apache#13877 refactors this class but keeps the unguarded Long.parseLong(line.split(" ")[0].trim()), so it does not fix this. I found no open PR or issue covering it.
Types of changes
How Has This Been Tested?
Adds LibvirtTakeBackupCommandWrapperTest (7 cases). Verified both ways in a 4.23 reactor build:
The three cases covering existing behaviour (multi-volume sum, single-volume last line, trailing tokens) pass both with and without the fix, so current semantics are unchanged.
Also confirmed in practice on a KVM/Ceph dev host: with the keyring absent, a stopped-instance backup wedged in BackingUp; with this change on the agent, the same backup completed at 2.55 GB.