Re: [PATCH proxmox-backup v2 3/6] fix #7904: api: implement "enable" for {sync,verify,tape,GC} jobs

[Christian Ebner <c.ebner@proxmox.com>]

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 <j.theisen@proxmox.com>

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::<Vec<_>>(),
>       };
>   
> -    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) {