[pbs-devel] [PATCH proxmox-backup v8 21/45] datastore: get and set owner for s3 store backend

[Lukas Wagner]

With my feedback addressed:

Reviewed-by: Lukas Wagner <l.wagner at proxmox.com>

On  2025-07-15 14:53, Christian Ebner wrote:
> Read or write the ownership information from/to the corresponding
> object in the S3 object store. Keep that information available if
> the bucket is reused as datastore.
> 
> Signed-off-by: Christian Ebner <c.ebner at proxmox.com>
> ---
> changes since version 7:
> - no changes
> 
>  pbs-datastore/src/datastore.rs | 28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index 265624229..ca099c1d0 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -7,6 +7,7 @@ use std::sync::{Arc, LazyLock, Mutex};
>  use std::time::Duration;
>  
>  use anyhow::{bail, format_err, Context, Error};
> +use http_body_util::BodyExt;
>  use nix::unistd::{unlinkat, UnlinkatFlags};
>  use pbs_tools::lru_cache::LruCache;
>  use tracing::{info, warn};
> @@ -832,6 +833,21 @@ impl DataStore {
>          backup_group: &pbs_api_types::BackupGroup,
>      ) -> Result<Authid, Error> {
>          let full_path = self.owner_path(ns, backup_group);
> +
> +        if let DatastoreBackend::S3(s3_client) = self.backend()? {
> +            let mut path = ns.path();
> +            path.push(format!("{backup_group}"));

nit: you can use .to_string() here, is a bit easier to read

> +            let object_key = crate::s3::object_key_from_path(&path, "owner")

I did not note it for the previously reviewed patches, but I think some (pub) consts for these
'static' key suffixes would be better than to repeat the same string in multiple places in the code
(mostly to avoid errors due to spelling mistakes)

> +                .context("invalid owner file object key")?;
> +            let response = proxmox_async::runtime::block_on(s3_client.get_object(object_key))?
> +                .ok_or_else(|| format_err!("fetching owner failed"))?;
> +            let content = proxmox_async::runtime::block_on(response.content.collect())?;
> +            let owner = String::from_utf8(content.to_bytes().trim_ascii_end().to_vec())?;
> +            return owner
> +                .parse()
> +                .map_err(|err| format_err!("parsing owner for {backup_group} failed: {err}"));
> +        }
> +
>          let owner = proxmox_sys::fs::file_read_firstline(full_path)?;
>          owner
>              .trim_end() // remove trailing newline
> @@ -860,6 +876,18 @@ impl DataStore {
>      ) -> Result<(), Error> {
>          let path = self.owner_path(ns, backup_group);
>  
> +        if let DatastoreBackend::S3(s3_client) = self.backend()? {
> +            let mut path = ns.path();
> +            path.push(format!("{backup_group}"));
> +            let object_key = crate::s3::object_key_from_path(&path, "owner")
> +                .context("invalid owner file object key")?;
> +            let data = hyper::body::Bytes::from(format!("{auth_id}\n"));
> +            let _is_duplicate = proxmox_async::runtime::block_on(
> +                s3_client.upload_with_retry(object_key, data, true),
> +            )
> +            .context("failed to set owner on s3 backend")?;
> +        }
> +
>          let mut open_options = std::fs::OpenOptions::new();
>          open_options.write(true);
>  

-- 
- Lukas