[pbs-devel] [RFC proxmox-backup] PruneJobConfig: make "schedule" optional

[pbs-devel] [RFC proxmox-backup] PruneJobConfig: make "schedule" optional

[Dietmar Maurer]

The API macro already mark this property as optional:

  schedule: {
            schema: PRUNE_SCHEDULE_SCHEMA,
            optional: true,
  },

So the correct representation is Option<String> (not String).

This is now consistent with other job types.
---

Honestly, I have no idea if it was indented to make schedule non-optional?

Also, why have prune jobs a disable property, while other jobs types
do not have it?

Also, update_to_prune_jobs_config() looks wrong now because it remove jobs
without a schedule. Was this really indendet?


 pbs-api-types/src/jobs.rs               |  3 ++-
 src/api2/admin/prune.rs                 |  2 +-
 src/api2/config/prune.rs                | 12 ++++++++----
 src/bin/proxmox-backup-proxy.rs         |  9 +++++++--
 src/bin/proxmox_backup_manager/prune.rs |  2 +-
 5 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index f3e64ee4..1fb74381 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -712,7 +712,8 @@ pub struct PruneJobConfig {
     #[updater(serde(skip_serializing_if = "Option::is_none"))]
     pub disable: bool,
 
-    pub schedule: String,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub schedule: Option<String>,
 
     #[serde(skip_serializing_if = "Option::is_none")]
     pub comment: Option<String>,
diff --git a/src/api2/admin/prune.rs b/src/api2/admin/prune.rs
index a5ebf297..85dab54f 100644
--- a/src/api2/admin/prune.rs
+++ b/src/api2/admin/prune.rs
@@ -76,7 +76,7 @@ pub fn list_prune_jobs(
         let last_state = JobState::load("prunejob", &job.id)
             .map_err(|err| format_err!("could not open statefile for {}: {}", &job.id, err))?;
 
-        let mut status = compute_schedule_status(&last_state, Some(&job.schedule))?;
+        let mut status = compute_schedule_status(&last_state, job.schedule.as_deref())?;
         if job.disable {
             status.next_run = None;
         }
diff --git a/src/api2/config/prune.rs b/src/api2/config/prune.rs
index 6f391722..4679431d 100644
--- a/src/api2/config/prune.rs
+++ b/src/api2/config/prune.rs
@@ -157,6 +157,8 @@ pub enum DeletableProperty {
     KeepMonthly,
     /// Delete number of yearly backups to keep.
     KeepYearly,
+    /// Delete the job schedule.
+    Schedule,
 }
 
 #[api(
@@ -248,6 +250,9 @@ pub fn update_prune_job(
                 DeletableProperty::KeepYearly => {
                     data.options.keep.keep_yearly = None;
                 }
+                DeletableProperty::Schedule => {
+                    data.schedule = None;
+                }
             }
         }
     }
@@ -268,10 +273,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 f38a02bd..e7a124bc 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -556,9 +556,14 @@ async fn schedule_datastore_prune_jobs() {
             continue; // no 'keep' values set, keep all
         }
 
+        let event_str = match job_config.schedule {
+            Some(ref event_str) => event_str.clone(),
+            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
@@ -568,7 +573,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/prune.rs b/src/bin/proxmox_backup_manager/prune.rs
index 923eb6f5..7738f661 100644
--- a/src/bin/proxmox_backup_manager/prune.rs
+++ b/src/bin/proxmox_backup_manager/prune.rs
@@ -245,7 +245,7 @@ pub(crate) fn update_to_prune_jobs_config() -> Result<(), Error> {
             store: store.clone(),
             disable: false,
             comment: None,
-            schedule,
+            schedule: Some(schedule),
             options,
         };
 
-- 
2.39.2

Re: [pbs-devel] [RFC proxmox-backup] PruneJobConfig: make "schedule" optional

[Thomas Lamprecht]

Am 16/11/2023 um 14:19 schrieb Dietmar Maurer:
> The API macro already mark this property as optional:
> 
>   schedule: {
>             schema: PRUNE_SCHEDULE_SCHEMA,
>             optional: true,
>   },
> 
> So the correct representation is Option<String> (not String).
> 
> This is now consistent with other job types.
> ---
> 
> Honestly, I have no idea if it was indented to make schedule non-optional?

The whole mix of behavior is due to originally having only one prune
job per datastore, directly encoded in the datastore config.

There it could be disabled by setting the prune schedule to None, see
the screenshot from 1.x times:

https://pbs.proxmox.com/docs-1/_images/pbs-gui-datastore-prunegc.png

Prune jobs got added to allow users to make better use of namespaces,
as there one might want to have different retention per namespace, and
we tried to stay backward compatible with the existing job from
datastore.cfg, but also use our "modern" disable flag like we added
on various places (IIRC one of my first patches was to add that feature
to PVE backup jobs almost a decade ago...).

> Also, why have prune jobs a disable property, while other jobs types
> do not have it?

Because prune jobs tried to use our modern feature set, which includes
a enable/disable flag so users can easily switch a job of without removing
it as a whole, or delete the job's schedule, which can be a nuisance to
restore again, especially if it was a more complex one.

In other words, sync and verify jobs should gain the disable flag for
feature parity, and to provide a slightly more obvious way to (temporarily)
disable a job without loss of (schedule) information.
 
> Also, update_to_prune_jobs_config() looks wrong now because it remove jobs
> without a schedule. Was this really indendet?

Without looking to closely into it: That seems indeed rather odd and
is likely a bug. I did not check your change to closely (sorry, a bit
overloaded with PVE stuff currently) but did you fix that here too?

Re: [pbs-devel] [RFC proxmox-backup] PruneJobConfig: make "schedule" optional

[Dietmar Maurer]

> In other words, sync and verify jobs should gain the disable flag for
> feature parity, and to provide a slightly more obvious way to (temporarily)
> disable a job without loss of (schedule) information.
>  
> > Also, update_to_prune_jobs_config() looks wrong now because it remove jobs
> > without a schedule. Was this really indendet?
> 
> Without looking to closely into it: That seems indeed rather odd and
> is likely a bug. I did not check your change to closely (sorry, a bit
> overloaded with PVE stuff currently) but did you fix that here too?

Ok, I guess I will then just remove the "optional" flag from the API macro.