Re: [pbs-devel] [PATCH proxmox-backup 1/2] datastore: s3 refresh: set/unset maintenance mode in api handler

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

On 11/11/25 11:09 AM, Fabian Grünbichler wrote:
> On November 4, 2025 2:19 pm, Christian Ebner wrote:
>> Instead of setting the maintenance mode in the datastores s3 refresh
>> helper method, do this in the api handler directly. Since this is
>> now mostly an sync task, adapt the api handler to be a sync function
>> and run the task on a dedicated thread.
>>
>> This is in preparation for fixing the s3 refresh to be able to start
>> a refresh without checking for active operations.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-datastore/src/datastore.rs | 26 --------------------------
>>   src/api2/admin/datastore.rs    | 32 ++++++++++++++++++++++++++++----
>>   2 files changed, 28 insertions(+), 30 deletions(-)
>>
>> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
>> index 127ba1c81..d5ff6e5f7 100644
>> --- a/pbs-datastore/src/datastore.rs
>> +++ b/pbs-datastore/src/datastore.rs
>> @@ -2208,16 +2208,6 @@ impl DataStore {
>>           match self.backend()? {
>>               DatastoreBackend::Filesystem => bail!("store '{}' not backed by S3", self.name()),
>>               DatastoreBackend::S3(s3_client) => {
>> -                let self_clone = Arc::clone(self);
>> -                tokio::task::spawn_blocking(move || {
>> -                    self_clone.maintenance_mode(Some(MaintenanceMode {
>> -                        ty: MaintenanceType::S3Refresh,
>> -                        message: None,
>> -                    }))
>> -                })
>> -                .await?
>> -                .context("failed to set maintenance mode")?;
>> -
>>                   let tmp_base = proxmox_sys::fs::make_tmp_dir(self.base_path(), None)
>>                       .context("failed to create temporary content folder in {store_base}")?;
>>   
>> @@ -2231,27 +2221,11 @@ impl DataStore {
>>                       let _ = std::fs::remove_dir_all(&tmp_base);
>>                       return Err(err);
>>                   }
>> -
>> -                let self_clone = Arc::clone(self);
>> -                tokio::task::spawn_blocking(move || self_clone.maintenance_mode(None))
>> -                    .await?
>> -                    .context("failed to clear maintenance mode")?;
>>               }
>>           }
>>           Ok(())
>>       }
>>   
>> -    // Set or clear the datastores maintenance mode by locking and updating the datastore config
>> -    fn maintenance_mode(&self, maintenance_mode: Option<MaintenanceMode>) -> Result<(), Error> {
>> -        let _lock = pbs_config::datastore::lock_config()?;
>> -        let (mut section_config, _digest) = pbs_config::datastore::config()?;
>> -        let mut datastore: DataStoreConfig = section_config.lookup("datastore", self.name())?;
>> -        datastore.set_maintenance_mode(maintenance_mode)?;
>> -        section_config.set_data(self.name(), "datastore", &datastore)?;
>> -        pbs_config::datastore::save_config(&section_config)?;
>> -        Ok(())
>> -    }
>> -
>>       // Fetch the contents (metadata, no chunks) of the datastore from the S3 object store to the
>>       // provided temporaray directory
>>       async fn fetch_tmp_contents(&self, tmp_base: &Path, s3_client: &S3Client) -> Result<(), Error> {
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index d192ee390..00110119f 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -2737,22 +2737,46 @@ pub async fn unmount(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<V
>>       },
>>   )]
>>   /// Refresh datastore contents from S3 to local cache store.
>> -pub async fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
>> +pub fn s3_refresh(store: String, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Error> {
>> +    maintenance_mode(
>> +        &store,
>> +        Some(MaintenanceMode {
>> +            ty: MaintenanceType::S3Refresh,
>> +            message: None,
>> +        }),
>> +    )
>> +    .context("failed to set maintenance mode")?;
>> +
>>       let datastore = DataStore::lookup_datastore(&store, Some(Operation::Lookup))?;
>>       let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
>>       let to_stdout = rpcenv.env_type() == RpcEnvironmentType::CLI;
>>   
>> -    let upid = WorkerTask::spawn(
>> +    let upid = WorkerTask::new_thread(
>>           "s3-refresh",
>> -        Some(store),
>> +        Some(store.clone()),
>>           auth_id.to_string(),
>>           to_stdout,
>> -        move |_worker| async move { datastore.s3_refresh().await },
>> +        move |_worker| {
>> +            proxmox_async::runtime::block_on(datastore.s3_refresh())?;
> 
> this helper's doc comments are now wrong..
> 
> but also, this would need to work more like unmounting IMHO, since there
> is no protecting against leavine S3Refresh maintenance mode while it is
> currently active??
> 
> we currently risk issues like the datastore not having a maintenance
> mode set, tasks being started, and then S3Refresh clearing out all the
> dirs to replace them with the just-downloaded ones, causing major
> inconsistencies?
> 
> I think we can re-use expect_maintenance_unmounting by making it
> generic, and then hold the maintenance mode lock while doing the
> refresh? that forces the refresh to be aborted before the maintenance
> mode can be lifted (and just leaves a crash or restart while refreshing
> as source of issues)
> 
> it also makes the `maintenance_mode` helper kinda unnecessary, as we'd
> now only set the maintenance mode once at the start, and then query that
> it is still as expected, and there already is a helper for removing
> maintenance mode at the end or as part of error/abortion handling..

Right, will rework this using the same logic as for unmounting then, 
incorporating all the comments. Thanks!


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