Re: [pbs-devel] [PATCH proxmox-backup 2/5] pbs-datastore: add check_maintenence function

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

On Tue, Sep 28, 2021 at 12:05:45PM +0200, Hannes Laimer wrote:
> This function checks if something is allowed in a specific maintenance
> type, it has to be called whenever something depends on maintenance to
> be off or less rescrictive than a specific type.
> ---
>  pbs-datastore/src/datastore.rs | 18 +++++++++++++++++-
>  1 file changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/pbs-datastore/src/datastore.rs b/pbs-datastore/src/datastore.rs
> index b068a9c9..1014b357 100644
> --- a/pbs-datastore/src/datastore.rs
> +++ b/pbs-datastore/src/datastore.rs
> @@ -11,7 +11,7 @@ use lazy_static::lazy_static;
>  
>  use proxmox::tools::fs::{replace_file, file_read_optional_string, CreateOptions};
>  
> -use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus};
> +use pbs_api_types::{UPID, DataStoreConfig, Authid, GarbageCollectionStatus, MaintenanceType};
>  use pbs_tools::format::HumanByte;
>  use pbs_tools::fs::{lock_dir_noblock, DirLockGuard};
>  use pbs_tools::process_locker::ProcessLockSharedGuard;
> @@ -98,6 +98,22 @@ impl DataStore {
>          Ok(())
>      }
>  
> +    /// checks if the current maintenace mode of the datastore allows something
> +    pub fn check_maintenance(&self, need_less_than: MaintenanceType) -> Result<(), Error> {

I'd very much like the method name itself to suggest the direction of
such a check, rather than have only the parameter name tell you this,
not even the doc comment ;-)
When you read `datastore.check_maintenance(foo)` the parameter name
isn't actually visible in code, but the code should speak for itself.

And like Dominik said, it's a confusing interface. Plus, I can't think
if a really good name for the method that fixes my remark above.

So maybe the public facing interface should just be:

    pub fn can_read(&self) -> bool;
    pub fn can_write(&self) -> bool;

and/or

    pub fn check_read(&self) -> Result<(), Error>;
    pub fn check_write(&self) -> Result<(), Error>;

with the error including the maintenance message

> +        let (config, _digest) = pbs_config::datastore::config()?;
> +        let config: DataStoreConfig = config.lookup("datastore", &self.name())?;
> +        if let Some(maintenance_type) = config.maintenance_type {
> +            if maintenance_type >= need_less_than {
> +                bail!("Datastore '{}' is in maintenance {} mode: {}", 
> +                    &self.name(),
> +                    maintenance_type,
> +                    config.maintenance_msg.unwrap_or(String::from("no description"))
> +                ); 
> +            }
> +        }
> +        Ok(())
> +    }
> +
>      fn open_with_path(store_name: &str, path: &Path, config: DataStoreConfig) -> Result<Self, Error> {
>          let chunk_store = ChunkStore::open(store_name, path)?;
>  
> -- 
> 2.30.2