From 3654ff0d9f95f716319646f74298a26eda28a09b Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Tue, 25 Aug 2026 20:30:24 +0200 Subject: [PATCH] kvm: do not fail a NAS backup on non-numeric output from nasbackup.sh 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. --- .../LibvirtTakeBackupCommandWrapper.java | 40 ++++++++- .../LibvirtTakeBackupCommandWrapperTest.java | 86 +++++++++++++++++++ 2 files changed, 122 insertions(+), 4 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java index 106fe31a0f18..23025bf95bbe 100644 --- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java +++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java @@ -212,19 +212,51 @@ private String validateBackupArgs(TakeBackupCommand command) { * Sum the per-disk size lines emitted by nasbackup.sh. Single-volume mode emits one * line containing just the byte count; multi-volume mode emits one line per disk * whose first whitespace-separated token is the byte count. + * + * Lines that do not start with a byte count are ignored rather than parsed. The + * script is not the only thing that can write to this stream: mount helpers and + * storage clients emit warnings of their own, and a single one of them used to + * abort the whole backup with a NumberFormatException. That failure is unusually + * expensive - the backup record stays in BackingUp with no timeout and no + * transition to Failed, and blocks every later restore of that instance - and it + * only ever hit the multi-volume branch, which is taken for a STOPPED instance, + * so it went unnoticed while running instances backed up normally. */ private long parseBackupSize(String stdout, List diskPaths) { long backupSize = 0L; if (CollectionUtils.isNullOrEmpty(diskPaths)) { - List outputLines = Arrays.asList(stdout.split("\n")); - if (!outputLines.isEmpty()) { - backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim()); + String[] outputLines = stdout.split("\n"); + for (int i = outputLines.length - 1; i >= 0; i--) { + Long size = parseSizeLine(outputLines[i]); + if (size != null) { + backupSize = size; + break; + } } } else { for (String line : stdout.split("\n")) { - backupSize = backupSize + Long.parseLong(line.split(" ")[0].trim()); + Long size = parseSizeLine(line); + if (size != null) { + backupSize = backupSize + size; + } } } return backupSize; } + + /** + * The byte count a size line starts with, or null when the line is not one. + */ + private Long parseSizeLine(String line) { + String token = line.trim().split("\\s+")[0]; + if (token.isEmpty()) { + return null; + } + try { + return Long.parseLong(token); + } catch (NumberFormatException e) { + logger.debug("Ignoring non-numeric line in backup script output: {}", line.trim()); + return null; + } + } } diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java new file mode 100644 index 000000000000..a564deaa5b77 --- /dev/null +++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java @@ -0,0 +1,86 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. +package com.cloud.hypervisor.kvm.resource.wrapper; + +import java.lang.reflect.Method; +import java.util.Arrays; +import java.util.List; + +import org.junit.Assert; +import org.junit.Test; + +public class LibvirtTakeBackupCommandWrapperTest { + + private final LibvirtTakeBackupCommandWrapper wrapper = new LibvirtTakeBackupCommandWrapper(); + + private long parseBackupSize(String stdout, List diskPaths) throws Exception { + Method method = LibvirtTakeBackupCommandWrapper.class + .getDeclaredMethod("parseBackupSize", String.class, List.class); + method.setAccessible(true); + return (long) method.invoke(wrapper, stdout, diskPaths); + } + + private static final List TWO_DISKS = Arrays.asList("/disk/a", "/disk/b"); + + @Test + public void testMultiVolumeSumsEveryDisk() throws Exception { + Assert.assertEquals(3145728L, parseBackupSize("1048576\n2097152", TWO_DISKS)); + } + + @Test + public void testSingleVolumeTakesTheLastLine() throws Exception { + Assert.assertEquals(2097152L, parseBackupSize("1048576\n2097152", null)); + } + + /** + * A mount helper or storage client warning on the same stream must not abort the + * backup. Before this was tolerated, one such line threw NumberFormatException and + * left the backup in BackingUp for ever, blocking every later restore of the + * instance. Only the multi-volume branch was affected, which is the branch taken + * for a STOPPED instance. + */ + @Test + public void testMultiVolumeIgnoresNonNumericLines() throws Exception { + String stdout = "2026-08-25T17:38:38.403+0000 -1 auth: unable to find a keyring on /etc/ceph/ceph.keyring\n" + + "1048576\n" + + "2097152"; + Assert.assertEquals(3145728L, parseBackupSize(stdout, TWO_DISKS)); + } + + @Test + public void testSingleVolumeSkipsTrailingNoise() throws Exception { + String stdout = "1048576\nmount: warning: something happened"; + Assert.assertEquals(1048576L, parseBackupSize(stdout, null)); + } + + @Test + public void testMultiVolumeKeepsTrailingTokensOnASizeLine() throws Exception { + // Size lines may carry the path after the byte count; only the first token counts. + Assert.assertEquals(3145728L, parseBackupSize("1048576 /disk/a\n2097152 /disk/b", TWO_DISKS)); + } + + @Test + public void testBlankLinesAreIgnored() throws Exception { + Assert.assertEquals(1048576L, parseBackupSize("\n1048576\n\n", TWO_DISKS)); + } + + @Test + public void testNoNumericLineYieldsZeroRatherThanThrowing() throws Exception { + Assert.assertEquals(0L, parseBackupSize("only warnings here\nand here", TWO_DISKS)); + Assert.assertEquals(0L, parseBackupSize("only warnings here", null)); + } +}