[pbs-devel] [PATCH proxmox-backup v3 00/13] add 'protected' setting for snapshots

[pbs-devel] [PATCH proxmox-backup v3 00/13] add 'protected' setting for snapshots

[Dominik Csapak]

this v3 is mostly to provide a current rebase on master, since some of
the code was moved

add the means to 'protect' a snapshot against pruning and removal by
adding a file '.protected' in the snapshot folder

in the v2, i opted against syncing the protected flag for two reasons:
* it gives the admin the choice on each datastore if he wants to keep
  the snapshot or not(if we sync it, we would have to sync it everytime?)
* it is much simpler to implement

changes from v2:
* rebase on master
* increase width of action column (so that all icons are visible)

changes from v1:
* rebase on master
* return the PruneMark enum instead of two booleans in compute_prune_info
* lock the snapshot when setting/removig the flag
* don't bail if the file does not exist on removal
* add a 'protected show' command to the client
* add a docs section to explain the protected flag

changes from rfc:
* added gui parts
* added tests
* fixed a bug (that the test uncovered)
* add pbs client command
* prevent removal (also during sync)

Dominik Csapak (13):
  pbs-datastore: add protection info to BackupInfo
  pbs-datastore: skip protected backups in pruning
  add protected info of snapshots to api and task logs
  tests/prune: add tests for protecteded backups
  backup/datastore: prevent protected snapshots to be removed
  pull_store/group: dont try remove locally protected snapshots
  api2: datastore/delete_group: throw error for partially removed group
  api2/admin/datastore: add get/set_protection
  proxmox-backup-client: add 'protected' commands
  ui: PruneInputPanel: add keepReason 'protected' for protected backups
  ui: add protected icon to snapshots
  fix #3602: ui: datastore/Content: add action to set protection status
  docs: add info about protection flag to client docs

 docs/backup-client.rst                |  19 +++++
 pbs-api-types/src/datastore.rs        |   2 +
 pbs-datastore/src/backup_info.rs      |  20 ++++-
 pbs-datastore/src/datastore.rs        |  61 +++++++++++---
 pbs-datastore/src/prune.rs            |  44 ++++++++--
 proxmox-backup-client/src/snapshot.rs | 113 ++++++++++++++++++++++++++
 src/api2/admin/datastore.rs           | 111 +++++++++++++++++++++++--
 src/server/prune_job.rs               |   6 +-
 src/server/pull.rs                    |  20 ++++-
 tests/prune.rs                        |  44 +++++++++-
 www/datastore/Content.js              |  77 +++++++++++++++++-
 www/datastore/Prune.js                |   4 +
 12 files changed, 483 insertions(+), 38 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 01/13] pbs-datastore: add protection info to BackupInfo

[Dominik Csapak]

and add necessary helper functions (protected_file/is_protected)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 20 ++++++++++++++++++--
 tests/prune.rs                   |  2 +-
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 9d1ab991..b94b9779 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -92,7 +92,9 @@ impl BackupGroup {
                     BackupDir::with_rfc3339(&self.backup_type, &self.backup_id, backup_time)?;
                 let files = list_backup_files(l2_fd, backup_time)?;
 
-                list.push(BackupInfo { backup_dir, files });
+                let protected = backup_dir.is_protected(base_path.to_owned());
+
+                list.push(BackupInfo { backup_dir, files, protected });
 
                 Ok(())
             },
@@ -253,6 +255,17 @@ impl BackupDir {
         relative_path
     }
 
+    pub fn protected_file(&self, mut path: PathBuf) -> PathBuf {
+        path.push(self.relative_path());
+        path.push(".protected");
+        path
+    }
+
+    pub fn is_protected(&self, base_path: PathBuf) -> bool {
+        let path = self.protected_file(base_path);
+        path.exists()
+    }
+
     pub fn backup_time_to_string(backup_time: i64) -> Result<String, Error> {
         // fixme: can this fail? (avoid unwrap)
         Ok(proxmox_time::epoch_to_rfc3339_utc(backup_time)?)
@@ -293,6 +306,8 @@ pub struct BackupInfo {
     pub backup_dir: BackupDir,
     /// List of data files
     pub files: Vec<String>,
+    /// Protection Status
+    pub protected: bool,
 }
 
 impl BackupInfo {
@@ -301,8 +316,9 @@ impl BackupInfo {
         path.push(backup_dir.relative_path());
 
         let files = list_backup_files(libc::AT_FDCWD, &path)?;
+        let protected = backup_dir.is_protected(base_path.to_owned());
 
-        Ok(BackupInfo { backup_dir, files })
+        Ok(BackupInfo { backup_dir, files, protected })
     }
 
     /// Finds the latest backup inside a backup group
diff --git a/tests/prune.rs b/tests/prune.rs
index 4853ee49..96c29f4a 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -42,7 +42,7 @@ fn create_info(
         files.push(String::from(MANIFEST_BLOB_NAME));
     }
 
-    BackupInfo { backup_dir, files }
+    BackupInfo { backup_dir, files, protected: false }
 }
 
 #[test]
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 02/13] pbs-datastore: skip protected backups in pruning

[Dominik Csapak]

as a separate keep reason so it will not be calculated for the other reasons

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/prune.rs | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
index 70c3d91c..0eaa8acd 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -7,7 +7,7 @@ use pbs_api_types::PruneOptions;
 
 use super::BackupInfo;
 
-enum PruneMark { Keep, KeepPartial, Remove }
+enum PruneMark { Protected, Keep, KeepPartial, Remove }
 
 fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>> (
     mark: &mut HashMap<PathBuf, PruneMark>,
@@ -30,6 +30,10 @@ fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>> (
     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; }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 03/13] add protected info of snapshots to api and task logs

