[pbs-devel] [RFC v2 proxmox-backup 01/21] datastore/api: mark snapshots as trash on destroy

[Christian Ebner <c.ebner@proxmox.com>]

In order to implement the trash can functionality, mark snapshots
as trash instead of removing them by default. However, provide a
`skip-trash` flag to opt-out and destroy the snapshot including it's
contents immediately.

Trashed snapshots are marked by creating a `.trashed` marker file
inside the snapshot folder. Actual removal of the snapshot will be
deferred to the garbage collection task.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 66 ++++++++++++++++++--------------
 pbs-datastore/src/datastore.rs   |  2 +-
 src/api2/admin/datastore.rs      | 18 ++++++++-
 3 files changed, 55 insertions(+), 31 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index d4732fdd9..76bcd15f5 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -21,6 +21,7 @@ use crate::manifest::{BackupManifest, MANIFEST_LOCK_NAME};
 use crate::{DataBlob, DataStore};
 
 pub const DATASTORE_LOCKS_DIR: &str = "/run/proxmox-backup/locks";
+pub const TRASH_MARKER_FILENAME: &str = ".trashed";
 
 // TODO: Remove with PBS 5
 // Note: The `expect()` call here will only happen if we can neither confirm nor deny the existence
@@ -228,7 +229,7 @@ impl BackupGroup {
                 delete_stats.increment_protected_snapshots();
                 continue;
             }
-            snap.destroy(false)?;
+            snap.destroy(false, false)?;
             delete_stats.increment_removed_snapshots();
         }
 
@@ -575,7 +576,8 @@ impl BackupDir {
     /// Destroy the whole snapshot, bails if it's protected
     ///
     /// Setting `force` to true skips locking and thus ignores if the backup is currently in use.
-    pub fn destroy(&self, force: bool) -> Result<(), Error> {
+    /// Setting `skip_trash` to true will remove the snapshot instead of marking it as trash.
+    pub fn destroy(&self, force: bool, skip_trash: bool) -> Result<(), Error> {
         let (_guard, _manifest_guard);
         if !force {
             _guard = self
@@ -588,37 +590,45 @@ impl BackupDir {
             bail!("cannot remove protected snapshot"); // use special error type?
         }
 
-        let full_path = self.full_path();
-        log::info!("removing backup snapshot {:?}", full_path);
-        std::fs::remove_dir_all(&full_path).map_err(|err| {
-            format_err!("removing backup snapshot {:?} failed - {}", full_path, err,)
-        })?;
+        let mut full_path = self.full_path();
+        log::info!("removing backup snapshot {full_path:?}");
+        if skip_trash {
+            std::fs::remove_dir_all(&full_path).map_err(|err| {
+                format_err!("removing backup snapshot {full_path:?} failed - {err}")
+            })?;
+        } else {
+            full_path.push(TRASH_MARKER_FILENAME);
+            let _trash_file =
+                std::fs::File::create(full_path).context("failed to set trash file")?;
+        }
 
         // remove no longer needed lock files
         let _ = std::fs::remove_file(self.manifest_lock_path()); // ignore errors
         let _ = std::fs::remove_file(self.lock_path()); // ignore errors
 
-        let group = BackupGroup::from(self);
-        let guard = group.lock().with_context(|| {
-            format!("while checking if group '{group:?}' is empty during snapshot destruction")
-        });
-
-        // Only remove the group if all of the following is true:
-        //
-        // - we can lock it: if we can't lock the group, it is still in use (either by another
-        //   backup process or a parent caller (who needs to take care that empty groups are
-        //   removed themselves).
-        // - it is now empty: if the group isn't empty, removing it will fail (to avoid removing
-        //   backups that might still be used).
-        // - the new locking mechanism is used: if the old mechanism is used, a group removal here
-        //   could lead to a race condition.
-        //
-        // Do not error out, as we have already removed the snapshot, there is nothing a user could
-        // do to rectify the situation.
-        if guard.is_ok() && group.list_backups()?.is_empty() && !*OLD_LOCKING {
-            group.remove_group_dir()?;
-        } else if let Err(err) = guard {
-            log::debug!("{err:#}");
+        if skip_trash {
+            let group = BackupGroup::from(self);
+            let guard = group.lock().with_context(|| {
+                format!("while checking if group '{group:?}' is empty during snapshot destruction")
+            });
+
+            // Only remove the group if all of the following is true:
+            //
+            // - we can lock it: if we can't lock the group, it is still in use (either by another
+            //   backup process or a parent caller (who needs to take care that empty groups are
+            //   removed themselves).
+            // - it is now empty: if the group isn't empty, removing it will fail (to avoid removing
+            //   backups that might still be used).
+            // - the new locking mechanism is used: if the old mechanism is used, a group removal here
+            //   could lead to a race condition.
+            //
+            // Do not error out, as we have already removed the snapshot, there is nothing a user could
+            // do to rectify the situation.
+            if guard.is_ok() && group.list_backups()?.is_empty() && !*OLD_LOCKING {
+                group.remove_group_dir()?;
+            } else if let Err(err) = guard {
+                log::debug!("{err:#}");
+            }
         }
 
         Ok(())
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index cbf78ecb6..6df26e825 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -686,7 +686,7 @@ impl DataStore {
     ) -> Result<(), Error> {
         let backup_dir = self.backup_dir(ns.clone(), backup_dir.clone())?;
 
-        backup_dir.destroy(force)
+        backup_dir.destroy(force, true)
     }
 
     /// Returns the time of the last successful backup
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 392494488..aafd1bbd7 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -402,6 +402,12 @@ pub async fn list_snapshot_files(
                 type: pbs_api_types::BackupDir,
                 flatten: true,
             },
+            "skip-trash": {
+                type: bool,
+                optional: true,
+                default: false,
+                description: "Immediately remove the snapshot, not marking it as trash.",
+            },
         },
     },
     access: {
@@ -415,6 +421,7 @@ pub async fn delete_snapshot(
     store: String,
     ns: Option<BackupNamespace>,
     backup_dir: pbs_api_types::BackupDir,
+    skip_trash: bool,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -435,7 +442,7 @@ pub async fn delete_snapshot(
 
         let snapshot = datastore.backup_dir(ns, backup_dir)?;
 
-        snapshot.destroy(false)?;
+        snapshot.destroy(false, skip_trash)?;
 
         Ok(Value::Null)
     })
@@ -979,6 +986,12 @@ pub fn verify(
                 optional: true,
                 description: "Spins up an asynchronous task that does the work.",
             },
+            "skip-trash": {
+                type: bool,
+                optional: true,
+                default: false,
+                description: "Immediately remove the snapshot, not marking it as trash.",
+            },
         },
     },
     returns: pbs_api_types::ADMIN_DATASTORE_PRUNE_RETURN_TYPE,
@@ -995,6 +1008,7 @@ pub fn prune(
     keep_options: KeepOptions,
     store: String,
     ns: Option<BackupNamespace>,
+    skip_trash: bool,
     param: Value,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -1098,7 +1112,7 @@ pub fn prune(
             });
 
             if !keep {
-                if let Err(err) = backup_dir.destroy(false) {
+                if let Err(err) = backup_dir.destroy(false, skip_trash) {
                     warn!(
                         "failed to remove dir {:?}: {}",
                         backup_dir.relative_path(),
-- 
2.39.5



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