[pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[Stefan Hanreich]

This relies on my small formatting fix in the email
[PATCH proxmox-backup 1/1] Fix formatting in proxmox-backup-manager
otherwise it will have a merge conflict.

This patch adds the possibility of configuring prune options for sync jobs. This
runs a prune job after a successful sync job that prunes the backup groups that
got synced by the respective sync job (meaning this respects group-filter,
max-depth, target namespace and so on).

The prune options can also be specified when running a single pull command.

I also added the possibility of configuring this via the Web UI in the same way
as configuring a regular prune job.

The new options are documented in the Sync Job documentation as well.

This patch series introduces a new struct, PruneJob, that encapsulates the
pruning functionality in its own struct that can be conveniently used. This
greatly reduces the code duplication between the sites that used pruning
functionality.

Changes from v2:
- Refactored pull function by extracting remove_vanished and prune into
their own methods
- Added PruneJob struct that encapsulates PruneJob functionality
- Refactored existing call sites to utilize new PruneJob struct
- Use KeepOptions::default where applicable
- minor formatting and cargo clippy fixes

Stefan Hanreich (7):
  Add KeepOptions parameters to pull & sync-job
  refactor prune.rs - add PruneJob struct for handling prune jobs
  Add pruning parameters to the pull command
  use new PruneJob in prune command
  use new PruneJob struct in Prune Jobs implementation
  add KeepOptions to Web UI of Sync Jobs
  Add documentation for prune options in Sync Jobs

 docs/managing-remotes.rst         |  14 ++
 pbs-api-types/src/jobs.rs         |   7 +-
 pbs-datastore/src/prune.rs        | 279 ++++++++++++++++++------------
 src/api2/admin/datastore.rs       |  71 ++++----
 src/api2/config/sync.rs           |  52 ++++++
 src/api2/pull.rs                  |  17 +-
 src/bin/proxmox-backup-manager.rs |  17 +-
 src/server/prune_job.rs           |  29 +---
 src/server/pull.rs                | 173 ++++++++++++------
 tests/prune.rs                    |   4 +-
 www/window/SyncJobEdit.js         |   5 +
 11 files changed, 433 insertions(+), 235 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 1/7] Add KeepOptions parameters to pull & sync-job

[Stefan Hanreich]

Do nothing with this information for now, just prepare the API and
client to accept the parameters so we can handle them in a later commit.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 pbs-api-types/src/jobs.rs         |  7 ++++-
 src/api2/config/sync.rs           | 52 +++++++++++++++++++++++++++++++
 src/api2/pull.rs                  | 13 ++++++--
 src/bin/proxmox-backup-manager.rs | 17 ++++++++--
 src/server/pull.rs                |  8 +++--
 5 files changed, 89 insertions(+), 8 deletions(-)

diff --git a/pbs-api-types/src/jobs.rs b/pbs-api-types/src/jobs.rs
index 7f029af7..c778039c 100644
--- a/pbs-api-types/src/jobs.rs
+++ b/pbs-api-types/src/jobs.rs
@@ -474,6 +474,9 @@ pub const GROUP_FILTER_LIST_SCHEMA: Schema =
         limit: {
             type: RateLimitConfig,
         },
+        keep: {
+            type: KeepOptions,
+        },
         schedule: {
             optional: true,
             schema: SYNC_SCHEDULE_SCHEMA,
@@ -511,6 +514,8 @@ pub struct SyncJobConfig {
     pub group_filter: Option<Vec<GroupFilter>>,
     #[serde(flatten)]
     pub limit: RateLimitConfig,
+    #[serde(flatten)]
+    pub keep: KeepOptions,
 }
 
 impl SyncJobConfig {
@@ -572,7 +577,7 @@ pub struct SyncJobStatus {
         },
     }
 )]
-#[derive(Serialize, Deserialize, Default, Updater)]
+#[derive(Serialize, Deserialize, Default, Updater, Clone)]
 #[serde(rename_all = "kebab-case")]
 /// Common pruning options
 pub struct KeepOptions {
diff --git a/src/api2/config/sync.rs b/src/api2/config/sync.rs
index 6f39b239..cafbf102 100644
--- a/src/api2/config/sync.rs
+++ b/src/api2/config/sync.rs
@@ -216,6 +216,18 @@ pub enum DeletableProperty {
     remote_ns,
     /// Delete the max_depth property,
     max_depth,
+    /// Delete keep_last prune option.
+    keep_last,
+    /// Delete keep_hourly prune option.
+    keep_hourly,
+    /// Delete keep_daily prune option.
+    keep_daily,
+    /// Delete keep_weekly prune option.
+    keep_weekly,
+    /// Delete keep_monthly prune option.
+    keep_monthly,
+    /// Delete keep_yearly prune option.
+    keep_yearly,
 }
 
 #[api(
@@ -310,6 +322,24 @@ pub fn update_sync_job(
                 DeletableProperty::max_depth => {
                     data.max_depth = None;
                 }
+                DeletableProperty::keep_last => {
+                    data.keep.keep_last = None;
+                }
+                DeletableProperty::keep_hourly => {
+                    data.keep.keep_hourly = None;
+                }
+                DeletableProperty::keep_daily => {
+                    data.keep.keep_daily = None;
+                }
+                DeletableProperty::keep_weekly => {
+                    data.keep.keep_weekly = None;
+                }
+                DeletableProperty::keep_monthly => {
+                    data.keep.keep_monthly = None;
+                }
+                DeletableProperty::keep_yearly => {
+                    data.keep.keep_yearly = None;
+                }
             }
         }
     }
@@ -381,6 +411,25 @@ pub fn update_sync_job(
         }
     }
 
+    if update.keep.keep_last.is_some() {
+        data.keep.keep_last = update.keep.keep_last;
+    }
+    if update.keep.keep_hourly.is_some() {
+        data.keep.keep_hourly = update.keep.keep_hourly;
+    }
+    if update.keep.keep_daily.is_some() {
+        data.keep.keep_daily = update.keep.keep_daily;
+    }
+    if update.keep.keep_weekly.is_some() {
+        data.keep.keep_weekly = update.keep.keep_weekly;
+    }
+    if update.keep.keep_monthly.is_some() {
+        data.keep.keep_monthly = update.keep.keep_monthly;
+    }
+    if update.keep.keep_yearly.is_some() {
+        data.keep.keep_yearly = update.keep.keep_yearly;
+    }
+
     if !check_sync_job_modify_access(&user_info, &auth_id, &data) {
         bail!("permission check failed");
     }
