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

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

[Dominik Csapak]

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

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 (12):
  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 update command'
  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

 pbs-api-types/src/lib.rs              |   2 +
 pbs-datastore/src/backup_info.rs      |  20 ++++-
 pbs-datastore/src/prune.rs            |  17 ++--
 proxmox-backup-client/src/snapshot.rs |  51 +++++++++++
 src/api2/admin/datastore.rs           | 116 +++++++++++++++++++++++++-
 src/backup/datastore.rs               |  37 +++++---
 src/server/prune_job.rs               |   4 +-
 src/server/pull.rs                    |  19 ++++-
 tests/prune.rs                        |  40 ++++++++-
 www/datastore/Content.js              |  75 +++++++++++++++++
 www/datastore/Prune.js                |   4 +
 11 files changed, 355 insertions(+), 30 deletions(-)

-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 01/12] 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                   |  4 ++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index f77098ee..032fad8b 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)
         proxmox::tools::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 3297b031..5c61cbf9 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -17,7 +17,7 @@ fn get_prune_list(
 
     prune_info
         .iter()
-        .filter_map(|(info, keep)| {
+        .filter_map(|(info, keep, _)| {
             if *keep != return_kept {
                 None
             } else {
@@ -40,7 +40,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 02/12] 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 4605e26f..6b76e9d7 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -17,7 +17,7 @@ use pbs_api_types::{
 
 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>,
@@ -40,6 +40,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 03/12] 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/lib.rs    |  2 ++
 pbs-datastore/src/prune.rs  | 11 +++++++----
 src/api2/admin/datastore.rs | 11 ++++++++---
 src/server/prune_job.rs     |  4 ++--
 4 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 427b2d9f..eba68ce5 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -411,6 +411,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 6b76e9d7..6b9c2bbb 100644
--- a/pbs-datastore/src/prune.rs
+++ b/pbs-datastore/src/prune.rs
@@ -225,7 +225,7 @@ impl PruneOptions {
 pub fn compute_prune_info(
     mut list: Vec<BackupInfo>,
     options: &PruneOptions,
-) -> Result<Vec<(BackupInfo, bool)>, Error> {
+) -> Result<Vec<(BackupInfo, bool, bool)>, Error> {
 
     let mut mark = HashMap::new();
 
@@ -273,15 +273,18 @@ pub fn compute_prune_info(
         })?;
     }
 
-    let prune_info: Vec<(BackupInfo, bool)> = list.into_iter()
+    let prune_info: Vec<(BackupInfo, bool, bool)> = list.into_iter()
         .map(|info| {
             let backup_id = info.backup_dir.relative_path();
+            let protected = info.protected;
             let keep = match mark.get(&backup_id) {
                 Some(PruneMark::Keep) => true,
                 Some(PruneMark::KeepPartial) => true,
-               _ => false,
+                Some(PruneMark::Protected) => true,
+               _ => if protected { true } else { false },
             };
-            (info, keep)
+
+            (info, keep, protected)
         })
         .collect();
 
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 5470de73..f49cdbee 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -444,6 +444,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)) => {
@@ -482,6 +483,7 @@ pub fn list_snapshots (
                     files,
                     size,
                     owner,
+                    protected,
                 }
             },
             Err(err) => {
@@ -506,6 +508,7 @@ pub fn list_snapshots (
                     files,
                     size: None,
                     owner,
+                    protected,
                 }
             },
         }
@@ -849,7 +852,7 @@ pub fn prune(
     let keep_all = !prune_options.keeps_something();
 
     if dry_run {
-        for (info, mut keep) in prune_info {
+        for (info, mut keep, protected) in prune_info {
             if keep_all { keep = true; }
 
             let backup_time = info.backup_dir.backup_time();
@@ -860,6 +863,7 @@ pub fn prune(
                 "backup-id": group.backup_id(),
                 "backup-time": backup_time,
                 "keep": keep,
+                "protected": protected,
             }));
         }
         return Ok(json!(prune_result));
@@ -877,7 +881,7 @@ pub fn prune(
                             store, backup_type, backup_id));
     }
 
-    for (info, mut keep) in prune_info {
+    for (info, mut keep, protected) in prune_info {
         if keep_all { keep = true; }
 
         let backup_time = info.backup_dir.backup_time();
@@ -890,7 +894,7 @@ pub fn prune(
             group.backup_type(),
             group.backup_id(),
             timestamp,
-            if keep { "keep" } else { "remove" },
+            if keep { if protected { "keep (protected)" } else { "keep" } } else { "remove" },
         );
 
         worker.log(msg);
@@ -900,6 +904,7 @@ pub fn prune(
             "backup-id": group.backup_id(),
             "backup-time": backup_time,
             "keep": keep,
+            "protected": protected,
         }));
 
         if !(dry_run || keep) {
diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
index 2ea8b713..bfe2bd9e 100644
--- a/src/server/prune_job.rs
+++ b/src/server/prune_job.rs
@@ -66,12 +66,12 @@ pub fn prune_datastore(
             group.backup_id()
         );
 
-        for (info, mut keep) in prune_info {
+        for (info, mut keep, protected) in prune_info {
             if keep_all { keep = true; }
             task_log!(
                 worker,
                 "{} {}/{}/{}",
-                if keep { "keep" } else { "remove" },
+                if keep { if protected { "keep (protected)" } else { "keep" } } else { "remove" },
                 group.backup_type(),
                 group.backup_id(),
                 info.backup_dir.backup_time_string()
-- 
2.30.2

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

[Fabian Grünbichler]

On September 6, 2021 12:57 pm, 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/lib.rs    |  2 ++
>  pbs-datastore/src/prune.rs  | 11 +++++++----
>  src/api2/admin/datastore.rs | 11 ++++++++---
>  src/server/prune_job.rs     |  4 ++--
>  4 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
> index 427b2d9f..eba68ce5 100644
> --- a/pbs-api-types/src/lib.rs
> +++ b/pbs-api-types/src/lib.rs
> @@ -411,6 +411,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 6b76e9d7..6b9c2bbb 100644
> --- a/pbs-datastore/src/prune.rs
> +++ b/pbs-datastore/src/prune.rs
> @@ -225,7 +225,7 @@ impl PruneOptions {
>  pub fn compute_prune_info(
>      mut list: Vec<BackupInfo>,
>      options: &PruneOptions,
> -) -> Result<Vec<(BackupInfo, bool)>, Error> {
> +) -> Result<Vec<(BackupInfo, bool, bool)>, Error> {

starting to get into territory where a PruneInfo type might make 
sense..

alternatively, maybe the PruneMark enum could be re-used instead of two 
bools? with serializing as

keep
keep-partial
protected
remove

it would also work as return type instead of the conversion into "keep 
(protected)" below, IMHO that protected implies keep is a given as that is the 
only reason to set protected in the first place ^^

>  
>      let mut mark = HashMap::new();
>  
> @@ -273,15 +273,18 @@ pub fn compute_prune_info(
>          })?;
>      }
>  
> -    let prune_info: Vec<(BackupInfo, bool)> = list.into_iter()
> +    let prune_info: Vec<(BackupInfo, bool, bool)> = list.into_iter()
>          .map(|info| {
>              let backup_id = info.backup_dir.relative_path();
> +            let protected = info.protected;
>              let keep = match mark.get(&backup_id) {
>                  Some(PruneMark::Keep) => true,
>                  Some(PruneMark::KeepPartial) => true,
> -               _ => false,
> +                Some(PruneMark::Protected) => true,
> +               _ => if protected { true } else { false },
>              };
> -            (info, keep)
> +
> +            (info, keep, protected)
>          })
>          .collect();
>  
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 5470de73..f49cdbee 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -444,6 +444,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)) => {
> @@ -482,6 +483,7 @@ pub fn list_snapshots (
>                      files,
>                      size,
>                      owner,
> +                    protected,
>                  }
>              },
>              Err(err) => {
> @@ -506,6 +508,7 @@ pub fn list_snapshots (
>                      files,
>                      size: None,
>                      owner,
> +                    protected,
>                  }
>              },
>          }
> @@ -849,7 +852,7 @@ pub fn prune(
>      let keep_all = !prune_options.keeps_something();
>  
>      if dry_run {
> -        for (info, mut keep) in prune_info {
> +        for (info, mut keep, protected) in prune_info {
>              if keep_all { keep = true; }
>  
>              let backup_time = info.backup_dir.backup_time();
> @@ -860,6 +863,7 @@ pub fn prune(
>                  "backup-id": group.backup_id(),
>                  "backup-time": backup_time,
>                  "keep": keep,
> +                "protected": protected,
>              }));
>          }
>          return Ok(json!(prune_result));
> @@ -877,7 +881,7 @@ pub fn prune(
>                              store, backup_type, backup_id));
>      }
>  
> -    for (info, mut keep) in prune_info {
> +    for (info, mut keep, protected) in prune_info {
>          if keep_all { keep = true; }
>  
>          let backup_time = info.backup_dir.backup_time();
> @@ -890,7 +894,7 @@ pub fn prune(
>              group.backup_type(),
>              group.backup_id(),
>              timestamp,
> -            if keep { "keep" } else { "remove" },
> +            if keep { if protected { "keep (protected)" } else { "keep" } } else { "remove" },
>          );
>  
>          worker.log(msg);
> @@ -900,6 +904,7 @@ pub fn prune(
>              "backup-id": group.backup_id(),
>              "backup-time": backup_time,
>              "keep": keep,
> +            "protected": protected,
>          }));
>  
>          if !(dry_run || keep) {
> diff --git a/src/server/prune_job.rs b/src/server/prune_job.rs
> index 2ea8b713..bfe2bd9e 100644
> --- a/src/server/prune_job.rs
> +++ b/src/server/prune_job.rs
> @@ -66,12 +66,12 @@ pub fn prune_datastore(
>              group.backup_id()
>          );
>  
> -        for (info, mut keep) in prune_info {
> +        for (info, mut keep, protected) in prune_info {
>              if keep_all { keep = true; }
>              task_log!(
>                  worker,
>                  "{} {}/{}/{}",
> -                if keep { "keep" } else { "remove" },
> +                if keep { if protected { "keep (protected)" } else { "keep" } } else { "remove" },
>                  group.backup_type(),
>                  group.backup_id(),
>                  info.backup_dir.backup_time_string()
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

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

[Dominik Csapak]

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

diff --git a/tests/prune.rs b/tests/prune.rs
index 5c61cbf9..1e72a74f 100644
--- a/tests/prune.rs
+++ b/tests/prune.rs
@@ -43,6 +43,42 @@ 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 options = PruneOptions::new().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 options = PruneOptions::new().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 05/12] 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>
---
 src/backup/datastore.rs | 37 +++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 12 deletions(-)

diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
index 7986c328..03f0a744 100644
--- a/src/backup/datastore.rs
+++ b/src/backup/datastore.rs
@@ -270,8 +270,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);
 
@@ -279,22 +280,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
@@ -308,6 +317,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

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

[Fabian Grünbichler]

On September 6, 2021 12:57 pm, Dominik Csapak wrote:
> by throwing an error for remove_backup_dir, and skipping for
> remove_backup_group
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/backup/datastore.rs | 37 +++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 12 deletions(-)
> 
> diff --git a/src/backup/datastore.rs b/src/backup/datastore.rs
> index 7986c328..03f0a744 100644
> --- a/src/backup/datastore.rs
> +++ b/src/backup/datastore.rs
> @@ -270,8 +270,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);
>  
> @@ -279,22 +280,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())? {

could also first iterate and check for protected status, and skip 
removal of any snapshot entirely if we find a protected snapshot?

it would still require the re-check in case the protection status 
changed in the meantime, since that is not guarded by any lock atm, and 
even if it were, it would be a snapshot level lock, and we can't hold 
all of those for the whole group here ;)

alternatively (since the remove group call bails anyway if a protected 
snapshot was skipped), we could bail directly here when encountering the 
first protected snapshot to simplify matters a bit?

> +            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
> @@ -308,6 +317,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 mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-backup 06/12] 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 | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 5214a218..910ba03b 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -609,6 +609,13 @@ pub async fn pull_group(
             if remote_snapshots.contains(&backup_time) {
                 continue;
             }
+            if info.backup_dir.is_protected(tgt_store.base_path()) {
+                worker.log(format!(
+                        "don't delete vanished snapshot {:?} (protected)",
+                        info.backup_dir.relative_path()
+                ));
+                continue;
+            }
             worker.log(format!(
                 "delete vanished snapshot {:?}",
                 info.backup_dir.relative_path()
@@ -722,9 +729,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) {
-                    worker.log(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

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

[Fabian Grünbichler]

On September 6, 2021 12:57 pm, Dominik Csapak wrote:
> and log if a vanished groups could not be completely deleted if it
> contains protected snapshots

I am not yet 100% if not syncing protected is the right way to go, it's 
messy and potentially a pitfall either way.. should be properly 
documented in any case, and maybe syncing a protected snapshot could 
indicate that the protection flag was not carried over?

> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/server/pull.rs | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index 5214a218..910ba03b 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -609,6 +609,13 @@ pub async fn pull_group(
>              if remote_snapshots.contains(&backup_time) {
>                  continue;
>              }
> +            if info.backup_dir.is_protected(tgt_store.base_path()) {
> +                worker.log(format!(
> +                        "don't delete vanished snapshot {:?} (protected)",
> +                        info.backup_dir.relative_path()
> +                ));
> +                continue;
> +            }
>              worker.log(format!(
>                  "delete vanished snapshot {:?}",
>                  info.backup_dir.relative_path()
> @@ -722,9 +729,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) {
> -                    worker.log(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 mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-backup 07/12] 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 f49cdbee..f88fd105 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -275,7 +275,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 08/12] 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>
---
 src/api2/admin/datastore.rs | 101 ++++++++++++++++++++++++++++++++++++
 1 file changed, 101 insertions(+)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index f88fd105..572c65a9 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1751,6 +1751,101 @@ 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)?;
+
+    let protected_path = backup_dir.protected_file(datastore.base_path());
+    if protected {
+        std::fs::File::create(protected_path)
+            .map_err(|err| format_err!("could not create protection file: {}", err))?;
+    } else {
+        std::fs::remove_file(protected_path)
+            .map_err(|err| format_err!("could not remove protection file: {}", err))?;
+    }
+
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -1899,6 +1994,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

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

[Fabian Ebner]

Am 06.09.21 um 12:57 schrieb Dominik Csapak:
> for gettin/setting the protected flag for snapshots (akin to notes)
> 

For notes, there is a 'show' command, but not for protected. Is there an 
other way to query the current protection of a single snapshot via 
proxmox-backup-client?

I'm working on bug #3307, which is the same feature for PVE and for 
integration with the PBS plugin it would be nicer to query only the 
single snapshot. Or should I list the whole group and extract the info 
from there instead?

> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>   src/api2/admin/datastore.rs | 101 ++++++++++++++++++++++++++++++++++++
>   1 file changed, 101 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f88fd105..572c65a9 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1751,6 +1751,101 @@ 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)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +    if protected {
> +        std::fs::File::create(protected_path)
> +            .map_err(|err| format_err!("could not create protection file: {}", err))?;
> +    } else {
> +        std::fs::remove_file(protected_path)
> +            .map_err(|err| format_err!("could not remove protection file: {}", err))?;
> +    }
> +
> +    Ok(())
> +}
> +
>   #[api(
>       input: {
>           properties: {
> @@ -1899,6 +1994,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()
> 

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

[Dominik Csapak]

On 9/10/21 14:43, Fabian Ebner wrote:
> Am 06.09.21 um 12:57 schrieb Dominik Csapak:
>> for gettin/setting the protected flag for snapshots (akin to notes)
>>
> 
> For notes, there is a 'show' command, but not for protected. Is there an 
> other way to query the current protection of a single snapshot via 
> proxmox-backup-client?
> 
> I'm working on bug #3307, which is the same feature for PVE and for 
> integration with the PBS plugin it would be nicer to query only the 
> single snapshot. Or should I list the whole group and extract the info 
> from there instead?
> 

well, i did not think we would need a 'show' command yet for a single 
snapshot, but if you'll need it, i can either add it later
or in a v2 (if the series needs some changes)

the api already contains the 'get' method anyway

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

[Fabian Ebner]

Am 13.09.21 um 10:34 schrieb Dominik Csapak:
> On 9/10/21 14:43, Fabian Ebner wrote:
>> Am 06.09.21 um 12:57 schrieb Dominik Csapak:
>>> for gettin/setting the protected flag for snapshots (akin to notes)
>>>
>>
>> For notes, there is a 'show' command, but not for protected. Is there 
>> an other way to query the current protection of a single snapshot via 
>> proxmox-backup-client?
>>
>> I'm working on bug #3307, which is the same feature for PVE and for 
>> integration with the PBS plugin it would be nicer to query only the 
>> single snapshot. Or should I list the whole group and extract the info 
>> from there instead?
>>
> 
> well, i did not think we would need a 'show' command yet for a single 
> snapshot, but if you'll need it, i can either add it later
> or in a v2 (if the series needs some changes)
> 
> the api already contains the 'get' method anyway
> 

Using the API instead of the client also makes distinguishing between 
"not supported" (i.e. 404 status code) and some other error easy. To do 
the same with proxmox-backup-client, it would be necessary to match the 
error message, but that would require refactoring run_client_cmd (in 
PBSPlugin.pm) AFAICT.

Anyways, during testing I noticed that the GET call does not fail if the 
requested snapshot doesn't even exist.

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

[Fabian Ebner]

Am 16.09.21 um 12:16 schrieb Fabian Ebner:
> Am 13.09.21 um 10:34 schrieb Dominik Csapak:
>> On 9/10/21 14:43, Fabian Ebner wrote:
>>> Am 06.09.21 um 12:57 schrieb Dominik Csapak:
>>>> for gettin/setting the protected flag for snapshots (akin to notes)
>>>>
>>>
>>> For notes, there is a 'show' command, but not for protected. Is there 
>>> an other way to query the current protection of a single snapshot via 
>>> proxmox-backup-client?
>>>
>>> I'm working on bug #3307, which is the same feature for PVE and for 
>>> integration with the PBS plugin it would be nicer to query only the 
>>> single snapshot. Or should I list the whole group and extract the 
>>> info from there instead?
>>>
>>
>> well, i did not think we would need a 'show' command yet for a single 
>> snapshot, but if you'll need it, i can either add it later
>> or in a v2 (if the series needs some changes)
>>
>> the api already contains the 'get' method anyway
>>
> 
> Using the API instead of the client also makes distinguishing between 
> "not supported" (i.e. 404 status code) and some other error easy. To do 
> the same with proxmox-backup-client, it would be necessary to match the 
> error message, but that would require refactoring run_client_cmd (in 
> PBSPlugin.pm) AFAICT.
> 
> Anyways, during testing I noticed that the GET call does not fail if the 
> requested snapshot doesn't even exist.
> 

Another small issue is that setting protected to false for a 
non-protected backup will fail, because the API call tries to remove a 
non-existent file then.

> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 

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

[Fabian Grünbichler]

On September 6, 2021 12:57 pm, Dominik Csapak wrote:
> for gettin/setting the protected flag for snapshots (akin to notes)
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  src/api2/admin/datastore.rs | 101 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 101 insertions(+)
> 
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index f88fd105..572c65a9 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1751,6 +1751,101 @@ 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.",

protected is already part of the CLI schema and API path, maybe simply 
'value' or 'enabled'?

> +            },
> +        },
> +    },
> +    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)?;
> +
> +    let protected_path = backup_dir.protected_file(datastore.base_path());
> +    if protected {
> +        std::fs::File::create(protected_path)
> +            .map_err(|err| format_err!("could not create protection file: {}", err))?;
> +    } else {
> +        std::fs::remove_file(protected_path)
> +            .map_err(|err| format_err!("could not remove protection file: {}", err))?;
> +    }

this is modifying a file related to the snapdir, shouldn't this have 
some sort of locking? to protect against other modifications of the 
protection flag, but also other operations that might make decisions 
based on the flag? haven't fully thought it through, but it seems to be 
there might be something missing here..

e.g., setting/removing notes is guarded by the manifest update 
mechanism, which does the locking.

> +
> +    Ok(())
> +}
> +
>  #[api(
>      input: {
>          properties: {
> @@ -1899,6 +1994,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 mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
> 

[pbs-devel] [PATCH proxmox-backup 09/12] proxmox-backup-client: add 'protected update command'

[Dominik Csapak]

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

diff --git a/proxmox-backup-client/src/snapshot.rs b/proxmox-backup-client/src/snapshot.rs
index b63e84a6..fbf91f1b 100644
--- a/proxmox-backup-client/src/snapshot.rs
+++ b/proxmox-backup-client/src/snapshot.rs
@@ -358,6 +358,56 @@ 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.",
+            },
+            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(
+            "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 +427,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 10/12] 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 11/12] 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 57693785..fbc3e12f 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -640,6 +640,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 12/12] 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 | 68 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 68 insertions(+)

diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index fbc3e12f..6bb29c2a 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -491,6 +491,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();
@@ -716,6 +778,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

Re: [pbs-devel] [PATCH proxmox-backup 00/12] add 'protected' setting for snapshots

[Fabian Grünbichler]

general looks okay, needs a rebase (so not tested, just read ;))

a few smaller notes on indivual patches

On September 6, 2021 12:57 pm, Dominik Csapak wrote:
> add the means to 'protect' a snapshot against pruning and removal by
> adding a file '.protected' in the snapshot folder
> 
> 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 (12):
>   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 update command'
>   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
> 
>  pbs-api-types/src/lib.rs              |   2 +
>  pbs-datastore/src/backup_info.rs      |  20 ++++-
>  pbs-datastore/src/prune.rs            |  17 ++--
>  proxmox-backup-client/src/snapshot.rs |  51 +++++++++++
>  src/api2/admin/datastore.rs           | 116 +++++++++++++++++++++++++-
>  src/backup/datastore.rs               |  37 +++++---
>  src/server/prune_job.rs               |   4 +-
>  src/server/pull.rs                    |  19 ++++-
>  tests/prune.rs                        |  40 ++++++++-
>  www/datastore/Content.js              |  75 +++++++++++++++++
>  www/datastore/Prune.js                |   4 +
>  11 files changed, 355 insertions(+), 30 deletions(-)
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>