[Dominik Csapak]

adds the info that a snapshot is protected to:
* snapshot list
* manual pruning (also dry-run)
* prune jobs

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-api-types/src/datastore.rs |  2 ++
 pbs-datastore/src/prune.rs     | 40 +++++++++++++++++++++++++++-------
 src/api2/admin/datastore.rs    | 15 ++++++++-----
 src/server/prune_job.rs        |  6 ++---
 tests/prune.rs                 |  4 ++--
 5 files changed, 49 insertions(+), 18 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 462081e4..77c1258f 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -390,6 +390,8 @@ pub struct SnapshotListItem {
     /// The owner of the snapshots group
     #[serde(skip_serializing_if = "Option::is_none")]
     pub owner: Option<Authid>,
+    /// Protection from prunes
+    pub protected: bool,
 }
 
 #[api(
diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
index 0eaa8acd..e66ed408 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -7,7 +7,30 @@ use pbs_api_types::PruneOptions;
 
 use super::BackupInfo;
 
-enum PruneMark { Protected, Keep, KeepPartial, Remove }
+#[derive(Clone, Copy, PartialEq, Eq)]
+pub enum PruneMark { Protected, Keep, KeepPartial, Remove }
+
+impl PruneMark {
+    pub fn keep(&self) -> bool {
+        *self != PruneMark::Remove
+    }
+
+    pub fn protected(&self) -> bool {
+        *self == PruneMark::Protected
+    }
+}
+
+impl std::fmt::Display for PruneMark {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let txt = match self {
+            PruneMark::Protected => "protected",
+            PruneMark::Keep => "keep",
+            PruneMark::KeepPartial => "keep-partial",
+            PruneMark::Remove => "remove",
+        };
+        write!(f, "{}", txt)
+    }
+}
 
 fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>> (
     mark: &mut HashMap<PathBuf, PruneMark>,
@@ -125,7 +148,7 @@ pub fn cli_options_string(options: &PruneOptions) -> String {
 pub fn compute_prune_info(
     mut list: Vec<BackupInfo>,
     options: &PruneOptions,
-) -> Result<Vec<(BackupInfo, bool)>, Error> {
+) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
 
     let mut mark = HashMap::new();
 
@@ -173,15 +196,16 @@ pub fn compute_prune_info(
         })?;
     }
 
