[pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Dominik Csapak]

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

sending as RFC because i am not sure about a couple things, and there
is no gui (yet):

* does it make sense to protect a snapshot this way? or would it be
  better to have a 'central' protected list somewhere?
  (sounds not right though..)
* would we want to protect also against manual removal ? or
 'remove-vanished' on sync?
* how should the ui look? do we want *another* button in the content
  view?
* would it make sense to specify this flag on backup creation too?

the finished series would fix #3602

Dominik Csapak (5):
  pbs-datastore: add protection info to BackupInfo
  pbs-datastore: skip protected backups in pruning
  add protected info of snapshots to api and task logs
  api2/admin/datastore: add get/set_protection
  ui: PruneInputPanel: add keepReason 'protected' for protected backups

 pbs-api-types/src/lib.rs         |   2 +
 pbs-datastore/src/backup_info.rs |  20 +++++-
 pbs-datastore/src/prune.rs       |  21 +++---
 src/api2/admin/datastore.rs      | 112 ++++++++++++++++++++++++++++++-
 src/server/prune_job.rs          |   4 +-
 www/datastore/Prune.js           |   4 ++
 6 files changed, 148 insertions(+), 15 deletions(-)

-- 
2.30.2

[pbs-devel] [RFC PATCH proxmox-backup 1/5] 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 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 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
-- 
2.30.2

[pbs-devel] [RFC PATCH proxmox-backup 2/5] 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] [RFC PATCH proxmox-backup 3/5] 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  | 15 ++++++++-------
 src/api2/admin/datastore.rs | 11 ++++++++---
 src/server/prune_job.rs     |  4 ++--
 4 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/pbs-api-types/src/lib.rs b/pbs-api-types/src/lib.rs
