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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 4, 2025 9:51 am, Christian Ebner wrote:
> On 11/3/25 3:51 PM, Fabian Grünbichler wrote:
>> 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@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..
> 
> Yes, but I do not have any alternatives here? This needs to be locked as 
> otherwise one might run into consistency issues again, especially 
> important when setting the backup owner. Or is there a way around this 
> which I'm failing to see?

either not lock here, or introduce a notes-specific lock file that is
held for the full upload+local replace, and lock the group only for the
local part

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


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