From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 737271FF0AF for ; Thu, 08 Oct 2026 11:37:19 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3A2E2213B9; Thu, 08 Oct 2026 11:37:19 +0200 (CEST) Message-ID: <1f945e56-59b8-4cef-ac31-389bf28d3c2d@proxmox.com> Date: Thu, 8 Oct 2026 11:37:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 3/6] fix #7904: api: implement "enable" for {sync,verify,tape,GC} jobs To: Jonas Theisen , pbs-devel@lists.proxmox.com References: <20261007134500.323872-1-j.theisen@proxmox.com> <20261007134500.323872-4-j.theisen@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20261007134500.323872-4-j.theisen@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1791452233407 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.570 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: AGSVNGTM5U2253SQ6VZRPIDWSKEUEOAF X-Message-ID-Hash: AGSVNGTM5U2253SQ6VZRPIDWSKEUEOAF X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On 10/7/26 3:45 PM, Jonas Theisen wrote: > To allow users to disable jobs without having to remove the > schedule this patch introduces the "enable" parameter. The commit message could be extended to also mention that it should be possible to configure a job which is intended for manual execution only by setting it disabled without requiring a schedule. > The value is inserted into the job config and honored by > the scheduler by skipping disabled jobs altogether. > For GC jobs the value is inserted into the datastore config. > > If the value is not present in a job config it is assumed the job > is to be enabled for backwards compatibility. This patch should however be extended to also check that a job has a schedule set when enabling it to reduce risk of misconfiguration, as suggested by Fabian in the referenced issue. Currently this is only done in the UI, not covering CLI/API. This must be checked in the API endpoint for the config creation and update of the jobs, covering both cases when config values are set and also both cases when values might be dropped via delete parameters. > Fixes: https://bugzilla.proxmox.com/show_bug.cgi?id=7904 > > Signed-off-by: Jonas Theisen nit: Trailers should not be separated by empty lines, just drop the empty line between Fixes and Signed-off-by trailer. > --- > src/api2/admin/datastore.rs | 1 + > src/api2/admin/gc.rs | 13 ++++++++++++- > src/api2/admin/sync.rs | 8 +++++++- > src/api2/admin/verify.rs | 8 +++++++- > src/api2/config/datastore.rs | 4 ++++ > src/api2/config/sync.rs | 4 ++++ > src/api2/config/verify.rs | 3 +++ > src/api2/tape/backup.rs | 7 ++++++- > src/bin/proxmox-backup-proxy.rs | 20 ++++++++++++++++++++ > 9 files changed, 64 insertions(+), 4 deletions(-) > > diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs > index 3367fb2c1..c4169b9c0 100644 > --- a/src/api2/admin/datastore.rs > +++ b/src/api2/admin/datastore.rs > @@ -1264,6 +1264,7 @@ pub(crate) fn garbage_collection_status_unchecked( > let mut info = GarbageCollectionJobStatus { > store: store.clone(), > schedule: store_config.gc_schedule, > + enable: store_config.gc_enable, > ..Default::default() > }; > > diff --git a/src/api2/admin/gc.rs b/src/api2/admin/gc.rs > index 3c8bc2946..21a03ea86 100644 > --- a/src/api2/admin/gc.rs > +++ b/src/api2/admin/gc.rs > @@ -58,7 +58,18 @@ pub fn list_all_gc_jobs( > .collect::>(), > }; > > - Ok(gc_info) > + let mut list = Vec::new(); > + > + for mut job in gc_info { > + // default set to `true` to be backwards compatible > + if !job.enable.unwrap_or(true) { > + job.next_run = Some(0); > + } > + > + list.push(job); > + } > + > + Ok(list) > } > > const GC_ROUTER: Router = Router::new().get(&API_METHOD_LIST_ALL_GC_JOBS); > diff --git a/src/api2/admin/sync.rs b/src/api2/admin/sync.rs > index 1799f51c0..2ecc2ab24 100644 > --- a/src/api2/admin/sync.rs > +++ b/src/api2/admin/sync.rs > @@ -112,7 +112,13 @@ pub fn list_config_sync_jobs( > continue; > } > > - let status = compute_schedule_status("syncjob", &job.id, job.schedule.as_deref())?; > + let mut status = > + compute_schedule_status("syncjob", &job.id, job.schedule.as_deref())?; > + > + // default set to `true` to be backwards compatible > + if !job.enable.unwrap_or(true) { > + status.next_run = Some(0); > + } > > list.push(SyncJobStatus { > config: job, > diff --git a/src/api2/admin/verify.rs b/src/api2/admin/verify.rs > index 80659ebe2..eab3347e2 100644 > --- a/src/api2/admin/verify.rs > +++ b/src/api2/admin/verify.rs > @@ -73,7 +73,13 @@ pub fn list_verification_jobs( > let mut list = Vec::new(); > > for job in job_config_iter { > - let status = compute_schedule_status("verificationjob", &job.id, job.schedule.as_deref())?; > + let mut status = > + compute_schedule_status("verificationjob", &job.id, job.schedule.as_deref())?; > + > + // default set to `true` to be backwards compatible > + if !job.enable.unwrap_or(true) { > + status.next_run = Some(0); > + } > > list.push(VerificationJobStatus { > config: job, > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index e7028480c..3c372b7f6 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -571,6 +571,10 @@ pub fn update_datastore( > data.gc_on_unmount = update.gc_on_unmount; > } > > + if update.gc_enable.is_some() { > + data.gc_enable = update.gc_enable; > + } > + > macro_rules! prune_disabled { > ($(($param:literal, $($member:tt)+)),+) => { > $( > diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs > index beed6e7e3..dabd4a3ef 100644 > --- a/src/api2/config/sync.rs > +++ b/src/api2/config/sync.rs > @@ -702,6 +702,9 @@ pub fn update_sync_job( > if update.schedule.is_some() { > data.schedule = update.schedule; > } > + if let Some(enable) = update.enable { > + data.enable = Some(enable); > + } > if update.remove_vanished.is_some() { > data.remove_vanished = update.remove_vanished; > } > @@ -847,6 +850,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator > max_depth: None, > group_filter: None, > schedule: None, > + enable: None, > limit: pbs_api_types::RateLimitConfig::default(), // no limit > transfer_last: None, > encrypted_only: None, > diff --git a/src/api2/config/verify.rs b/src/api2/config/verify.rs > index 35be9b175..a0c23841e 100644 > --- a/src/api2/config/verify.rs > +++ b/src/api2/config/verify.rs > @@ -262,6 +262,9 @@ pub fn update_verification_job( > if update.schedule.is_some() { > data.schedule = update.schedule; > } > + if let Some(enable) = update.enable { > + data.enable = Some(enable); > + } > if let Some(ns) = update.ns { > if !ns.is_root() { > data.ns = Some(ns); > diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs > index c0a4ca65b..2be8c3fe3 100644 > --- a/src/api2/tape/backup.rs > +++ b/src/api2/tape/backup.rs > @@ -83,7 +83,12 @@ pub fn list_tape_backup_jobs( > > let status = compute_schedule_status("tape-backup-job", &job.id, job.schedule.as_deref())?; > > - let next_run = status.next_run.unwrap_or(current_time); > + // default set to `true` to be backwards compatible > + let next_run = if job.enable.unwrap_or(true) { > + status.next_run.unwrap_or(current_time) > + } else { > + 0 > + }; > > let mut next_media_label = None; > > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index 22abfdbdb..a6426ccbe 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -537,6 +537,11 @@ async fn schedule_datastore_garbage_collection() { > } > }; > > + let gc_enabled = store_config.gc_enable.unwrap_or(true); > + if !gc_enabled { > + continue; > + } > + > let event_str = match store_config.gc_schedule { > Some(event_str) => event_str, > None => continue, > @@ -684,6 +689,11 @@ async fn schedule_datastore_sync_jobs() { > None => continue, > }; > > + let job_enabled = job_config.enable.unwrap_or(true); > + if !job_enabled { > + continue; > + } > + > let worker_type = "syncjob"; > if check_schedule(worker_type, &event_str, &job_id) { > let job = match Job::new(worker_type, &job_id) { > @@ -720,6 +730,11 @@ async fn schedule_datastore_verify_jobs() { > None => continue, > }; > > + let job_enabled = job_config.enable.unwrap_or(true); > + if !job_enabled { > + continue; > + } > + > let worker_type = "verificationjob"; > let auth_id = Authid::root_auth_id().clone(); > if check_schedule(worker_type, &event_str, &job_id) { > @@ -756,6 +771,11 @@ async fn schedule_tape_backup_jobs() { > None => continue, > }; > > + let job_enabled = job_config.enable.unwrap_or(true); > + if !job_enabled { > + continue; > + } > + > let worker_type = "tape-backup-job"; > let auth_id = Authid::root_auth_id().clone(); > if check_schedule(worker_type, &event_str, &job_id) {