From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <c.ebner@proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by lists.proxmox.com (Postfix) with ESMTPS id 24591B86B0
 for <pbs-devel@lists.proxmox.com>; Fri,  8 Mar 2024 14:02:09 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
 by firstgate.proxmox.com (Proxmox) with ESMTP id 042D6DC69
 for <pbs-devel@lists.proxmox.com>; Fri,  8 Mar 2024 14:02:09 +0100 (CET)
Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com
 [94.136.29.106])
 (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits)
 key-exchange X25519 server-signature RSA-PSS (2048 bits))
 (No client certificate requested)
 by firstgate.proxmox.com (Proxmox) with ESMTPS
 for <pbs-devel@lists.proxmox.com>; Fri,  8 Mar 2024 14:02:05 +0100 (CET)
Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1])
 by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 23EDB488CE
 for <pbs-devel@lists.proxmox.com>; Fri,  8 Mar 2024 14:02:05 +0100 (CET)
From: Christian Ebner <c.ebner@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Fri,  8 Mar 2024 14:01:48 +0100
Message-Id: <20240308130150.310352-2-c.ebner@proxmox.com>
X-Mailer: git-send-email 2.39.2
In-Reply-To: <20240308130150.310352-1-c.ebner@proxmox.com>
References: <20240308130150.310352-1-c.ebner@proxmox.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results:  0
 AWL 0.040 Adjusted score from AWL reputation of From: address
 BAYES_00                 -1.9 Bayes spam probability is 0 to 1%
 DMARC_MISSING             0.1 Missing DMARC policy
 KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment
 SPF_HELO_NONE           0.001 SPF: HELO does not publish an SPF Record
 SPF_PASS               -0.001 SPF: sender matches SPF record
 T_SCC_BODY_TEXT_LINE    -0.01 -
Subject: [pbs-devel] [PATCH proxmox-backup 1/3] datastore: group: return
 basic stats on backup group destroy
X-BeenThere: pbs-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox Backup Server development discussion
 <pbs-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/>
List-Post: <mailto:pbs-devel@lists.proxmox.com>
List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, 
 <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Fri, 08 Mar 2024 13:02:09 -0000

No functional change intended: In preparation for including the
removed vanished groups and snapshots statistics in a sync jobs task
log output.

Instead of returning a boolean value showing whether all of the
snapshots of the group have been removed, return an instance of
`BackupGroupDeleteStats`, containing the count of deleted and
protected snapshots, the latter not having been removed from the
group.

The `removed_all` method is introduced as replacement for the previous
boolean return value and can be used to check if all snapshots have
been removed. If there are no protected snapshots, the group is
considered to be deleted.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 pbs-datastore/src/backup_info.rs | 44 +++++++++++++++++++++++++++-----
 pbs-datastore/src/datastore.rs   | 11 ++++----
 src/api2/admin/datastore.rs      |  3 ++-
 src/server/pull.rs               | 21 +++++++--------
 4 files changed, 57 insertions(+), 22 deletions(-)

diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 128315ba..bdfaeabc 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -17,6 +17,36 @@ use crate::manifest::{
 };
 use crate::{DataBlob, DataStore};
 
+#[derive(Default)]
+pub struct BackupGroupDeleteStats {
+    // Count of protected snapshots, therefore not removed
+    unremoved_protected: usize,
+    // Count of deleted snapshots
+    removed_snapshots: usize,
+}
+
+impl BackupGroupDeleteStats {
+    pub fn all_removed(&self) -> bool {
+        self.unremoved_protected == 0
+    }
+
+    pub fn removed_snapshots(&self) -> usize {
+        self.removed_snapshots
+    }
+
+    pub fn protected_snapshots(&self) -> usize {
+        self.unremoved_protected
+    }
+
+    fn increment_removed_snapshots(&mut self) {
+        self.removed_snapshots += 1;
+    }
+
+    fn increment_protected_snapshots(&mut self) {
+        self.unremoved_protected += 1;
+    }
+}
+
 /// BackupGroup is a directory containing a list of BackupDir
 #[derive(Clone)]
 pub struct BackupGroup {
@@ -197,30 +227,32 @@ impl BackupGroup {
 
     /// Destroy the group inclusive all its backup snapshots (BackupDir's)
     ///
-    /// Returns true if all snapshots were removed, and false if some were protected
-    pub fn destroy(&self) -> Result<bool, Error> {
+    /// Returns `BackupGroupDeleteStats`, containing the number of deleted snapshots
+    /// and number of protected snaphsots, which therefore were not removed.
+    pub fn destroy(&self) -> Result<BackupGroupDeleteStats, Error> {
         let path = self.full_group_path();
         let _guard =
             proxmox_sys::fs::lock_dir_noblock(&path, "backup group", "possible running backup")?;
 
         log::info!("removing backup group {:?}", path);
-        let mut removed_all_snaps = true;
+        let mut delete_stats = BackupGroupDeleteStats::default();
         for snap in self.iter_snapshots()? {
             let snap = snap?;
             if snap.is_protected() {
-                removed_all_snaps = false;
+                delete_stats.increment_protected_snapshots();
                 continue;
             }
             snap.destroy(false)?;
+            delete_stats.increment_removed_snapshots();
         }
 
-        if removed_all_snaps {
+        if delete_stats.all_removed() {
             std::fs::remove_dir_all(&path).map_err(|err| {
                 format_err!("removing group directory {:?} failed - {}", path, err)
             })?;
         }
 
-        Ok(removed_all_snaps)
+        Ok(delete_stats)
     }
 
     /// Returns the backup owner.
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 2f0e5279..2afaf7a1 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -23,7 +23,7 @@ use pbs_api_types::{
     DatastoreTuning, GarbageCollectionStatus, Operation, UPID,
 };
 
-use crate::backup_info::{BackupDir, BackupGroup};
+use crate::backup_info::{BackupDir, BackupGroup, BackupGroupDeleteStats};
 use crate::chunk_store::ChunkStore;
 use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
 use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
@@ -464,8 +464,8 @@ impl DataStore {
         let mut removed_all_groups = true;
 
         for group in self.iter_backup_groups(ns.to_owned())? {
-            let removed_group = group?.destroy()?;
-            removed_all_groups = removed_all_groups && removed_group;
+            let delete_stats = group?.destroy()?;
+            removed_all_groups = removed_all_groups && delete_stats.all_removed();
         }
 
         let base_file = std::fs::File::open(self.base_path())?;
@@ -545,12 +545,13 @@ impl DataStore {
 
     /// Remove a complete backup group including all snapshots.
     ///
-    /// Returns true if all snapshots were removed, and false if some were protected
+    /// Returns `BackupGroupDeleteStats`, containing the number of deleted snapshots
+    /// and number of protected snaphsots, which therefore were not removed.
     pub fn remove_backup_group(
         self: &Arc<Self>,
         ns: &BackupNamespace,
         backup_group: &pbs_api_types::BackupGroup,
-    ) -> Result<bool, Error> {
+    ) -> Result<BackupGroupDeleteStats, Error> {
         let backup_group = self.backup_group(ns.clone(), backup_group.clone());
 
         backup_group.destroy()
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index a95031e7..f7164b87 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -294,7 +294,8 @@ pub async fn delete_group(
             &group,
         )?;
 
-        if !datastore.remove_backup_group(&ns, &group)? {
+        let delete_stats = datastore.remove_backup_group(&ns, &group)?;
+        if !delete_stats.all_removed() {
             bail!("group only partially deleted due to protected snapshots");
         }
 
diff --git a/src/server/pull.rs b/src/server/pull.rs
index fc369056..d2bdd35b 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -1498,18 +1498,19 @@ pub(crate) async fn pull_ns(
                     continue;
                 }
                 task_log!(worker, "delete vanished group '{local_group}'",);
-                match params
+                let delete_stats_result = params
                     .target
                     .store
-                    .remove_backup_group(&target_ns, local_group)
-                {
-                    Ok(true) => {}
-                    Ok(false) => {
-                        task_log!(
-                            worker,
-                            "kept some protected snapshots of group '{}'",
-                            local_group
-                        );
+                    .remove_backup_group(&target_ns, local_group);
+
+                match delete_stats_result {
+                    Ok(stats) => {
+                        if !stats.all_removed() {
+                            task_log!(
+                                worker,
+                                "kept some protected snapshots of group '{local_group}'",
+                            );
+                        }
                     }
                     Err(err) => {
                         task_log!(worker, "{}", err);
-- 
2.39.2