Re: [pbs-devel] [PATCH proxmox-backup v3 10/24] pbs-api-types: add removable/is-available flag to DataStoreListItem

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

there is a typo in commit title, s/scheme/schema
also maybe you can explicitly include the usage of `SchemaDeserializer` 
by stating something like:

`pbs-api-types: use SchemaDeserializer for maintenance mode`

and a comment inline

On 4/9/24 12:59, Hannes Laimer wrote:
> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
> ---
>   pbs-api-types/src/datastore.rs |  7 ++++++-
>   src/api2/admin/datastore.rs    |  1 +
>   src/api2/status.rs             | 18 +++++++++++++++---
>   3 files changed, 22 insertions(+), 4 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index 738ba96f..ce53c375 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -390,6 +390,8 @@ impl DataStoreConfig {
>   pub struct DataStoreListItem {
>       pub store: String,
>       pub comment: Option<String>,
> +    /// Datastore is removable
> +    pub removable: bool,
>       /// If the datastore is in maintenance mode, information about it
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub maintenance: Option<String>,
> @@ -1357,6 +1359,8 @@ pub struct DataStoreStatusListItem {
>       /// The available bytes of the underlying storage. (-1 on error)
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub avail: Option<u64>,
> +    /// The datastore is available, relevant if removable
> +    pub is_available: bool,
>       /// A list of usages of the past (last Month).
>       #[serde(skip_serializing_if = "Option::is_none")]
>       pub history: Option<Vec<Option<f64>>>,
> @@ -1381,12 +1385,13 @@ pub struct DataStoreStatusListItem {
>   }
>   
>   impl DataStoreStatusListItem {
> -    pub fn empty(store: &str, err: Option<String>) -> Self {
> +    pub fn empty(store: &str, err: Option<String>, is_available: bool) -> Self {
>           DataStoreStatusListItem {
>               store: store.to_owned(),
>               total: None,
>               used: None,
>               avail: None,
> +            is_available,

this could default to true, and instead of passing the `is_available` 
flag as argument. That could be set by a dedicates
`fn datastore_unavailable(self) -> Self` method for 
`DataStoreStatusListItem`, which takes the list item, sets the flag and 
returns it to be pushed to the list, so the opaque argument to `empty` 
is avoided.

So you would get:
`list.push(DataStoreStatusListItem::empty(store, 
None).datastore_unavailable());`

But not sure about this...

>               history: None,
>               history_start: None,
>               history_delta: None,
> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
> index 92f6adc2..acf19fa2 100644
> --- a/src/api2/admin/datastore.rs
> +++ b/src/api2/admin/datastore.rs
> @@ -1245,6 +1245,7 @@ pub fn get_datastore_list(
>                   } else {
>                       data["comment"].as_str().map(String::from)
>                   },
> +                removable: data["backing-device"].as_str().is_some(),
>                   maintenance: data["maintenance-mode"].as_str().map(String::from),
>               });
>           }
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index 78bc06b5..43f0b55d 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -13,7 +13,7 @@ use pbs_api_types::{
>   };
>   
>   use pbs_config::CachedUserInfo;
> -use pbs_datastore::DataStore;
> +use pbs_datastore::{check_if_available, DataStore};
>   
>   use crate::rrd_cache::extract_rrd_data;
>   use crate::tools::statistics::linear_regression;
> @@ -48,10 +48,17 @@ pub async fn datastore_status(
>       for (store, (_, _)) in &config.sections {
>           let user_privs = user_info.lookup_privs(&auth_id, &["datastore", store]);
>           let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
> +
> +        let store_config = config.lookup("datastore", store)?;
> +        if check_if_available(&store_config).is_err() {
> +            list.push(DataStoreStatusListItem::empty(store, None, false));
> +            continue;
> +        }
> +
>           if !allowed {
>               if let Ok(datastore) = DataStore::lookup_datastore(store, Some(Operation::Lookup)) {
>                   if can_access_any_namespace(datastore, &auth_id, &user_info) {
> -                    list.push(DataStoreStatusListItem::empty(store, None));
> +                    list.push(DataStoreStatusListItem::empty(store, None, true));
>                   }
>               }
>               continue;
> @@ -60,7 +67,11 @@ pub async fn datastore_status(
>           let datastore = match DataStore::lookup_datastore(store, Some(Operation::Read)) {
>               Ok(datastore) => datastore,
>               Err(err) => {
> -                list.push(DataStoreStatusListItem::empty(store, Some(err.to_string())));
> +                list.push(DataStoreStatusListItem::empty(
> +                    store,
> +                    Some(err.to_string()),
> +                    true,
> +                ));
>                   continue;
>               }
>           };
> @@ -71,6 +82,7 @@ pub async fn datastore_status(
>               total: Some(status.total),
>               used: Some(status.used),
>               avail: Some(status.available),
> +            is_available: true,
>               history: None,
>               history_start: None,
>               history_delta: None,