[pbs-devel] [PATCH proxmox-backup 0/3] add removed vanished stats to sync job log

[pbs-devel] [PATCH proxmox-backup 0/3] add removed vanished stats to sync job log

[Christian Ebner]

Adds a global summary of the vanished and therefore removed snapshots,
backup groups and namespaces to the sync jobs task log.

Patch 1/3 introduces a BackupGroupDeleteStats object, used to count
the number of removed snapshots within a backup group as well as the
number of not removed snapshots, since protected.

Patch 2/3 adds an optional RemovedVanishedStats object to the PullStats,
counting also the removed backup groups and namespaces and utilizing the
returned counts of removed snapshots as introduced by the previous
patch.

Patch 3/3 finally adds the removed vanished entities output to the sync
jobs task log.

The series was tested by creating a local sync job and syncing a
datastore, containing nested namespaces with snapshots performing the
following actions:

The `remove vanished` flag was unset for the sync job and the absence
of the line in the sync jobs task log verified for that case; then the
flag was set once again.

Further, removed snapshots on the source store, set some of the removed
snapshots to be protected on the target store and compared the removed
entities of the sync job by the logged output.

Finally, removed the protected flag on the snapshots in the target
datastore and removed all namespaces and snapshots in the source
datastore. Checked once again the sync jobs log output as compared to
the actual removed entities.

Christian Ebner (3):
  datastore: group: return basic stats on backup group destroy
  server: sync job: include removed vanished stats
  api: sync job: log stats for removed vanished entities

 pbs-datastore/src/backup_info.rs | 44 ++++++++++++++---
 pbs-datastore/src/datastore.rs   | 11 +++--
 src/api2/admin/datastore.rs      |  3 +-
 src/api2/pull.rs                 | 10 ++++
 src/server/pull.rs               | 85 ++++++++++++++++++++++++++------
 5 files changed, 127 insertions(+), 26 deletions(-)

-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 1/3] datastore: group: return basic stats on backup group destroy

[Christian Ebner]

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

[pbs-devel] [PATCH proxmox-backup 2/3] server: sync job: include removed vanished stats

[Christian Ebner]

Include statistics of vanished and therefore removed snapshots, backup
groups and namespaces in the `PullStats`.

In preparation for including these values in the sync jobs task log
output.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 64 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 4 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index d2bdd35b..14744e9c 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -65,11 +65,36 @@ pub(crate) struct LocalSource {
     ns: BackupNamespace,
 }
 
+#[derive(Default)]
+pub(crate) struct RemovedVanishedStats {
+    pub(crate) groups: usize,
+    pub(crate) snapshots: usize,
+    pub(crate) namespaces: usize,
+}
+
+impl RemovedVanishedStats {
+    fn add(&mut self, rhs: RemovedVanishedStats) {
+        self.groups += rhs.groups;
+        self.snapshots += rhs.snapshots;
+        self.namespaces += rhs.namespaces;
+    }
+}
+
 #[derive(Default)]
 pub(crate) struct PullStats {
     pub(crate) chunk_count: usize,
     pub(crate) bytes: usize,
     pub(crate) elapsed: Duration,
+    pub(crate) removed: Option<RemovedVanishedStats>,
+}
+
+impl From<RemovedVanishedStats> for PullStats {
+    fn from(removed: RemovedVanishedStats) -> Self {
+        Self {
+            removed: Some(removed),
+            ..Default::default()
+        }
+    }
 }
 
 impl PullStats {
@@ -77,6 +102,14 @@ impl PullStats {
         self.chunk_count += rhs.chunk_count;
         self.bytes += rhs.bytes;
         self.elapsed += rhs.elapsed;
+
+        if let Some(rhs_removed) = rhs.removed {
+            if let Some(ref mut removed) = self.removed {
+                removed.add(rhs_removed);
+            } else {
+                self.removed = Some(rhs_removed);
+            }
+        }
     }
 }
 
@@ -667,6 +700,7 @@ async fn pull_index_chunks<I: IndexFile>(
         chunk_count,
         bytes,
         elapsed,
+        removed: None,
     })
 }
 
@@ -1147,6 +1181,11 @@ async fn pull_group(
                 .target
                 .store
                 .remove_backup_dir(&target_ns, snapshot.as_ref(), false)?;
+            pull_stats.add(PullStats::from(RemovedVanishedStats {
+                snapshots: 1,
+                groups: 0,
+                namespaces: 0,
+            }));
         }
     }
 
