[pbs-devel] [PATCH proxmox-backup 02/17] api/datastore: move group notes setting to the datastore

[Fabian Grünbichler]

On November 3, 2025 12:31 pm, Christian Ebner wrote:
> In an effort to abstract away the datastore backend related logic
> from the api, move the set_group_notes to a helper method on the
> datastore.
> 
> The new helper method now also assures that the exclusive lock on
> the backup group is acquired before updating the notes, in order to
> avoid possible race conditions, e.g. with backup group destruction.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
>  pbs-datastore/src/datastore.rs | 26 ++++++++++++++++++++++++++
>  src/api2/admin/datastore.rs    | 17 ++++-------------
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 127ba1c81..45f315aeb 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -2418,4 +2418,30 @@ impl DataStore {
>              .map_err(|err| format_err!("{err:#}"))?;
>          Ok((backend_type, Some(s3_client)))
>      }
> +
> +    /// Creates or updates the notes associated with a backup group.
> +    /// Acquires exclusive lock on the backup group.
> +    pub fn set_group_notes(
> +        self: &Arc<Self>,
> +        notes: String,
> +        backup_group: BackupGroup,
> +    ) -> Result<(), Error> {
> +        let _lock = backup_group.lock().context("failed to lock backup group")?;

this takes an exclusive lock on group, which means all sorts of other
operations (including creating new snapshots?) are blocked while it is
held

> +
> +        if let DatastoreBackend::S3(s3_client) = self.backend()? {
> +            let mut path = backup_group.backup_ns().path();
> +            path.push(backup_group.group().to_string());
> +            let object_key = crate::s3::object_key_from_path(&path, "notes")
> +                .context("invalid owner file object key")?;
> +            let data = hyper::body::Bytes::copy_from_slice(notes.as_bytes());
> +            let _is_duplicate = proxmox_async::runtime::block_on(
> +                s3_client.upload_replace_with_retry(object_key, data),

but this can take a while, right? FWIW, the same is true of setting the
backup owner..

> +            )
> +            .context("failed to set notes on s3 backend")?;
> +        }
> +        let notes_path = self.group_notes_path(backup_group.backup_ns(), backup_group.group());
> +        replace_file(notes_path, notes.as_bytes(), CreateOptions::new(), false)
> +            .context("failed to replace group notes file")?;
> +        Ok(())
> +    }
>  }
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index d192ee390..131cdae51 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2009,19 +2009,10 @@ pub fn set_group_notes(
>          &backup_group,
>      )?;
>  
> -    if let DatastoreBackend::S3(s3_client) = datastore.backend()? {
> -        let mut path = ns.path();
> -        path.push(backup_group.to_string());
> -        let object_key = pbs_datastore::s3::object_key_from_path(&path, "notes")
> -            .context("invalid owner file object key")?;
> -        let data = hyper::body::Bytes::copy_from_slice(notes.as_bytes());
> -        let _is_duplicate =
> -            proxmox_async::runtime::block_on(s3_client.upload_replace_with_retry(object_key, data))
> -                .context("failed to set notes on s3 backend")?;
> -    }
> -    let notes_path = datastore.group_notes_path(&ns, &backup_group);
> -    replace_file(notes_path, notes.as_bytes(), CreateOptions::new(), false)?;
> -
> +    let backup_group = datastore.backup_group(ns, backup_group);
> +    datastore
> +        .set_group_notes(notes, backup_group)
> +        .map_err(|err| format_err!("failed to set group notes - {err:#?}"))?;
>      Ok(())
>  }
>  
> -- 
> 2.47.3
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>