From f2edac40c1a572862cd0fc826780519a57e8e21f Mon Sep 17 00:00:00 2001 From: Mitch <25337396+MitchDrage@users.noreply.github.com> Date: Mon, 5 Oct 2026 00:16:24 +0000 Subject: [PATCH 1/4] NAS backup: keep the schedule id on a failed backup left in Error state --- .../cloudstack/backup/NASBackupProvider.java | 7 ++- .../backup/NASBackupProviderTest.java | 58 +++++++++++++++---- .../cloudstack/backup/BackupManagerImpl.java | 23 +++++--- .../cloudstack/backup/BackupManagerTest.java | 54 +++++++++++++++++ 4 files changed, 121 insertions(+), 21 deletions(-) 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 08c54100bc22..b7cb0a0b21ac 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 @@ -645,10 +645,11 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce 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()); backupVO.setStatus(Backup.Status.Error); backupDao.update(backupVO.getId(), backupVO); - } else { - backupVO.setStatus(Backup.Status.Failed); - backupDao.remove(backupVO.getId()); + // The row is kept, so return it for the caller to record its schedule. + return new Pair<>(false, backupVO); } + backupVO.setStatus(Backup.Status.Failed); + backupDao.remove(backupVO.getId()); return new Pair<>(false, null); } } 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 7c5edba1e2b6..f414669ffdcd 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 @@ -202,16 +202,7 @@ public void testGetBackupStorageStats() { 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 +232,16 @@ public void takeBackupSuccessfully() throws AgentUnavailableException, Operation 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 +270,43 @@ public void takeBackupSuccessfully() throws AgentUnavailableException, Operation 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); + + BackupAnswer answer = mock(BackupAnswer.class); + Mockito.when(answer.getResult()).thenReturn(false); + Mockito.when(answer.getNeedsCleanup()).thenReturn(true); + 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 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()); + } + + @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 result = nasBackupProvider.takeBackup(vm, false); + + Assert.assertFalse(result.first()); + Assert.assertNull(result.second()); + Mockito.verify(backupDao).remove(Mockito.anyLong()); + } + @Test public void testGetVMHypervisorHost() { Long hostId = 1L; 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 867f4111b66a..dc3506593d0c 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -826,18 +826,15 @@ private void createCheckedBackup(CreateBackupCmd cmd, Account owner, boolean isS true, 0); Pair result = backupProvider.takeBackup(vm, cmd.getQuiesceVM()); + Backup backup = result.second(); if (!result.first()) { + if (backup != null) { + updateBackupFromCmd(backup.getId(), cmd, backupScheduleId); + } 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); resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup); resourceLimitMgr.incrementResourceCount(vm.getAccountId(), Resource.ResourceType.backup_storage, backup.getSize()); } @@ -850,6 +847,16 @@ private void createCheckedBackup(CreateBackupCmd cmd, Account owner, boolean isS } } + 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 db75602b600d..095289a692f6 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,60 @@ public void createBackupTestCreateScheduledBackup() throws ResourceAllocationExc } } + @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 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 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, Mockito.never()).incrementResourceCount(accountId, Resource.ResourceType.backup); + Mockito.verify(backupManager, Mockito.never()).deleteOldestBackupFromScheduleIfRequired(vmId, scheduleId); + } + } + @Test(expected = ResourceAllocationException.class) public void createBackupTestResourceLimitReached() throws ResourceAllocationException { Long vmId = 1L; From fd1dbfbdd19b932945ad578ec5fc3d1f1f6245ae Mon Sep 17 00:00:00 2001 From: Mitch <25337396+MitchDrage@users.noreply.github.com> Date: Mon, 5 Oct 2026 00:16:33 +0000 Subject: [PATCH 2/4] NAS backup: add nas.backup.quiesce.agent.timeout for guest agent freeze and thaw --- .../cloudstack/backup/TakeBackupCommand.java | 9 ++ .../cloudstack/backup/NASBackupProvider.java | 15 ++- .../backup/NASBackupProviderTest.java | 7 +- .../LibvirtTakeBackupCommandWrapper.java | 8 +- .../LibvirtTakeBackupCommandWrapperTest.java | 102 ++++++++++++++++++ scripts/vm/hypervisor/kvm/nasbackup.sh | 19 +++- 6 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapperTest.java 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 34f8d7b8bcdd..7c2d3fc76fc1 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 volumePools; private List volumePaths; private Boolean quiesce; + private Integer quiesceTimeout; @LogLevel(LogLevel.Log4jLevel.Off) private String mountOptions; @@ -117,6 +118,14 @@ public void setQuiesce(Boolean quiesce) { 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 b7cb0a0b21ac..20f02ca4c48e 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 @@ -117,6 +117,15 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co ConfigKey.Scope.Zone, NASBackupIncrementalEnabled.key()); + ConfigKey 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; @@ -578,6 +587,7 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce 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); @@ -645,7 +655,7 @@ public Pair takeBackup(final VirtualMachine vm, Boolean quiesce 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()); backupVO.setStatus(Backup.Status.Error); backupDao.update(backupVO.getId(), backupVO); - // The row is kept, so return it for the caller to record its schedule. + // 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); @@ -1255,7 +1265,8 @@ public ConfigKey[] getConfigKeys() { 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 f414669ffdcd..52b64d7df412 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 @@ -277,7 +277,8 @@ public void takeBackupFailureNeedingCleanupReturnsErrorBackup() throws AgentUnav BackupAnswer answer = mock(BackupAnswer.class); Mockito.when(answer.getResult()).thenReturn(false); Mockito.when(answer.getNeedsCleanup()).thenReturn(true); - Mockito.when(agentManager.send(anyLong(), Mockito.any(TakeBackupCommand.class))).thenReturn(answer); + ArgumentCaptor 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)); @@ -287,6 +288,10 @@ public void takeBackupFailureNeedingCleanupReturnsErrorBackup() throws AgentUnav 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()); } @Test 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 e76c9a2a3871..b7e609587650 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 @@ private Pair runBackupScript(LibvirtComputingResource libvirtCo TakeBackupCommand command, String vmName, String backupRepoType, String backupRepoAddress, String mountOptions, String backupPath, List diskPaths, String mode, String bitmapNew, String bitmapParent, List parentPaths, int timeout) { + boolean quiesce = command.getQuiesce() != null && command.getQuiesce(); List argv = new ArrayList<>(Arrays.asList( libvirtComputingResource.getNasBackupPath(), "-o", "backup", @@ -151,9 +152,14 @@ private Pair runBackupScript(LibvirtComputingResource libvirtCo "-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 000000000000..7ef64fd53b50 --- /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 runAndCaptureArgv() { + try (MockedStatic