index 024181ee..4e581358 100644
--- a/pbs-api-types/src/lib.rs
+++ b/pbs-api-types/src/lib.rs
@@ -387,6 +387,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..3d4eb18e 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,16 @@ 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 keep = match mark.get(&backup_id) {
-                Some(PruneMark::Keep) => true,
-                Some(PruneMark::KeepPartial) => true,
-               _ => false,
+            let (keep, protected) = match mark.get(&backup_id) {
+                Some(PruneMark::Keep) => (true, false),
+                Some(PruneMark::KeepPartial) => (true, false),
+                Some(PruneMark::Protected) => (true, true),
+               _ => (false, 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] [RFC PATCH proxmox-backup 4/5] 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 f49cdbee..483bf63f 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -1749,6 +1749,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: {
@@ -1897,6 +1992,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] [RFC PATCH proxmox-backcup 5/5] 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

Re: [pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Thomas Lamprecht]

On 01.09.21 10:25, Dominik Csapak wrote:
> add the means to 'protect' a snapshot against pruning by
> adding a file '.protected' in the snapshot folder
> 
> sending as RFC because i am not sure about a couple things, and there
> is no gui (yet):
> 
> * does it make sense to protect a snapshot this way? or would it be
>   better to have a 'central' protected list somewhere?
>   (sounds not right though..)

I find the flag file quite ok, other approach could be to either make the index
immutable, which is probably not a good idea as the trouble-maker FS like NFS surely
make this a PITA. We could also safe it in the unprotected part of the manifest,
similar to how we do with comments, would reduce the file count and often we have
that info already anyway, but if not it may be a bit more expensive to check
than a stat on a separate file.


> * would we want to protect also against manual removal ? or
>  'remove-vanished' on sync?

Would make sense as long as we allow to "unprotect" it, that's how we do it for guests
in PVE too. IMO it's weird/unexpected to mark it protected and allow some API mechanisms
to still remove it.

> * how should the ui look? do we want *another* button in the content
>   view?

Why not? We explicitly choose the actions columns for that reason, and the row for
a specific backup has only verify + delete anyway.

Alternatives could be:
- add protection column and add an edit button there, similar to how I did the
  edit pencil for the comments
- make a more general edit button for the snapshots, for now this could open a
  dialogue for owner, protection and comment.

Just throwing out things here.

> * would it make sense to specify this flag on backup creation too?

IMO that could make sense, depends on how we see the actual use case, i.e., reasons
for setting it.
If I want to make an safety backup before starting a dist upgrade or the like in my
guest in PVE, I'd probably like to specify this flag immediately and not wait until
the backup is done then login into PBS and set it there.

> 
> the finished series would fix #3602

please add that to the commit subject, at least of the GUI and API patches, i.e., those
that actually enable this feature for (non-local-root) users.

> 
> Dominik Csapak (5):
>   pbs-datastore: add protection info to BackupInfo
>   pbs-datastore: skip protected backups in pruning
>   add protected info of snapshots to api and task logs
>   api2/admin/datastore: add get/set_protection
>   ui: PruneInputPanel: add keepReason 'protected' for protected backups
> 
>  pbs-api-types/src/lib.rs         |   2 +
>  pbs-datastore/src/backup_info.rs |  20 +++++-
>  pbs-datastore/src/prune.rs       |  21 +++---
>  src/api2/admin/datastore.rs      | 112 ++++++++++++++++++++++++++++++-
>  src/server/prune_job.rs          |   4 +-
>  www/datastore/Prune.js           |   4 ++
>  6 files changed, 148 insertions(+), 15 deletions(-)
> 

Re: [pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Dietmar Maurer]

> > * would we want to protect also against manual removal ? or
> >  'remove-vanished' on sync?
> 
> Would make sense as long as we allow to "unprotect" it, that's how we do it for guests
> in PVE too. IMO it's weird/unexpected to mark it protected and allow some API mechanisms
> to still remove it.

I would not consider the protected flag for syncs, i.e:
- do not sync the protected flag itself
- remove vanished backups even with protected flag set (to be in sync with source)

Re: [pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Thomas Lamprecht]

On 02.09.21 08:45, Dietmar Maurer wrote:
>>> * would we want to protect also against manual removal ? or
>>>  'remove-vanished' on sync?
>>
>> Would make sense as long as we allow to "unprotect" it, that's how we do it for guests
>> in PVE too. IMO it's weird/unexpected to mark it protected and allow some API mechanisms
>> to still remove it.
> 
> I would not consider the protected flag for syncs, i.e:
> - do not sync the protected flag itself

That I agree (and I did not meant to suggest otherwise).

> - remove vanished backups even with protected flag set (to be in sync with source)

That I do not agree, if I marked a snapshot explicitly protected, which
is a must for the situation to happen with the first point above in mind
(no syncing of the protection flag itself) then I'm pretty sure that I want
to keep that snapshot no matter what.

Re: [pbs-devel] [RFC PATCH proxmox-backup 0/5] add 'protected' setting for snapshots

[Dietmar Maurer]

> On 09/02/2021 8:48 AM Thomas Lamprecht <t.lamprecht@proxmox.com> wrote:
> 
>  
> On 02.09.21 08:45, Dietmar Maurer wrote:
> >>> * would we want to protect also against manual removal ? or
> >>>  'remove-vanished' on sync?
> >>
> >> Would make sense as long as we allow to "unprotect" it, that's how we do it for guests
> >> in PVE too. IMO it's weird/unexpected to mark it protected and allow some API mechanisms
> >> to still remove it.
> > 
> > I would not consider the protected flag for syncs, i.e:
> > - do not sync the protected flag itself
> 
> That I agree (and I did not meant to suggest otherwise).
> 
> > - remove vanished backups even with protected flag set (to be in sync with source)
> 
> That I do not agree, if I marked a snapshot explicitly protected, which
> is a must for the situation to happen with the first point above in mind
> (no syncing of the protection flag itself) then I'm pretty sure that I want
> to keep that snapshot no matter what.

Also OK for me.