jmsperu commented on code in PR #12847:
URL: https://github.com/apache/cloudstack/pull/12847#discussion_r4196707577
##########
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:
You were right, thank you for catching it. The three ConfigKey constants
went in between `@Inject` and `hostDao`, so the annotation ended up on a static
field (Spring ignores it) and `hostDao` stayed null; every backup would have
failed on the host lookup. Fixed in 3340496b0a: the keys now sit above the
injected fields and `hostDao` has its `@Inject` back. I also added
`NASBackupProviderTest.everyInjectedFieldIsAnInstanceFieldAndHostDaoIsInjected`,
which fails if `@Inject` is on any static field or if a Dao/Manager field is
not injected. I ran it against the previous commit to make sure it catches this
exact case (it fails with "@Inject on static field NASBackupParallelExecution
is ignored by Spring").
##########
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:
Yes, it is per management server. The count is kept in memory on each one,
so with two management servers a host can see up to 2 x the cap, which is 4 at
the default and leaves one of the agent's 5 workers free. Making it
cluster-wide would mean either a `host_id` on the `backups` table (a schema
change) or holding a DB lock for the whole backup, which ties up a dedicated
MySQL connection for hours per in-flight backup. I did not want either in this
PR.
So in 3340496b0a I have documented it instead. The setting's description now
says the cap applies per management server and gives the sizing rule: cap x
number of management servers must stay below the agent's `workers`
(agent.properties, default 5). With two management servers, a cap of 1 keeps 3
workers free; with one, the default of 2 does. If you or the other maintainers
would rather have a cluster-wide count, I am happy to do it as a follow-up.
--
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]
