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 2D1D41FF0BC for ; Thu, 15 Oct 2026 13:14:50 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id A6CE921400; Thu, 15 Oct 2026 13:14:48 +0200 (CEST) Message-ID: Date: Thu, 15 Oct 2026 13:14:44 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Christian Ebner Subject: Re: [PATCH proxmox-backup v3 04/10] api: prune job: align prune job schedule to other jobs To: Jonas Theisen , pbs-devel@lists.proxmox.com References: <20261009145445.282494-1-j.theisen@proxmox.com> <20261009145445.282494-2-j.theisen@proxmox.com> Content-Language: en-US, de-DE In-Reply-To: <20261009145445.282494-2-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: 1792062884652 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.509 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: FQRO2WWVUDXZLJGT4LN7FCR5L6B7SCD2 X-Message-ID-Hash: FQRO2WWVUDXZLJGT4LN7FCR5L6B7SCD2 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: one comment and one nit inline. On 10/9/26 4:55 PM, Jonas Theisen wrote: > To allow creation of prune jobs without a schedule the parameter > needs to be optional and be able to handle no content. > > This is already the case for the schedule parameters of the other > job types, so this patch aligns the prune job schedules by using > an Option instead of a plain String. > > This patch also integrates the API check for disable = true if no > schedule is provided. > > Signed-off-by: Jonas Theisen > --- > src/api2/admin/prune.rs | 2 +- > src/api2/config/datastore.rs | 2 +- > src/api2/config/prune.rs | 16 +++++++++++----- > src/bin/proxmox-backup-proxy.rs | 9 +++++++-- > src/bin/proxmox_backup_manager/migrate_config.rs | 2 +- > 5 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/src/api2/admin/prune.rs b/src/api2/admin/prune.rs > index 0846d8893..784b0ac6f 100644 > --- a/src/api2/admin/prune.rs > +++ b/src/api2/admin/prune.rs > @@ -73,7 +73,7 @@ pub fn list_prune_jobs( > let mut list = Vec::new(); > > for job in job_config_iter { > - let mut status = compute_schedule_status("prunejob", &job.id, Some(&job.schedule))?; > + let mut status = compute_schedule_status("prunejob", &job.id, job.schedule.as_deref())?; > if job.disable { > status.next_run = None; > } comment: mentioning it also here, but more relevant for the next patch: The common pattern here (with the next patch applied) is to first call compute_schedule_status() with the optional schedule, but then to check if the job is disabled and clear the next_run (except for tape, there we do set it to 0, but that can be handled accordingly). I would therefore suggest to pass the disable flag as additional parameter to compute_schedule_status() and directly set the next_run to None there. This not only deduplicates these checks but also protects from forgetting to clear it and should help to clean up the next patch as well. Refactoring this should be its own patch. > diff --git a/src/api2/config/datastore.rs b/src/api2/config/datastore.rs > index e7028480c..a8b003601 100644 > --- a/src/api2/config/datastore.rs > +++ b/src/api2/config/datastore.rs > @@ -321,7 +321,7 @@ pub fn create_datastore( > store: config.name.clone(), > comment: None, > disable: false, > - schedule: schedule.clone(), > + schedule: Some(schedule.clone()), > options: PruneJobOptions { > keep: config.keep.clone(), > max_depth: None, > diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs > index 91d2324f9..3186fad64 100644 > --- a/src/api2/config/prune.rs > +++ b/src/api2/config/prune.rs > @@ -1,4 +1,4 @@ > -use anyhow::Error; > +use anyhow::{Error, format_err}; > use serde::{Deserialize, Serialize}; > use serde_json::Value; > > @@ -112,6 +112,13 @@ pub fn create_prune_job( > > user_info.check_privs(&auth_id, &config.acl_path(), PRIV_DATASTORE_MODIFY, true)?; > > + if !config.disable && config.schedule.is_none() { > + param_bail!( > + "schedule", > + format_err!("no schedule provided but job also not disabled") nit: would rephrase to 'job enabled but no schedule provided', which is IMHO a bit easier to grasp. > + ); > + } > + > do_create_prune_job(config) > } > > @@ -284,10 +291,9 @@ pub fn update_prune_job( > user_info.check_privs(&auth_id, &data.acl_path(), PRIV_DATASTORE_MODIFY, true)?; > } > > - let mut schedule_changed = false; > - if let Some(schedule) = update.schedule { > - schedule_changed = data.schedule != schedule; > - data.schedule = schedule; > + let schedule_changed = data.schedule != update.schedule; > + if update.schedule.is_some() { > + data.schedule = update.schedule; > } > > if let Some(max_depth) = update.options.max_depth { > diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs > index 22abfdbdb..c66cffeb4 100644 > --- a/src/bin/proxmox-backup-proxy.rs > +++ b/src/bin/proxmox-backup-proxy.rs > @@ -641,9 +641,14 @@ async fn schedule_datastore_prune_jobs() { > continue; > } > > + let event_str = match job_config.schedule { > + Some(event_str) => event_str, > + None => continue, > + }; > + > let worker_type = "prunejob"; > let auth_id = Authid::root_auth_id().clone(); > - if check_schedule(worker_type, &job_config.schedule, &job_id) { > + if check_schedule(worker_type, &event_str, &job_id) { > let job = match Job::new(worker_type, &job_id) { > Ok(job) => job, > Err(_) => continue, // could not get lock > @@ -653,7 +658,7 @@ async fn schedule_datastore_prune_jobs() { > job_config.options, > job_config.store, > &auth_id, > - Some(job_config.schedule), > + Some(event_str), > ) { > eprintln!("unable to start datastore prune job {job_id} - {err}"); > } > diff --git a/src/bin/proxmox_backup_manager/migrate_config.rs b/src/bin/proxmox_backup_manager/migrate_config.rs > index 69f908dc5..b48d53104 100644 > --- a/src/bin/proxmox_backup_manager/migrate_config.rs > +++ b/src/bin/proxmox_backup_manager/migrate_config.rs > @@ -84,7 +84,7 @@ pub(crate) fn update_to_prune_jobs_config() -> Result<(), Error> { > store: store.clone(), > disable: false, > comment: None, > - schedule, > + schedule: Some(schedule), > options, > }; >