Re: [pbs-devel] [PATCH v2 proxmox-backup 2/2] status: use Option on avail/used datastore attrs

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 11/12/2023 um 09:59 schrieb Gabriel Goller:
> Instead of returning -1 if we can't get the attributes, we use an
> Option which will not be serialized on `None`.
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>  pbs-api-types/src/datastore.rs | 19 +++++++++++--------
>  src/api2/status.rs             |  6 +++---
>  2 files changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
> index d4ead1d1..74f610d1 100644
> --- a/pbs-api-types/src/datastore.rs
> +++ b/pbs-api-types/src/datastore.rs
> @@ -1302,12 +1302,15 @@ pub struct DataStoreStatus {
>  /// Status of a Datastore
>  pub struct DataStoreStatusListItem {
>      pub store: String,
> -    /// The Size of the underlying storage in bytes. (-1 on error)
> -    pub total: i64,
> -    /// The used bytes of the underlying storage. (-1 on error)
> -    pub used: i64,
> +    /// The Size of the underlying storage in bytes.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub total: Option<u64>,
> +    /// The used bytes of the underlying storage.
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub used: Option<u64>,
>      /// The available bytes of the underlying storage. (-1 on error)
> -    pub avail: i64,
> +    #[serde(skip_serializing_if = "Option::is_none")]
> +    pub avail: Option<u64>,
>      /// A list of usages of the past (last Month).
>      #[serde(skip_serializing_if = "Option::is_none")]
>      pub history: Option<Vec<Option<f64>>>,

Talked a bit with Dietmar off-list and checked this out a bit closer, having
to check each property separately seems like slightly annoying extra work we
might be able to avoid while not loosing anything.

I.e., what about keeping this as is and instead add a new usage property that
is an option of a struct with all three values as u64?

We get this info from file-system level, so if we cannot get all at once it'd
be rather wrong anyway (from both our access control system and how we gather
those data from the kernel), so I think this is an all or nothing.

We could then can mark the other three i64 properties as deprecated and remove
them with the next major version, thus having a clean compatibility cut and a
easier to use API.


Not directly related, but while at it we could also improve how such things are
marked as "to be removed at version X).
In PVE we use FIXME comments with a (roughly) common style, but that is naturally
error-prone, but here we have a full compilation step, so we could add a warning
that is only emitted if the crate version is >= next-major version (and possibly
some "future-deprecation feature to more easily check for them manually), and thus
find all of those occurrences easily.