NAS backup: run take-backup commands concurrently on the KVM host, bounded per host

[jmsperu]

Summary

• TakeBackupCommand no longer executes in sequence on the KVM agent: NAS take-backup commands run concurrently with the host's other commands, the way start/stop already do.
• Concurrency is bounded per host on the management server: at most backup.nas.parallel.max.per.host (default 2) backups are in flight on one host; further backups wait, on the async job thread, up to backup.nas.parallel.queue.timeout (default 7200 s).
• backup.nas.parallel.execution.enabled (zone, default true) is the switch back to strictly sequential behaviour.
• RestoreBackupCommand and PrepareForBackupRestorationCommand stay sequential (they change VM state). DeleteBackupCommand is unchanged (an earlier revision touched it; that was reverted).

Problem

TakeBackupCommand.executeInSequence() was true, so a host ran one backup at a time and every backup queued behind the previous one. A single large VM (100+ GB, 2+ hours) delays every other backup on that host; on hosts with many VMs the schedule slips for the whole day and jobs time out.

Design

Execution model. The KVM agent runs commands on a small worker pool (workers, default 5). In-sequence commands run one at a time; everything else shares the pool. Moving take-backup out of the sequence queue fixes the queueing problem but, unbounded, would let long backups occupy every worker and stall start/stop/reboot/migrate for that host. So the change is only safe with a bound.

The bound. NASBackupProvider keeps an in-flight counter per host. Before it sends a take-backup it takes one of the host's slots and holds it for the whole agent round-trip; when the host already has max.per.host backups running, the async job thread waits (never the agent). The default of 2 leaves at least 3 workers free on a default agent. The wait is capped by queue.timeout; on timeout the backup fails with a clear message and no backup row is left behind.

Why concurrent backups are safe on the agent. Each backup mounts its own temporary directory, runs its own QEMU block jobs against its own VM's disks, and shares no state with other backups. Incremental backups (#13074) keep bitmaps and checkpoints per VM. What backups do share is NAS bandwidth, which is why the per-host cap doubles as the throttle knob.

Controls. All three settings are zone-scoped, dynamic, and owned by the NAS provider (getConfigKeys()), since they mean nothing to other providers.

Out of scope. A cluster-wide scheduler (the backup schedule already staggers VMs), and rate limiting on the NAS itself.

Changes

• core: TakeBackupCommand.executeInSequence() becomes a settable flag (default false).
• plugins/backup/nas: the three settings, applyExecutionPolicy(), the per-host gate (acquireHostBackupSlot / releaseHostBackupSlot), and the gate around the send in takeBackup().
• api/server: no NAS-specific settings any more.

Tests

• NASBackupProviderTest (new): the setting reaches the command; the per-host cap holds and releases; hosts are independent; a waiter proceeds once a slot frees; release on an unknown host is harmless.
• TakeBackupCommandTest: the command-level default and setter.
• Trillian on the previous revision: 149 OK, 0 errors.

🤖 Generated with Claude Code

[codecov]

Codecov Report

❌ Patch coverage is 74.60317% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.36%. Comparing base (61afb4c) to head (f0096a6).
⚠️ Report is 73 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...rg/apache/cloudstack/backup/NASBackupProvider.java	72.41%	13 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               4.20   #12847      +/-   ##
============================================
+ Coverage     16.24%   16.36%   +0.11%     
- Complexity    13411    13597     +186     
============================================
  Files          5664     5669       +5     
  Lines        500463   501602    +1139     
  Branches      60779    60922     +143     
============================================
+ Hits          81308    82086     +778     
- Misses       410059   410313     +254     
- Partials       9096     9203     +107     

