[pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[Daniel Tschlatscher]

The rrd data now includes tracking the total disk usage for the unpri-
vileged backup user. The calculation for the estimated_time_full was
adapted to use the total for the unpriviliged user.

The unpriv_total is the sum of the used space in the file system, plus
the available space for the unpriviliged user.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
Reviewed-by: Matthias Heiserer <m.heiserer@proxmox.com>
---
No changes from v3

 src/api2/status.rs              | 3 ++-
 src/bin/proxmox-backup-proxy.rs | 2 ++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/api2/status.rs b/src/api2/status.rs
index 50d401c2..b918156e 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -84,7 +84,8 @@ pub async fn datastore_status(
         let get_rrd =
             |what: &str| extract_rrd_data(&rrd_dir, what, RRDTimeFrame::Month, RRDMode::Average);
 
-        let total_res = get_rrd("total")?;
+        // Use the space for the unpriviliged user, as e.g. ext4 reserves 5% of disks for root only
+        let total_res = get_rrd("unpriv_total")?;
         let used_res = get_rrd("used")?;
 
         if let (
diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
index 214bc9b1..e5c8813f 100644
--- a/src/bin/proxmox-backup-proxy.rs
+++ b/src/bin/proxmox-backup-proxy.rs
@@ -1273,6 +1273,8 @@ fn rrd_update_disk_stat(disk: &DiskStat, rrd_prefix: &str) {
         rrd_update_gauge(&rrd_key, status.total as f64);
         let rrd_key = format!("{}/used", rrd_prefix);
         rrd_update_gauge(&rrd_key, status.used as f64);
+        let rrd_key = format!("{}/unpriv_total", rrd_prefix);
+        rrd_update_gauge(&rrd_key, (status.used + status.available) as f64);
     }
 
     if let Some(stat) = &disk.dev {
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v4 2/3] expose the unpriviliged total in the api and use it in the GUI

[Daniel Tschlatscher]

Until now, the total size for a datastore was reported as the total
space on the filesystem. On ext4 filesystems this number will not
match the observed behaviour when some amount of blocks are reserved
though, as the proxmox-backup-proxy uses the unpriviliged 'backup'
user. Therefore backup-ing new data would fail, even though the GUI
still displays that X% of the datastore is free.
I think using the unpriviliged total makes more sense as it
communicates to the user how much they can actually still store on the
datastore, rather than the full total of which some part might not be
usable.

This will also not lead to issues in the GUI when the reserved space
is written to, because the value reported by statfs will automatically
increase accordingly.
I.e. when the unprivliged space is full and a 500MB file is written by
the root user, the fields for "unpriv_total" and "used" will both
increase by this amount, simply keeping the usage at 100%.

Note: I've opted to create a new field in the API result for the
unpriviliged total of the datastore, as overwriting the existing total
might break compatibility for users who retrieve the actual datastore
disk size this way.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
No changes from v3

 pbs-api-types/src/datastore.rs        | 5 +++++
 src/api2/admin/datastore.rs           | 3 +++
 src/api2/status.rs                    | 1 +
 www/dashboard/DataStoreStatistics.js  | 6 +++---
 www/datastore/DataStoreListSummary.js | 4 ++--
 5 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/pbs-api-types/src/datastore.rs b/pbs-api-types/src/datastore.rs
index 0af11b33..d1730715 100644
--- a/pbs-api-types/src/datastore.rs
+++ b/pbs-api-types/src/datastore.rs
@@ -1235,6 +1235,8 @@ pub struct GarbageCollectionStatus {
 pub struct DataStoreStatus {
     /// Total space (bytes).
     pub total: u64,
+    /// Total space available to unpriviliged users (bytes)
+    pub unpriv_total: u64,
     /// Used space (bytes).
     pub used: u64,
     /// Available space (bytes).
@@ -1269,6 +1271,8 @@ pub struct DataStoreStatusListItem {
     pub store: String,
     /// The Size of the underlying storage in bytes. (-1 on error)
     pub total: i64,
+    /// The total space available to unpriviliged users (-1 on error)
+    pub unpriv_total: i64,
     /// The used bytes of the underlying storage. (-1 on error)
     pub used: i64,
     /// The available bytes of the underlying storage. (-1 on error)
@@ -1301,6 +1305,7 @@ impl DataStoreStatusListItem {
         DataStoreStatusListItem {
             store: store.to_owned(),
             total: -1,
+            unpriv_total: -1,
             used: -1,
             avail: -1,
             history: None,
diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 7b103fb7..cd5d01e8 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -715,12 +715,14 @@ pub async fn status(
             total: storage.total,
             used: storage.used,
             avail: storage.available,
+            unpriv_total: storage.used + storage.available,
             gc_status,
             counts,
         }
     } else {
         DataStoreStatus {
             total: 0,
+            unpriv_total: 0,
             used: 0,
             avail: 0,
             gc_status,
@@ -1804,6 +1806,7 @@ pub fn get_rrd_stats(
 
     let mut rrd_fields = vec![
         "total",
+        "unpriv_total",
         "used",
         "read_ios",
         "read_bytes",
diff --git a/src/api2/status.rs b/src/api2/status.rs
index b918156e..26ca750c 100644
--- a/src/api2/status.rs
+++ b/src/api2/status.rs
@@ -69,6 +69,7 @@ pub async fn datastore_status(
         let mut entry = DataStoreStatusListItem {
             store: store.clone(),
             total: status.total as i64,
+            unpriv_total: (status.used + status.available) as i64,
             used: status.used as i64,
             avail: status.available as i64,
             history: None,
diff --git a/www/dashboard/DataStoreStatistics.js b/www/dashboard/DataStoreStatistics.js
index 38f7a2fe..8dbd1caf 100644
--- a/www/dashboard/DataStoreStatistics.js
+++ b/www/dashboard/DataStoreStatistics.js
@@ -3,7 +3,7 @@ Ext.define('pbs-datastore-statistics', {
 
     fields: [
 	'store',
-	'total',
+	'unpriv_total',
 	'used',
 	'avail',
 	'estimated-full-date',
@@ -27,7 +27,7 @@ Ext.define('pbs-datastore-statistics', {
 	    name: 'usage',
 	    calculate: function(data) {
 		let used = data.used || 0;
-		let total = data.total || 0;
+		let total = data["unpriv-total"] || 0;
 		if (total > 0) {
 		    return used/total;
 		} else {
@@ -78,7 +78,7 @@ Ext.define('PBS.DatastoreStatistics', {
 	},
 	{
 	    text: gettext('Size'),
-	    dataIndex: 'total',
+	    dataIndex: 'unpriv-total',
 	    sortable: true,
 	    width: 90,
 	    renderer: v => v === undefined || v < 0 ? '-' : Proxmox.Utils.format_size(v, true),
diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index c7b67d56..3714528e 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -52,10 +52,10 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
 	    vm.set('maintenance', '');
 	}
 
-	let usage = statusData.used/statusData.total;
+	let usage = statusData.used/statusData["unpriv-total"];
 	let usagetext = Ext.String.format(gettext('{0} of {1}'),
 	    Proxmox.Utils.format_size(statusData.used, true),
-	    Proxmox.Utils.format_size(statusData.total, true),
+	    Proxmox.Utils.format_size(statusData["unpriv-total"], true),
 	);
 
 	let usagePanel = me.lookup('usage');
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup v4 3/3] gui: change reporting of the estimated_time_full to "Full" if no space

[Daniel Tschlatscher]

is left in the datastore. Before, the GUI would report "Never" for the
estimated time full, because the value provided in the backend was in
the past. To get around this, the GUI now report "Full" if the value
for available reaches 0.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* Moved the check for whether to display "Full" into render_estimate

 www/Utils.js                          | 6 +++++-
 www/datastore/DataStoreListSummary.js | 3 ++-
 www/datastore/Summary.js              | 6 +++---
 3 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/www/Utils.js b/www/Utils.js
index ad451c9f..f6d353ef 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -285,7 +285,11 @@ Ext.define('PBS.Utils', {
 	return tokenid.match(/^(.+)!([^!]+)$/)[2];
     },
 
-    render_estimate: function(value) {
+    render_estimate: function(value, metaData, record) {
+	if (record.data.avail === 0) {
+	    return gettext("Full");
+	}
+
 	if (value === undefined) {
 	    return gettext('Not enough data');
 	}
diff --git a/www/datastore/DataStoreListSummary.js b/www/datastore/DataStoreListSummary.js
index 3714528e..a88cacef 100644
--- a/www/datastore/DataStoreListSummary.js
+++ b/www/datastore/DataStoreListSummary.js
@@ -61,7 +61,8 @@ Ext.define('PBS.datastore.DataStoreListSummary', {
 	let usagePanel = me.lookup('usage');
 	usagePanel.updateValue(usage, usagetext);
 
-	let estimate = PBS.Utils.render_estimate(statusData['estimated-full-date']);
+	let estimate = PBS.Utils.render_estimate(statusData['estimated-full-date'], null, { data: statusData });
+
 	vm.set('full', estimate);
 	vm.set('deduplication', PBS.Utils.calculate_dedup_factor(statusData['gc-status']).toFixed(2));
 	vm.set('stillbad', statusData['gc-status']['still-bad']);
diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index 94be9559..4025949c 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -2,7 +2,7 @@ Ext.define('pve-rrd-datastore', {
     extend: 'Ext.data.Model',
     fields: [
 	'used',
-	'total',
+	'unpriv_total',
 	'read_ios',
 	'read_bytes',
 	'write_ios',
@@ -66,7 +66,7 @@ Ext.define('PBS.DataStoreInfo', {
 	    let vm = me.getViewModel();
 
 	    let counts = store.getById('counts').data.value;
-	    let total = store.getById('total').data.value;
+	    let total = store.getById('unpriv-total').data.value;
 	    let used = store.getById('used').data.value;
 
 	    let usage = Proxmox.Utils.render_size_usage(used, total, true);
@@ -236,7 +236,7 @@ Ext.define('PBS.DataStoreSummary', {
 	{
 	    xtype: 'proxmoxRRDChart',
 	    title: gettext('Storage usage (bytes)'),
-	    fields: ['total', 'used'],
+	    fields: ['unpriv_total', 'used'],
 	    fieldTitles: [gettext('Total'), gettext('Storage usage')],
 	},
 	{
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[Thomas Lamprecht]

Am 24/08/2022 um 12:26 schrieb Daniel Tschlatscher:
> The rrd data now includes tracking the total disk usage for the unpri-
> vileged backup user. The calculation for the estimated_time_full was
> adapted to use the total for the unpriviliged user.
> 
> The unpriv_total is the sum of the used space in the file system, plus
> the available space for the unpriviliged user.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> Reviewed-by: Matthias Heiserer <m.heiserer@proxmox.com>
> ---
> No changes from v3
> 
>  src/api2/status.rs              | 3 ++-
>  src/bin/proxmox-backup-proxy.rs | 2 ++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/src/api2/status.rs b/src/api2/status.rs
> index 50d401c2..b918156e 100644
> --- a/src/api2/status.rs
> +++ b/src/api2/status.rs
> @@ -84,7 +84,8 @@ pub async fn datastore_status(
>          let get_rrd =
>              |what: &str| extract_rrd_data(&rrd_dir, what, RRDTimeFrame::Month, RRDMode::Average);
>  
> -        let total_res = get_rrd("total")?;
> +        // Use the space for the unpriviliged user, as e.g. ext4 reserves 5% of disks for root only
> +        let total_res = get_rrd("unpriv_total")?;

But we only just started to even record that, so it's not _that_ ideal to directly switch
as the user will notice a possible big jump after the upgrade. Can we check how many data is
in there to decide which one to use, or do it more heuristically and start recording now but
switch only in a later usage (e.g., the point release after the next one).

>          let used_res = get_rrd("used")?;
>  
>          if let (
> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
> index 214bc9b1..e5c8813f 100644
> --- a/src/bin/proxmox-backup-proxy.rs
> +++ b/src/bin/proxmox-backup-proxy.rs
> @@ -1273,6 +1273,8 @@ fn rrd_update_disk_stat(disk: &DiskStat, rrd_prefix: &str) {
>          rrd_update_gauge(&rrd_key, status.total as f64);
>          let rrd_key = format!("{}/used", rrd_prefix);
>          rrd_update_gauge(&rrd_key, status.used as f64);
> +        let rrd_key = format!("{}/unpriv_total", rrd_prefix);
> +        rrd_update_gauge(&rrd_key, (status.used + status.available) as f64);

what speaks against broadcasting available? I mean one got the same information
either way, but mirroring the disk stats and returning total, used, available instead
of "two" totals sounds somewhat nicer to be from an API-User POV, but no hard feelings
here, just stuck quite a bit out to me after reviewing this series finally.

>      }
>  
>      if let Some(stat) = &disk.dev {

Re: [pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[Daniel Tschlatscher]

On 10/5/22 17:56, Thomas Lamprecht wrote:
> Am 24/08/2022 um 12:26 schrieb Daniel Tschlatscher:
>> The rrd data now includes tracking the total disk usage for the unpri-
>> vileged backup user. The calculation for the estimated_time_full was
>> adapted to use the total for the unpriviliged user.
>>
>> The unpriv_total is the sum of the used space in the file system, plus
>> the available space for the unpriviliged user.
>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>> Reviewed-by: Matthias Heiserer <m.heiserer@proxmox.com>
>> ---
>> No changes from v3
>>
>>  src/api2/status.rs              | 3 ++-
>>  src/bin/proxmox-backup-proxy.rs | 2 ++
>>  2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/api2/status.rs b/src/api2/status.rs
>> index 50d401c2..b918156e 100644
>> --- a/src/api2/status.rs
>> +++ b/src/api2/status.rs
>> @@ -84,7 +84,8 @@ pub async fn datastore_status(
>>          let get_rrd =
>>              |what: &str| extract_rrd_data(&rrd_dir, what, RRDTimeFrame::Month, RRDMode::Average);
>>  
>> -        let total_res = get_rrd("total")?;
>> +        // Use the space for the unpriviliged user, as e.g. ext4 reserves 5% of disks for root only
>> +        let total_res = get_rrd("unpriv_total")?;
> 
> But we only just started to even record that, so it's not _that_ ideal to directly switch
> as the user will notice a possible big jump after the upgrade. Can we check how many data is
> in there to decide which one to use, or do it more heuristically and start recording now but
> switch only in a later usage (e.g., the point release after the next one).
> 

It seems to me, finding a way to tell which dataset to use for the
calculation is the preferable way here, as the heuristic approach of
recording it anyway and switching at a later date would show the same
jump for users upgrading from an earlier version or in cases where the
RRDB did not have enough time to populate between versions.
Or would you reckon this is acceptable in such (less frequent) cases, as
it is more of a cosmetic feature anyway?

>>          let used_res = get_rrd("used")?;
>>  
>>          if let (
>> diff --git a/src/bin/proxmox-backup-proxy.rs b/src/bin/proxmox-backup-proxy.rs
>> index 214bc9b1..e5c8813f 100644
>> --- a/src/bin/proxmox-backup-proxy.rs
>> +++ b/src/bin/proxmox-backup-proxy.rs
>> @@ -1273,6 +1273,8 @@ fn rrd_update_disk_stat(disk: &DiskStat, rrd_prefix: &str) {
>>          rrd_update_gauge(&rrd_key, status.total as f64);
>>          let rrd_key = format!("{}/used", rrd_prefix);
>>          rrd_update_gauge(&rrd_key, status.used as f64);
>> +        let rrd_key = format!("{}/unpriv_total", rrd_prefix);
>> +        rrd_update_gauge(&rrd_key, (status.used + status.available) as f64);
> 
> what speaks against broadcasting available? I mean one got the same information
> either way, but mirroring the disk stats and returning total, used, available instead
> of "two" totals sounds somewhat nicer to be from an API-User POV, but no hard feelings
> here, just stuck quite a bit out to me after reviewing this series finally.
> 

Mostly, it made it easier to just drop in place instead of the value for
total, rather than querying combining the data for used and available. I
didn't overwrite it also, as that would be a breaking change.

In my eyes, it is also more easily understandable from the POV of an API
user because the naming for available does not inherently carry the
information that this value excludes any unusable storage for
unprivileged users.
I see potential for confusion here, as one might incorrectly assume that
used + available = total, when that is, in my testing, really never the
case. Potentially, because of reserved blocks on ext4 file systems and
because of a small amount which seems to be claimed by the kernel.

>>      }
>>  
>>      if let Some(stat) = &disk.dev {
> 

Re: [pbs-devel] [PATCH proxmox-backup v4 1/3] fix #4077: Estimated Full metric on ext4 file systems

[Thomas Lamprecht]

Am 10/10/2022 um 16:23 schrieb Daniel Tschlatscher:
> It seems to me, finding a way to tell which dataset to use for the
> calculation is the preferable way here, as the heuristic approach of
> recording it anyway and switching at a later date would show the same
> jump for users upgrading from an earlier version or in cases where the
> RRDB did not have enough time to populate between versions.

sure, if you don't add a dynamic check on runtime, but the one that update
are often those that update frequently or on every point release, so it
probably would cover a major part of users - albeit, that's rather
guesstimation and it's also a burden to keep track and communicate to users,
so probably better to either mix in dynamically, if new value hasn't enough
data but old value has, or just ignore it altogether; I'd at least try to do
the former, if it's a lot of complexity we can still drop it.


> Or would you reckon this is acceptable in such (less frequent) cases, as
> it is more of a cosmetic feature anyway?

certainly no deal breaker, not sure how much its actually used, or relied
on in practice anyway.

> Mostly, it made it easier to just drop in place instead of the value for
> total, rather than querying combining the data for used and available. I
> didn't overwrite it also, as that would be a breaking change.
> 
> In my eyes, it is also more easily understandable from the POV of an API
> user because the naming for available does not inherently carry the
> information that this value excludes any unusable storage for
> unprivileged users.
> I see potential for confusion here, as one might incorrectly assume that
> used + available = total, when that is, in my testing, really never the
> case. Potentially, because of reserved blocks on ext4 file systems and
> because of a small amount which seems to be claimed by the kernel.
> 

not convinced. df or findmnt --df all show size (total), used and avail, and
those tools a pretty much standard since decades. total_unpriv is really not
_that_ telling if one comes from the outside, as it isn't clear what unrpiv
refers to at all IMO and why there are two totals, if they should be sumed
together (i.e., is total the one for priv + unrpiv or just priv)...
I'd rather just stick to established defaults/terms and avoid inventing new
ones.