jmsperu commented on code in PR #12847:
URL: https://github.com/apache/cloudstack/pull/12847#discussion_r3967701906
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -89,6 +90,13 @@ public void setVolumePaths(List<String> volumePaths) {
@Override
public boolean executeInSequence() {
Review Comment:
Not any more. f0096a6 adds a per-host gate in the provider: at most
`backup.nas.parallel.max.per.host` (default 2) take-backup commands are in
flight on a host, and the rest wait on the management server, in the async job
thread, bounded by `backup.nas.parallel.queue.timeout` (default 2 h). A wait
that runs out fails cleanly without leaving a BackingUp row behind. So 20 VMs
on one schedule on one host run two at a time.
##########
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java:
##########
@@ -89,6 +90,13 @@ public void setVolumePaths(List<String> volumePaths) {
@Override
public boolean executeInSequence() {
Review Comment:
That was the real gap, thank you. The cap is deliberately below the agent's
default of 5 workers, so at least 3 workers stay free for start, stop, reboot
and migrate at all times; an operator who raises the agent's `workers` can
raise the cap to match. The waiting never happens on the agent side.
##########
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<Boolean> NASBackupParallelExecution = new
ConfigKey<>("Advanced", Boolean.class,
+ "backup.nas.parallel.execution.enabled",
+ "true",
Review Comment:
With the per-host cap the default is safe by construction (3 of the 5
workers are always free), and a false default would leave the serialisation
problem in place for everyone who does not read the release notes, so I have
kept `true`. It is a one-character change and I will flip it if the maintainers
prefer opt-in on 4.20; @DaanHoogland @abh1sar your call.
##########
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<Boolean> NASBackupParallelExecution = new
ConfigKey<>("Advanced", Boolean.class,
Review Comment:
Agreed and moved. All three settings now live in `NASBackupProvider` and are
published through its `getConfigKeys()`; `BackupManager` and
`BackupManagerImpl` are back to their original shape.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
