Re: [pbs-devel] [PATCH proxmox-backup v13 09/26] pbs-api-types: add mount_status field to DataStoreListItem

["Hannes Laimer" <h.laimer@proxmox.com>]

On Thu Nov 21, 2024 at 3:27 PM CET, Fabian Grünbichler wrote:
> On November 13, 2024 4:00 pm, Hannes Laimer wrote:
>> Only removable datastores have a mount status, so normal ones will have
>> `None`, and for removable ones it is either mounted (`Some(true)`) or
>> not mounted (`Some(false)`).
>> 
>> Signed-off-by: Hannes Laimer <h.laimer@proxmox.com>
>> ---
>> changes since v12:
>>  * replace is_availabl+removable field combo, with single mount_status
>>     field
>> 
>>  pbs-api-types/src/datastore.rs |  9 ++++++++-
>>  src/api2/admin/datastore.rs    | 22 ++++++++++++++--------
>>  src/api2/status/mod.rs         | 29 +++++++++++++++++++++++++----
>>  3 files changed, 47 insertions(+), 13 deletions(-)
>> 
>> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
>> index 888f5d5b..e111d692 100644
>> --- a/pbs-api-types/src/datastore.rs
>> +++ b/pbs-api-types/src/datastore.rs
>> @@ -454,6 +454,9 @@ impl DataStoreConfig {
>>  pub struct DataStoreListItem {
>>      pub store: String,
>>      pub comment: Option<String>,
>> +    /// Is datastore mounted, None for not-removable datastores
>> +    #[serde(skip_serializing_if = "Option::is_none")]
>> +    pub mount_status: Option<bool>,
>
> Option<bool> is okay for internal usage, but in an api type, wouldn't a
> proper enum be nicer?
>
> NonRemovable, Mounted, NotMounted
>

I had, but `NonRemovable` kind of bothered me since it is not really a
mount status. But what you are saying makes sense. 

>>      /// If the datastore is in maintenance mode, information about it
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub maintenance: Option<String>,
>> @@ -1453,6 +1456,9 @@ 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 mounted, None for not-removable datastores
>> +    #[serde(skip_serializing_if = "Option::is_none")]
>> +    pub mount_status: Option<bool>,
>
> Option<bool> is okay for internal usage, but in an api type, wouldn't a
> proper enum be nicer? also would allow differentiating datastore types
> more easily in client code (if just for display purposes)
>
> NonRemovable, Mounted, NotMounted
>
>>      /// A list of usages of the past (last Month).
>>      #[serde(skip_serializing_if = "Option::is_none")]
>>      pub history: Option<Vec<Option<f64>>>,
>> @@ -1477,12 +1483,13 @@ pub struct DataStoreStatusListItem {
>>  }
>>  
>>  impl DataStoreStatusListItem {
>> -    pub fn empty(store: &str, err: Option<String>) -> Self {
>> +    pub fn empty(store: &str, err: Option<String>, mount_status: Option<bool>) -> Self {
>>          DataStoreStatusListItem {
>>              store: store.to_owned(),
>>              total: None,
>>              used: None,
>>              avail: None,
>> +            mount_status,
>>              history: None,
>>              history_start: None,
>>              history_delta: None,
>> diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
>> index a12262e7..a9d9040f 100644
>> --- a/src/api2/admin/datastore.rs
>> +++ b/src/api2/admin/datastore.rs
>> @@ -1310,8 +1310,8 @@ pub fn get_datastore_list(
>>  
>>      let mut list = Vec::new();
>>  
>> -    for (store, (_, data)) in &config.sections {
>> -        let acl_path = &["datastore", store];
>> +    for (store, (_, data)) in config.sections {
>> +        let acl_path = &["datastore", &store];
>>          let user_privs = user_info.lookup_privs(&auth_id, acl_path);
>>          let allowed = (user_privs & (PRIV_DATASTORE_AUDIT | PRIV_DATASTORE_BACKUP)) != 0;
>>  
>> @@ -1322,15 +1322,21 @@ pub fn get_datastore_list(
>>              }
>>          }
>>  
>> +        let store_config: DataStoreConfig = serde_json::from_value(data)?;
>> +
>> +        let mount_status = store_config
>> +            .get_mount_point()
>> +            .zip(store_config.backing_device.as_ref())
>> +            .map(|(mount_point, device_uuid)| {
>> +                is_datastore_mounted_at(mount_point, device_uuid.to_string())
>> +            });
>
> another variant of this helper ;)
>
>> +
>>          if allowed || allow_id {
>>              list.push(DataStoreListItem {
>>                  store: store.clone(),
>> -                comment: if !allowed {
>> -                    None
>> -                } else {
>> -                    data["comment"].as_str().map(String::from)
>> -                },
>> -                maintenance: data["maintenance-mode"].as_str().map(String::from),
>> +                comment: store_config.comment.filter(|_| allowed),
>> +                mount_status,
>> +                maintenance: store_config.maintenance_mode,
>>              });
>>          }
>>      }
>> diff --git a/src/api2/status/mod.rs b/src/api2/status/mod.rs
>> index 113aa985..508331fe 100644
>> --- a/src/api2/status/mod.rs
>> +++ b/src/api2/status/mod.rs
>> @@ -10,11 +10,12 @@ use proxmox_schema::api;
>>  use proxmox_sortable_macro::sortable;
>>  
>>  use pbs_api_types::{
>> -    Authid, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT, PRIV_DATASTORE_BACKUP,
>> +    Authid, DataStoreConfig, DataStoreStatusListItem, Operation, PRIV_DATASTORE_AUDIT,
>> +    PRIV_DATASTORE_BACKUP,
>>  };
>>  
>>  use pbs_config::CachedUserInfo;
>> -use pbs_datastore::DataStore;
>> +use pbs_datastore::{is_datastore_mounted_at, DataStore};
>>  
>>  use crate::server::metric_collection::rrd::extract_rrd_data;
>>  use crate::tools::statistics::linear_regression;
>> @@ -51,10 +52,25 @@ 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::<DataStoreConfig>("datastore", store)?;
>> +
>> +        let mount_status = store_config
>> +            .get_mount_point()
>> +            .zip(store_config.backing_device.as_ref())
>> +            .map(|(mount_point, device_uuid)| {
>> +                is_datastore_mounted_at(mount_point, device_uuid.to_string())
>> +            });
>> +
>> +        if let Some(false) = mount_status {
>> +            list.push(DataStoreStatusListItem::empty(store, None, mount_status));
>> +            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, mount_status));
>>                  }
>>              }
>>              continue;
>> @@ -63,7 +79,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()),
>> +                    mount_status,
>> +                ));
>>                  continue;
>>              }
>>          };
>> @@ -74,6 +94,7 @@ pub async fn datastore_status(
>>              total: Some(status.total),
>>              used: Some(status.used),
>>              avail: Some(status.available),
>> +            mount_status,
>>              history: None,
>>              history_start: None,
>>              history_delta: None,
>> -- 
>> 2.39.5
>> 
>> 
>> 
>> _______________________________________________
>> 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



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