Flag	Coverage Δ
uitests	4.14% <ø> (-0.02%)	⬇️
unittests	17.22% <74.60%> (+0.12%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

This PR changes CloudStack backup agent command scheduling by making the NAS backup take and delete commands eligible for parallel execution on the agent (instead of being forced through the single-threaded “in-sequence” queue).

Changes:

• Make TakeBackupCommand.executeInSequence() return false.
• Make DeleteBackupCommand.executeInSequence() return false.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
core/src/main/java/org/apache/cloudstack/backup/TakeBackupCommand.java	Marks backup creation command as non-sequential (parallelizable) on the agent.
core/src/main/java/org/apache/cloudstack/backup/DeleteBackupCommand.java	Marks backup deletion command as non-sequential (parallelizable) on the agent.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[winterhazel]

@JoaoJandre could you have a look at whether this change would affect KBOSS?

[JoaoJandre]

@JoaoJandre could you have a look at whether this change would affect KBOSS?

@winterhazel KBOSS uses its own commands to send messages to the agent, thus this does not affect it.
In any case, KBOSS commands have this flag as false as well.

[winterhazel]

@jmsperu could you have a look at @abh1sar's comment at #12847 (comment)?

[jmsperu]

Thanks for the ping @winterhazel — and @abh1sar's point at #12847 (comment) is well taken. DeleteBackup needs sequential execution because parallel deletes against the same NAS path can race on the cleanup of the chain (we've also seen the rm -rf vs mount/unmount interleave produce stuck mounts). Will restrict the parallelization to TakeBackup only in the next push — DeleteBackup keeps its current sequential behaviour. Thanks @JoaoJandre for the KBOSS confirmation too.

[jmsperu]

Pushed b2cd0bf — reverted the DeleteBackupCommand change so only TakeBackupCommand flips to executeInSequence() = false. Delete stays serialized as you suggested @abh1sar (incremental chains share parent qcow2 files on the NAS, concurrent sibling/descendant deletes can race on shared file refs).

[winterhazel]

@blueorangutan package

[blueorangutan]

@winterhazel a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress.

[jmsperu]

Notes on the current red checks — all three appear unrelated to this change (which is a single executeInSequence() boolean flip on TakeBackupCommand and touches no other code path):

• codecov/project — reports 4.15% (-12.10%) project-coverage delta. That number is noise: the patch surface is one line so the codecov math is degenerate. codecov/patch (the one that actually reflects this PR) passes.
• build (component/test_persistent_networks ...) — failure is test_vpc_force_delete throwing builtins.Exception plus test_admin_project_creation going EXCEPTION on one retry while the other retry passed. Known flake pattern; VPC teardown / project-creation tests are not on any code path reachable from a backup executeInSequence flip.
• build (component/test_lb_secondary_ip ...) — failure is test_03_delete_instance in test_memory_limits. Memory-limits resource-accounting tests are timing-sensitive on the simulator and have no overlap with backup orchestration.

Happy to rebase to retrigger if a reviewer prefers a fresh CI run, but the change itself is unaffected by these matrices. Other 14 checks green, 8 still running.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17997

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-16181)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 50661 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12847-t16181-kvm-ol8.zip
Smoke tests completed. 141 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File

[sbrueseke]

Does it make sense to add new global settings parameter so be able to edit this?

[jmsperu]

@sbrueseke good suggestion — added a global setting backup.nas.parallel.execution.enabled (Advanced, Zone-scoped, default true). It gates the take-backup command's parallelism, so an operator can force sequential execution per zone without a code change if they ever hit contention on a busy NAS. Default keeps the new parallel behaviour.

[DaanHoogland]

@blueorangutan package

[DaanHoogland]

@sbrueseke are you willing to test this?

[Damans227]

if a host has 20 VMs on the same backup schedule, do all 20 get sent at once now? is there anything that caps how many run together on one host?

[jmsperu]

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.

[Damans227]

the agent has a small pool of worker threads and everything the mgmt server sends goes through it. if long backups fill it up, does anything else sent to that host wait behind them?

[jmsperu]

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.

[Damans227]

