From 81f5dd20ca09fb2dbb356e33dcdf65bd5b4ec0ad Mon Sep 17 00:00:00 2001 From: jmsperu Date: Tue, 17 Mar 2026 22:33:45 +0300 Subject: [PATCH 1/8] Allow parallel execution of backup and delete commands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change executeInSequence() to return false for TakeBackupCommand and DeleteBackupCommand, allowing the KVM agent to process multiple backup/delete operations concurrently via its worker thread pool. Previously, all backup commands were serialized — a large VM backup (e.g. 100+ GB taking 2+ hours) would block all other backup and delete operations on the same host. Since each backup mounts its own temporary NFS directory and operates on independent VM disks, there is no shared state requiring serialization. Restore and PrepareForBackupRestoration commands remain sequential as they modify VM state that should not be concurrent. Co-Authored-By: Claude Opus 4.6 --- .../java/org/apache/cloudstack/backup/DeleteBackupCommand.java | 2 +- .../java/org/apache/cloudstack/backup/TakeBackupCommand.java | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java b/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java index 16c611af998e..7a37463f7da4 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java @@ -71,6 +71,6 @@ public void setMountOptions(String mountOptions) { @Override public boolean executeInSequence() { - return true; + return false; } } 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 93855ea17211..7ad08dbafae9 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -89,6 +89,6 @@ public void setVolumePaths(List volumePaths) { @Override public boolean executeInSequence() { - return true; + return false; } } From b2cd0bf02e03821728e65f5828769313a8380683 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Sat, 23 May 2026 00:52:27 +0300 Subject: [PATCH 2/8] fix(backup): restrict parallel execution to TakeBackup only Revert DeleteBackupCommand to executeInSequence()=true per @abh1sar's review. Backup creation is safe to parallelize across VMs because each VM writes to its own NAS path. Backup deletion is not safe to parallelize the same way: incremental chains share parent qcow2 files on the NAS, and concurrent deletions of sibling or descendant nodes can race on the underlying file references and the chain metadata. Until that's modeled explicitly, keep DeleteBackup serialized. TakeBackup parallelization stands. --- .../java/org/apache/cloudstack/backup/DeleteBackupCommand.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java b/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java index 7a37463f7da4..16c611af998e 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java @@ -71,6 +71,6 @@ public void setMountOptions(String mountOptions) { @Override public boolean executeInSequence() { - return false; + return true; } } From 4338c9cfa0cc675ac6ac505e250c42f2b03af999 Mon Sep 17 00:00:00 2001 From: jmsperu Date: Tue, 26 May 2026 21:22:10 +0300 Subject: [PATCH 3/8] ci: retrigger workflow (flaky/stale shards) From b166bfdec257c13ee7003d97608594df1f35566a Mon Sep 17 00:00:00 2001 From: jmsperu Date: Thu, 28 May 2026 16:09:01 +0300 Subject: [PATCH 4/8] docs(backup): comment on TakeBackupCommand.executeInSequence=false MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per Daan's review on PR #12847, document inline why we return false from executeInSequence — the same reasoning previously only given in the PR thread. The comment explains the design intent (parallel dispatch with DeleteBackupCommand) and the safety case (per-VM on-NAS paths + per-script NFS mount lifecycle). --- .../org/apache/cloudstack/backup/TakeBackupCommand.java | 6 ++++++ 1 file changed, 6 insertions(+) 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 7ad08dbafae9..a5500044f86e 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -89,6 +89,12 @@ public void setVolumePaths(List volumePaths) { @Override public boolean executeInSequence() { + // Returning false lets the KVM agent dispatch this command in parallel with + // other backup commands (notably DeleteBackupCommand). A long-running snapshot + // copy must not block a concurrent delete on a different VM's backup. Safe + // because each backup operates on its own on-NAS path (named after the VM + // uuid + timestamp) and the NFS mount lifecycle is per-script-invocation, + // not shared — so there is no path-level contention between parallel runs. return false; } } From e38c5c029dd578e251336a1e9f949da1f7e01af8 Mon Sep 17 00:00:00 2001 From: James Peru Date: Tue, 28 Jul 2026 08:33:30 +0300 Subject: [PATCH 5/8] address review (#12847): trim executeInSequence comment to the essential reasoning --- .../org/apache/cloudstack/backup/TakeBackupCommand.java | 8 ++------ 1 file changed, 2 insertions(+), 6 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 a5500044f86e..dbe3ea64e861 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -89,12 +89,8 @@ public void setVolumePaths(List volumePaths) { @Override public boolean executeInSequence() { - // Returning false lets the KVM agent dispatch this command in parallel with - // other backup commands (notably DeleteBackupCommand). A long-running snapshot - // copy must not block a concurrent delete on a different VM's backup. Safe - // because each backup operates on its own on-NAS path (named after the VM - // uuid + timestamp) and the NFS mount lifecycle is per-script-invocation, - // not shared — so there is no path-level contention between parallel runs. + // Run in parallel with other backup/delete commands; each backup uses its + // own per-VM on-NAS path, so concurrent runs do not contend. return false; } } From 8514c35268cc288fd801654793f0017feabafcd3 Mon Sep 17 00:00:00 2001 From: James Peru Date: Tue, 28 Jul 2026 08:46:37 +0300 Subject: [PATCH 6/8] address review (#12847): add backup.nas.parallel.execution.enabled setting (Zone, default true) to toggle parallel take-backup execution --- .../org/apache/cloudstack/backup/BackupManager.java | 6 ++++++ .../apache/cloudstack/backup/TakeBackupCommand.java | 12 +++++++++--- .../apache/cloudstack/backup/NASBackupProvider.java | 1 + .../apache/cloudstack/backup/BackupManagerImpl.java | 3 ++- 4 files changed, 18 insertions(+), 4 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java index 0090b4f6b16c..257dc1cfcd40 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java @@ -59,6 +59,12 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer "false", "Enable volume attach/detach operations for VMs that are assigned to Backup Offerings.", true); + ConfigKey NASBackupParallelExecution = new ConfigKey<>("Advanced", Boolean.class, + "backup.nas.parallel.execution.enabled", + "true", + "Allow NAS take-backup commands to run in parallel with other backup and delete commands on the KVM host. Disable to force sequential execution.", + true, ConfigKey.Scope.Zone); + /** * List backup provider offerings * @param zoneId zone id 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 dbe3ea64e861..f225c55bb46e 100644 --- a/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java +++ b/core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java @@ -32,6 +32,7 @@ public class TakeBackupCommand extends Command { private List volumePaths; @LogLevel(LogLevel.Log4jLevel.Off) private String mountOptions; + private boolean executeInSequence = false; public TakeBackupCommand(String vmName, String backupPath) { super(); @@ -89,8 +90,13 @@ public void setVolumePaths(List volumePaths) { @Override public boolean executeInSequence() { - // Run in parallel with other backup/delete commands; each backup uses its - // own per-VM on-NAS path, so concurrent runs do not contend. - return false; + // Parallel by default: each backup uses its own per-VM on-NAS path, so concurrent + // runs do not contend. Operators can force sequential execution per zone via the + // backup.nas.parallel.execution.enabled setting. + return executeInSequence; + } + + public void setExecuteInSequence(boolean executeInSequence) { + this.executeInSequence = executeInSequence; } } 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 565ea29acf8b..33d71668ca9f 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 @@ -159,6 +159,7 @@ public boolean takeBackup(final VirtualMachine vm) { command.setBackupRepoType(backupRepository.getType()); command.setBackupRepoAddress(backupRepository.getAddress()); command.setMountOptions(backupRepository.getMountOptions()); + command.setExecuteInSequence(!BackupManager.NASBackupParallelExecution.valueIn(vm.getDataCenterId())); if (VirtualMachine.State.Stopped.equals(vm.getState())) { List vmVolumes = volumeDao.findByInstance(vm.getId()); 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 b3b22e98e45b..2c6cc10b8fd2 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -1023,7 +1023,8 @@ public ConfigKey[] getConfigKeys() { BackupFrameworkEnabled, BackupProviderPlugin, BackupSyncPollingInterval, - BackupEnableAttachDetachVolumes + BackupEnableAttachDetachVolumes, + NASBackupParallelExecution }; } From da829173794090f5c364e1cef84250f3170a26f2 Mon Sep 17 00:00:00 2001 From: James Peru Date: Tue, 28 Jul 2026 10:40:07 +0300 Subject: [PATCH 7/8] address review (#12847): add TakeBackupCommand unit test covering the executeInSequence toggle --- .../backup/TakeBackupCommandTest.java | 41 +++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 core/src/test/java/org/apache/cloudstack/backup/TakeBackupCommandTest.java diff --git a/core/src/test/java/org/apache/cloudstack/backup/TakeBackupCommandTest.java b/core/src/test/java/org/apache/cloudstack/backup/TakeBackupCommandTest.java new file mode 100644 index 000000000000..0f86c2142bdb --- /dev/null +++ b/core/src/test/java/org/apache/cloudstack/backup/TakeBackupCommandTest.java @@ -0,0 +1,41 @@ +// 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 org.apache.cloudstack.backup; + +import org.junit.Assert; +import org.junit.Test; + +public class TakeBackupCommandTest { + + @Test + public void testExecuteInSequenceDefaultsToParallel() { + TakeBackupCommand command = new TakeBackupCommand("vm-1", "/backups/vm-1"); + // Default: run in parallel with other backup/delete commands. + Assert.assertFalse(command.executeInSequence()); + } + + @Test + public void testExecuteInSequenceIsSettable() { + TakeBackupCommand command = new TakeBackupCommand("vm-1", "/backups/vm-1"); + + command.setExecuteInSequence(true); + Assert.assertTrue(command.executeInSequence()); + + command.setExecuteInSequence(false); + Assert.assertFalse(command.executeInSequence()); + } +} From f0096a66eac67e3856736878d1955d6e95941113 Mon Sep 17 00:00:00 2001 From: James Peru Date: Wed, 9 Sep 2026 14:10:19 +0300 Subject: [PATCH 8/8] address review (#12847): bound concurrent NAS backups per host, move the settings into the provider Parallel take-backup commands no longer compete for every agent worker thread: the NAS provider now holds a per-host slot for the whole agent round-trip and waits on the management server (the async job thread) when the host already has backup.nas.parallel.max.per.host (default 2) backups in flight, so start, stop, reboot and migrate commands are never queued behind backups. The wait is bounded by backup.nas.parallel.queue.timeout (default 7200 s) and fails cleanly with no backup row left behind. The settings are NAS-only, so they move from the BackupManager interface into NASBackupProvider.getConfigKeys(), and the enable flag's description now says what it really does: the command runs concurrently with every other command on the host, bounded per host, rather than only "with other backup commands". NASBackupProviderTest covers the setting reaching the command, the per-host cap holding and releasing, independence between hosts, a waiter proceeding once a slot frees, and release on an unknown host. Claude-Session: https://claude.ai/code/session_01MHXx4k6gi77ZCG5jx3wK4J Signed-off-by: James Peru --- .../cloudstack/backup/BackupManager.java | 6 - .../cloudstack/backup/NASBackupProvider.java | 114 +++++++++++++++++- .../backup/NASBackupProviderTest.java | 114 ++++++++++++++++++ .../cloudstack/backup/BackupManagerImpl.java | 3 +- 4 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java diff --git a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java index 257dc1cfcd40..0090b4f6b16c 100644 --- a/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java +++ b/api/src/main/java/org/apache/cloudstack/backup/BackupManager.java @@ -59,12 +59,6 @@ public interface BackupManager extends BackupService, Configurable, PluggableSer "false", "Enable volume attach/detach operations for VMs that are assigned to Backup Offerings.", true); - ConfigKey NASBackupParallelExecution = new ConfigKey<>("Advanced", Boolean.class, - "backup.nas.parallel.execution.enabled", - "true", - "Allow NAS take-backup commands to run in parallel with other backup and delete commands on the KVM host. Disable to force sequential execution.", - true, ConfigKey.Scope.Zone); - /** * List backup provider offerings * @param zoneId zone id 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 33d71668ca9f..b26c61927e6d 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 @@ -49,6 +49,7 @@ import org.apache.logging.log4j.LogManager; import javax.inject.Inject; import java.text.SimpleDateFormat; +import java.util.concurrent.ConcurrentHashMap; import java.util.ArrayList; import java.util.Collections; import java.util.Comparator; @@ -74,6 +75,35 @@ public class NASBackupProvider extends AdapterBase implements BackupProvider, Co private BackupOfferingDao backupOfferingDao; @Inject + public static final ConfigKey NASBackupParallelExecution = new ConfigKey<>("Advanced", Boolean.class, + "backup.nas.parallel.execution.enabled", + "true", + "Let NAS take-backup commands run concurrently on a KVM host instead of queueing behind every earlier command on that host " + + "and holding up every later one. Concurrency is bounded per host by backup.nas.parallel.max.per.host. " + + "Disable to restore strictly sequential execution.", + true, ConfigKey.Scope.Zone); + + public static final ConfigKey NASBackupParallelMaxPerHost = new ConfigKey<>("Advanced", Integer.class, + "backup.nas.parallel.max.per.host", + "2", + "Maximum number of NAS take-backup commands in flight on one KVM host when parallel execution is enabled; further backups " + + "for that host wait on the management server. Keep it below the agent's worker thread count (default 5) so start, stop, " + + "reboot and migrate commands are never queued behind backups.", + true, ConfigKey.Scope.Zone); + + public static final ConfigKey NASBackupParallelQueueTimeout = new ConfigKey<>("Advanced", Integer.class, + "backup.nas.parallel.queue.timeout", + "7200", + "Seconds a NAS take-backup may wait for a free per-host slot before it fails.", + true, ConfigKey.Scope.Zone); + + /** In-flight take-backup commands per host, so backups never occupy every agent worker thread. */ + private final ConcurrentHashMap hostBackupSlots = new ConcurrentHashMap<>(); + + private static final class HostBackupSlots { + private int inFlight; + } + private HostDao hostDao; @Inject @@ -154,12 +184,12 @@ public boolean takeBackup(final VirtualMachine vm) { final String backupPath = String.format("%s/%s", vm.getInstanceName(), new SimpleDateFormat("yyyy.MM.dd.HH.mm.ss").format(creationDate)); - BackupVO backupVO = createBackupObject(vm, backupPath); TakeBackupCommand command = new TakeBackupCommand(vm.getInstanceName(), backupPath); command.setBackupRepoType(backupRepository.getType()); command.setBackupRepoAddress(backupRepository.getAddress()); command.setMountOptions(backupRepository.getMountOptions()); - command.setExecuteInSequence(!BackupManager.NASBackupParallelExecution.valueIn(vm.getDataCenterId())); + final long zoneId = vm.getDataCenterId(); + final boolean parallel = applyExecutionPolicy(command, zoneId); if (VirtualMachine.State.Stopped.equals(vm.getState())) { List vmVolumes = volumeDao.findByInstance(vm.getId()); @@ -168,13 +198,26 @@ public boolean takeBackup(final VirtualMachine vm) { command.setVolumePaths(volumePaths); } + // Concurrent backups are bounded per host: wait here, on the management server, for one of the + // host's slots so the agent's worker threads are never all taken by long-running backups. + boolean slotHeld = false; + if (parallel) { + acquireHostBackupSlot(host.getId(), NASBackupParallelMaxPerHost.valueIn(zoneId), NASBackupParallelQueueTimeout.valueIn(zoneId)); + slotHeld = true; + } + BackupVO backupVO = null; BackupAnswer answer = null; try { + backupVO = createBackupObject(vm, backupPath); answer = (BackupAnswer) agentManager.send(host.getId(), command); } catch (AgentUnavailableException e) { throw new CloudRuntimeException("Unable to contact backend control plane to initiate backup"); } catch (OperationTimedoutException e) { throw new CloudRuntimeException("Operation to initiate backup timed out, please try again"); + } finally { + if (slotHeld) { + releaseHostBackupSlot(host.getId()); + } } if (answer != null && answer.getResult()) { @@ -190,6 +233,70 @@ public boolean takeBackup(final VirtualMachine vm) { return Objects.nonNull(answer) && answer.getResult(); } + /** + * Applies the zone's execution policy to the command and returns true when the command will run + * concurrently with other commands on the host (and so must be gated per host). + */ + protected boolean applyExecutionPolicy(final TakeBackupCommand command, final long zoneId) { + final boolean parallel = Boolean.TRUE.equals(NASBackupParallelExecution.valueIn(zoneId)); + command.setExecuteInSequence(!parallel); + return parallel; + } + + /** + * Waits for one of the host's backup slots. The caller holds it for the whole agent round-trip and + * releases it in a finally block. The wait happens on the management server (the async job thread), + * never on the agent's worker threads. Fails with CloudRuntimeException once the timeout is reached. + */ + protected void acquireHostBackupSlot(final long hostId, final Integer maxPerHost, final Integer timeoutSeconds) { + final int max = Math.max(1, maxPerHost == null ? 1 : maxPerHost); + final int timeout = Math.max(0, timeoutSeconds == null ? 0 : timeoutSeconds); + final HostBackupSlots slots = hostBackupSlots.computeIfAbsent(hostId, id -> new HostBackupSlots()); + final long deadline = System.currentTimeMillis() + timeout * 1000L; + synchronized (slots) { + while (slots.inFlight >= max) { + final long remaining = deadline - System.currentTimeMillis(); + if (remaining <= 0) { + throw new CloudRuntimeException(String.format( + "Timed out after %d seconds waiting for a NAS backup slot on host %d (%d of %d in flight); " + + "raise %s or %s, or retry when the host's backups finish", + timeout, hostId, slots.inFlight, max, NASBackupParallelMaxPerHost.key(), NASBackupParallelQueueTimeout.key())); + } + try { + slots.wait(remaining); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + throw new CloudRuntimeException("Interrupted while waiting for a NAS backup slot on host " + hostId); + } + } + slots.inFlight++; + LOG.debug("Host {} now has {} of {} NAS backups in flight", hostId, slots.inFlight, max); + } + } + + protected void releaseHostBackupSlot(final long hostId) { + final HostBackupSlots slots = hostBackupSlots.get(hostId); + if (slots == null) { + return; + } + synchronized (slots) { + if (slots.inFlight > 0) { + slots.inFlight--; + } + slots.notifyAll(); + } + } + + protected int getInFlightBackups(final long hostId) { + final HostBackupSlots slots = hostBackupSlots.get(hostId); + if (slots == null) { + return 0; + } + synchronized (slots) { + return slots.inFlight; + } + } + private BackupVO createBackupObject(VirtualMachine vm, String backupPath) { BackupVO backup = new BackupVO(); backup.setVmId(vm.getId()); @@ -451,6 +558,9 @@ public boolean isValidProviderOffering(Long zoneId, String uuid) { @Override public ConfigKey[] getConfigKeys() { return new ConfigKey[]{ + NASBackupParallelExecution, + NASBackupParallelMaxPerHost, + NASBackupParallelQueueTimeout }; } 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 new file mode 100644 index 000000000000..f70e5ee642b9 --- /dev/null +++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java @@ -0,0 +1,114 @@ +// 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 org.apache.cloudstack.backup; + +import java.lang.reflect.Field; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicBoolean; + +import org.apache.cloudstack.framework.config.ConfigKey; +import org.junit.Assert; +import org.junit.Test; + +import com.cloud.utils.exception.CloudRuntimeException; + +public class NASBackupProviderTest { + + private static void overrideDefaultConfigValue(final ConfigKey key, final String value) throws Exception { + final Field field = ConfigKey.class.getDeclaredField("_defaultValue"); + field.setAccessible(true); + field.set(key, value); + } + + @Test + public void executionPolicyReachesTheCommand() throws Exception { + final NASBackupProvider provider = new NASBackupProvider(); + final TakeBackupCommand command = new TakeBackupCommand("vm-1", "/backups/vm-1"); + + overrideDefaultConfigValue(NASBackupProvider.NASBackupParallelExecution, "false"); + Assert.assertFalse(provider.applyExecutionPolicy(command, 1L)); + Assert.assertTrue("disabled setting must make the command sequential", command.executeInSequence()); + + overrideDefaultConfigValue(NASBackupProvider.NASBackupParallelExecution, "true"); + Assert.assertTrue(provider.applyExecutionPolicy(command, 1L)); + Assert.assertFalse("enabled setting must let the command run concurrently", command.executeInSequence()); + } + + @Test + public void perHostCapHoldsAndReleases() { + final NASBackupProvider provider = new NASBackupProvider(); + provider.acquireHostBackupSlot(7L, 2, 1); + provider.acquireHostBackupSlot(7L, 2, 1); + Assert.assertEquals(2, provider.getInFlightBackups(7L)); + try { + provider.acquireHostBackupSlot(7L, 2, 1); + Assert.fail("third backup on a host capped at 2 must not get a slot"); + } catch (CloudRuntimeException expected) { + Assert.assertTrue(expected.getMessage().contains("host 7")); + } + Assert.assertEquals(2, provider.getInFlightBackups(7L)); + provider.releaseHostBackupSlot(7L); + provider.acquireHostBackupSlot(7L, 2, 1); + Assert.assertEquals(2, provider.getInFlightBackups(7L)); + provider.releaseHostBackupSlot(7L); + provider.releaseHostBackupSlot(7L); + Assert.assertEquals(0, provider.getInFlightBackups(7L)); + } + + @Test + public void hostsAreIndependent() { + final NASBackupProvider provider = new NASBackupProvider(); + provider.acquireHostBackupSlot(1L, 1, 1); + provider.acquireHostBackupSlot(2L, 1, 1); + Assert.assertEquals(1, provider.getInFlightBackups(1L)); + Assert.assertEquals(1, provider.getInFlightBackups(2L)); + provider.releaseHostBackupSlot(1L); + provider.releaseHostBackupSlot(2L); + } + + @Test + public void waiterProceedsWhenASlotFrees() throws Exception { + final NASBackupProvider provider = new NASBackupProvider(); + provider.acquireHostBackupSlot(9L, 1, 10); + final CountDownLatch acquired = new CountDownLatch(1); + final AtomicBoolean failed = new AtomicBoolean(false); + final Thread waiter = new Thread(() -> { + try { + provider.acquireHostBackupSlot(9L, 1, 10); + acquired.countDown(); + } catch (CloudRuntimeException e) { + failed.set(true); + } + }); + waiter.start(); + Assert.assertFalse("waiter must block while the only slot is held", acquired.await(300, TimeUnit.MILLISECONDS)); + provider.releaseHostBackupSlot(9L); + Assert.assertTrue("waiter must get the slot once it is released", acquired.await(5, TimeUnit.SECONDS)); + Assert.assertFalse(failed.get()); + Assert.assertEquals(1, provider.getInFlightBackups(9L)); + provider.releaseHostBackupSlot(9L); + waiter.join(1000); + } + + @Test + public void releaseOnUnknownHostIsHarmless() { + final NASBackupProvider provider = new NASBackupProvider(); + provider.releaseHostBackupSlot(404L); + Assert.assertEquals(0, provider.getInFlightBackups(404L)); + } +} 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 2c6cc10b8fd2..b3b22e98e45b 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -1023,8 +1023,7 @@ public ConfigKey[] getConfigKeys() { BackupFrameworkEnabled, BackupProviderPlugin, BackupSyncPollingInterval, - BackupEnableAttachDetachVolumes, - NASBackupParallelExecution + BackupEnableAttachDetachVolumes }; }