jmsperu commented on PR #12847: URL: https://github.com/apache/cloudstack/pull/12847#issuecomment-5600895779
@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. -- 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]