@@ -463,6 +512,8 @@ pub const ROUTER: Router = Router::new()
 
 #[test]
 fn sync_job_access_test() -> Result<(), Error> {
+    use pbs_api_types::KeepOptions;
+
     let (user_cfg, _) = pbs_config::user::test_cfg_from_str(
         r###"
 user: noperm@pbs
@@ -508,6 +559,7 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
         group_filter: None,
         schedule: None,
         limit: pbs_api_types::RateLimitConfig::default(), // no limit
+        keep: KeepOptions::default(),
     };
 
     // should work without ACLs
diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 193f28fe..f3b31e05 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -9,8 +9,8 @@ use proxmox_schema::api;
 use proxmox_sys::task_log;
 
 use pbs_api_types::{
-    Authid, BackupNamespace, GroupFilter, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
-    GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
+    Authid, BackupNamespace, GroupFilter, KeepOptions, RateLimitConfig, SyncJobConfig,
+    DATASTORE_SCHEMA, GROUP_FILTER_LIST_SCHEMA, NS_MAX_DEPTH_REDUCED_SCHEMA, PRIV_DATASTORE_BACKUP,
     PRIV_DATASTORE_PRUNE, PRIV_REMOTE_READ, REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA,
 };
 use pbs_config::CachedUserInfo;
@@ -78,6 +78,7 @@ impl TryFrom<&SyncJobConfig> for PullParameters {
             sync_job.max_depth,
             sync_job.group_filter.clone(),
             sync_job.limit.clone(),
+            sync_job.keep.clone(),
         )
     }
 }
