From 524ed4048bed09d7a130fedcc0ba2762d6ddf897 Mon Sep 17 00:00:00 2001 From: Christian Ebner Date: Fri, 8 Mar 2024 14:01:48 +0100 Subject: [PATCH] datastore: group: return basic stats on backup group destroy 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 Signed-off-by: Thomas Lamprecht --- 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 { + /// Returns `BackupGroupDeleteStats`, containing the number of deleted snapshots + /// and number of protected snaphsots, which therefore were not removed. + pub fn destroy(&self) -> Result { 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 2508e3d7..0685cc84 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}; @@ -500,8 +500,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())?; @@ -581,12 +581,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, ns: &BackupNamespace, backup_group: &pbs_api_types::BackupGroup, - ) -> Result { + ) -> Result { 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);