-    let prune_info: Vec<(BackupInfo, bool)> = list.into_iter()
+    let prune_info: Vec<(BackupInfo, PruneMark)> = list.into_iter()
         .map(|info| {
             let backup_id = info.backup_dir.relative_path();
-            let keep = match mark.get(&backup_id) {
-                Some(PruneMark::Keep) => true,
-                Some(PruneMark::KeepPartial) => true,
-               _ => false,
+            let mark = if info.protected {
+                PruneMark::Protected
+            } else {
+                *mark.get(&backup_id).unwrap_or(&PruneMark::Remove)
             };
-            (info, keep)
+
+            (info, mark)
         })
         .collect();
 
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 3bf8e104..b739d430 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -441,6 +441,7 @@ pub fn list_snapshots (
         let backup_type = group.backup_type().to_string();
         let backup_id = group.backup_id().to_string();
         let backup_time = info.backup_dir.backup_time();
+        let protected = info.backup_dir.is_protected(base_path.clone());
 
         match get_all_snapshot_files(&datastore, &info) {
             Ok((manifest, files)) => {
@@ -479,6 +480,7 @@ pub fn list_snapshots (
                     files,
                     size,
                     owner,
+                    protected,
                 }
             },
             Err(err) => {
@@ -503,6 +505,7 @@ pub fn list_snapshots (
                     files,
                     size: None,
                     owner,
+                    protected,
                 }
             },
         }
@@ -846,8 +849,8 @@ pub fn prune(
     let keep_all = !pbs_datastore::prune::keeps_something(&prune_options);
 
     if dry_run {
-        for (info, mut keep) in prune_info {
-            if keep_all { keep = true; }
+        for (info, mark) in prune_info {
+            let keep = keep_all || mark.keep();
 
             let backup_time = info.backup_dir.backup_time();
             let group = info.backup_dir.group();
@@ -857,6 +860,7 @@ pub fn prune(
                 "backup-id": group.backup_id(),
                 "backup-time": backup_time,
                 "keep": keep,
+                "protected": mark.protected(),
             }));
         }
         return Ok(json!(prune_result));
@@ -874,8 +878,8 @@ pub fn prune(
                   store, backup_type, backup_id);
     }
 
-    for (info, mut keep) in prune_info {
-        if keep_all { keep = true; }
+    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();
@@ -887,7 +891,7 @@ pub fn prune(
             group.backup_type(),
             group.backup_id(),
             timestamp,
-            if keep { "keep" } else { "remove" },
+            mark,
         );
 
         task_log!(worker, "{}", msg);
@@ -897,6 +901,7 @@ pub fn prune(
             "backup-id": group.backup_id(),
             "backup-time": backup_time,
             "keep": keep,
+            "protected": mark.protected(),
         }));
 
         if !(dry_run || keep) {
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index fc6443e9..0fc68118 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -63,12 +63,12 @@ pub fn prune_datastore(
             group.backup_id()
         );
 
-        for (info, mut keep) in prune_info {
-            if keep_all { keep = true; }
+        for (info, mark) in prune_info {
+            let keep = keep_all || mark.keep();
             task_log!(
                 worker,
                 "{} {}/{}/{}",
-                if keep { "keep" } else { "remove" },
+                mark,
                 group.backup_type(),
                 group.backup_id(),
                 info.backup_dir.backup_time_string()
diff --git a/tests/prune.rs b/tests/prune.rs
index 96c29f4a..448588b6 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -19,8 +19,8 @@ fn get_prune_list(
 
     prune_info
         .iter()
-        .filter_map(|(info, keep)| {
-            if *keep != return_kept {
+        .filter_map(|(info, mark)| {
+            if mark.keep() != return_kept {
                 None
             } else {
                 Some(info.backup_dir.relative_path())
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 04/13] tests/prune: add tests for protecteded backups

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 tests/prune.rs | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/tests/prune.rs b/tests/prune.rs
index 448588b6..dc5a8284 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -45,6 +45,44 @@ fn create_info(
     BackupInfo { backup_dir, files, protected: false }
 }
 
+fn create_info_protected(
+    snapshot: &str,
+    partial: bool,
+) -> BackupInfo {
+    let mut info = create_info(snapshot, partial);
+    info.protected = true;
+    info
+}
+
+#[test]
+fn test_prune_protected() -> Result<(), Error> {
+    let mut orig_list = Vec::new();
+
+    orig_list.push(create_info_protected("host/elsa/2019-11-15T09:39:15Z", false));
+    orig_list.push(create_info("host/elsa/2019-11-15T10:39:15Z", false));
+    orig_list.push(create_info("host/elsa/2019-11-15T10:49:15Z", false));
+    orig_list.push(create_info_protected("host/elsa/2019-11-15T10:59:15Z", false));
+
+    eprintln!("{:?}", orig_list);
+
+    let mut options = PruneOptions::default();
+    options.keep_last = Some(1);
+    let remove_list = get_prune_list(orig_list.clone(), false, &options);
+    let expect: Vec<PathBuf> = vec![
+        PathBuf::from("host/elsa/2019-11-15T10:39:15Z"),
+    ];
+    assert_eq!(remove_list, expect);
+
+    let mut options = PruneOptions::default();
+    options.keep_hourly = Some(1);
+    let remove_list = get_prune_list(orig_list.clone(), false, &options);
+    let expect: Vec<PathBuf> = vec![
+        PathBuf::from("host/elsa/2019-11-15T10:39:15Z"),
+    ];
+    assert_eq!(remove_list, expect);
+    Ok(())
+}
+
 #[test]
 fn test_prune_hourly() -> Result<(), Error> {
 
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 05/13] backup/datastore: prevent protected snapshots to be removed

[Dominik Csapak]

by throwing an error for remove_backup_dir, and skipping for
remove_backup_group

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 37 +++++++++++++++++++++++-----------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 7159e578..e32887de 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -266,8 +266,9 @@ impl DataStore {
         full_path
     }
 
-    /// Remove a complete backup group including all snapshots
-    pub fn remove_backup_group(&self, backup_group: &BackupGroup) ->  Result<(), Error> {
+    /// Remove a complete backup group including all snapshots, returns true
+    /// if all snapshots were removed, and false if some were protected
+    pub fn remove_backup_group(&self, backup_group: &BackupGroup) ->  Result<bool, Error> {
 
         let full_path = self.group_path(backup_group);
 
@@ -275,22 +276,30 @@ impl DataStore {
 
         log::info!("removing backup group {:?}", full_path);
 
+        let mut removed_all = true;
+
         // remove all individual backup dirs first to ensure nothing is using them
         for snap in backup_group.list_backups(&self.base_path())? {
+            if snap.backup_dir.is_protected(self.base_path()) {
+                removed_all = false;
+                continue;
+            }
             self.remove_backup_dir(&snap.backup_dir, false)?;
         }
 
-        // no snapshots left, we can now safely remove the empty folder
-        std::fs::remove_dir_all(&full_path)
-            .map_err(|err| {
-                format_err!(
-                    "removing backup group directory {:?} failed - {}",
-                    full_path,
-                    err,
-                )
-            })?;
+        if removed_all {
+            // no snapshots left, we can now safely remove the empty folder
+            std::fs::remove_dir_all(&full_path)
+                .map_err(|err| {
+                    format_err!(
+                        "removing backup group directory {:?} failed - {}",
+                        full_path,
+                        err,
+                    )
+                })?;
+        }
 
-        Ok(())
+        Ok(removed_all)
     }
 
     /// Remove a backup directory including all content
@@ -304,6 +313,10 @@ impl DataStore {
             _manifest_guard = self.lock_manifest(backup_dir)?;
         }
 
+        if backup_dir.is_protected(self.base_path()) {
+            bail!("cannot remove protected snapshot");
+        }
+
         log::info!("removing backup snapshot {:?}", full_path);
         std::fs::remove_dir_all(&full_path)
             .map_err(|err| {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 06/13] pull_store/group: dont try remove locally protected snapshots

[Dominik Csapak]

and log if a vanished groups could not be completely deleted if it
contains protected snapshots

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/server/pull.rs | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index a15d3bb5..555f0a94 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -605,6 +605,14 @@ pub async fn pull_group(
             if remote_snapshots.contains(&backup_time) {
                 continue;
             }
+            if info.backup_dir.is_protected(tgt_store.base_path()) {
+                task_log!(
+                    worker,
+                    "don't delete vanished snapshot {:?} (protected)",
+                    info.backup_dir.relative_path()
+                );
+                continue;
+            }
             task_log!(worker, "delete vanished snapshot {:?}", info.backup_dir.relative_path());
             tgt_store.remove_backup_dir(&info.backup_dir, false)?;
         }
@@ -719,9 +727,15 @@ pub async fn pull_store(
                     local_group.backup_type(),
                     local_group.backup_id()
                 );
-                if let Err(err) = tgt_store.remove_backup_group(&local_group) {
-                    task_log!(worker, "{}", err.to_string());
-                    errors = true;
+                match tgt_store.remove_backup_group(&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(())
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 07/13] api2: datastore/delete_group: throw error for partially removed group

[Dominik Csapak]

when a group could not be completely removed due to protected snapshot,
throw an error

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/admin/datastore.rs | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index b739d430..fe962e4e 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -272,7 +272,9 @@ pub fn delete_group(
 
     check_priv_or_backup_owner(&datastore, &group, &auth_id, PRIV_DATASTORE_MODIFY)?;
 
-    datastore.remove_backup_group(&group)?;
+    if !datastore.remove_backup_group(&group)? {
+        bail!("did not delete whole group because of protected snapthots");
+    }
 
     Ok(Value::Null)
 }
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 08/13] api2/admin/datastore: add get/set_protection

[Dominik Csapak]

for gettin/setting the protected flag for snapshots (akin to notes)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 pbs-datastore/src/datastore.rs | 24 +++++++++
 src/api2/admin/datastore.rs    | 92 ++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index e32887de..5049cb3d 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -858,6 +858,30 @@ impl DataStore {
         Ok(())
     }
 
+    /// Updates the protection status of the specified snapshot.
+    pub fn update_protection(
+        &self,
+        backup_dir: &BackupDir,
+        protection: bool
+    ) -> Result<(), Error> {
+        let full_path = self.snapshot_path(backup_dir);
+
+        let _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?;
+
+        let protected_path = backup_dir.protected_file(self.base_path());
+        if protection {
+            std::fs::File::create(protected_path)
+                .map_err(|err| format_err!("could not create protection file: {}", err))?;
+        } else if let Err(err) = std::fs::remove_file(protected_path) {
+            // ignore error for non-existing file
+            if err.kind() != std::io::ErrorKind::NotFound {
+                bail!("could not remove protection file: {}", err);
+            }
+        }
+
+        Ok(())
+    }
+
     pub fn verify_new(&self) -> bool {
         self.verify_new
     }
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index fe962e4e..b9bcde12 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1750,6 +1750,92 @@ pub fn set_notes(
     Ok(())
 }
 
+#[api(
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+            "backup-type": {
+                schema: BACKUP_TYPE_SCHEMA,
+            },
+            "backup-id": {
+                schema: BACKUP_ID_SCHEMA,
+            },
+            "backup-time": {
+                schema: BACKUP_TIME_SCHEMA,
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
+    },
+)]
+/// Query protection for a specific backup
+pub fn get_protection(
+    store: String,
+    backup_type: String,
+    backup_id: String,
+    backup_time: i64,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<bool, Error> {
+    let datastore = DataStore::lookup_datastore(&store)?;
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
+
+    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_AUDIT)?;
+
+    let protected_path = backup_dir.protected_file(datastore.base_path());
+
+    Ok(protected_path.exists())
+}
+
+#[api(
+    input: {
+        properties: {
+            store: {
+                schema: DATASTORE_SCHEMA,
+            },
+            "backup-type": {
+                schema: BACKUP_TYPE_SCHEMA,
+            },
+            "backup-id": {
+                schema: BACKUP_ID_SCHEMA,
+            },
+            "backup-time": {
+                schema: BACKUP_TIME_SCHEMA,
+            },
+            protected: {
+                description: "Enable/disable protection.",
+            },
+        },
+    },
+    access: {
+        permission: &Permission::Privilege(&["datastore", "{store}"],
+                                           PRIV_DATASTORE_MODIFY | PRIV_DATASTORE_BACKUP,
+                                           true),
+    },
+)]
+/// En- or disable protection for a specific backup
+pub fn set_protection(
+    store: String,
+    backup_type: String,
+    backup_id: String,
+    backup_time: i64,
+    protected: bool,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<(), Error> {
+    let datastore = DataStore::lookup_datastore(&store)?;
+
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
+
+    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_MODIFY)?;
+
+    datastore.update_protection(&backup_dir, protected)
+}
+
 #[api(
     input: {
         properties: {
@@ -1898,6 +1984,12 @@ const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
             .get(&API_METHOD_GET_NOTES)
             .put(&API_METHOD_SET_NOTES)
     ),
+    (
+        "protected",
+        &Router::new()
+            .get(&API_METHOD_GET_PROTECTION)
+            .put(&API_METHOD_SET_PROTECTION)
+    ),
     (
         "prune",
         &Router::new()
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 09/13] proxmox-backup-client: add 'protected' commands

[Dominik Csapak]

includes 'update' and 'show' similar to the notes commands

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 proxmox-backup-client/src/snapshot.rs | 113 ++++++++++++++++++++++++++
 1 file changed, 113 insertions(+)

diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
index 6b563d79..46b1db09 100644
--- a/proxmox-backup-client/src/snapshot.rs
+++ b/proxmox-backup-client/src/snapshot.rs
@@ -358,6 +358,118 @@ async fn update_notes(param: Value) -> Result<Value, Error> {
     Ok(Value::Null)
 }
 
+#[api(
+    input: {
+        properties: {
+            repository: {
+                schema: REPO_URL_SCHEMA,
+                optional: true,
+            },
+            snapshot: {
+                type: String,
+                description: "Snapshot path.",
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        }
+    }
+)]
+/// Show protection status of the specified snapshot
+async fn show_protection(param: Value) -> Result<Value, Error> {
+    let repo = extract_repository_from_value(&param)?;
+    let path = required_string_param(&param, "snapshot")?;
+
+    let snapshot: BackupDir = path.parse()?;
+    let client = connect(&repo)?;
+
+    let path = format!("api2/json/admin/datastore/{}/protected", repo.store());
+
+    let args = json!({
+        "backup-type": snapshot.group().backup_type(),
+        "backup-id": snapshot.group().backup_id(),
+        "backup-time": snapshot.backup_time(),
+    });
+
+    let output_format = get_output_format(&param);
+
+    let mut result = client.get(&path, Some(args)).await?;
+
+    let protected = result["data"].take();
+
+    if output_format == "text" {
+        if let Some(protected) = protected.as_bool() {
+            println!("{}", protected);
+        }
+    } else {
+        format_and_print_result(
+            &json!({
+                "protected": protected,
+            }),
+            &output_format,
+        );
+    }
+
+    Ok(Value::Null)
+}
+
+#[api(
+    input: {
+        properties: {
+            repository: {
+                schema: REPO_URL_SCHEMA,
+                optional: true,
+            },
+            snapshot: {
+                type: String,
+                description: "Snapshot path.",
+            },
+            protected: {
+                type: bool,
+                description: "The protection status.",
+            },
+        }
+    }
+)]
+/// Update Protection Status of a snapshot
+async fn update_protection(protected: bool, param: Value) -> Result<Value, Error> {
+    let repo = extract_repository_from_value(&param)?;
+    let path = required_string_param(&param, "snapshot")?;
+
+    let snapshot: BackupDir = path.parse()?;
+    let mut client = connect(&repo)?;
+
+    let path = format!("api2/json/admin/datastore/{}/protected", repo.store());
+
+    let args = json!({
+        "backup-type": snapshot.group().backup_type(),
+        "backup-id": snapshot.group().backup_id(),
+        "backup-time": snapshot.backup_time(),
+        "protected": protected,
+    });
+
+    client.put(&path, Some(args)).await?;
+
+    Ok(Value::Null)
+}
+
+fn protected_cli() -> CliCommandMap {
+    CliCommandMap::new()
+        .insert(
+            "show",
+            CliCommand::new(&API_METHOD_SHOW_PROTECTION)
+                .arg_param(&["snapshot"])
+                .completion_cb("snapshot", complete_backup_snapshot),
+        )
+        .insert(
+            "update",
+            CliCommand::new(&API_METHOD_UPDATE_PROTECTION)
+                .arg_param(&["snapshot", "protected"])
+                .completion_cb("snapshot", complete_backup_snapshot),
+        )
+}
+
 fn notes_cli() -> CliCommandMap {
     CliCommandMap::new()
         .insert(
@@ -377,6 +489,7 @@ fn notes_cli() -> CliCommandMap {
 pub fn snapshot_mgtm_cli() -> CliCommandMap {
     CliCommandMap::new()
         .insert("notes", notes_cli())
+        .insert("protected", protected_cli())
         .insert(
             "list",
             CliCommand::new(&API_METHOD_LIST_SNAPSHOTS)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 10/13] ui: PruneInputPanel: add keepReason 'protected' for protected backups

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/datastore/Prune.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/www/datastore/Prune.js b/www/datastore/Prune.js
index 42ea21bf..e4db4b6c 100644
--- a/www/datastore/Prune.js
+++ b/www/datastore/Prune.js
@@ -103,6 +103,10 @@ Ext.define('PBS.Datastore.PruneInputPanel', {
 		let rule = nextRule();
 		for (let backup of backups) {
 		    if (backup.keep) {
+			if (backup.protected) {
+			    backup.keepReason = 'protected';
+			    continue;
+			}
 			counter[rule]++;
 			if (rule !== 'keep-all') {
 			    backup.keepReason = rule + ': ' + counter[rule];
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 11/13] ui: add protected icon to snapshots

[Dominik Csapak]

if they are protected

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/datastore/Content.js | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index 87a9079d..375e782b 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -654,6 +654,13 @@ Ext.define('PBS.DataStoreContent', {
 	    xtype: 'treecolumn',
 	    header: gettext("Backup Group"),
 	    dataIndex: 'text',
+	    renderer: (value, meta, record) => {
+		let protect = "";
+		if (record.data.protected) {
+		    protect = ` <i class="fa fa-shield"></i>`;
+		}
+		return value + protect;
+	    },
 	    flex: 1,
 	},
 	{
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 12/13] fix #3602: ui: datastore/Content: add action to set protection status

[Dominik Csapak]

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/datastore/Content.js | 70 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 69 insertions(+), 1 deletion(-)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index 375e782b..361271a7 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -494,6 +494,68 @@ Ext.define('PBS.DataStoreContent', {
 	    });
 	},
 
+	onProtectionChange: function(view, rI, cI, item, e, rec) {
+	    let me = this;
+	    view = this.getView();
+
+	    if (!(rec && rec.data)) return;
+	    let data = rec.data;
+	    if (!view.datastore) return;
+
+	    let type = data["backup-type"];
+	    let id = data["backup-id"];
+	    let time = (data["backup-time"].getTime()/1000).toFixed(0);
+
+	    let params = {
+		'backup-type': type,
+		'backup-id': id,
+		'backup-time': time,
+	    };
+
+	    let url = `/api2/extjs/admin/datastore/${view.datastore}/protected`;
+
+	    Ext.create('Proxmox.window.Edit', {
+		subject: gettext('Protection') + ` - ${data.text}`,
+		width: 400,
+
+		method: 'PUT',
+		autoShow: true,
+		isCreate: false,
+		autoLoad: true,
+
+		loadUrl: `${url}?${Ext.Object.toQueryString(params)}`,
+		url,
+
+		items: [
+		    {
+			xtype: 'hidden',
+			name: 'backup-type',
+			value: type,
+		    },
+		    {
+			xtype: 'hidden',
+			name: 'backup-id',
+			value: id,
+		    },
+		    {
+			xtype: 'hidden',
+			name: 'backup-time',
+			value: time,
+		    },
+		    {
+			xtype: 'proxmoxcheckbox',
+			fieldLabel: gettext('Protected'),
+			uncheckedValue: 0,
+			name: 'protected',
+			value: data.protected,
+		    },
+		],
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	},
+
 	onForget: function(view, rI, cI, item, e, rec) {
 	    let me = this;
 	    view = this.getView();
@@ -710,7 +772,7 @@ Ext.define('PBS.DataStoreContent', {
 	    header: gettext('Actions'),
 	    xtype: 'actioncolumn',
 	    dataIndex: 'text',
-	    width: 140,
+	    width: 150,
 	    items: [
 		{
 		    handler: 'onVerify',
@@ -730,6 +792,12 @@ Ext.define('PBS.DataStoreContent', {
 		    getClass: (v, m, rec) => rec.parentNode.id ==='root' ? 'fa fa-scissors' : 'pmx-hidden',
 		    isActionDisabled: (v, r, c, i, rec) => rec.parentNode.id !=='root',
 		},
+		{
+		    handler: 'onProtectionChange',
+		    getTip: (v, m, rec) => Ext.String.format(gettext("Change protection of '{0}'"), v),
+		    getClass: (v, m, rec) => !rec.data.leaf && rec.parentNode.id !== 'root' ? 'fa fa-shield' : 'pmx-hidden',
+		    isActionDisabled: (v, r, c, i, rec) => !!rec.data.leaf || rec.parentNode.id === 'root',
+		},
 		{
 		    handler: 'onForget',
 		    getTip: (v, m, rec) => rec.parentNode.id !=='root'
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v3 13/13] docs: add info about protection flag to client docs

[Dominik Csapak]

and mention that sync/pull does not sync the protected flag

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 docs/backup-client.rst | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/docs/backup-client.rst b/docs/backup-client.rst
index 2b51c62c..fec6ae59 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -654,6 +654,25 @@ shows the list of existing snapshots and what actions prune would take.
    in the chunk-store. The chunk-store still contains the data blocks. To free
    space you need to perform :ref:`client_garbage-collection`.
 
+It is also possible to protect single snapshots from being pruned or deleted:
+
+.. code-block:: console
+
+  # proxmox-backup-client snapshot protected update <snapshot> true
+
+This will set the protected flag on the snapshot and prevent pruning or manual
+deletion of this snapshot untilt he flag is removed again with:
+
+.. code-block:: console
+
+  # proxmox-backup-client snapshot protected update <snapshot> false
+
+When a group is with a protected snapshot is deleted, only the non-protected
+ones are removed and the group will remain.
+
+.. note:: This flag will not be synced when using pull or sync jobs. If you
+   want to protect a synced snapshot, you have to manually to this again on
+   the target :ref:`backup server.
 
 .. _client_garbage-collection:
 
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v3 01/13] pbs-datastore: add protection info to BackupInfo

[Wolfgang Bumiller]

minor nitpicking below

On Wed, Oct 27, 2021 at 01:22:26PM +0200, Dominik Csapak wrote:
> and add necessary helper functions (protected_file/is_protected)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/backup_info.rs | 20 ++++++++++++++++++--
>  tests/prune.rs                   |  2 +-
>  2 files changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
> index 9d1ab991..b94b9779 100644
> --- a/pbs-datastore/src/backup_info.rs
> +++ b/pbs-datastore/src/backup_info.rs
> @@ -92,7 +92,9 @@ impl BackupGroup {
>                      BackupDir::with_rfc3339(&self.backup_type, &self.backup_id, backup_time)?;
>                  let files = list_backup_files(l2_fd, backup_time)?;
>  
> -                list.push(BackupInfo { backup_dir, files });
> +                let protected = backup_dir.is_protected(base_path.to_owned());
> +
> +                list.push(BackupInfo { backup_dir, files, protected });
>  
>                  Ok(())
>              },
> @@ -253,6 +255,17 @@ impl BackupDir {
>          relative_path
>      }
>  
> +    pub fn protected_file(&self, mut path: PathBuf) -> PathBuf {

Please also name the parameter `base_path`.

It's already weird because it's `BackupDir::relative_path`
(explicit 'relative' in the name) vs `BackupGroup::group_path` (which is
also relative, but doesn't explicitly say that it is).

I'm also not convinced this should take a `PathBuf`, I find `&Path` (or
even Cow<Path> or AsRef<Path>) makes a lot more sense here, and all our
callers will be cloning anyway.
(and in the remaining patches, the only places where the path is already
an owned PathBuf should in fact not actually have an owned PathBuf in
the first place, so I'd rather have that cleaned up where it comes
from...)

> +        path.push(self.relative_path());
> +        path.push(".protected");
> +        path
> +    }
> +
> +    pub fn is_protected(&self, base_path: PathBuf) -> bool {

same

> +        let path = self.protected_file(base_path);
> +        path.exists()
> +    }
> +
>      pub fn backup_time_to_string(backup_time: i64) -> Result<String, Error> {
>          // fixme: can this fail? (avoid unwrap)
>          Ok(proxmox_time::epoch_to_rfc3339_utc(backup_time)?)
> @@ -293,6 +306,8 @@ pub struct BackupInfo {
>      pub backup_dir: BackupDir,
>      /// List of data files
>      pub files: Vec<String>,
> +    /// Protection Status
> +    pub protected: bool,
>  }
>  
>  impl BackupInfo {

Re: [pbs-devel] [PATCH proxmox-backup v3 03/13] add protected info of snapshots to api and task logs

[Wolfgang Bumiller]

more minor nitpicking

On Wed, Oct 27, 2021 at 01:22:28PM +0200, Dominik Csapak wrote:
> adds the info that a snapshot is protected to:
> * snapshot list
> * manual pruning (also dry-run)
> * prune jobs
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs |  2 ++
>  pbs-datastore/src/prune.rs     | 40 +++++++++++++++++++++++++++-------
>  src/api2/admin/datastore.rs    | 15 ++++++++-----
>  src/server/prune_job.rs        |  6 ++---
>  tests/prune.rs                 |  4 ++--
>  5 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 462081e4..77c1258f 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -390,6 +390,8 @@ pub struct SnapshotListItem {
>      /// The owner of the snapshots group
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub owner: Option<Authid>,
> +    /// Protection from prunes
> +    pub protected: bool,
>  }
>  
>  #[api(
> diff --git a/pbs-datastore/src/prune.rs b/pbs-datastore/src/prune.rs
> index 0eaa8acd..e66ed408 100644
> --- a/pbs-datastore/src/prune.rs
> +++ b/pbs-datastore/src/prune.rs
> @@ -7,7 +7,30 @@ use pbs_api_types::PruneOptions;
>  
>  use super::BackupInfo;
>  
> -enum PruneMark { Protected, Keep, KeepPartial, Remove }
> +#[derive(Clone, Copy, PartialEq, Eq)]
> +pub enum PruneMark { Protected, Keep, KeepPartial, Remove }
> +
> +impl PruneMark {
> +    pub fn keep(&self) -> bool {

It's a tiny `Copy` type, you can drop the `&` above and `*` below in
boht methods.

> +        *self != PruneMark::Remove
> +    }
> +
> +    pub fn protected(&self) -> bool {
> +        *self == PruneMark::Protected
> +    }
> +}
> +
> +impl std::fmt::Display for PruneMark {
> +    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
> +        let txt = match self {

    f.write_str(match self {

> +            PruneMark::Protected => "protected",
> +            PruneMark::Keep => "keep",
> +            PruneMark::KeepPartial => "keep-partial",
> +            PruneMark::Remove => "remove",
> +        };

    })
drop the `write!` usage

> +        write!(f, "{}", txt)
> +    }
> +}
>  
>  fn mark_selections<F: Fn(&BackupInfo) -> Result<String, Error>> (
>      mark: &mut HashMap<PathBuf, PruneMark>,
> @@ -125,7 +148,7 @@ pub fn cli_options_string(options: &PruneOptions) -> String {
>  pub fn compute_prune_info(
>      mut list: Vec<BackupInfo>,
>      options: &PruneOptions,
> -) -> Result<Vec<(BackupInfo, bool)>, Error> {
> +) -> Result<Vec<(BackupInfo, PruneMark)>, Error> {
>  
>      let mut mark = HashMap::new();
>  
> @@ -173,15 +196,16 @@ pub fn compute_prune_info(
>          })?;
>      }
>  
> -    let prune_info: Vec<(BackupInfo, bool)> = list.into_iter()
> +    let prune_info: Vec<(BackupInfo, PruneMark)> = list.into_iter()
>          .map(|info| {
>              let backup_id = info.backup_dir.relative_path();
> -            let keep = match mark.get(&backup_id) {
> -                Some(PruneMark::Keep) => true,
> -                Some(PruneMark::KeepPartial) => true,
> -               _ => false,
> +            let mark = if info.protected {
> +                PruneMark::Protected
> +            } else {
> +                *mark.get(&backup_id).unwrap_or(&PruneMark::Remove)

(could use `.copied()` instead of `*` and `&`)

>              };
> -            (info, keep)
> +
> +            (info, mark)
>          })
>          .collect();
>  

Re: [pbs-devel] [PATCH proxmox-backup v3 08/13] api2/admin/datastore: add get/set_protection

[Wolfgang Bumiller]

1 error handling issue, and another nit

On Wed, Oct 27, 2021 at 01:22:33PM +0200, Dominik Csapak wrote:
> for gettin/setting the protected flag for snapshots (akin to notes)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 24 +++++++++
>  src/api2/admin/datastore.rs    | 92 ++++++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index e32887de..5049cb3d 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -858,6 +858,30 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    /// Updates the protection status of the specified snapshot.
> +    pub fn update_protection(
> +        &self,
> +        backup_dir: &BackupDir,
> +        protection: bool
> +    ) -> Result<(), Error> {
> +        let full_path = self.snapshot_path(backup_dir);
> +
> +        let _guard = lock_dir_noblock(&full_path, "snapshot", "possibly running or in use")?;
> +
> +        let protected_path = backup_dir.protected_file(self.base_path());
> +        if protection {
> +            std::fs::File::create(protected_path)
> +                .map_err(|err| format_err!("could not create protection file: {}", err))?;

Should we not also ignore `ErrorKind::AlreadyExists` here?
After all, we ignore `NotFound` when removing it.

> +        } else if let Err(err) = std::fs::remove_file(protected_path) {
> +            // ignore error for non-existing file
> +            if err.kind() != std::io::ErrorKind::NotFound {
> +                bail!("could not remove protection file: {}", err);
> +            }
> +        }
> +
> +        Ok(())
> +    }
> +
>      pub fn verify_new(&self) -> bool {
>          self.verify_new
>      }
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index fe962e4e..b9bcde12 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1750,6 +1750,92 @@ pub fn set_notes(
>      Ok(())
>  }
>  
> +#[api(
> +    input: {
> +        properties: {
> +            store: {
> +                schema: DATASTORE_SCHEMA,
> +            },
> +            "backup-type": {
> +                schema: BACKUP_TYPE_SCHEMA,
> +            },
> +            "backup-id": {
> +                schema: BACKUP_ID_SCHEMA,
> +            },
> +            "backup-time": {
> +                schema: BACKUP_TIME_SCHEMA,
> +            },
> +        },
> +    },
> +    access: {
> +        permission: &Permission::Privilege(&["datastore", "{store}"], PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP, true),
> +    },
> +)]
> +/// Query protection for a specific backup
> +pub fn get_protection(
> +    store: String,
> +    backup_type: String,
> +    backup_id: String,
> +    backup_time: i64,
> +    rpcenv: &mut dyn RpcEnvironment,
> +) -> Result<bool, Error> {
> +    let datastore = DataStore::lookup_datastore(&store)?;
> +
> +    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
> +    let backup_dir = BackupDir::new(backup_type, backup_id, backup_time)?;
> +
> +    check_priv_or_backup_owner(&datastore, backup_dir.group(), &auth_id, PRIV_DATASTORE_AUDIT)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +
> +    Ok(protected_path.exists())

since you don't need the path, use `.is_protected()`

> +}
> +

Re: [pbs-devel] [PATCH proxmox-backup v3 09/13] proxmox-backup-client: add 'protected' commands

[Wolfgang Bumiller]

just an FYI about all the explicit `Null` values we have by now...
We *can* just return `()` ;-)

On Wed, Oct 27, 2021 at 01:22:34PM +0200, Dominik Csapak wrote:
> includes 'update' and 'show' similar to the notes commands
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  proxmox-backup-client/src/snapshot.rs | 113 ++++++++++++++++++++++++++
>  1 file changed, 113 insertions(+)
> 
> diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
> index 6b563d79..46b1db09 100644
> --- a/proxmox-backup-client/src/snapshot.rs
> +++ b/proxmox-backup-client/src/snapshot.rs
> @@ -358,6 +358,118 @@ async fn update_notes(param: Value) -> Result<Value, Error> {
>      Ok(Value::Null)
>  }
>  
> +#[api(
(...)
> +)]
> +/// Show protection status of the specified snapshot
> +async fn show_protection(param: Value) -> Result<Value, Error> {
 ^ `-> Result<(), Error>`
(...)
 v `Ok(())`
> +    Ok(Value::Null)
> +}
> +
> +#[api(
(...)
> +)]
> +/// Update Protection Status of a snapshot
> +async fn update_protection(protected: bool, param: Value) -> Result<Value, Error> {
 ^ `-> Result<(), Error>`
(...)
 v `Ok(())`
> +    Ok(Value::Null)
> +}

[pbs-devel] applied series: [PATCH proxmox-backup v3 00/13] add 'protected' setting for snapshots

[Wolfgang Bumiller]

applied series

The mentioned error handling thing is a non-issue as File::create
doesn't use O_EXCL anyway, and the cleanup can be done as follow-up.

On Wed, Oct 27, 2021 at 01:22:25PM +0200, Dominik Csapak wrote:
> this v3 is mostly to provide a current rebase on master, since some of
> the code was moved
> 
> add the means to 'protect' a snapshot against pruning and removal by
> adding a file '.protected' in the snapshot folder
> 
> in the v2, i opted against syncing the protected flag for two reasons:
> * it gives the admin the choice on each datastore if he wants to keep
>   the snapshot or not(if we sync it, we would have to sync it everytime?)
> * it is much simpler to implement
> 
> changes from v2:
> * rebase on master
> * increase width of action column (so that all icons are visible)
> 
> changes from v1:
> * rebase on master
> * return the PruneMark enum instead of two booleans in compute_prune_info
> * lock the snapshot when setting/removig the flag
> * don't bail if the file does not exist on removal
> * add a 'protected show' command to the client
> * add a docs section to explain the protected flag
> 
> changes from rfc:
> * added gui parts
> * added tests
> * fixed a bug (that the test uncovered)
> * add pbs client command
> * prevent removal (also during sync)
> 
> Dominik Csapak (13):
>   pbs-datastore: add protection info to BackupInfo
>   pbs-datastore: skip protected backups in pruning
>   add protected info of snapshots to api and task logs
>   tests/prune: add tests for protecteded backups
>   backup/datastore: prevent protected snapshots to be removed
>   pull_store/group: dont try remove locally protected snapshots
>   api2: datastore/delete_group: throw error for partially removed group
>   api2/admin/datastore: add get/set_protection
>   proxmox-backup-client: add 'protected' commands
>   ui: PruneInputPanel: add keepReason 'protected' for protected backups
>   ui: add protected icon to snapshots
>   fix #3602: ui: datastore/Content: add action to set protection status
>   docs: add info about protection flag to client docs
> 
>  docs/backup-client.rst                |  19 +++++
>  pbs-api-types/src/datastore.rs        |   2 +
>  pbs-datastore/src/backup_info.rs      |  20 ++++-
>  pbs-datastore/src/datastore.rs        |  61 +++++++++++---
>  pbs-datastore/src/prune.rs            |  44 ++++++++--
>  proxmox-backup-client/src/snapshot.rs | 113 ++++++++++++++++++++++++++
>  src/api2/admin/datastore.rs           | 111 +++++++++++++++++++++++--
>  src/server/prune_job.rs               |   6 +-
>  src/server/pull.rs                    |  20 ++++-
>  tests/prune.rs                        |  44 +++++++++-
>  www/datastore/Content.js              |  77 +++++++++++++++++-
>  www/datastore/Prune.js                |   4 +
>  12 files changed, 483 insertions(+), 38 deletions(-)
> 
> -- 
> 2.30.2