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 4E4791FF164
for <inbox@lore.proxmox.com>; Fri, 9 May 2025 14:27:28 +0200 (CEST)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 7E7993CFCD;
Fri, 9 May 2025 14:27:47 +0200 (CEST)
Date: Fri, 09 May 2025 14:27:09 +0200
From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com>
References: <20250508130555.494782-1-c.ebner@proxmox.com>
<20250508130555.494782-11-c.ebner@proxmox.com>
In-Reply-To: <20250508130555.494782-11-c.ebner@proxmox.com>
MIME-Version: 1.0
User-Agent: astroid/0.16.0 (https://github.com/astroidmail/astroid)
Message-Id: <1746793154.56dzxckyhs.astroid@yuna.none>
X-SPAM-LEVEL: Spam detection results: 0
AWL 0.046 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: Re: [pbs-devel] [RFC v2 proxmox-backup 10/21] datastore: mark
namespace as trash instead of deleting it
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>
On May 8, 2025 3:05 pm, Christian Ebner wrote:
> As for backup snapshots and groups, mark the namespace as trash
> instead of removing it and the contents right away, if the trash
> should not be bypassed.
>
> Actual removal of the hirarchical folder structure has to be taken
> care of by the garbage collection.
>
> In order to avoid races during removal, first mark the namespaces as
> trash pending, mark the snapshots and groups as trash and only after
> rename the pending marker file to the trash marker file. By this,
> concurrent backups can remove the trash pending marker to avoid the
> namespace being trashed.
>
> On re-creation of a trashed namespace remove the marker file on itself
> and any parent component from deepest to shallowest. As trashing a full
> namespace can also set the trash pending state for recursive namespace
> cleanup, remove encounters of that marker file as well to avoid the
> namespace or its parent being trashed.
this is fairly involved since we don't have locks on namespaces..
should we have them for creation/removal/trashing/untrashing?
I assume those are fairly rare occurrences, I haven't yet analyzed the
interactions here to see whether the two-marker approach is actually
race-free..
OTOH, do we really need to (be able to) trash namespaces?
>
> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
> ---
> pbs-datastore/src/datastore.rs | 135 +++++++++++++++++++++++++++++----
> src/api2/admin/namespace.rs | 2 +-
> src/server/pull.rs | 2 +-
> 3 files changed, 123 insertions(+), 16 deletions(-)
>
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 023a6a12e..b1d81e199 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -28,7 +28,9 @@ use pbs_api_types::{
> };
> use pbs_config::BackupLockGuard;
>
> -use crate::backup_info::{BackupDir, BackupGroup, BackupInfo, ListBackupFilter, OLD_LOCKING};
> +use crate::backup_info::{
> + BackupDir, BackupGroup, BackupInfo, ListBackupFilter, OLD_LOCKING, TRASH_MARKER_FILENAME,
> +};
> use crate::chunk_store::ChunkStore;
> use crate::dynamic_index::{DynamicIndexReader, DynamicIndexWriter};
> use crate::fixed_index::{FixedIndexReader, FixedIndexWriter};
> @@ -42,6 +44,8 @@ use crate::DataBlob;
> static DATASTORE_MAP: LazyLock<Mutex<HashMap<String, Arc<DataStoreImpl>>>> =
> LazyLock::new(|| Mutex::new(HashMap::new()));
>
> +const TRASH_PENDING_MARKER_FILENAME: &str = ".pending";
> +
> /// checks if auth_id is owner, or, if owner is a token, if
> /// auth_id is the user of the token
> pub fn check_backup_owner(owner: &Authid, auth_id: &Authid) -> Result<(), Error> {
> @@ -554,6 +558,7 @@ impl DataStore {
> ns_full_path.push(ns.path());
>
> std::fs::create_dir_all(ns_full_path)?;
> + self.untrash_namespace(&ns)?;
>
> Ok(ns)
> }
> @@ -565,6 +570,34 @@ impl DataStore {
> path.exists()
> }
>
> + /// Remove the namespace and all it's parent components from the trash by removing the trash or
> + /// trash-pending marker file for each namespace level from deepest to shallowest. Missing files
> + /// are ignored.
> + pub fn untrash_namespace(&self, namespace: &BackupNamespace) -> Result<(), Error> {
> + let mut namespace = namespace.clone();
> + while namespace.depth() > 0 {
> + let mut trash_file_path = self.base_path();
> + trash_file_path.push(namespace.path());
> + let mut pending_file_path = trash_file_path.clone();
> + pending_file_path.push(TRASH_PENDING_MARKER_FILENAME);
> + if let Err(err) = std::fs::remove_file(&pending_file_path) {
> + // ignore not found, either not trashed or un-trashed by concurrent operation
> + if err.kind() != std::io::ErrorKind::NotFound {
> + bail!("failed to remove trash-pending file {trash_file_path:?}: {err}");
> + }
> + }
> + trash_file_path.push(TRASH_MARKER_FILENAME);
> + if let Err(err) = std::fs::remove_file(&trash_file_path) {
> + // ignore not found, either not trashed or un-trashed by concurrent operation
> + if err.kind() != std::io::ErrorKind::NotFound {
> + bail!("failed to remove trash file {trash_file_path:?}: {err}");
> + }
> + }
> + namespace.pop();
> + }
> + Ok(())
> + }
> +
> /// Remove all backup groups of a single namespace level but not the namespace itself.
> ///
> /// Does *not* descends into child-namespaces and doesn't remoes the namespace itself either.
> @@ -574,6 +607,7 @@ impl DataStore {
> pub fn remove_namespace_groups(
> self: &Arc<Self>,
> ns: &BackupNamespace,
> + skip_trash: bool,
> ) -> Result<(bool, BackupGroupDeleteStats), Error> {
> // FIXME: locking? The single groups/snapshots are already protected, so may not be
> // necessary (depends on what we all allow to do with namespaces)
> @@ -583,20 +617,22 @@ impl DataStore {
> let mut stats = BackupGroupDeleteStats::default();
>
> for group in self.iter_backup_groups(ns.to_owned())? {
> - let delete_stats = group?.destroy(true)?;
> + let delete_stats = group?.destroy(skip_trash)?;
> stats.add(&delete_stats);
> removed_all_groups = removed_all_groups && delete_stats.all_removed();
> }
>
> - let base_file = std::fs::File::open(self.base_path())?;
> - let base_fd = base_file.as_raw_fd();
> - for ty in BackupType::iter() {
> - let mut ty_dir = ns.path();
> - ty_dir.push(ty.to_string());
> - // best effort only, but we probably should log the error
> - if let Err(err) = unlinkat(Some(base_fd), &ty_dir, UnlinkatFlags::RemoveDir) {
> - if err != nix::errno::Errno::ENOENT {
> - log::error!("failed to remove backup type {ty} in {ns} - {err}");
> + if skip_trash {
> + let base_file = std::fs::File::open(self.base_path())?;
> + let base_fd = base_file.as_raw_fd();
> + for ty in BackupType::iter() {
> + let mut ty_dir = ns.path();
> + ty_dir.push(ty.to_string());
> + // best effort only, but we probably should log the error
> + if let Err(err) = unlinkat(Some(base_fd), &ty_dir, UnlinkatFlags::RemoveDir) {
> + if err != nix::errno::Errno::ENOENT {
> + log::error!("failed to remove backup type {ty} in {ns} - {err}");
> + }
> }
> }
> }
> @@ -613,6 +649,7 @@ impl DataStore {
> self: &Arc<Self>,
> ns: &BackupNamespace,
> delete_groups: bool,
> + skip_trash: bool,
> ) -> Result<(bool, BackupGroupDeleteStats), Error> {
> let store = self.name();
> let mut removed_all_requested = true;
> @@ -620,16 +657,68 @@ impl DataStore {
> if delete_groups {
> log::info!("removing whole namespace recursively below {store}:/{ns}",);
> for ns in self.recursive_iter_backup_ns(ns.to_owned(), NamespaceListFilter::Active)? {
> - let (removed_ns_groups, delete_stats) = self.remove_namespace_groups(&ns?)?;
> + let namespace = ns?;
> +
> + if !skip_trash {
> + let mut path = self.base_path();
> + path.push(namespace.path());
> + path.push(TRASH_PENDING_MARKER_FILENAME);
> + if let Err(err) = std::fs::File::create(&path) {
pending marker created here iff delete_groups && !skip_trash
> + if err.kind() != std::io::ErrorKind::AlreadyExists {
> + return Err(err).context("failed to set trash pending marker file");
> + }
> + }
> + }
> +
> + let (removed_ns_groups, delete_stats) =
> + self.remove_namespace_groups(&namespace, skip_trash)?;
> stats.add(&delete_stats);
> removed_all_requested = removed_all_requested && removed_ns_groups;
> +
> + if !skip_trash && !removed_ns_groups {
> + self.untrash_namespace(&namespace)?;
> + }
> }
> } else {
> log::info!("pruning empty namespace recursively below {store}:/{ns}");
> }
>
> - removed_all_requested =
> - removed_all_requested && self.destroy_namespace_recursive(ns, delete_groups)?;
> + if skip_trash {
> + removed_all_requested =
> + removed_all_requested && self.destroy_namespace_recursive(ns, delete_groups)?;
> + return Ok((removed_all_requested, stats));
early return here iff skip_trash
> + }
> +
> + let mut children = self
> + .recursive_iter_backup_ns(ns.to_owned(), NamespaceListFilter::Active)?
> + .collect::<Result<Vec<BackupNamespace>, Error>>()?;
> +
> + children.sort_by_key(|b| std::cmp::Reverse(b.depth()));
> +
> + let mut all_trashed = true;
> + for ns in children.iter() {
> + let mut ns_dir = ns.path();
> + ns_dir.push("ns");
> +
> + if !ns.is_root() {
> + let mut path = self.base_path();
> + path.push(ns.path());
> +
> + let pending_path = path.join(TRASH_PENDING_MARKER_FILENAME);
> + path.push(TRASH_MARKER_FILENAME);
> + if let Err(err) = std::fs::rename(pending_path, path) {
pending marker assumed to exist here iff !skip_trash
> + if err.kind() == std::io::ErrorKind::NotFound {
> + all_trashed = false;
> + } else {
> + return Err(err).context("Renaming pending marker to trash marker failed");
> + }
> + }
> + }
> + }
> +
> + if !all_trashed {
> + bail!("failed to prune namespace, not empty");
wrong error returned as a result..
> + }
>
> Ok((removed_all_requested, stats))
> }
> @@ -657,6 +746,24 @@ impl DataStore {
> let _ = unlinkat(Some(base_fd), &ns_dir, UnlinkatFlags::RemoveDir);
>
> if !ns.is_root() {
> + let rel_trash_path = ns.path().join(TRASH_MARKER_FILENAME);
> + if let Err(err) =
> + unlinkat(Some(base_fd), &rel_trash_path, UnlinkatFlags::NoRemoveDir)
> + {
> + if err != nix::errno::Errno::ENOENT {
> + bail!("removing the trash file '{rel_trash_path:?}' failed - {err}")
> + }
> + }
> + let rel_pending_path = ns.path().join(TRASH_PENDING_MARKER_FILENAME);
> + if let Err(err) =
> + unlinkat(Some(base_fd), &rel_pending_path, UnlinkatFlags::NoRemoveDir)
> + {
> + if err != nix::errno::Errno::ENOENT {
> + bail!(
> + "removing the trash pending file '{rel_pending_path:?}' failed - {err}"
> + )
> + }
> + }
> match unlinkat(Some(base_fd), &ns.path(), UnlinkatFlags::RemoveDir) {
> Ok(()) => log::debug!("removed namespace {ns}"),
> Err(nix::errno::Errno::ENOENT) => {
> diff --git a/src/api2/admin/namespace.rs b/src/api2/admin/namespace.rs
> index e5463524a..a12d97883 100644
> --- a/src/api2/admin/namespace.rs
> +++ b/src/api2/admin/namespace.rs
> @@ -166,7 +166,7 @@ pub fn delete_namespace(
>
> let datastore = DataStore::lookup_datastore(&store, Some(Operation::Write))?;
>
> - let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups)?;
> + let (removed_all, stats) = datastore.remove_namespace_recursive(&ns, delete_groups, true)?;
> if !removed_all {
> let err_msg = if delete_groups {
> if datastore.old_locking() {
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> index d3c6fcf6a..a60ccbf10 100644
> --- a/src/server/pull.rs
> +++ b/src/server/pull.rs
> @@ -729,7 +729,7 @@ fn check_and_remove_ns(params: &PullParameters, local_ns: &BackupNamespace) -> R
> let (removed_all, _delete_stats) = params
> .target
> .store
> - .remove_namespace_recursive(local_ns, true)?;
> + .remove_namespace_recursive(local_ns, true, true)?;
>
> Ok(removed_all)
> }
> --
> 2.39.5
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel