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

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

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..

> +
> +            maintenance_mode(&store, None).context("failed to clear maintenance mode")
> +        },
>      )?;
>  
>      Ok(json!(upid))
>  }
>  
> +// Set or clear the datastores maintenance mode by locking and updating the datastore config
> +fn maintenance_mode(store: &str, 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", store)?;
> +    datastore.set_maintenance_mode(maintenance_mode)?;
> +    section_config.set_data(store, "datastore", &datastore)?;
> +    pbs_config::datastore::save_config(&section_config)?;
> +    Ok(())
> +}
> +
>  #[sortable]
>  const DATASTORE_INFO_SUBDIRS: SubdirMap = &[
>      (
> -- 
> 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