namespace deletion: make destroying groups separate choice

And make that opt-in in the API endpoint, to avoid bad surprises by
default.

If not set we'll only prune empty namespaces.

Signed-off-by: Thomas Lamprecht <t.lamprecht@proxmox.com>

pbs-datastore/src/datastore.rs

@@ -441,22 +441,26 @@ impl DataStore {
         Ok(removed_all_groups)
     }
 
-    /// Remove a complete backup namespace including all it's, and child namespaces', groups.
+    /// Remove a complete backup namespace optionally including all it's, and child namespaces',
+    /// groups. If  `removed_groups` is false this only prunes empty namespaces.
     ///
-    /// Returns true if all groups were removed, and false if some were protected
+    /// Returns true if everything requested, and false if some groups were protected or if some
+    /// namespaces weren't empty even though all groups were deleted (race with new backup)
     pub fn remove_namespace_recursive(
         self: &Arc<Self>,
         ns: &BackupNamespace,
+        delete_groups: bool,
     ) -> Result<bool, Error> {
-        // FIXME: locking? The single groups/snapshots are already protected, so may not be
-        // necesarry (depends on what we all allow to do with namespaces)
-        log::info!("removing whole namespace recursively {}:/{ns}", self.name());
-
-        let mut removed_all_groups = true;
-        for ns in self.recursive_iter_backup_ns(ns.to_owned())? {
-            let removed_ns_groups = self.remove_namespace_groups(&ns?)?;
-
-            removed_all_groups = removed_all_groups && removed_ns_groups;
+        let store = self.name();
+        let mut removed_all_requested = true;
+        if delete_groups {
+            log::info!("removing whole namespace recursively below {store}:/{ns}",);
+            for ns in self.recursive_iter_backup_ns(ns.to_owned())? {
+                let removed_ns_groups = self.remove_namespace_groups(&ns?)?;
+                removed_all_requested = removed_all_requested && removed_ns_groups;
+            }
+        } else {
+            log::info!("pruning empty namespace recursively below {store}:/{ns}");
         }
 
         // now try to delete the actual namespaces, bottom up so that we can use safe rmdir that
@@ -477,13 +481,22 @@ impl DataStore {
 
             if !ns.is_root() {
                 match unlinkat(Some(base_fd), &ns.path(), UnlinkatFlags::RemoveDir) {
-                    Ok(()) => log::info!("removed namespace {ns}"),
-                    Err(err) => log::error!("failed to remove namespace {ns} - {err}"),
+                    Ok(()) => log::debug!("removed namespace {ns}"),
+                    Err(nix::Error::Sys(nix::errno::Errno::ENOENT)) => {
+                        log::debug!("namespace {ns} already removed")
+                    }
+                    Err(nix::Error::Sys(nix::errno::Errno::ENOTEMPTY)) if !delete_groups => {
+                        log::debug!("skip removal of non-empty namespace {ns}")
+                    }
+                    Err(err) => {
+                        removed_all_requested = false;
+                        log::warn!("failed to remove namespace {ns} - {err}")
+                    }
                 }
             }
         }
 
-        Ok(removed_all_groups)
+        Ok(removed_all_requested)
     }
 
     /// Remove a complete backup group including all snapshots.

src/api2/admin/namespace.rs

@@ -134,6 +134,13 @@ pub fn list_namespaces(
             ns: {
                 type: BackupNamespace,
             },
+            "delete-groups": {
+                type: bool,
+                description: "If set, all groups will be destroyed in the whole hierachy below and\
+                    including `ns`. If not set, only empty namespaces will be pruned.",
+                optional: true,
+                default: false,
+            },
         },
     },
     access: {
@@ -144,6 +151,7 @@ pub fn list_namespaces(
 pub fn delete_namespace(
     store: String,
     ns: BackupNamespace,
+    delete_groups: bool,
     _info: &ApiMethod,
     rpcenv: &mut dyn RpcEnvironment,
 ) -> Result<Value, Error> {
@@ -159,7 +167,7 @@ pub fn delete_namespace(
 
     let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
 
-    if !datastore.remove_namespace_recursive(&ns)? {
+    if !datastore.remove_namespace_recursive(&ns, delete_groups)? {
         bail!("group only partially deleted due to protected snapshots");
     }
 

src/server/pull.rs

@@ -864,7 +864,7 @@ fn check_and_remove_ns(params: &PullParameters, local_ns: &BackupNamespace) -> R
         &params.owner,
         PRIV_DATASTORE_MODIFY,
     )?;
-    params.store.remove_namespace_recursive(local_ns)
+    params.store.remove_namespace_recursive(local_ns, true)
 }
 
 fn check_and_remove_vanished_ns(