@@ -1199,8 +1238,9 @@ fn check_and_remove_vanished_ns(
     worker: &WorkerTask,
     params: &PullParameters,
     synced_ns: HashSet<BackupNamespace>,
-) -> Result<bool, Error> {
+) -> Result<(bool, RemovedVanishedStats), Error> {
     let mut errors = false;
+    let mut removed_stats = RemovedVanishedStats::default();
     let user_info = CachedUserInfo::new()?;
 
     // clamp like remote does so that we don't list more than we can ever have synced.
@@ -1235,7 +1275,10 @@ fn check_and_remove_vanished_ns(
             continue;
         }
         match check_and_remove_ns(params, &local_ns) {
-            Ok(true) => task_log!(worker, "Removed namespace {}", local_ns),
+            Ok(true) => {
+                task_log!(worker, "Removed namespace {local_ns}");
+                removed_stats.namespaces += 1;
+            }
             Ok(false) => task_log!(
                 worker,
                 "Did not remove namespace {} - protected snapshots remain",
@@ -1248,7 +1291,7 @@ fn check_and_remove_vanished_ns(
         }
     }
 
-    Ok(errors)
+    Ok((errors, removed_stats))
 }
 
 /// Pulls a store according to `params`.
@@ -1372,7 +1415,9 @@ pub(crate) async fn pull_store(
     }
 
     if params.remove_vanished {
-        errors |= check_and_remove_vanished_ns(worker, &params, synced_ns)?;
+        let (has_errors, stats) = check_and_remove_vanished_ns(worker, &params, synced_ns)?;
+        errors |= has_errors;
+        pull_stats.add(PullStats::from(stats));
     }
 
     if errors {
@@ -1510,6 +1555,17 @@ pub(crate) async fn pull_ns(
                                 worker,
                                 "kept some protected snapshots of group '{local_group}'",
                             );
+                            pull_stats.add(PullStats::from(RemovedVanishedStats {
+                                snapshots: stats.removed_snapshots(),
+                                groups: 0,
+                                namespaces: 0,
+                            }));
+                        } else {
+                            pull_stats.add(PullStats::from(RemovedVanishedStats {
+                                snapshots: stats.removed_snapshots(),
+                                groups: 1,
+                                namespaces: 0,
+                            }));
                         }
                     }
                     Err(err) => {
-- 
2.39.2

[pbs-devel] [PATCH proxmox-backup 3/3] api: sync job: log stats for removed vanished entities

[Christian Ebner]

Extend the current task log summary to include a log entry stating the
number of removed because vanished on the source side snapshots,
backup groups and namespaces.

The additional task log line states, e.g.:
> Summary: removed vanished: snapshots: 2, groups: 1, namespaces: 0

The log line is not shown if the sync jobs `remove_vanished` flag was
not set and therefore no removed vanished stats are present.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/pull.rs | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/api2/pull.rs b/src/api2/pull.rs
index 29abc1c2..59db3660 100644
--- a/src/api2/pull.rs
+++ b/src/api2/pull.rs
@@ -161,6 +161,16 @@ pub fn do_sync_job(
                     task_log!(worker, "Summary: sync job found no new data to pull");
                 }
 
+                if let Some(removed) = pull_stats.removed {
+                    task_log!(
+                        worker,
+                        "Summary: removed vanished: snapshots: {}, groups: {}, namespaces: {}",
+                        removed.snapshots,
+                        removed.groups,
+                        removed.namespaces,
+                    );
+                }
+
                 task_log!(worker, "sync job '{}' end", &job_id);
 
                 Ok(())
-- 
2.39.2

[pbs-devel] applied-series: [PATCH proxmox-backup 0/3] add removed vanished stats to sync job log

[Thomas Lamprecht]

Am 08/03/2024 um 14:01 schrieb Christian Ebner:
> Adds a global summary of the vanished and therefore removed snapshots,
> backup groups and namespaces to the sync jobs task log.
> 
> Patch 1/3 introduces a BackupGroupDeleteStats object, used to count
> the number of removed snapshots within a backup group as well as the
> number of not removed snapshots, since protected.
> 
> Patch 2/3 adds an optional RemovedVanishedStats object to the PullStats,
> counting also the removed backup groups and namespaces and utilizing the
> returned counts of removed snapshots as introduced by the previous
> patch.
> 
> Patch 3/3 finally adds the removed vanished entities output to the sync
> jobs task log.
> 
> The series was tested by creating a local sync job and syncing a
> datastore, containing nested namespaces with snapshots performing the
> following actions:
> 
> The `remove vanished` flag was unset for the sync job and the absence
> of the line in the sync jobs task log verified for that case; then the
> flag was set once again.
> 
> Further, removed snapshots on the source store, set some of the removed
> snapshots to be protected on the target store and compared the removed
> entities of the sync job by the logged output.
> 
> Finally, removed the protected flag on the snapshots in the target
> datastore and removed all namespaces and snapshots in the source
> datastore. Checked once again the sync jobs log output as compared to
> the actual removed entities.
> 
> Christian Ebner (3):
>   datastore: group: return basic stats on backup group destroy
>   server: sync job: include removed vanished stats
>   api: sync job: log stats for removed vanished entities
> 
>  pbs-datastore/src/backup_info.rs | 44 ++++++++++++++---
>  pbs-datastore/src/datastore.rs   | 11 +++--
>  src/api2/admin/datastore.rs      |  3 +-
>  src/api2/pull.rs                 | 10 ++++
>  src/server/pull.rs               | 85 ++++++++++++++++++++++++++------
>  5 files changed, 127 insertions(+), 26 deletions(-)
> 


applied series, thanks!