@@ -203,7 +204,11 @@ pub fn do_sync_job(
             limit: {
                 type: RateLimitConfig,
                 flatten: true,
-            }
+            },
+            "keep-options": {
+                type: KeepOptions,
+                flatten: true,
+            },
         },
     },
     access: {
@@ -227,6 +232,7 @@ async fn pull(
     max_depth: Option<usize>,
     group_filter: Option<Vec<GroupFilter>>,
     limit: RateLimitConfig,
+    keep_options: KeepOptions,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<String, Error> {
     let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
@@ -259,6 +265,7 @@ async fn pull(
         max_depth,
         group_filter,
         limit,
+        keep_options,
     )?;
     let client = pull_params.client().await?;
 
diff --git a/src/bin/proxmox-backup-manager.rs b/src/bin/proxmox-backup-manager.rs
index 06330c78..0d6680b2 100644
--- a/src/bin/proxmox-backup-manager.rs
+++ b/src/bin/proxmox-backup-manager.rs
@@ -11,7 +11,7 @@ use proxmox_sys::fs::CreateOptions;
 
 use pbs_api_types::percent_encoding::percent_encode_component;
 use pbs_api_types::{
-    BackupNamespace, GroupFilter, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
+    BackupNamespace, GroupFilter, KeepOptions, RateLimitConfig, SyncJobConfig, DATASTORE_SCHEMA,
     GROUP_FILTER_LIST_SCHEMA, IGNORE_VERIFIED_BACKUPS_SCHEMA, NS_MAX_DEPTH_SCHEMA,
     REMOTE_ID_SCHEMA, REMOVE_VANISHED_BACKUPS_SCHEMA, UPID_SCHEMA,
     VERIFICATION_OUTDATED_AFTER_SCHEMA,
@@ -268,6 +268,10 @@ fn task_mgmt_cli() -> CommandLineInterface {
                 type: RateLimitConfig,
                 flatten: true,
             },
+            "keep-options": {
+                type: KeepOptions,
+                flatten: true,
+            },
             "output-format": {
                 schema: OUTPUT_FORMAT,
                 optional: true,
@@ -287,6 +291,7 @@ async fn pull_datastore(
     max_depth: Option<usize>,
     group_filter: Option<Vec<GroupFilter>>,
     limit: RateLimitConfig,
+    keep_options: KeepOptions,
     param: Value,
 ) -> Result<Value, Error> {
     let output_format = get_output_format(&param);
@@ -324,7 +329,15 @@ async fn pull_datastore(
         .as_object_mut()
         .ok_or_else(|| format_err!("limit is not an Object"))?;
 
-    args.as_object_mut().unwrap().append(limit_map);
+    let mut keep_json = json!(keep_options);
+    let keep_map = keep_json
+        .as_object_mut()
+        .ok_or_else(|| format_err!("keep_options is not an Object"))?;
+
+    let args_map = args.as_object_mut().unwrap();
+
+    args_map.append(limit_map);
+    args_map.append(keep_map);
 
     let result = client.post("api2/json/pull", Some(args)).await?;
 
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 77caf327..634a0b70 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -16,8 +16,8 @@ use proxmox_router::HttpError;
 use proxmox_sys::task_log;
 
 use pbs_api_types::{
-    print_store_and_ns, Authid, BackupNamespace, GroupFilter, GroupListItem, NamespaceListItem,
-    Operation, RateLimitConfig, Remote, SnapshotListItem, MAX_NAMESPACE_DEPTH,
+    print_store_and_ns, Authid, BackupNamespace, GroupFilter, GroupListItem, KeepOptions,
+    NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, MAX_NAMESPACE_DEPTH,
     PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
 };
 
@@ -60,6 +60,8 @@ pub(crate) struct PullParameters {
     group_filter: Option<Vec<GroupFilter>>,
     /// Rate limits for all transfers from `remote`
     limit: RateLimitConfig,
+    /// Options for pruning after pulling
+    keep_options: KeepOptions,
 }
 
 impl PullParameters {
@@ -79,6 +81,7 @@ impl PullParameters {
         max_depth: Option<usize>,
         group_filter: Option<Vec<GroupFilter>>,
         limit: RateLimitConfig,
+        keep_options: KeepOptions,
     ) -> Result<Self, Error> {
         let store = DataStore::lookup_datastore(store, Some(Operation::Write))?;
 
@@ -110,6 +113,7 @@ impl PullParameters {
             max_depth,
             group_filter,
             limit,
+            keep_options,
         })
     }
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 2/7] refactor prune.rs - add PruneJob struct for handling prune jobs

[Stefan Hanreich]

This encapsulates the previously available methods into a struct that is
responsible for creating PruneJobs. This struct can be used to create a
new, configurable PruneJob. It can handle several configuration options
that needed to be handled by the call sites before. Moving logging and
dry-run into this struct reduces the need for manually writing code for
those two features. Additionally the log messages are unified across all
instances where pruning is used.

This commit only introduces the PruneJob struct, the callsites are only
slightly adjusted so they call the compute_prune_info static method now.
They will be refactored in future commits to use the newly exposed
functionality from PruneJob.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 pbs-datastore/src/prune.rs  | 279 ++++++++++++++++++++++--------------
 src/api2/admin/datastore.rs |   4 +-
 src/server/prune_job.rs     |   4 +-
 tests/prune.rs              |   4 +-
 4 files changed, 179 insertions(+), 112 deletions(-)

diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
index 96da5826..b840fef6 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -1,7 +1,9 @@
 use std::collections::{HashMap, HashSet};
 use std::path::PathBuf;
+use std::sync::Arc;
 
 use anyhow::Error;
+use proxmox_sys::{task_log, task_warn, WorkerTaskContext};
 
 use pbs_api_types::KeepOptions;
 
@@ -36,137 +38,202 @@ impl std::fmt::Display for PruneMark {
     }
 }
 
-fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>>(
-    mark: &mut HashMap<PathBuf, PruneMark>,
-    list: &[BackupInfo],
-    keep: usize,
-    select_id: F,
-) -> Result<(), Error> {
-    let mut include_hash = HashSet::new();
-
-    let mut already_included = HashSet::new();
-    for info in list {
-        let backup_id = info.backup_dir.relative_path();
-        if let Some(PruneMark::Keep) = mark.get(&backup_id) {
-            let sel_id: String = select_id(info)?;
-            already_included.insert(sel_id);
-        }
+pub struct PruneJob {
+    marks: Vec<(BackupInfo, PruneMark)>,
+    dry_run: bool,
+    worker: Option<Arc<dyn WorkerTaskContext>>,
+}
+
+impl PruneJob {
+    pub fn new(list: Vec<BackupInfo>, options: &KeepOptions) -> Result<Self, Error> {
+        let mut prune_info = PruneJob::compute_prune_info(list, options)?;
+        prune_info.reverse();
+
+        Ok(Self {
+            marks: prune_info,
+            dry_run: false,
+            worker: None,
+        })
     }
 
-    for info in list {
-        let backup_id = info.backup_dir.relative_path();
-        if mark.get(&backup_id).is_some() {
-            continue;
-        }
-        if info.protected {
-            mark.insert(backup_id, PruneMark::Protected);
-            continue;
-        }
-        let sel_id: String = select_id(info)?;
+    pub fn logging(&mut self, worker: Arc<dyn WorkerTaskContext>) -> &mut Self {
+        self.worker = Some(worker);
+        self
+    }
 
-        if already_included.contains(&sel_id) {
-            continue;
-        }
+    pub fn dry_run(&mut self, enabled: bool) -> &mut Self {
+        self.dry_run = enabled;
+        self
+    }
+
+    pub fn run(&self) {
+        self.run_with_callback(|_, _, _| ())
+    }
 
-        if !include_hash.contains(&sel_id) {
-            if include_hash.len() >= keep {
-                break;
+    pub fn run_with_callback<E>(&self, callback: E)
+    where
+        E: Fn(&BackupInfo, &PruneMark, Result<(), Error>),
+    {
+        for (info, mark) in &self.marks {
+            if let Some(worker) = &self.worker {
+                task_log!(
+                    worker,
+                    "{}{} {}/{}/{}",
+                    if self.dry_run { "would " } else { "" },
+                    mark,
+                    info.backup_dir.backup_type(),
+                    info.backup_dir.backup_id(),
+                    info.backup_dir.backup_time_string()
+                );
             }
-            include_hash.insert(sel_id);
-            mark.insert(backup_id, PruneMark::Keep);
-        } else {
-            mark.insert(backup_id, PruneMark::Remove);
+
+            let mut result = Ok(());
+
+            if !self.dry_run && !mark.keep() {
+                result = info.backup_dir.destroy(false);
+
+                if let (Err(err), Some(worker)) = (&result, &self.worker) {
+                    let path = info.backup_dir.relative_path();
+                    task_warn!(worker, "failed to remove dir {path:?}: {err}")
+                }
+            }
+
+            callback(info, mark, result);
         }
     }
 
-    Ok(())
-}
+    fn mark_selections<E: Fn(&BackupInfo) -> Result<String, Error>>(
+        mark: &mut HashMap<PathBuf, PruneMark>,
+        list: &[BackupInfo],
+        keep: usize,
+        select_id: E,
+    ) -> Result<(), Error> {
+        let mut include_hash = HashSet::new();
 
-fn remove_incomplete_snapshots(mark: &mut HashMap<PathBuf, PruneMark>, list: &[BackupInfo]) {
-    let mut keep_unfinished = true;
-    for info in list.iter() {
-        // backup is considered unfinished if there is no manifest
-        if info.is_finished() {
-            // There is a new finished backup, so there is no need
-            // to keep older unfinished backups.
-            keep_unfinished = false;
-        } else {
+        let mut already_included = HashSet::new();
+        for info in list {
             let backup_id = info.backup_dir.relative_path();
-            if keep_unfinished {
-                // keep first unfinished
-                mark.insert(backup_id, PruneMark::KeepPartial);
+            if let Some(PruneMark::Keep) = mark.get(&backup_id) {
+                let sel_id: String = select_id(info)?;
+                already_included.insert(sel_id);
+            }
+        }
+
+        for info in list {
+            let backup_id = info.backup_dir.relative_path();
+            if mark.get(&backup_id).is_some() {
+                continue;
+            }
+            if info.protected {
+                mark.insert(backup_id, PruneMark::Protected);
+                continue;
+            }
+            let sel_id: String = select_id(info)?;
+
+            if already_included.contains(&sel_id) {
+                continue;
+            }
+
+            if !include_hash.contains(&sel_id) {
+                if include_hash.len() >= keep {
+                    break;
+                }
+                include_hash.insert(sel_id);
+                mark.insert(backup_id, PruneMark::Keep);
             } else {
                 mark.insert(backup_id, PruneMark::Remove);
             }
-            keep_unfinished = false;
         }
-    }
-}
 
-/// This filters incomplete and kept backups.
-pub fn compute_prune_info(
-    mut list: Vec<BackupInfo>,
-    options: &KeepOptions,
-) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
-    let mut mark = HashMap::new();
+        Ok(())
+    }
 
-    BackupInfo::sort_list(&mut list, false);
+    fn remove_incomplete_snapshots(mark: &mut HashMap<PathBuf, PruneMark>, list: &[BackupInfo]) {
+        let mut keep_unfinished = true;
+        for info in list.iter() {
+            // backup is considered unfinished if there is no manifest
+            if info.is_finished() {
+                // There is a new finished backup, so there is no need
+                // to keep older unfinished backups.
+                keep_unfinished = false;
+            } else {
+                let backup_id = info.backup_dir.relative_path();
+                if keep_unfinished {
+                    // keep first unfinished
+                    mark.insert(backup_id, PruneMark::KeepPartial);
+                } else {
+                    mark.insert(backup_id, PruneMark::Remove);
+                }
+                keep_unfinished = false;
+            }
+        }
+    }
 
-    remove_incomplete_snapshots(&mut mark, &list);
+    /// This filters incomplete and kept backups.
+    pub fn compute_prune_info(
+        mut list: Vec<BackupInfo>,
+        options: &KeepOptions,
+    ) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
+        let mut mark = HashMap::new();
 
-    if let Some(keep_last) = options.keep_last {
-        mark_selections(&mut mark, &list, keep_last as usize, |info| {
-            Ok(info.backup_dir.backup_time_string().to_owned())
-        })?;
-    }
+        BackupInfo::sort_list(&mut list, false);
 
-    use proxmox_time::strftime_local;
+        Self::remove_incomplete_snapshots(&mut mark, &list);
 
-    if let Some(keep_hourly) = options.keep_hourly {
-        mark_selections(&mut mark, &list, keep_hourly as usize, |info| {
-            strftime_local("%Y/%m/%d/%H", info.backup_dir.backup_time()).map_err(Error::from)
-        })?;
-    }
+        if let Some(keep_last) = options.keep_last {
+            Self::mark_selections(&mut mark, &list, keep_last as usize, |info| {
+                Ok(info.backup_dir.backup_time_string().to_owned())
+            })?;
+        }
 
-    if let Some(keep_daily) = options.keep_daily {
-        mark_selections(&mut mark, &list, keep_daily as usize, |info| {
-            strftime_local("%Y/%m/%d", info.backup_dir.backup_time()).map_err(Error::from)
-        })?;
-    }
+        use proxmox_time::strftime_local;
 
-    if let Some(keep_weekly) = options.keep_weekly {
-        mark_selections(&mut mark, &list, keep_weekly as usize, |info| {
-            // Note: Use iso-week year/week here. This year number
-            // might not match the calendar year number.
-            strftime_local("%G/%V", info.backup_dir.backup_time()).map_err(Error::from)
-        })?;
-    }
+        if let Some(keep_hourly) = options.keep_hourly {
+            Self::mark_selections(&mut mark, &list, keep_hourly as usize, |info| {
+                strftime_local("%Y/%m/%d/%H", info.backup_dir.backup_time()).map_err(Error::from)
+            })?;
+        }
 
-    if let Some(keep_monthly) = options.keep_monthly {
-        mark_selections(&mut mark, &list, keep_monthly as usize, |info| {
-            strftime_local("%Y/%m", info.backup_dir.backup_time()).map_err(Error::from)
-        })?;
-    }
+        if let Some(keep_daily) = options.keep_daily {
+            Self::mark_selections(&mut mark, &list, keep_daily as usize, |info| {
+                strftime_local("%Y/%m/%d", info.backup_dir.backup_time()).map_err(Error::from)
+            })?;
+        }
 
-    if let Some(keep_yearly) = options.keep_yearly {
-        mark_selections(&mut mark, &list, keep_yearly as usize, |info| {
-            strftime_local("%Y", info.backup_dir.backup_time()).map_err(Error::from)
-        })?;
-    }
+        if let Some(keep_weekly) = options.keep_weekly {
+            Self::mark_selections(&mut mark, &list, keep_weekly as usize, |info| {
+                // Note: Use iso-week year/week here. This year number
+                // might not match the calendar year number.
+                strftime_local("%G/%V", info.backup_dir.backup_time()).map_err(Error::from)
+            })?;
+        }
 
-    let prune_info: Vec<(BackupInfo, PruneMark)> = list
-        .into_iter()
-        .map(|info| {
-            let backup_id = info.backup_dir.relative_path();
-            let mark = if info.protected {
-                PruneMark::Protected
-            } else {
-                mark.get(&backup_id).copied().unwrap_or(PruneMark::Remove)
-            };
+        if let Some(keep_monthly) = options.keep_monthly {
+            Self::mark_selections(&mut mark, &list, keep_monthly as usize, |info| {
+                strftime_local("%Y/%m", info.backup_dir.backup_time()).map_err(Error::from)
+            })?;
+        }
 
-            (info, mark)
-        })
-        .collect();
+        if let Some(keep_yearly) = options.keep_yearly {
+            Self::mark_selections(&mut mark, &list, keep_yearly as usize, |info| {
+                strftime_local("%Y", info.backup_dir.backup_time()).map_err(Error::from)
+            })?;
+        }
 
-    Ok(prune_info)
+        let prune_info: Vec<(BackupInfo, PruneMark)> = list
+            .into_iter()
+            .map(|info| {
+                let backup_id = info.backup_dir.relative_path();
+                let mark = if info.protected {
+                    PruneMark::Protected
+                } else {
+                    mark.get(&backup_id).copied().unwrap_or(PruneMark::Remove)
+                };
+
+                (info, mark)
+            })
+            .collect();
+
+        Ok(prune_info)
+    }
 }
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index c8e86b07..6797844e 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -52,7 +52,7 @@ use pbs_datastore::dynamic_index::{BufferedDynamicReader, DynamicIndexReader, Lo
 use pbs_datastore::fixed_index::FixedIndexReader;
 use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::{BackupManifest, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME};
-use pbs_datastore::prune::compute_prune_info;
+use pbs_datastore::prune::PruneJob;
 use pbs_datastore::{
     check_backup_owner, task_tracking, BackupDir, BackupGroup, DataStore, LocalChunkReader,
     StoreProgress, CATALOG_NAME,
@@ -982,7 +982,7 @@ pub fn prune(
 
     let list = group.list_backups()?;
 
-    let mut prune_info = compute_prune_info(list, &keep_options)?;
+    let mut prune_info = PruneJob::compute_prune_info(list, &keep_options)?;
 
     prune_info.reverse(); // delete older snapshots first
 
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index 2de34973..b9f7ebdf 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -8,7 +8,7 @@ use pbs_api_types::{
     print_store_and_ns, Authid, KeepOptions, Operation, PruneJobOptions, MAX_NAMESPACE_DEPTH,
     PRIV_DATASTORE_MODIFY, PRIV_DATASTORE_PRUNE,
 };
-use pbs_datastore::prune::compute_prune_info;
+use pbs_datastore::prune::PruneJob;
 use pbs_datastore::DataStore;
 use proxmox_rest_server::WorkerTask;
 
@@ -58,7 +58,7 @@ pub fn prune_datastore(
         let ns = group.backup_ns();
         let list = group.list_backups()?;
 
-        let mut prune_info = compute_prune_info(list, &prune_options.keep)?;
+        let mut prune_info = PruneJob::compute_prune_info(list, &prune_options.keep)?;
         prune_info.reverse(); // delete older snapshots first
 
         task_log!(
diff --git a/tests/prune.rs b/tests/prune.rs
index 3b320969..47e99841 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -4,7 +4,7 @@ use anyhow::Error;
 
 use pbs_api_types::PruneJobOptions;
 use pbs_datastore::manifest::MANIFEST_BLOB_NAME;
-use pbs_datastore::prune::compute_prune_info;
+use pbs_datastore::prune::PruneJob;
 use pbs_datastore::{BackupDir, BackupInfo};
 
 fn get_prune_list(
@@ -12,7 +12,7 @@ fn get_prune_list(
     return_kept: bool,
     options: &PruneJobOptions,
 ) -> Vec<PathBuf> {
-    let mut prune_info = compute_prune_info(list, &options.keep).unwrap();
+    let mut prune_info = PruneJob::compute_prune_info(list, &options.keep).unwrap();
 
     prune_info.reverse();
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 3/7] Add pruning parameters to the pull command

[Stefan Hanreich]

Added the prune options to the pull command, that now optionally executes
a prune job after pulling. This can be used to automatically prune the pulled
groups. group filters still apply to the pruning.

In order to use the new PruneJob function I had to adjust the
reference to the WorkerTask to an Arc<WorkerTask>, since this is what is
needed by the new PruneJob struct.

Additionally I refactored the pull method by extracting the
remove_vanished functionality into its own function. This should make
the code easier to read.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/api2/pull.rs   |   4 +-
 src/server/pull.rs | 169 ++++++++++++++++++++++++++++++---------------
 2 files changed, 115 insertions(+), 58 deletions(-)

diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index f3b31e05..719b7ca1 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -129,7 +129,7 @@ pub fn do_sync_job(
                     sync_job.remote_store,
                 );
 
-                pull_store(&worker, &client, pull_params).await?;
+                pull_store(worker.clone(), &client, pull_params).await?;
 
                 task_log!(worker, "sync job '{}' end", &job_id);
 
@@ -285,7 +285,7 @@ async fn pull(
                 remote_store,
             );
 
-            let pull_future = pull_store(&worker, &client, pull_params);
+            let pull_future = pull_store(worker.clone(), &client, pull_params);
             (select! {
                 success = pull_future.fuse() => success,
                 abort = worker.abort_future().map(|_| Err(format_err!("pull aborted"))) => abort,
diff --git a/src/server/pull.rs b/src/server/pull.rs
index 634a0b70..44068c3b 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -13,12 +13,12 @@ use pbs_config::CachedUserInfo;
 use serde_json::json;
 
 use proxmox_router::HttpError;
-use proxmox_sys::task_log;
+use proxmox_sys::{task_log, task_warn};
 
 use pbs_api_types::{
-    print_store_and_ns, Authid, BackupNamespace, GroupFilter, GroupListItem, KeepOptions,
-    NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem, MAX_NAMESPACE_DEPTH,
-    PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
+    print_store_and_ns, Authid, BackupGroup, BackupNamespace, GroupFilter, GroupListItem,
+    KeepOptions, NamespaceListItem, Operation, RateLimitConfig, Remote, SnapshotListItem,
+    MAX_NAMESPACE_DEPTH, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
 };
 
 use pbs_client::{
@@ -31,6 +31,7 @@ use pbs_datastore::index::IndexFile;
 use pbs_datastore::manifest::{
     archive_type, ArchiveType, BackupManifest, FileInfo, CLIENT_LOG_BLOB_NAME, MANIFEST_BLOB_NAME,
 };
+use pbs_datastore::prune::PruneJob;
 use pbs_datastore::{check_backup_owner, DataStore, StoreProgress};
 use pbs_tools::sha::sha256;
 use proxmox_rest_server::WorkerTask;
@@ -901,7 +902,7 @@ fn check_and_remove_vanished_ns(
 /// - creation and removal of sub-NS checked here
 /// - access to sub-NS checked here
 pub(crate) async fn pull_store(
-    worker: &WorkerTask,
+    worker: Arc<WorkerTask>,
     client: &HttpClient,
     mut params: PullParameters,
 ) -> Result<(), Error> {
@@ -913,7 +914,7 @@ pub(crate) async fn pull_store(
     let namespaces = if params.remote_ns.is_root() && params.max_depth == Some(0) {
         vec![params.remote_ns.clone()] // backwards compat - don't query remote namespaces!
     } else {
-        query_namespaces(worker, client, &mut params).await?
+        query_namespaces(&worker, client, &mut params).await?
     };
     errors |= old_max_depth != params.max_depth; // fail job if we switched to backwards-compat mode
 
@@ -952,7 +953,15 @@ pub(crate) async fn pull_store(
             }
         }
 
-        match pull_ns(worker, client, &params, namespace.clone(), target_ns).await {
+        match pull_ns(
+            worker.clone(),
+            client,
+            &params,
+            namespace.clone(),
+            target_ns,
+        )
+        .await
+        {
             Ok((ns_progress, ns_errors)) => {
                 errors |= ns_errors;
 
@@ -981,7 +990,7 @@ pub(crate) async fn pull_store(
     }
 
     if params.remove_vanished {
-        errors |= check_and_remove_vanished_ns(worker, &params, synced_ns)?;
+        errors |= check_and_remove_vanished_ns(&worker, &params, synced_ns)?;
     }
 
     if errors {
@@ -1004,7 +1013,7 @@ pub(crate) async fn pull_store(
 /// - remote namespaces are filtered by remote
 /// - owner check for vanished groups done here
 pub(crate) async fn pull_ns(
-    worker: &WorkerTask,
+    worker: Arc<WorkerTask>,
     client: &HttpClient,
     params: &PullParameters,
     source_ns: BackupNamespace,
@@ -1037,10 +1046,6 @@ pub(crate) async fn pull_ns(
         }
     });
 
-    let apply_filters = |group: &pbs_api_types::BackupGroup, filters: &[GroupFilter]| -> bool {
-        filters.iter().any(|filter| group.matches(filter))
-    };
-
     // Get groups with target NS set
     let list: Vec<pbs_api_types::BackupGroup> = list.into_iter().map(|item| item.backup).collect();
 
@@ -1071,7 +1076,7 @@ pub(crate) async fn pull_ns(
 
     let mut progress = StoreProgress::new(list.len() as u64);
 
-    for (done, group) in list.into_iter().enumerate() {
+    for (done, group) in list.iter().enumerate() {
         progress.done_groups = done as u64;
         progress.done_snapshots = 0;
         progress.group_snapshots = 0;
@@ -1079,14 +1084,14 @@ pub(crate) async fn pull_ns(
         let (owner, _lock_guard) =
             match params
                 .store
-                .create_locked_backup_group(&target_ns, &group, &params.owner)
+                .create_locked_backup_group(&target_ns, group, &params.owner)
             {
                 Ok(result) => result,
                 Err(err) => {
                     task_log!(
                         worker,
                         "sync group {} failed - group lock failed: {}",
-                        &group,
+                        group,
                         err
                     );
                     errors = true; // do not stop here, instead continue
@@ -1100,66 +1105,118 @@ pub(crate) async fn pull_ns(
             task_log!(
                 worker,
                 "sync group {} failed - owner check failed ({} != {})",
-                &group,
+                group,
                 params.owner,
                 owner
             );
             errors = true; // do not stop here, instead continue
         } else if let Err(err) = pull_group(
-            worker,
+            worker.clone().as_ref(),
             client,
             params,
-            &group,
+            group,
             source_ns.clone(),
             &mut progress,
         )
         .await
         {
-            task_log!(worker, "sync group {} failed - {}", &group, err,);
+            task_log!(worker, "sync group {} failed - {}", group, err,);
             errors = true; // do not stop here, instead continue
         }
     }
 
     if params.remove_vanished {
-        let result: Result<(), Error> = proxmox_lang::try_block!({
-            for local_group in params.store.iter_backup_groups(target_ns.clone())? {
-                let local_group = local_group?;
-                let local_group = local_group.group();
-                if new_groups.contains(local_group) {
-                    continue;
-                }
-                let owner = params.store.get_owner(&target_ns, local_group)?;
-                if check_backup_owner(&owner, &params.owner).is_err() {
-                    continue;
-                }
-                if let Some(ref group_filter) = &params.group_filter {
-                    if !apply_filters(local_group, group_filter) {
-                        continue;
-                    }
-                }
-                task_log!(worker, "delete vanished group '{local_group}'",);
-                match params.store.remove_backup_group(&target_ns, local_group) {
-                    Ok(true) => {}
-                    Ok(false) => {
-                        task_log!(
-                            worker,
-                            "kept some protected snapshots of group '{}'",
-                            local_group
-                        );
-                    }
-                    Err(err) => {
-                        task_log!(worker, "{}", err);
-                        errors = true;
-                    }
-                }
-            }
-            Ok(())
-        });
-        if let Err(err) = result {
-            task_log!(worker, "error during cleanup: {}", err);
+        if let Err(err) = remove_vanished(worker.clone(), params, target_ns.clone(), &new_groups) {
+            task_warn!(worker, "error during cleanup: {}", err);
+            errors = true;
+        };
+    }
+
+    if params.keep_options.keeps_something() {
+        if let Err(err) = prune_namespace(worker.clone(), params, target_ns.clone(), list) {
+            task_warn!(worker, "error during pruning: {}", err);
             errors = true;
         };
     }
 
     Ok((progress, errors))
 }
+
+fn apply_filters(group: &BackupGroup, filters: &[GroupFilter]) -> bool {
+    filters.iter().any(|filter| group.matches(filter))
+}
+
+fn remove_vanished(
+    worker: Arc<WorkerTask>,
+    params: &PullParameters,
+    target_ns: BackupNamespace,
+    new_groups: &HashSet<BackupGroup>,
+) -> Result<(), Error> {
+    let list_groups = params.store.iter_backup_groups(target_ns.clone())?;
+
+    for local_group in list_groups {
+        let local_group = local_group?;
+        let local_group = local_group.group();
+
+        if new_groups.contains(local_group) {
+            continue;
+        }
+
+        let owner = params.store.get_owner(&target_ns, local_group)?;
+        if check_backup_owner(&owner, &params.owner).is_err() {
+            continue;
+        }
+
+        if let Some(ref group_filter) = &params.group_filter {
+            if !apply_filters(local_group, group_filter) {
+                continue;
+            }
+        }
+
+        task_log!(worker, "delete vanished group '{local_group}'");
+
+        if !params.store.remove_backup_group(&target_ns, local_group)? {
+            task_log!(
+                worker,
+                "kept some protected snapshots of group '{}'",
+                local_group
+            );
+        }
+    }
+
+    Ok(())
+}
+
+fn prune_namespace(
+    worker: Arc<WorkerTask>,
+    params: &PullParameters,
+    target_ns: BackupNamespace,
+    backup_groups: Vec<BackupGroup>,
+) -> Result<(), Error> {
+    task_log!(worker, "running prune job");
+
+    for local_group in backup_groups.into_iter() {
+        let owner = params.store.get_owner(&target_ns, &local_group)?;
+        if check_backup_owner(&owner, &params.owner).is_err() {
+            continue;
+        }
+
+        if let Some(ref group_filter) = &params.group_filter {
+            if !apply_filters(&local_group, group_filter) {
+                continue;
+            }
+        }
+
+        task_log!(worker, "pruning backup group {}", &local_group);
+
+        let backup_group = params
+            .store
+            .backup_group(target_ns.clone(), local_group.clone());
+
+        PruneJob::new(backup_group.list_backups()?, &params.keep_options)?
+            .logging(worker.clone())
+            .run();
+    }
+
+    Ok(())
+}
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 4/7] use new PruneJob in prune command

[Stefan Hanreich]

The prune command has been slightly refactored in order to use the
functionality of the newly created PruneJob struct, instead of simply
calling compute_prune_info and the pruning the respective groups
manually.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/api2/admin/datastore.rs | 69 ++++++++++++++++---------------------
 1 file changed, 30 insertions(+), 39 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 6797844e..90f9cb48 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1,5 +1,6 @@
 //! Datastore Management
 
+use std::cell::RefCell;
 use std::collections::HashSet;
 use std::ffi::OsStr;
 use std::os::unix::ffi::OsStrExt;
@@ -26,7 +27,7 @@ use proxmox_sys::fs::{
     file_read_firstline, file_read_optional_string, replace_file, CreateOptions,
 };
 use proxmox_sys::sortable;
-use proxmox_sys::{task_log, task_warn};
+use proxmox_sys::task_log;
 
 use pxar::accessor::aio::Accessor;
 use pxar::EntryKind;
@@ -978,18 +979,18 @@ pub fn prune(
     let worker_id = format!("{}:{}:{}", store, ns, group);
     let group = datastore.backup_group(ns.clone(), group);
 
-    let mut prune_result = Vec::new();
+    // RefCell is okay to use here, since we access the Vec sequentially via the callback
+    let prune_result = RefCell::new(Vec::new());
 
     let list = group.list_backups()?;
 
-    let mut prune_info = PruneJob::compute_prune_info(list, &keep_options)?;
-
-    prune_info.reverse(); // delete older snapshots first
+    let mut prune_job = PruneJob::new(list, &keep_options)?;
+    prune_job.dry_run(dry_run);
 
     let keep_all = !keep_options.keeps_something();
 
     if dry_run {
-        for (info, mark) in prune_info {
+        prune_job.run_with_callback(|info, mark, _| {
             let keep = keep_all || mark.keep();
 
             let mut result = json!({
@@ -999,13 +1000,17 @@ pub fn prune(
                 "keep": keep,
                 "protected": mark.protected(),
             });
+
             let prune_ns = info.backup_dir.backup_ns();
+
             if !prune_ns.is_root() {
-                result["ns"] = serde_json::to_value(prune_ns)?;
+                result["ns"] = json!(prune_ns);
             }
-            prune_result.push(result);
-        }
-        return Ok(json!(prune_result));
+
+            prune_result.borrow_mut().push(result);
+        });
+
+        return Ok(json!(prune_result.into_inner()));
     }
 
     // We use a WorkerTask just to have a task log, but run synchrounously
@@ -1029,40 +1034,26 @@ pub fn prune(
         );
     }
 
-    for (info, mark) in prune_info {
-        let keep = keep_all || mark.keep();
-
-        let backup_time = info.backup_dir.backup_time();
-        let timestamp = info.backup_dir.backup_time_string();
-        let group: &pbs_api_types::BackupGroup = info.backup_dir.as_ref();
-
-        let msg = format!("{}/{}/{} {}", group.ty, group.id, timestamp, mark,);
-
-        task_log!(worker, "{}", msg);
+    prune_job
+        .logging(worker.clone())
+        .run_with_callback(|info, mark, _| {
+            let keep = keep_all || mark.keep();
 
-        prune_result.push(json!({
-            "backup-type": group.ty,
-            "backup-id": group.id,
-            "backup-time": backup_time,
-            "keep": keep,
-            "protected": mark.protected(),
-        }));
+            let backup_time = info.backup_dir.backup_time();
+            let group: &pbs_api_types::BackupGroup = info.backup_dir.as_ref();
 
-        if !(dry_run || keep) {
-            if let Err(err) = info.backup_dir.destroy(false) {
-                task_warn!(
-                    worker,
-                    "failed to remove dir {:?}: {}",
-                    info.backup_dir.relative_path(),
-                    err,
-                );
-            }
-        }
-    }
+            prune_result.borrow_mut().push(json!({
+                "backup-type": group.ty,
+                "backup-id": group.id,
+                "backup-time": backup_time,
+                "keep": keep,
+                "protected": mark.protected(),
+            }));
+        });
 
     worker.log_result(&Ok(()));
 
-    Ok(json!(prune_result))
+    Ok(json!(prune_result.into_inner()))
 }
 
 #[api(
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 5/7] use new PruneJob struct in Prune Jobs implementation

[Stefan Hanreich]

Refactor Prune Jobs to utilize the newly created PruneJob struct
instead of manually computing the prune_info and executing the prune
task.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 src/server/prune_job.rs | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index b9f7ebdf..0999ea6c 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -2,7 +2,7 @@ use std::sync::Arc;
 
 use anyhow::Error;
 
-use proxmox_sys::{task_log, task_warn};
+use proxmox_sys::task_log;
 
 use pbs_api_types::{
     print_store_and_ns, Authid, KeepOptions, Operation, PruneJobOptions, MAX_NAMESPACE_DEPTH,
@@ -58,9 +58,6 @@ pub fn prune_datastore(
         let ns = group.backup_ns();
         let list = group.list_backups()?;
 
-        let mut prune_info = PruneJob::compute_prune_info(list, &prune_options.keep)?;
-        prune_info.reverse(); // delete older snapshots first
-
         task_log!(
             worker,
             "Pruning group {ns}:\"{}/{}\"",
@@ -68,24 +65,10 @@ pub fn prune_datastore(
             group.backup_id()
         );
 
-        for (info, mark) in prune_info {
-            let keep = keep_all || mark.keep();
-            task_log!(
-                worker,
-                "{}{} {}/{}/{}",
-                if dry_run { "would " } else { "" },
-                mark,
-                group.backup_type(),
-                group.backup_id(),
-                info.backup_dir.backup_time_string()
-            );
-            if !keep && !dry_run {
-                if let Err(err) = datastore.remove_backup_dir(ns, info.backup_dir.as_ref(), false) {
-                    let path = info.backup_dir.relative_path();
-                    task_warn!(worker, "failed to remove dir {path:?}: {err}");
-                }
-            }
-        }
+        PruneJob::new(list, &prune_options.keep)?
+            .dry_run(dry_run)
+            .logging(worker.clone())
+            .run();
     }
 
     Ok(())
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 6/7] add KeepOptions to Web UI of Sync Jobs

[Stefan Hanreich]

This enables users to configure the values for a Prune Job that gets
executed after the Sync Job. If no values are configured, then no Prune
Job runs.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 www/window/SyncJobEdit.js | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index 948ad5da..5ef7062e 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -257,6 +257,11 @@ Ext.define('PBS.window.SyncJobEdit', {
 		    },
 		],
 	    },
+	    {
+		xtype: 'pbsPruneInputPanel',
+		title: gettext('Prune Options'),
+		onGetValues: (values) => values,
+	    },
 	],
     },
 });
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v2 7/7] Add documentation for prune options in Sync Jobs

[Stefan Hanreich]

Short explanation on where/how to set the options for the Prune Job in
the Web UI/CLI.

Signed-off-by: Stefan Hanreich <s.hanreich@proxmox.com>
---
 docs/managing-remotes.rst | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/docs/managing-remotes.rst b/docs/managing-remotes.rst
index 1c52e120..441710f4 100644
--- a/docs/managing-remotes.rst
+++ b/docs/managing-remotes.rst
@@ -210,3 +210,17 @@ the web interface or using the ``proxmox-backup-manager`` command-line tool:
 .. code-block:: console
 
     # proxmox-backup-manager sync-job update ID --rate-in 20MiB
+
+Additional Pruning
+^^^^^^^^^^^^^^^^^^
+Sync jobs can be configured to run an automatic prune job after their
+completion. This can be configured via the Web UI under the tab
+**Prune Options** in the **Add** or **Edit** dialogue of Sync Jobs. This can
+also be configured using the ``proxmox-backup-manager`` command-line tool:
+
+.. code-block:: console
+
+    # proxmox-backup-manager sync-job update ID --keep-last 2
+
+For more information on how Prune Jobs work, please consult the respective
+chapter :ref:`maintenance_pruning` in the documentation.
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[Thomas Lamprecht]

Am 30/11/2022 um 16:00 schrieb Stefan Hanreich:
> This patch adds the possibility of configuring prune options for sync jobs. This
> runs a prune job after a successful sync job that prunes the backup groups that
> got synced by the respective sync job (meaning this respects group-filter,
> max-depth, target namespace and so on).

Why? Ain't the existing prune job and their flexible schedules enough? Why
should one conflate syncing with pruning? Isn't that just complicating things
(many interfaces that can do everything of other interfaces).

Am 30/11/2022 um 16:00 schrieb Stefan Hanreich:
>   Add KeepOptions parameters to pull & sync-job
>   refactor prune.rs - add PruneJob struct for handling prune jobs
>   Add pruning parameters to the pull command
>   use new PruneJob in prune command
>   use new PruneJob struct in Prune Jobs implementation
>   add KeepOptions to Web UI of Sync Jobs
>   Add documentation for prune options in Sync Jobs

having a lot of changelogs written, especially recently the lacks of human
readable tags (e.g., "docs:", "ui:" "sync job:", ...) and mixed casing stuck
quite out, even much more so the use of filenames in commit subjects which I
already pointed out quite directly and visible as being useless and annoying.
Structs are not always as worse, but not very often _that_ useful either for
skimming a bigger git log or assembling change somewhat meaningful logs.

Re: [pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[Stefan Hanreich]

On 11/30/22 17:23, Thomas Lamprecht wrote:
> Am 30/11/2022 um 16:00 schrieb Stefan Hanreich:
>> This patch adds the possibility of configuring prune options for sync jobs. This
>> runs a prune job after a successful sync job that prunes the backup groups that
>> got synced by the respective sync job (meaning this respects group-filter,
>> max-depth, target namespace and so on).
> 
> Why? Ain't the existing prune job and their flexible schedules enough? Why
> should one conflate syncing with pruning? Isn't that just complicating things
> (many interfaces that can do everything of other interfaces).
> 

It's what me and Fabian came up with in response to #3701. The idea 
behind it was that it would be a convenience feature, so one doesn't 
have to additionally assemble a second prune command with the exact same 
options. Particularly because this prune operation would honor the 
group-filter option from the sync job, which is currently not possible 
with prune jobs alone. Maybe we could approach it from the other way and 
make something like group-filter possible for prune jobs?

> Am 30/11/2022 um 16:00 schrieb Stefan Hanreich:
>>    Add KeepOptions parameters to pull & sync-job
>>    refactor prune.rs - add PruneJob struct for handling prune jobs
>>    Add pruning parameters to the pull command
>>    use new PruneJob in prune command
>>    use new PruneJob struct in Prune Jobs implementation
>>    add KeepOptions to Web UI of Sync Jobs
>>    Add documentation for prune options in Sync Jobs
> 
> having a lot of changelogs written, especially recently the lacks of human
> readable tags (e.g., "docs:", "ui:" "sync job:", ...) and mixed casing stuck
> quite out, even much more so the use of filenames in commit subjects which I
> already pointed out quite directly and visible as being useless and annoying.
> Structs are not always as worse, but not very often _that_ useful either for
> skimming a bigger git log or assembling change somewhat meaningful logs.
> 
I thought in this case it would be okay to include the filename, since I 
wasn't sure how to otherwise refer to the part of the code I refactored. 
I guess that was a lapse in judgement, would it be better to just refer 
to the respective functions? I also should have included the respective 
Bugzilla id for context. I don't know how I missed that in hindsight.

Re: [pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[Lukas Wagner]

Hi,

gave this test series a quick test on the latest master. Set up a second PBS and
set up a sync job with enabled pruning. Only tested "keep-last" prune option due to
practical reasons. In further testing I made sure that the group filters also
apply to the pruning operation. Everything seems to work as expected.

Also looked through the code, which looks good to me.

In summary, consider this

Tested-by: Lukas Wagner <l.wagner@proxmox.com>
Reviewed-by: Lukas Wagner <l.wagner@proxmox.com>

On 11/30/22 16:00, Stefan Hanreich wrote:
> This relies on my small formatting fix in the email
> [PATCH proxmox-backup 1/1] Fix formatting in proxmox-backup-manager
> otherwise it will have a merge conflict.
> 
> This patch adds the possibility of configuring prune options for sync jobs. This
> runs a prune job after a successful sync job that prunes the backup groups that
> got synced by the respective sync job (meaning this respects group-filter,
> max-depth, target namespace and so on).
> 
> The prune options can also be specified when running a single pull command.
> 
> I also added the possibility of configuring this via the Web UI in the same way
> as configuring a regular prune job.
> 
> The new options are documented in the Sync Job documentation as well.
> 
> This patch series introduces a new struct, PruneJob, that encapsulates the
> pruning functionality in its own struct that can be conveniently used. This
> greatly reduces the code duplication between the sites that used pruning
> functionality.
> 
> Changes from v2:
> - Refactored pull function by extracting remove_vanished and prune into
> their own methods
> - Added PruneJob struct that encapsulates PruneJob functionality
> - Refactored existing call sites to utilize new PruneJob struct
> - Use KeepOptions::default where applicable
> - minor formatting and cargo clippy fixes
> 
> Stefan Hanreich (7):
>    Add KeepOptions parameters to pull & sync-job
>    refactor prune.rs - add PruneJob struct for handling prune jobs
>    Add pruning parameters to the pull command
>    use new PruneJob in prune command
>    use new PruneJob struct in Prune Jobs implementation
>    add KeepOptions to Web UI of Sync Jobs
>    Add documentation for prune options in Sync Jobs
> 
>   docs/managing-remotes.rst         |  14 ++
>   pbs-api-types/src/jobs.rs         |   7 +-
>   pbs-datastore/src/prune.rs        | 279 ++++++++++++++++++------------
>   src/api2/admin/datastore.rs       |  71 ++++----
>   src/api2/config/sync.rs           |  52 ++++++
>   src/api2/pull.rs                  |  17 +-
>   src/bin/proxmox-backup-manager.rs |  17 +-
>   src/server/prune_job.rs           |  29 +---
>   src/server/pull.rs                | 173 ++++++++++++------
>   tests/prune.rs                    |   4 +-
>   www/window/SyncJobEdit.js         |   5 +
>   11 files changed, 433 insertions(+), 235 deletions(-)
> 

-- 
Best Regards,
Lukas Wagner

Re: [pbs-devel] [PATCH proxmox-backup v2 0/7] Add Prune Options to Sync Jobs

[Thomas Lamprecht]

Am 01/12/2022 um 10:50 schrieb Stefan Hanreich:
> Maybe we could approach it from the other way and make something like group-filter possible for prune jobs?

sounds more reasonable and useful in general.

> I thought in this case it would be okay to include the filename, since I wasn't sure how to otherwise refer to the part of the code I refactored.

For that, IMO it might be still better to either use the module name (there
can be more than one prune.rs in a code base) or some more general like
"refactor datastore prune implementation"