Repository navigation
Conversation
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 <[email protected]>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12847 +/- ##
============================================
+ Coverage 16.24% 16.43% +0.19%
- Complexity 13411 13681 +270
============================================
Files 5664 5669 +5
Lines 500463 501822 +1359
Branches 60779 60962 +183
============================================
+ Hits 81308 82496 +1188
- Misses 410059 410079 +20
- Partials 9096 9247 +151
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
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()returnfalse. - Make
DeleteBackupCommand.executeInSequence()returnfalse.
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.
|
@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. |
|
@jmsperu could you have a look at @abh1sar's comment at #12847 (comment)? |
|
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. |
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.
|
@blueorangutan package |
|
@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. |
|
Notes on the current red checks — all three appear unrelated to this change (which is a single
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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17997 |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-16181)
|
Per Daan's review on PR apache#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).
|
Does it make sense to add new global settings parameter so be able to edit this? |
|
title and description still say the delete command is included. that was reverted, might be worth updating. |
|
All of @Damans227 comments are valid. Most important is takeBackup using up all of the 5 worker threads present on each kvm host. |
|
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 |
… 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 <[email protected]>
jmsperu
changed the title
|
@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 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 Open question for the maintainers: the default stays 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. |
|
title and description read right now, thanks |
…hat the per-host cap is per management server The new ConfigKey constants were inserted between hostDao's @Inject and the field, so the annotation landed on a static ConfigKey (ignored by Spring) and hostDao was left null. Move the keys above the injected fields and add a test that fails if any @Inject sits on a static field or a Dao/Manager field is not injected. backup.nas.parallel.max.per.host is enforced in memory on each management server; say so in the description and give the sizing rule.
|
Pushed 3340496 for @Damans227's two questions (hostDao injection fixed with a regression test; the per-host cap is per management server, now documented with the sizing rule). NASBackupProviderTest 6/6 locally, checkstyle clean. On the red Simulator CI shard from the previous run: the failure is |
|
@blueorangutan package |
|
@DaanHoogland 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. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19440 |
|
tested 3340496 on a 2 host kvm lab with nas backup. ran 4 backups at once on one host, twice. all 8 ended BackedUp, only 2 ran on the host at a time and the rest waited their turn. a stop and a start of another vm on that host went through while the backups ran. no errors in the management or agent logs. |
|
@blueorangutan test |
DaanHoogland
added
status:waiting-for-smoketest-results
and removed
needs-testing
status:ready-for-testing
labels
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17090)
|
| true, ConfigKey.Scope.Zone); | ||
|
|
||
| /** In-flight take-backup commands per host, so backups never occupy every agent worker thread. */ | ||
| private final ConcurrentHashMap<Long, HostBackupSlots> hostBackupSlots = new ConcurrentHashMap<>(); |
There was a problem hiding this comment.
What if the requests are coming from multiple management servers and each is maintaining a copy of the hostBackupSlots? Will it cause over allocation?
There was a problem hiding this comment.
@abh1sar thank you, done in 1ad9593. The per-host cap no longer lives in memory: the backup's own BackingUp row is now the reservation of a host slot. Under a DB backed GlobalLock per host (nas-backup-host-<hostId>), held only for the count-and-insert step, the provider counts the NAS backups in flight on that host from the backups table and creates the row only if the count is below backup.nas.parallel.max.per.host. Every management server reads the same rows under the same lock, so the limit now holds across all of them.
A few details:
- A backup is attributed to the host its VM is on (or was last on when stopped), the same rule
getVMHypervisorHostuses to pick the host. - The slot is released when the row leaves
BackingUp. A failed, timed out or unreachable backup is now marked Failed and removed in afinallyblock instead of stayingBackingUp. BackingUprows older than the TakeBackupCommand timeout plus 5 minutes are not counted, so a management server that dies mid-backup cannot hold a slot forever.- A full host is retried with back-off until
backup.nas.parallel.queue.timeout; waiters on the same management server wake as soon as one of its backups finishes.
No schema change (BackupDao gains listByZoneAndStatus), the settings and defaults are unchanged, and with one management server it behaves as before. NASBackupProviderTest now runs two providers against one shared mocked database and checks the combined in-flight count never goes above the cap (10/10 green, checkstyle clean). Ready for another look.
@Damans227 your lab test was on 3340496, before this change, so a quick re-run on 1ad9593 would be very welcome.
…er wide
The per-host cap was an in-memory counter on each management server, so with
several management servers a host could run the cap times the number of
management servers backups at once (raised by abh1sar and Damans227).
The backup's own BackingUp row is now the reservation of a host slot:
- under a DB backed GlobalLock per host ("nas-backup-host-<id>"), held only
for the count-and-insert step, the provider counts the NAS backups in flight
on that host from the database and creates the BackingUp row only if the
count is below backup.nas.parallel.max.per.host;
- a backup belongs to the host its VM is on (or was last on when stopped),
the same rule getVMHypervisorHost uses to pick the host;
- the slot is released when the row leaves BackingUp; a failed, timed out or
unreachable backup is now marked Failed and removed in a finally block
instead of being left BackingUp;
- BackingUp rows older than the TakeBackupCommand timeout (commands.timeout
or wait) plus 5 minutes are not counted, so a management server that stops
mid-backup cannot hold a slot forever;
- a full host is retried with back-off (1 s up to 15 s) until
backup.nas.parallel.queue.timeout, and local waiters are woken as soon as a
backup on the same management server finishes.
No schema change: BackupDao gains listByZoneAndStatus. Settings and their
defaults are unchanged; with one management server the behaviour is the same
as before. NASBackupProviderTest now runs two providers against one shared,
mocked database and checks the combined in-flight count never exceeds the cap.
Claude-Session: https://claude.ai/code/session_01BhiE1Abhs315m3n3UPy6V7
| backupDao.remove(backupVO.getId()); | ||
| } finally { | ||
| if (!succeeded) { | ||
| // Releases the slot: a failed or interrupted backup must never stay BackingUp. |
There was a problem hiding this comment.
what happens when a backup runs past the 30 min wait? the host keeps going but the record is removed here, so the slot frees up early and the backup goes missing from the list
|
@blueorangutan package |
|
@Damans227 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. |
|
ill test this once the long backup question is settled. ill run it with two management servers sending backups to the same host. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19524 |
Summary
TakeBackupCommandno 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.backup.nas.parallel.max.per.host(default 2) backups are in flight on one host; further backups wait, on the async job thread, up tobackup.nas.parallel.queue.timeout(default 7200 s).backup.nas.parallel.execution.enabled(zone, default true) is the switch back to strictly sequential behaviour.RestoreBackupCommandandPrepareForBackupRestorationCommandstay sequential (they change VM state).DeleteBackupCommandis unchanged (an earlier revision touched it; that was reverted).Problem
TakeBackupCommand.executeInSequence()wastrue, 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.
NASBackupProviderkeeps 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 hasmax.per.hostbackups 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 byqueue.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 intakeBackup().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.🤖 Generated with Claude Code