This is an automated email from the ASF dual-hosted git repository.
abh1sar pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.22 by this push:
new 08831859497 Nasbackup quiesce fixes (#14305)
08831859497 is described below
commit 088318594973ce2283a3f2171d6ce207899515fb
Author: MitchDrage <[email protected]>
AuthorDate: Mon Oct 12 21:55:00 2026 +1100
Nasbackup quiesce fixes (#14305)
* NAS backup: keep the schedule id on a failed backup left in Error state
* NAS backup: add nas.backup.quiesce.agent.timeout for guest agent freeze
and thaw
* nasbackup.sh: abort the backup job before cleanup removes its destination
* nasbackup.sh: always thaw after a freeze attempt and log freeze failures
* nasbackup.sh: confirm a failed thaw against guest-fsfreeze-status before
continuing
* nasbackup.sh: only end the abort wait on a successful domjobinfo query
* nasbackup.sh: retry failed job queries in the backup polling loop
* NAS backup: count a backup kept in Error state in the account resource
count
* NAS backup: alert when cleanup fails after a failed backup
---
.../cloudstack/backup/TakeBackupCommand.java | 9 ++
.../cloudstack/backup/NASBackupProvider.java | 37 ++++++-
.../cloudstack/backup/NASBackupProviderTest.java | 79 +++++++++++--
.../wrapper/LibvirtTakeBackupCommandWrapper.java | 8 +-
.../LibvirtTakeBackupCommandWrapperTest.java | 102 +++++++++++++++++
scripts/vm/hypervisor/kvm/nasbackup.sh | 122 +++++++++++++++++++--
.../cloudstack/backup/BackupManagerImpl.java | 30 +++--
.../cloudstack/backup/BackupManagerTest.java | 55 ++++++++++
8 files changed, 406 insertions(+), 36 deletions(-)
diff --git
a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java
b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java
index 34f8d7b8bcd..7c2d3fc76fc 100644
--- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java
+++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java
@@ -33,6 +33,7 @@ public class TakeBackupCommand extends Command {
private List<PrimaryDataStoreTO> volumePools;
private List<String> volumePaths;
private Boolean quiesce;
+ private Integer quiesceTimeout;
@LogLevel(LogLevel.Log4jLevel.Off)
private String mountOptions;
@@ -117,6 +118,14 @@ public class TakeBackupCommand extends Command {
this.quiesce = quiesce;
}
+ public Integer getQuiesceTimeout() {
+ return quiesceTimeout;
+ }
+
+ public void setQuiesceTimeout(Integer quiesceTimeout) {
+ this.quiesceTimeout = quiesceTimeout;
+ }
+
public String getMode() {
return mode;
}
diff --git
a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
index 08c54100bc2..a72920fbae8 100644
---
a/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
+++
b/plugins/backup/nas/src/main/java/org/apache/cloudstack/backup/NASBackupProvider.java
@@ -17,6 +17,7 @@
package org.apache.cloudstack.backup;
import com.cloud.agent.AgentManager;
+import com.cloud.alert.AlertManager;
import com.cloud.exception.AgentUnavailableException;
import com.cloud.exception.OperationTimedoutException;
import com.cloud.configuration.Resource;
@@ -117,6 +118,15 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
ConfigKey.Scope.Zone,
NASBackupIncrementalEnabled.key());
+ ConfigKey<Integer> NASBackupQuiesceAgentTimeout = new
ConfigKey<>("Advanced", Integer.class,
+ "nas.backup.quiesce.agent.timeout",
+ "30",
+ "Timeout in seconds for the guest agent filesystem freeze and thaw
commands of a quiesced NAS backup. " +
+ "Set to 0 to use the libvirt default (5 seconds unless
changed on the host).",
+ true,
+ ConfigKey.Scope.Zone,
+ BackupFrameworkEnabled.key());
+
@Inject
private BackupDao backupDao;
@@ -171,6 +181,9 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
@Inject
private BackupDetailsDao backupDetailsDao;
+ @Inject
+ private AlertManager alertManager;
+
private Long getClusterIdFromRootVolume(VirtualMachine vm) {
VolumeVO rootVolume = volumeDao.getInstanceRootVolume(vm.getId());
StoragePoolVO rootDiskPool =
primaryDataStoreDao.findById(rootVolume.getPoolId());
@@ -578,6 +591,7 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
command.setBackupRepoAddress(backupRepository.getAddress());
command.setMountOptions(backupRepository.getMountOptions());
command.setQuiesce(quiesceVM);
+
command.setQuiesceTimeout(NASBackupQuiesceAgentTimeout.valueIn(vm.getDataCenterId()));
command.setMode(decision.mode);
command.setBitmapNew(decision.bitmapNew);
command.setBitmapParent(decision.bitmapParent);
@@ -643,16 +657,30 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
logger.error("Failed to take backup for VM {}: {}",
vm.getInstanceName(), answer != null ? answer.getDetails() : "No answer
received");
if (answer != null && answer.getNeedsCleanup()) {
logger.error("Backup cleanup failed for VM {}. Leaving the
backup in Error state. Backup should be manually deleted to free up the space",
vm.getInstanceName());
+ sendBackupCleanupFailedAlert(vm, host, answer.getDetails());
backupVO.setStatus(Backup.Status.Error);
backupDao.update(backupVO.getId(), backupVO);
- } else {
- backupVO.setStatus(Backup.Status.Failed);
- backupDao.remove(backupVO.getId());
+ // Return the row to the caller to record its schedule - e.g.
Scheduled or Manual.
+ return new Pair<>(false, backupVO);
}
+ backupVO.setStatus(Backup.Status.Failed);
+ backupDao.remove(backupVO.getId());
return new Pair<>(false, null);
}
}
+ /**
+ * Alerts when nasbackup.sh could not clean up after a failed backup. The
script's output
+ * names the backup repository mount point it left on the host.
+ */
+ private void sendBackupCleanupFailedAlert(VirtualMachine vm, Host host,
String details) {
+ String subject = String.format("NAS backup cleanup failed for VM %s on
host %s", vm.getInstanceName(), host.getName());
+ String body = String.format("A failed backup of VM %s could not be
cleaned up on host %s (%s). " +
+ "The backup repository may still be mounted on that host.
Script output: %s",
+ vm.getInstanceName(), host.getName(),
host.getPrivateIpAddress(), details);
+
alertManager.sendAlert(AlertManager.AlertType.ALERT_TYPE_BACKUP_STORAGE,
vm.getDataCenterId(), host.getPodId(), subject, body);
+ }
+
private BackupVO createBackupObject(VirtualMachine vm, String backupPath,
String type) {
BackupVO backup = new BackupVO();
backup.setVmId(vm.getId());
@@ -1254,7 +1282,8 @@ public class NASBackupProvider extends AdapterBase
implements BackupProvider, Co
return new ConfigKey[]{
NASBackupRestoreMountTimeout,
NASBackupFullEvery,
- NASBackupIncrementalEnabled
+ NASBackupIncrementalEnabled,
+ NASBackupQuiesceAgentTimeout
};
}
diff --git
a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
index 7c5edba1e2b..787d3ff9456 100644
---
a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
+++
b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
@@ -36,6 +36,7 @@ import org.mockito.junit.MockitoJUnitRunner;
import org.springframework.test.util.ReflectionTestUtils;
import com.cloud.agent.AgentManager;
+import com.cloud.alert.AlertManager;
import com.cloud.exception.AgentUnavailableException;
import com.cloud.exception.OperationTimedoutException;
import com.cloud.configuration.Resource;
@@ -125,6 +126,9 @@ public class NASBackupProviderTest {
@Mock
private DataStoreManager dataStoreMgr;
+ @Mock
+ private AlertManager alertManager;
+
@Test
public void testDeleteBackup() throws OperationTimedoutException,
AgentUnavailableException {
Long hostId = 1L;
@@ -202,16 +206,7 @@ public class NASBackupProviderTest {
Assert.assertEquals(Long.valueOf(3000L), result.second());
}
- @Test
- public void takeBackupSuccessfully() throws AgentUnavailableException,
OperationTimedoutException {
- Long vmId = 1L;
- Long hostId = 2L;
- Long backupOfferingId = 3L;
- Long accountId = 4L;
- Long domainId = 5L;
- Long zoneId = 6L;
- Long backupId = 7L;
-
+ private VMInstanceVO mockRunningVmForBackup(Long vmId, Long hostId, Long
backupOfferingId, Long accountId, Long domainId, Long zoneId) {
VMInstanceVO vm = mock(VMInstanceVO.class);
Mockito.when(vm.getId()).thenReturn(vmId);
Mockito.when(vm.getHostId()).thenReturn(hostId);
@@ -241,6 +236,16 @@ public class NASBackupProviderTest {
Mockito.when(volume2.getState()).thenReturn(Volume.State.Ready);
Mockito.when(volume2.getSize()).thenReturn(200L);
Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(volume1,
volume2));
+ return vm;
+ }
+
+ @Test
+ public void takeBackupSuccessfully() throws AgentUnavailableException,
OperationTimedoutException {
+ Long backupOfferingId = 3L;
+ Long accountId = 4L;
+ Long domainId = 5L;
+ Long zoneId = 6L;
+ VMInstanceVO vm = mockRunningVmForBackup(1L, 2L, backupOfferingId,
accountId, domainId, zoneId);
BackupAnswer answer = mock(BackupAnswer.class);
Mockito.when(answer.getResult()).thenReturn(true);
@@ -269,6 +274,60 @@ public class NASBackupProviderTest {
Mockito.verify(agentManager).send(anyLong(),
Mockito.any(TakeBackupCommand.class));
}
+ @Test
+ public void takeBackupFailureNeedingCleanupReturnsErrorBackup() throws
AgentUnavailableException, OperationTimedoutException {
+ VMInstanceVO vm = mockRunningVmForBackup(1L, 2L, 3L, 4L, 5L, 6L);
+ HostVO host = hostDao.findById(2L);
+ Mockito.when(host.getName()).thenReturn("kvm01");
+ Mockito.when(host.getPodId()).thenReturn(7L);
+
+ BackupAnswer answer = mock(BackupAnswer.class);
+ Mockito.when(answer.getResult()).thenReturn(false);
+ Mockito.when(answer.getNeedsCleanup()).thenReturn(true);
+ Mockito.when(answer.getDetails()).thenReturn("leaving
/tmp/csbackup.abcde/i-2-3-VM mounted at /tmp/csbackup.abcde");
+ ArgumentCaptor<TakeBackupCommand> commandCaptor =
ArgumentCaptor.forClass(TakeBackupCommand.class);
+ Mockito.when(agentManager.send(anyLong(),
commandCaptor.capture())).thenReturn(answer);
+
+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation
-> invocation.getArgument(0));
+
+ Pair<Boolean, Backup> result = nasBackupProvider.takeBackup(vm, true);
+
+ Assert.assertFalse(result.first());
+ Assert.assertNotNull(result.second());
+ Assert.assertEquals(Backup.Status.Error, result.second().getStatus());
+ Mockito.verify(backupDao, Mockito.never()).remove(Mockito.anyLong());
+
+ TakeBackupCommand command = commandCaptor.getValue();
+ Assert.assertTrue(command.getQuiesce());
+ Assert.assertEquals(Integer.valueOf(30), command.getQuiesceTimeout());
+
+ ArgumentCaptor<String> subjectCaptor =
ArgumentCaptor.forClass(String.class);
+ ArgumentCaptor<String> bodyCaptor =
ArgumentCaptor.forClass(String.class);
+
Mockito.verify(alertManager).sendAlert(Mockito.eq(AlertManager.AlertType.ALERT_TYPE_BACKUP_STORAGE),
Mockito.eq(6L), Mockito.eq(7L),
+ subjectCaptor.capture(), bodyCaptor.capture());
+ Assert.assertTrue(subjectCaptor.getValue().contains("kvm01"));
+
Assert.assertTrue(bodyCaptor.getValue().contains("/tmp/csbackup.abcde"));
+ }
+
+ @Test
+ public void takeBackupFailureWithoutCleanupRemovesBackup() throws
AgentUnavailableException, OperationTimedoutException {
+ VMInstanceVO vm = mockRunningVmForBackup(1L, 2L, 3L, 4L, 5L, 6L);
+
+ BackupAnswer answer = mock(BackupAnswer.class);
+ Mockito.when(answer.getResult()).thenReturn(false);
+ Mockito.when(answer.getNeedsCleanup()).thenReturn(false);
+ Mockito.when(agentManager.send(anyLong(),
Mockito.any(TakeBackupCommand.class))).thenReturn(answer);
+
+
Mockito.when(backupDao.persist(Mockito.any(BackupVO.class))).thenAnswer(invocation
-> invocation.getArgument(0));
+
+ Pair<Boolean, Backup> result = nasBackupProvider.takeBackup(vm, false);
+
+ Assert.assertFalse(result.first());
+ Assert.assertNull(result.second());
+ Mockito.verify(backupDao).remove(Mockito.anyLong());
+ Mockito.verifyNoInteractions(alertManager);
+ }
+
@Test
public void testGetVMHypervisorHost() {
Long hostId = 1L;
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 e76c9a2a387..b7e60958765 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
@@ -143,6 +143,7 @@ public class LibvirtTakeBackupCommandWrapper extends
CommandWrapper<TakeBackupCo
TakeBackupCommand command, String vmName, String backupRepoType,
String backupRepoAddress,
String mountOptions, String backupPath, List<String> diskPaths,
String mode,
String bitmapNew, String bitmapParent, List<String> parentPaths,
int timeout) {
+ boolean quiesce = command.getQuiesce() != null && command.getQuiesce();
List<String> argv = new ArrayList<>(Arrays.asList(
libvirtComputingResource.getNasBackupPath(),
"-o", "backup",
@@ -151,9 +152,14 @@ public class LibvirtTakeBackupCommandWrapper extends
CommandWrapper<TakeBackupCo
"-s", backupRepoAddress,
"-m", Objects.nonNull(mountOptions) ? mountOptions : "",
"-p", backupPath,
- "-q", command.getQuiesce() != null && command.getQuiesce() ?
"true" : "false",
+ "-q", quiesce ? "true" : "false",
"-d", CollectionUtils.isEmpty(diskPaths) ? "" :
String.join(",", diskPaths)
));
+ Integer quiesceTimeout = command.getQuiesceTimeout();
+ if (quiesce && quiesceTimeout != null && quiesceTimeout > 0) {
+ argv.add("--quiesce-timeout");
+ argv.add(String.valueOf(quiesceTimeout));
+ }
if (StringUtils.isNotBlank(mode)) {
argv.add("-M");
argv.add(mode);
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 00000000000..7ef64fd53b5
--- /dev/null
+++
b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java
@@ -0,0 +1,102 @@
+// 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 static org.mockito.ArgumentMatchers.anyList;
+import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.mockStatic;
+import static org.mockito.Mockito.when;
+
+import java.util.Arrays;
+import java.util.List;
+
+import org.apache.cloudstack.backup.BackupAnswer;
+import org.apache.cloudstack.backup.TakeBackupCommand;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import com.cloud.hypervisor.kvm.resource.LibvirtComputingResource;
+import com.cloud.utils.Pair;
+import com.cloud.utils.script.Script;
+
+@RunWith(MockitoJUnitRunner.class)
+public class LibvirtTakeBackupCommandWrapperTest {
+
+ private LibvirtTakeBackupCommandWrapper wrapper;
+ private LibvirtComputingResource libvirtComputingResource;
+ private TakeBackupCommand command;
+
+ @Before
+ public void setUp() {
+ wrapper = new LibvirtTakeBackupCommandWrapper();
+ libvirtComputingResource =
Mockito.mock(LibvirtComputingResource.class);
+
when(libvirtComputingResource.getNasBackupPath()).thenReturn("nasbackup.sh");
+ command = new TakeBackupCommand("i-2-3-VM",
"i-2-3-VM/2026.10.04.00.00.00");
+ command.setBackupRepoType("nfs");
+ command.setBackupRepoAddress("10.0.0.1:/backup");
+ command.setWait(60);
+ }
+
+ @SuppressWarnings("unchecked")
+ private List<String> runAndCaptureArgv() {
+ try (MockedStatic<Script> scriptMock = mockStatic(Script.class)) {
+ scriptMock.when(() -> Script.executePipedCommands(anyList(),
anyLong())).thenReturn(new Pair<>(0, "1024"));
+
+ BackupAnswer answer = (BackupAnswer) wrapper.execute(command,
libvirtComputingResource);
+ Assert.assertTrue(answer.getResult());
+
+ ArgumentCaptor<List<String[]>> captor =
ArgumentCaptor.forClass(List.class);
+ scriptMock.verify(() ->
Script.executePipedCommands(captor.capture(), anyLong()));
+ return Arrays.asList(captor.getValue().get(0));
+ }
+ }
+
+ @Test
+ public void executePassesQuiesceTimeoutWhenQuiescing() {
+ command.setQuiesce(true);
+ command.setQuiesceTimeout(30);
+
+ List<String> argv = runAndCaptureArgv();
+
+ int idx = argv.indexOf("--quiesce-timeout");
+ Assert.assertTrue(idx > 0);
+ Assert.assertEquals("30", argv.get(idx + 1));
+ Assert.assertEquals("true", argv.get(argv.indexOf("-q") + 1));
+ }
+
+ @Test
+ public void executeOmitsQuiesceTimeoutWithoutQuiesce() {
+ command.setQuiesce(false);
+ command.setQuiesceTimeout(30);
+
+ Assert.assertFalse(runAndCaptureArgv().contains("--quiesce-timeout"));
+ }
+
+ @Test
+ public void executeOmitsQuiesceTimeoutWhenNotPositive() {
+ command.setQuiesce(true);
+ command.setQuiesceTimeout(0);
+
+ Assert.assertFalse(runAndCaptureArgv().contains("--quiesce-timeout"));
+ }
+}
diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh
b/scripts/vm/hypervisor/kvm/nasbackup.sh
index ee044459a1b..04cc7cd86f2 100755
--- a/scripts/vm/hypervisor/kvm/nasbackup.sh
+++ b/scripts/vm/hypervisor/kvm/nasbackup.sh
@@ -32,6 +32,7 @@ MOUNT_OPTS=""
BACKUP_DIR=""
DISK_PATHS=""
QUIESCE=""
+QUIESCE_TIMEOUT="" # Seconds to wait for the guest agent freeze/thaw; empty
=> libvirt default
# Incremental backup parameters (all optional; legacy callers omit them)
MODE="" # "full" or "incremental"; empty => legacy full-only
behavior (no checkpoint created)
BITMAP_NEW="" # Bitmap/checkpoint name to create with this backup
(e.g. "backup-1711586400")
@@ -44,6 +45,7 @@ PARENT_PATHS="" # For incremental: comma-separated list
of parent backup f
logFile="/var/log/cloudstack/agent/agent.log"
EXIT_CLEANUP_FAILED=20
+BACKUP_JOB_ACTIVE=0
log() {
[[ "$verb" -eq 1 ]] && builtin echo "$@"
@@ -134,6 +136,53 @@ get_linstor_uuid_from_device() {
return 1
}
+qemu_agent_command() {
+ local timeout_args=()
+ if [[ -n "$QUIESCE_TIMEOUT" ]]; then
+ timeout_args=(--timeout "$QUIESCE_TIMEOUT")
+ fi
+ virsh -c qemu:///system qemu-agent-command "$VM" "${timeout_args[@]}" "$1"
+}
+
+guest_fsfreeze_status() {
+ local response
+ response=$(qemu_agent_command '{"execute":"guest-fsfreeze-status"}'
2>>"$logFile") || return 1
+ grep -oE 'thawed|frozen' <<< "$response" | head -n 1
+}
+
+guest_agent_connected() {
+ local xml
+ xml=$(virsh -c qemu:///system dumpxml "$VM" 2>/dev/null) || return 1
+ grep -q "name='org.qemu.guest_agent.0' state='connected'" <<< "$xml"
+}
+
+# Thaws the guest filesystems. A thaw that times out on the host can still
complete in the
+# guest, so each failure is checked against guest-fsfreeze-status before
retrying. Succeeds
+# once the guest reports thawed, or when the freeze failed and the guest agent
is not
+# connected, which means the freeze never reached the guest.
+thaw_guest() {
+ local freeze_ok=$1
+ local attempt response state
+ for ((attempt = 1; attempt <= 3; attempt++)); do
+ if response=$(qemu_agent_command '{"execute":"guest-fsfreeze-thaw"}'
2>&1); then
+ return 0
+ fi
+ log -e "Thaw attempt $attempt for vm $VM failed: $response"
+ state=$(guest_fsfreeze_status || true)
+ if [[ "$state" == "thawed" ]]; then
+ log -e "Guest filesystem of vm $VM reports thawed after the failed thaw"
+ return 0
+ fi
+ if [[ -z "$state" && $freeze_ok -eq 0 ]] && ! guest_agent_connected; then
+ log -e "Guest agent of vm $VM is not connected, so the failed freeze
never reached the guest"
+ return 0
+ fi
+ sleep 5
+ done
+ echo "Failed to thaw the filesystem for vm $VM, guest filesystem state:
${state:-unknown}: $response"
+ return 1
+}
+
backup_running_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit
1; }
@@ -257,9 +306,15 @@ print(len(files))
fi
local thaw=0
+ local freeze_ok=0
if [[ ${QUIESCE} == "true" ]]; then
- if virsh -c qemu:///system qemu-agent-command "$VM"
'{"execute":"guest-fsfreeze-freeze"}' > /dev/null 2>/dev/null; then
- thaw=1
+ # Always thaw after a freeze attempt: a freeze that times out on the host
can still
+ # complete inside the guest, and nothing else would unfreeze it.
+ thaw=1
+ if freeze_err=$(qemu_agent_command '{"execute":"guest-fsfreeze-freeze"}'
2>&1 > /dev/null); then
+ freeze_ok=1
+ else
+ log -e "Failed to freeze the filesystem for vm $VM, continuing without
quiescing: $freeze_err"
fi
fi
@@ -276,13 +331,13 @@ print(len(files))
backup_begin=1;
fi
fi
+ if [[ $backup_begin -eq 1 ]]; then
+ BACKUP_JOB_ACTIVE=1
+ fi
- if [[ $thaw -eq 1 ]]; then
- if ! response=$(virsh -c qemu:///system qemu-agent-command "$VM"
'{"execute":"guest-fsfreeze-thaw"}' 2>&1); then
- echo "Failed to thaw the filesystem for vm $VM: $response"
- cleanup
- exit 1
- fi
+ if [[ $thaw -eq 1 ]] && ! thaw_guest "$freeze_ok"; then
+ cleanup
+ exit 1
fi
if [[ $backup_begin -ne 1 ]]; then
@@ -296,14 +351,30 @@ print(len(files))
virsh -c qemu:///system domiflist $VM > $dest/domiflist.xml 2>/dev/null
virsh -c qemu:///system domblklist $VM > $dest/domblklist.xml 2>/dev/null
+ local info query_failures=0
while true; do
- status=$(virsh -c qemu:///system domjobinfo $VM --completed
--keep-completed | awk '/Job type:/ {print $3}')
+ # A failed query says nothing about the job, so retry before giving up on
it.
+ if ! info=$(virsh -c qemu:///system domjobinfo $VM --completed
--keep-completed 2>&1); then
+ log -e "Backup job query $((query_failures + 1)) for vm $VM failed:
$info"
+ if (( ++query_failures >= 12 )); then
+ echo "Unable to query the backup job for vm $VM"
+ cleanup
+ exit 1
+ fi
+ sleep 5
+ continue
+ fi
+ query_failures=0
+ status=$(awk '/Job type:/ {print $3}' <<< "$info")
case "$status" in
Completed)
+ BACKUP_JOB_ACTIVE=0
break ;;
Failed)
+ BACKUP_JOB_ACTIVE=0
echo "Virsh backup job failed"
- cleanup ;;
+ cleanup
+ exit 1 ;;
esac
sleep 5
done
@@ -503,9 +574,34 @@ mount_operation() {
fi
}
+abort_backup_job() {
+ [[ $BACKUP_JOB_ACTIVE -eq 1 ]] || return 0
+
+ # The push backup job writes into $dest until it ends, so it must be gone
before the
+ # destination is removed or unmounted.
+ virsh -c qemu:///system domjobabort "$VM" > /dev/null 2>>"$logFile" || true
+ local i info
+ for ((i = 0; i < 60; i++)); do
+ # Only a successful query reporting no job proves the job has ended.
+ if info=$(virsh -c qemu:///system domjobinfo "$VM" 2>>"$logFile") \
+ && [[ "$(awk '/Job type:/ {print $3}' <<< "$info")" == "None" ]]; then
+ BACKUP_JOB_ACTIVE=0
+ return 0
+ fi
+ sleep 1
+ done
+ return 1
+}
+
cleanup() {
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"
+ echo "Backup cleanup failed"
+ exit $EXIT_CLEANUP_FAILED
+ fi
+
rm -rf "$dest" || { echo "Failed to delete $dest"; status=1; }
umount "$mount_point" || { echo "Failed to unmount $mount_point"; status=1; }
rmdir "$mount_point" || { echo "Failed to remove mount point $mount_point";
status=1; }
@@ -519,6 +615,7 @@ cleanup() {
function usage {
echo ""
echo "Usage: $0 -o <operation> -v|--vm <domain name> -t <storage type> -s
<storage address> -m <mount options> -p <backup path> -d <disks path>
-q|--quiesce <true|false>"
+ echo " [--quiesce-timeout <seconds>]"
echo " [-M|--mode <full|incremental>] [--bitmap-new <name>]
[--bitmap-parent <name>] [--parent-paths <p1,p2,...>]"
echo ""
echo "Incremental backup options (running VMs only; requires QEMU >= 4.2 and
libvirt >= 7.2):"
@@ -569,6 +666,11 @@ while [[ $# -gt 0 ]]; do
shift
shift
;;
+ --quiesce-timeout)
+ QUIESCE_TIMEOUT="$2"
+ shift
+ shift
+ ;;
-d|--diskpaths)
DISK_PATHS="$2"
shift
diff --git
a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
index 867f4111b66..1f42bc413eb 100644
--- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
+++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
@@ -826,20 +826,18 @@ public class BackupManagerImpl extends ManagerBase
implements BackupManager {
true, 0);
Pair<Boolean, Backup> result = backupProvider.takeBackup(vm,
cmd.getQuiesceVM());
- if (!result.first()) {
- throw new CloudRuntimeException("Failed to create VM backup");
- }
Backup backup = result.second();
if (backup != null) {
- BackupVO vmBackup =
backupDao.findById(result.second().getId());
- vmBackup.setBackupScheduleId(backupScheduleId);
- if (cmd.getName() != null) {
- vmBackup.setName(cmd.getName());
- }
- vmBackup.setDescription(cmd.getDescription());
- backupDao.update(vmBackup.getId(), vmBackup);
+ updateBackupFromCmd(backup.getId(), cmd, backupScheduleId);
+ // A failed backup returned here is kept in Error state and
still holds space on
+ // the repository. The account's backup resource count
recalculation includes such
+ // rows and deleting one decrements it, so add it to that
count here as well.
resourceLimitMgr.incrementResourceCount(vm.getAccountId(),
Resource.ResourceType.backup);
- resourceLimitMgr.incrementResourceCount(vm.getAccountId(),
Resource.ResourceType.backup_storage, backup.getSize());
+ resourceLimitMgr.incrementResourceCount(vm.getAccountId(),
Resource.ResourceType.backup_storage,
+ backup.getSize() != null ? backup.getSize() : 0L);
+ }
+ if (!result.first()) {
+ throw new CloudRuntimeException("Failed to create VM backup");
}
} catch (ResourceAllocationException e) {
if (isScheduledBackup &&
(Resource.ResourceType.backup.equals(e.getResourceType()) ||
@@ -850,6 +848,16 @@ public class BackupManagerImpl extends ManagerBase
implements BackupManager {
}
}
+ private void updateBackupFromCmd(long backupId, CreateBackupCmd cmd, Long
backupScheduleId) {
+ BackupVO vmBackup = backupDao.findById(backupId);
+ vmBackup.setBackupScheduleId(backupScheduleId);
+ if (cmd.getName() != null) {
+ vmBackup.setName(cmd.getName());
+ }
+ vmBackup.setDescription(cmd.getDescription());
+ backupDao.update(vmBackup.getId(), vmBackup);
+ }
+
/**
* Sends an alert when the backup limit has been exceeded for a given
account.
*
diff --git
a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java
b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java
index db75602b600..67e263a3267 100644
--- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java
+++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java
@@ -733,6 +733,61 @@ public class BackupManagerTest {
}
}
+ @Test
+ public void createBackupTestFailedScheduledBackupKeepsSchedule() throws
ResourceAllocationException {
+ Long vmId = 1L;
+ Long zoneId = 2L;
+ Long scheduleId = 3L;
+ Long backupOfferingId = 4L;
+ Long accountId = 5L;
+ Long backupId = 6L;
+ long domainId = 101L;
+
+ when(vmInstanceDao.findById(vmId)).thenReturn(vmInstanceVOMock);
+ when(vmInstanceVOMock.getDataCenterId()).thenReturn(zoneId);
+
when(vmInstanceVOMock.getBackupOfferingId()).thenReturn(backupOfferingId);
+ when(vmInstanceVOMock.getAccountId()).thenReturn(accountId);
+
+ overrideBackupFrameworkConfigValue();
+
when(backupOfferingDao.findById(backupOfferingId)).thenReturn(backupOfferingVOMock);
+
when(backupOfferingVOMock.isUserDrivenBackupAllowed()).thenReturn(true);
+
when(backupOfferingVOMock.getProvider()).thenReturn("testbackupprovider");
+
+
Mockito.doReturn(scheduleId).when(backupManager).getBackupScheduleId(asyncJobVOMock);
+
+ when(accountManager.getAccount(accountId)).thenReturn(accountVOMock);
+ when(accountVOMock.getDomainId()).thenReturn(domainId);
+
+ when(volumeDao.findByInstance(vmId)).thenReturn(List.of());
+
+ BackupProvider backupProvider = mock(BackupProvider.class);
+ Backup backup = mock(Backup.class);
+ when(backup.getId()).thenReturn(backupId);
+ when(backupProvider.getName()).thenReturn("testbackupprovider");
+ when(backupProvider.takeBackup(vmInstanceVOMock, null)).thenReturn(new
Pair<>(false, backup));
+ Map<String, BackupProvider> backupProvidersMap = new HashMap<>();
+ backupProvidersMap.put(backupProvider.getName().toLowerCase(),
backupProvider);
+ ReflectionTestUtils.setField(backupManager, "backupProvidersMap",
backupProvidersMap);
+
+ BackupVO backupVO = mock(BackupVO.class);
+ when(backupVO.getId()).thenReturn(backupId);
+ when(backupDao.findById(backupId)).thenReturn(backupVO);
+
+ CreateBackupCmd cmd = Mockito.mock(CreateBackupCmd.class);
+ when(cmd.getVmId()).thenReturn(vmId);
+ when(cmd.getQuiesceVM()).thenReturn(null);
+
+ try (MockedStatic<ActionEventUtils> ignored =
Mockito.mockStatic(ActionEventUtils.class)) {
+ Assert.assertThrows(CloudRuntimeException.class, () ->
backupManager.createBackup(cmd, asyncJobVOMock));
+
+ Mockito.verify(backupVO, times(1)).setBackupScheduleId(scheduleId);
+ Mockito.verify(backupDao, times(1)).update(backupId, backupVO);
+ Mockito.verify(resourceLimitMgr,
times(1)).incrementResourceCount(accountId, Resource.ResourceType.backup);
+ Mockito.verify(resourceLimitMgr,
times(1)).incrementResourceCount(accountId,
Resource.ResourceType.backup_storage, 0L);
+ Mockito.verify(backupManager,
Mockito.never()).deleteOldestBackupFromScheduleIfRequired(vmId, scheduleId);
+ }
+ }
+
@Test(expected = ResourceAllocationException.class)
public void createBackupTestResourceLimitReached() throws
ResourceAllocationException {
Long vmId = 1L;