should this default to false on 4.20? anyone upgrading gets the new behaviour without asking for it.

[jmsperu]

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.

[Damans227]

why here and not in NASBackupProvider? it is a nas only setting and the provider already has an empty getConfigKeys.

[jmsperu]

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.

[Damans227]

does this only affect other backup and delete commands? looks like it affects everything sent to that host, not just backups.

[jmsperu]

Correct, and the description now says so: the command runs concurrently with every other command on the host, which is what executeInSequence=false means, bounded per host by the new cap. Restore and prepare-for-restore stay sequential because they touch VM state.

[Damans227]

this tests the setter. worth testing that the setting actually reaches the command instead?

[jmsperu]

Added NASBackupProviderTest: it drives applyExecutionPolicy() with the setting at true and at false and asserts the command's executeInSequence() follows it, plus four tests on the per-host gate (the cap holds and releases, hosts are independent, a waiter proceeds when a slot frees, release on an unknown host is harmless). The setter test in core stays as the command-level check.

[Damans227]

title and description still say the delete command is included. that was reverted, might be worth updating.

[abh1sar]

All of @Damans227 comments are valid.

Most important is takeBackup using up all of the 5 worker threads present on each kvm host.
Operations like Start/Stop/Reboot/Migrate didn't use to wait for in-sequence commands like TakeBackup.
But now someone can start 5 long running TakeBackup commands on a host. Until they finish Start/Stop/Reboot/Migrate operations will be stuck.
We need a concurrency cap for TakeBackup commands per kvm host.
Maybe TakeBackup can take 2 or 3 slots per host, but using all 5 slots at a time is not a good idea.

[sbrueseke]

Thank you very much @Damans227. Should we go back to the drawing board? To me it looks like there are a lot of design questions, and right now it feels like patching holes in a carpet. Was there a design proposal in the first place? I would love to take a further look at it. Maybe we need to reconsider this feature to provide enterprise-level backup.

What are your thoughts? @abh1sar @DaanHoogland

[jmsperu]

@sbrueseke fair challenge, and the honest answer is that this PR started from a one-line observation (backups queue behind each other on a host) and grew review by review, which is exactly what "patching holes" looks like from outside. So here is the design in one place, now also in the PR description.

Problem. Take-backup ran in sequence, so a host did one backup at a time and a single 2-hour VM delayed every other backup on that host.

Change. Take-backup joins the agent's worker pool like start/stop already do. Unbounded, that is the problem @Damans227 and @abh1sar spotted: long backups could occupy every worker and stall start/stop/reboot/migrate on that host.

The bound (pushed in f0096a6). The provider holds a per-host slot for each backup for the whole agent round-trip. When a host already has backup.nas.parallel.max.per.host backups in flight (default 2, so 3 of the 5 default workers stay free), the next backup waits on the management server, in the async job thread, never on the agent, up to backup.nas.parallel.queue.timeout (default 2 h). On timeout it fails cleanly with no backup row left behind. backup.nas.parallel.execution.enabled per zone restores the sequential behaviour outright.

Why concurrent backups are safe. Each backup has its own temporary mount and its own QEMU block jobs on its own VM's disks; incremental state (#13074) is per VM. The shared resource is NAS bandwidth, which is what the per-host cap also throttles.

Housekeeping in the same push. The settings moved into NASBackupProvider (NAS-only), the enable flag's description now says what it really does (concurrent with every command on the host, bounded per host), the title and description drop the delete command (reverted earlier), and NASBackupProviderTest covers the setting reaching the command and the gate.

Open question for the maintainers: the default stays true because the cap makes it safe by construction, but if you prefer opt-in on 4.20 it is a one-character change.

If the group would rather have a fuller design discussion before merging, I am happy to write this up as a discussion thread and hold the PR. My preference is to ship the bounded version: it is a smaller change than the unbounded serialisation problem it fixes, and the cap is the enterprise-grade part.