[pbs-devel] [PATCH v2 proxmox-backup 0/2] Fix DatastoreListSummary on limited permissions

[pbs-devel] [PATCH v2 proxmox-backup 0/2] Fix DatastoreListSummary on limited permissions

[Gabriel Goller]

When the user has limited permissions on a datastore (e.g `NoAccess` on the whole 
datastore and `DatastoreBackup` on a namespace inside the datastore) the Datastore
List shows weird values. 

Instead of returning -1 we return an Option and handle missing values on the
frontend.
  
Changes version 2:
 - Use u64 instead of i64, as value is never negative.



proxmox-backup:

Gabriel Goller (2):
  ui: datastore summary handle non-existent values
  status: use Option on avail/used datastore attrs

 pbs-api-types/src/datastore.rs        | 19 +++++++++++--------
 src/api2/status.rs                    |  6 +++---
 www/datastore/DataStoreListSummary.js | 20 ++++++++++++++------
 3 files changed, 28 insertions(+), 17 deletions(-)


Summary over all repositories:
  3 files changed, 28 insertions(+), 17 deletions(-)

-- 
murpp v0.4.0

[pbs-devel] [PATCH v2 proxmox-backup 1/2] ui: datastore summary handle non-existent values

[Gabriel Goller]

Correctly display missing 'avail' and 'used' attributes in the
datatstore summary. This simply sets it to 0, so that we don't get any
errors in the console.

Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/datastore/DataStoreListSummary.js | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index 968239b0..b908034d 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -52,12 +52,20 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
 	    vm.set('maintenance', '');
 	}
 
-	let total = statusData.avail + statusData.used;
-	let usage = statusData.used / total;
-	let usagetext = Ext.String.format(gettext('{0} of {1}'),
-	    Proxmox.Utils.format_size(statusData.used, true),
-	    Proxmox.Utils.format_size(total, true),
-	);
+	let usagetext;
+	let usage;
+
+	if (Object.hasOwn(statusData, 'avail') && Object.hasOwn(statusData, 'used')) {
+	    let total = statusData.avail + statusData.used;
+	    usage = statusData.used / total;
+	    usagetext = Ext.String.format(gettext('{0} of {1}'),
+		Proxmox.Utils.format_size(statusData.used, true),
+		Proxmox.Utils.format_size(total, true),
+	    );
+	} else {
+	    usagetext = Ext.String.format(gettext('{0} of {1}'), 0, 0);
+	    usage = 0;
+	}
 
 	let usagePanel = me.lookup('usage');
 	usagePanel.updateValue(usage, usagetext);
-- 
2.39.2

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

[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>>>,
@@ -1335,9 +1338,9 @@ impl DataStoreStatusListItem {
     pub fn empty(store: &str, err: Option<String>) -> Self {
         DataStoreStatusListItem {
             store: store.to_owned(),
-            total: -1,
-            used: -1,
-            avail: -1,
+            total: None,
+            used: None,
+            avail: None,
             history: None,
             history_start: None,
             history_delta: None,
diff --git a/src/api2/status.rs b/src/api2/status.rs
index ff7d4cbd..78bc06b5 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -68,9 +68,9 @@ pub async fn datastore_status(
 
         let mut entry = DataStoreStatusListItem {
             store: store.clone(),
-            total: status.total as i64,
-            used: status.used as i64,
-            avail: status.available as i64,
+            total: Some(status.total),
+            used: Some(status.used),
+            avail: Some(status.available),
             history: None,
             history_start: None,
             history_delta: None,
-- 
2.39.2

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

[Thomas Lamprecht]

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.

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

[Gabriel Goller]

On 12/14/23 08:55, Thomas Lamprecht wrote:
> Am 11/12/2023 um 09:59 schrieb Gabriel Goller:
>> [..]
> 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.
I like this idea.
> 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.
Hmm I looked into this a little bit and wrote a quick macro inspired by this
crate [0]. It's basically a `proc_macro_attribute` that takes a specific 
semver condition
and gets the current crate version using the `CARGO_PKG_VERSION` 
environment variable.
Then if the version matches, it adds a `#[deprecated]` attribute or 
panics (which produces
a compiler error). For example:

#[proxmox_api_macro::deprecate_from(remove = ">= 4.x.x", note = "remove 
total and use total_new")]
pub struct DataStoreStatusListItem {
     pub total: i64,
     pub total_new: Option<u64>,
}

This won't change anything currently, but will deprecate the struct with 
the specified
message when the version hits `>=4.x.x`.

This would be perfect, but it has two problems:
  1) This is a `proc_macro_attribute`, so it can only be set on the 
struct/function and not on a
     specific field. I don't think this is possible to implement using a 
derive macro (although
     please correct me if I'm wrong) and using a function-style macro 
would be kinda ugly, cause we would
     have to write (If this is even possible):
     deprecate_from!(pub total: i64, remove=">=4.x.x")
     Although I don't think this is necessarily a deal breaker, cause we 
can use this to simply trigger
     the deprecation notice + nearly always we have to change multiple 
fields in the struct to remove a field.

  2) The bigger problem is that we (AFAIK) can't get the workspace 
package version inside the macro. `CARGO_PKG_VERSION`
     only returns the version of the crate where the invocation is in. 
For example for the example above, the version is
     `0.1.0`, the version of `pbs-api-types`.

Let me know what you think!

[0]: https://crates.io/crates/deprecate_until

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

[Gabriel Goller]

On 14-12-2023 08:55, Thomas Lamprecht wrote:
> 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.
It looks like this series has been applied already, was there any off-list
discussion? Do we still need the backwards compat (and above suggested 
changes)
here?
If yes, should I submit a new patch and base it off master, or submit a 
new version?

[pbs-devel] applied: [PATCH v2 proxmox-backup 0/2] Fix DatastoreListSummary on limited permissions

[Dietmar Maurer]

applied, thanks!

> On 11.12.2023 09:59 CET Gabriel Goller <g.goller@proxmox.com> wrote:
> 
>  
> When the user has limited permissions on a datastore (e.g `NoAccess` on the whole 
> datastore and `DatastoreBackup` on a namespace inside the datastore) the Datastore
> List shows weird values. 
> 
> Instead of returning -1 we return an Option and handle missing values on the
> frontend.
>   
> Changes version 2:
>  - Use u64 instead of i64, as value is never negative.
> 
> 
> 
> proxmox-backup:
> 
> Gabriel Goller (2):
>   ui: datastore summary handle non-existent values
>   status: use Option on avail/used datastore attrs
> 
>  pbs-api-types/src/datastore.rs        | 19 +++++++++++--------
>  src/api2/status.rs                    |  6 +++---
>  www/datastore/DataStoreListSummary.js | 20 ++++++++++++++------
>  3 files changed, 28 insertions(+), 17 deletions(-)
> 
> 
> Summary over all repositories:
>   3 files changed, 28 insertions(+), 17 deletions(-)
> 
> -- 
> murpp v0.4.0
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel