Nasbackup quiesce fixes

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 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;
     }

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<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;
 
@@ -578,6 +587,7 @@ public Pair<Boolean, Backup> 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,10 +655,11 @@ public Pair<Boolean, Backup> 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());
+                // 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);
         }
     }
@@ -1254,7 +1265,8 @@ public ConfigKey<?>[] getConfigKeys() {
         return new ConfigKey[]{
                 NASBackupRestoreMountTimeout,
                 NASBackupFullEvery,
-                NASBackupIncrementalEnabled
+                NASBackupIncrementalEnabled,
+                NASBackupQuiesceAgentTimeout
         };
     }
 

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,48 @@ 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);
+        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());
+    }
+
+    @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());
+    }
+
     @Test
     public void testGetVMHypervisorHost() {
         Long hostId = 1L;

plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java

@@ -143,6 +143,7 @@ private Pair<Integer, String> runBackupScript(LibvirtComputingResource libvirtCo
             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 @@ private Pair<Integer, String> 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);

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"));
+    }
+}