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

[Christian Ebner <c.ebner@proxmox.com>]

On 7/18/25 11:25 AM, Lukas Wagner wrote:
> With my feedback addressed:
> 
> Reviewed-by: Lukas Wagner <l.wagner@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@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

adapted, thanks!

> 
>> +            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)

Yeah, agreed. I did this already for some other but for the owner 
filename this was indeed missing. Adapted it here and the previous patch 
as well.

> 
>> +                .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);
>>   
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel