Re: [PATCH proxmox-backup 1/6] datastore: propagate maintenance mode parse errors

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

On 6/2/26 3:00 PM, Robert Obkircher wrote:
> Handle maintenance mode parse errors since they are no longer silently
> mapped to None. That was problematic because during upgrades the older
> version would have potentially ignored a newly introduced maintenance
> mode.

LGTM. Dropping from caches is local to process, so fine and s3-refresh 
and unmount run in a maintenance-locked state anyways. They might fail 
if the maintenance mode was not in the expected one or parsing errors 
prior to entering the mode, but that is intended behavior.

> 
> Signed-off-by: Robert Obkircher <r.obkircher@proxmox.com>

Reviewed-by: Christian Ebner <c.ebner@proxmox.com>

> ---
>   pbs-datastore/src/datastore.rs      | 17 ++++++++++++-----
>   src/api2/admin/datastore.rs         |  2 +-
>   src/server/metric_collection/mod.rs |  7 +++----
>   3 files changed, 16 insertions(+), 10 deletions(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index e2d1ae67c..da7eba80e 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -303,9 +303,16 @@ impl Drop for DataStore {
>                   Ok((section_config, _gen)) => {
>                       match section_config.lookup::<DataStoreConfig>("datastore", self.name()) {
>                           // second check: check if maintenance mode requires closing FDs
> -                        Ok(config) => config
> -                            .get_maintenance_mode()
> -                            .is_some_and(|m| m.clear_from_cache()),
> +                        Ok(config) => match config.get_maintenance_mode() {
> +                            Ok(m) => m.is_some_and(|m| m.clear_from_cache()),
> +                            Err(err) => {
> +                                log::warn!(
> +                                    "DataStore::drop: datastore '{}' in unknown maintenance mode; evicting cached instance: {err}",
> +                                    self.name()
> +                                );
> +                                true
> +                            }
> +                        },
>                           Err(err) => {
>                               // datastore removed from config; evict cached entry if available (without checking maintenance mode)
>                               log::warn!(
> @@ -576,7 +583,7 @@ impl DataStore {
>           let (section_config, gen_num) = datastore_section_config_cached(true)?;
>           let config: DataStoreConfig = section_config.lookup("datastore", lookup.name)?;
>   
> -        if let Some(maintenance_mode) = config.get_maintenance_mode() {
> +        if let Some(maintenance_mode) = config.get_maintenance_mode()? {
>               if let Err(error) = maintenance_mode.check(lookup.operation) {
>                   bail!("datastore '{}' is unavailable: {error}", lookup.name);
>               }
> @@ -660,7 +667,7 @@ impl DataStore {
>           let datastore: DataStoreConfig = config.lookup("datastore", name)?;
>           if datastore
>               .get_maintenance_mode()
> -            .is_some_and(|m| m.clear_from_cache())
> +            .map_or(true, |m| m.is_some_and(|m| m.clear_from_cache()))
>           {
>               // the datastore drop handler does the checking if tasks are running and clears the
>               // cache entry, so we just have to trigger it here
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 64c73ba06..94272c32d 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -2695,7 +2695,7 @@ fn expect_maintenance_type(
>       let store_config: DataStoreConfig = section_config.lookup("datastore", store)?;
>   
>       if store_config
> -        .get_maintenance_mode()
> +        .get_maintenance_mode()?
>           .is_none_or(|m| m.ty != maintenance_type)
>       {
>           bail!("maintenance mode is not '{maintenance_type}'");
> diff --git a/src/server/metric_collection/mod.rs b/src/server/metric_collection/mod.rs
> index 18625b1a5..299156f8e 100644
> --- a/src/server/metric_collection/mod.rs
> +++ b/src/server/metric_collection/mod.rs
> @@ -286,10 +286,9 @@ fn collect_disk_stats_sync() -> (DiskStat, Vec<DatastoreStats>) {
>                   .unwrap_or_default();
>   
>               for config in datastore_list {
> -                if config
> -                    .get_maintenance_mode()
> -                    .is_some_and(|mode| mode.check(Operation::Read).is_err())
> -                {
> +                if config.get_maintenance_mode().map_or(true, |m| {
> +                    m.is_some_and(|mode| mode.check(Operation::Read).is_err())
> +                }) {
>                       continue;
>                   }
>