From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <pbs-devel-bounces@lists.proxmox.com>
Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9])
by lore.proxmox.com (Postfix) with ESMTPS id 3ECD91FF165
for <inbox@lore.proxmox.com>; Thu, 27 Mar 2025 11:34:56 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 1DBD598FA;
Thu, 27 Mar 2025 11:34:50 +0100 (CET)
From: Shannon Sterz <s.sterz@proxmox.com>
To: pbs-devel@lists.proxmox.com
Date: Thu, 27 Mar 2025 11:34:14 +0100
Message-Id: <20250327103414.37840-1-s.sterz@proxmox.com>
X-Mailer: git-send-email 2.39.5
MIME-Version: 1.0
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.018 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
Subject: [pbs-devel] [PATCH proxmox-backup] fix #3336: datastore: remove
group if the last snapshot is removed
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>
Reply-To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Errors-To: pbs-devel-bounces@lists.proxmox.com
Sender: "pbs-devel" <pbs-devel-bounces@lists.proxmox.com>
empty groups are not visible in the gui. this led to a confusing issue
where users were unable to create a group because it already existed
and was still owned by another user. resolve this issue by removing
the group if its last snapshot is removed.
also fixes an issue where removing a group used the non-atomic
`remove_dir_all()` function when destroying a group unconditionally.
this could lead to two different threads suddenly holding a lock to
the same group. make sure that the new locking mechanism is used,
which prevents that, before removing the group. this is also a bit
more conservative now, as it specifically removes the owner file and
group directory separately to avoid accidentaly removing snapshots in
case we made an oversight.
Signed-off-by: Shannon Sterz <s.sterz@proxmox.com>
---
pbs-datastore/src/backup_info.rs | 36 +++++++++++++++++++++++++++-----
pbs-datastore/src/datastore.rs | 6 +++++-
2 files changed, 36 insertions(+), 6 deletions(-)
diff --git a/pbs-datastore/src/backup_info.rs b/pbs-datastore/src/backup_info.rs
index 396b6bde..557bb196 100644
--- a/pbs-datastore/src/backup_info.rs
+++ b/pbs-datastore/src/backup_info.rs
@@ -232,17 +232,34 @@ impl BackupGroup {
delete_stats.increment_removed_snapshots();
}
- if delete_stats.all_removed() {
- std::fs::remove_dir_all(&path).map_err(|err| {
- format_err!("removing group directory {:?} failed - {}", path, err)
- })?;
+ // Note: make sure the old locking mechanism isn't used as `remove_dir_all` is not safe in
+ // that case
+ if delete_stats.all_removed() && !*OLD_LOCKING {
+ self.remove_group_dir()?;
delete_stats.increment_removed_groups();
}
- let _ = std::fs::remove_file(self.lock_path());
Ok(delete_stats)
}
+ /// Helper function, assumes that no more snapshots are present in the group.
+ fn remove_group_dir(&self) -> Result<(), Error> {
+ let owner_path = self.store.owner_path(&self.ns, &self.group);
+
+ std::fs::remove_file(&owner_path).map_err(|err| {
+ format_err!("removing the owner file '{owner_path:?}' failed - {err}")
+ })?;
+
+ let path = self.full_group_path();
+
+ std::fs::remove_dir(&path)
+ .map_err(|err| format_err!("removing group directory {path:?} failed - {err}"))?;
+
+ let _ = std::fs::remove_file(self.lock_path());
+
+ Ok(())
+ }
+
/// Returns the backup owner.
///
/// The backup owner is the entity who first created the backup group.
@@ -581,6 +598,15 @@ impl BackupDir {
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")
+ })?;
+
+ if group.list_backups()?.is_empty() && !*OLD_LOCKING {
+ group.remove_group_dir()?;
+ }
+
Ok(())
}
diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
index 1e6157c0..ae4fb7f8 100644
--- a/pbs-datastore/src/datastore.rs
+++ b/pbs-datastore/src/datastore.rs
@@ -706,7 +706,11 @@ impl DataStore {
}
/// Return the path of the 'owner' file.
- fn owner_path(&self, ns: &BackupNamespace, group: &pbs_api_types::BackupGroup) -> PathBuf {
+ pub(super) fn owner_path(
+ &self,
+ ns: &BackupNamespace,
+ group: &pbs_api_types::BackupGroup,
+ ) -> PathBuf {
self.group_path(ns, group).join("owner")
}
--
2.39.5
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel