[pbs-devel] [PATCH proxmox-backup 1/5] tape/pool_writer: give proper types to 'contains_snapshot'

[pbs-devel] [PATCH proxmox-backup 1/5] tape/pool_writer: give proper types to 'contains_snapshot'

[Dominik Csapak]

instead of a string. The underlying catalog implementation has to
care about how this is formatted, not the external caller

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/backup.rs             | 12 ++++++++++--
 src/tape/media_catalog.rs           | 19 +++++++++++++++----
 src/tape/pool_writer/catalog_set.rs | 12 +++++++++---
 src/tape/pool_writer/mod.rs         |  9 +++++++--
 4 files changed, 41 insertions(+), 11 deletions(-)

diff --git a/src/api2/tape/backup.rs b/src/api2/tape/backup.rs
index c7d042fb..ba08994f 100644
--- a/src/api2/tape/backup.rs
+++ b/src/api2/tape/backup.rs
@@ -496,7 +496,11 @@ fn backup_worker(
             if let Some(info) = snapshot_list.pop() {
                 let rel_path =
                     print_ns_and_snapshot(info.backup_dir.backup_ns(), info.backup_dir.as_ref());
-                if pool_writer.contains_snapshot(datastore_name, &rel_path) {
+                if pool_writer.contains_snapshot(
+                    datastore_name,
+                    &info.backup_dir.backup_ns(),
+                    info.backup_dir.as_ref(),
+                ) {
                     task_log!(worker, "skip snapshot {}", rel_path);
                     continue;
                 }
@@ -517,7 +521,11 @@ fn backup_worker(
                 let rel_path =
                     print_ns_and_snapshot(info.backup_dir.backup_ns(), info.backup_dir.as_ref());
 
-                if pool_writer.contains_snapshot(datastore_name, &rel_path) {
+                if pool_writer.contains_snapshot(
+                    datastore_name,
+                    &info.backup_dir.backup_ns(),
+                    info.backup_dir.as_ref(),
+                ) {
                     task_log!(worker, "skip snapshot {}", rel_path);
                     continue;
                 }
diff --git a/src/tape/media_catalog.rs b/src/tape/media_catalog.rs
index 85128f80..38fdcc6e 100644
--- a/src/tape/media_catalog.rs
+++ b/src/tape/media_catalog.rs
@@ -411,10 +411,16 @@ impl MediaCatalog {
     }
 
     /// Test if the catalog already contain a snapshot
-    pub fn contains_snapshot(&self, store: &str, snapshot: &str) -> bool {
+    pub fn contains_snapshot(
+        &self,
+        store: &str,
+        ns: &BackupNamespace,
+        snapshot: &BackupDir,
+    ) -> bool {
+        let path = print_ns_and_snapshot(ns, snapshot);
         match self.content.get(store) {
             None => false,
-            Some(content) => content.snapshot_index.contains_key(snapshot),
+            Some(content) => content.snapshot_index.contains_key(&path),
         }
     }
 
@@ -960,9 +966,14 @@ impl MediaSetCatalog {
     }
 
     /// Test if the catalog already contain a snapshot
-    pub fn contains_snapshot(&self, store: &str, snapshot: &str) -> bool {
+    pub fn contains_snapshot(
+        &self,
+        store: &str,
+        ns: &BackupNamespace,
+        snapshot: &BackupDir,
+    ) -> bool {
         for catalog in self.catalog_list.values() {
-            if catalog.contains_snapshot(store, snapshot) {
+            if catalog.contains_snapshot(store, ns, snapshot) {
                 return true;
             }
         }
diff --git a/src/tape/pool_writer/catalog_set.rs b/src/tape/pool_writer/catalog_set.rs
index 1acc108a..d588df18 100644
--- a/src/tape/pool_writer/catalog_set.rs
+++ b/src/tape/pool_writer/catalog_set.rs
@@ -29,13 +29,19 @@ impl CatalogSet {
     }
 
     /// Test if the catalog already contains a snapshot
-    pub fn contains_snapshot(&self, store: &str, snapshot: &str) -> bool {
+    pub fn contains_snapshot(
+        &self,
+        store: &str,
+        ns: &pbs_api_types::BackupNamespace,
+        snapshot: &pbs_api_types::BackupDir,
+    ) -> bool {
         if let Some(ref catalog) = self.catalog {
-            if catalog.contains_snapshot(store, snapshot) {
+            if catalog.contains_snapshot(store, ns, snapshot) {
                 return true;
             }
         }
-        self.media_set_catalog.contains_snapshot(store, snapshot)
+        self.media_set_catalog
+            .contains_snapshot(store, ns, snapshot)
     }
 
     /// Test if the catalog already contains a chunk
diff --git a/src/tape/pool_writer/mod.rs b/src/tape/pool_writer/mod.rs
index d00c16e6..b2ff8620 100644
--- a/src/tape/pool_writer/mod.rs
+++ b/src/tape/pool_writer/mod.rs
@@ -100,11 +100,16 @@ impl PoolWriter {
         Ok(())
     }
 
-    pub fn contains_snapshot(&self, store: &str, snapshot: &str) -> bool {
+    pub fn contains_snapshot(
+        &self,
+        store: &str,
+        ns: &pbs_api_types::BackupNamespace,
+        snapshot: &pbs_api_types::BackupDir,
+    ) -> bool {
         self.catalog_set
             .lock()
             .unwrap()
-            .contains_snapshot(store, snapshot)
+            .contains_snapshot(store, ns, snapshot)
     }
 
     /// Eject media and drop PoolWriterState (close drive)
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 2/5] ui: form/NamespaceSelector: show proper emptyText

[Dominik Csapak]

by filtering out the empty namespace from the api, and putting the
correct div with xclass around the gettext

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/form/NamespaceSelector.js | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/www/form/NamespaceSelector.js b/www/form/NamespaceSelector.js
index 44b7ca0d..c847cd4e 100644
--- a/www/form/NamespaceSelector.js
+++ b/www/form/NamespaceSelector.js
@@ -34,8 +34,8 @@ Ext.define('PBS.form.NamespaceSelector', {
 	minWidth: 170,
 	maxWidth: 500,
 	// below doesn't work :/
-	//minHeight: 30,
-	//emptyText: gettext('No namespaces accesible.'),
+	minHeight: 30,
+	emptyText: `<div class="x-grid-empty">${gettext('No namespaces accessible.')}</div>`,
     },
 
     triggers: {
@@ -82,6 +82,7 @@ Ext.define('PBS.form.NamespaceSelector', {
 	me.store = Ext.create('Ext.data.Store', {
 	    model: 'pbs-namespaces',
 	    autoLoad: !!me.datastore,
+	    filters: (rec) => rec.data.ns !== '',
 	    proxy: {
 		type: 'proxmox',
 		timeout: 30 * 1000,
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 3/5] ui: form/DataStoreSelector: show maintenance mode in selector

[Dominik Csapak]

to not having to query the activeTasks everywhere, change the renderer
to omit the check/spinner when no activeTasks are given

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/Utils.js                  | 17 ++++++++++-------
 www/form/DataStoreSelector.js |  9 +++++++++
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/www/Utils.js b/www/Utils.js
index 838b4511..b277cd99 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -662,14 +662,17 @@ Ext.define('PBS.Utils', {
 
 	let [type, message] = PBS.Utils.parseMaintenanceMode(mode);
 
-	const conflictingTasks = activeTasks.write + (type === 'offline' ? activeTasks.read : 0);
-
 	let extra = '';
-	if (conflictingTasks > 0) {
-	    extra += '| <i class="fa fa-spinner fa-pulse fa-fw"></i> ';
-	    extra += Ext.String.format(gettext('{0} conflicting tasks still active.'), conflictingTasks);
-	} else {
-	    extra += '<i class="fa fa-check"></i>';
+
+	if (activeTasks !== undefined) {
+	    const conflictingTasks = activeTasks.write + (type === 'offline' ? activeTasks.read : 0);
+
+	    if (conflictingTasks > 0) {
+		extra += '| <i class="fa fa-spinner fa-pulse fa-fw"></i> ';
+		extra += Ext.String.format(gettext('{0} conflicting tasks still active.'), conflictingTasks);
+	    } else {
+		extra += '<i class="fa fa-check"></i>';
+	    }
 	}
 
 	if (message) {
diff --git a/www/form/DataStoreSelector.js b/www/form/DataStoreSelector.js
index a1471a84..90f20bee 100644
--- a/www/form/DataStoreSelector.js
+++ b/www/form/DataStoreSelector.js
@@ -29,6 +29,15 @@ Ext.define('PBS.form.DataStoreSelector', {
 		renderer: Ext.String.htmlEncode,
 		flex: 1,
 	    },
+	    {
+		header: gettext('Maintenance'),
+		sortable: true,
+		dataIndex: 'maintenance',
+		renderer: (value) => {
+		    return PBS.Utils.renderMaintenance(value);
+		},
+		flex: 1,
+	    }
 	],
     },
 });
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 4/5] ui: datastore/Summary: change storage axis to power of two

[Dominik Csapak]

so that the unit is the same as in the overview panel on the same page

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/datastore/Summary.js | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/www/datastore/Summary.js b/www/datastore/Summary.js
index 328aa561..3c07d512 100644
--- a/www/datastore/Summary.js
+++ b/www/datastore/Summary.js
@@ -237,6 +237,8 @@ Ext.define('PBS.DataStoreSummary', {
 	    xtype: 'proxmoxRRDChart',
 	    title: gettext('Storage usage (bytes)'),
 	    fields: ['total', 'used'],
+	    unit: 'bytes',
+	    powerOfTwo: true,
 	    fieldTitles: [gettext('Total'), gettext('Storage usage')],
 	},
 	{
-- 
2.30.2

[pbs-devel] [PATCH proxmox-backup 5/5] ui: SyncView/SyncJobEdit: unify store/namespace gettext

[Dominik Csapak]

so that the labels are the same in the list as in the edit window

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/config/SyncView.js    | 6 +++---
 www/window/SyncJobEdit.js | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/www/config/SyncView.js b/www/config/SyncView.js
index a90e9a70..f149cd8a 100644
--- a/www/config/SyncView.js
+++ b/www/config/SyncView.js
@@ -197,14 +197,14 @@ Ext.define('PBS.config.SyncJobView', {
 	    sortable: true,
 	},
 	{
-		header: gettext('Namespace'),
+		header: gettext('Local Namespace'),
 		dataIndex: 'ns',
-		width: 120,
+		width: 130,
 		sortable: true,
 		renderer: PBS.Utils.render_optional_namespace,
 	},
 	{
-	    header: gettext('Remote ID'),
+	    header: gettext('Source Remote'),
 	    dataIndex: 'remote',
 	    width: 120,
 	    sortable: true,
diff --git a/www/window/SyncJobEdit.js b/www/window/SyncJobEdit.js
index efa1adfc..2c09b354 100644
--- a/www/window/SyncJobEdit.js
+++ b/www/window/SyncJobEdit.js
@@ -73,7 +73,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 		column1: [
 		    {
 			xtype: 'pmxDisplayEditField',
-			fieldLabel: gettext('Local Datastore'),
+			fieldLabel: gettext('Local Store'),
 			name: 'store',
 			submitValue: true,
 			cbind: {
@@ -150,7 +150,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 			},
 		    },
 		    {
-			fieldLabel: gettext('Source Datastore'),
+			fieldLabel: gettext('Remote Store'),
 			xtype: 'pbsRemoteStoreSelector',
 			allowBlank: false,
 			autoSelect: false,
@@ -169,7 +169,7 @@ Ext.define('PBS.window.SyncJobEdit', {
 			},
 		    },
 		    {
-			fieldLabel: gettext('Source Namespace'),
+			fieldLabel: gettext('Remote Namespace'),
 			xtype: 'pbsRemoteNamespaceSelector',
 			allowBlank: true,
 			autoSelect: false,
-- 
2.30.2

Re: [pbs-devel] [PATCH proxmox-backup 4/5] ui: datastore/Summary: change storage axis to power of two

[Thomas Lamprecht]

Am 5/17/22 um 08:52 schrieb Dominik Csapak:
> so that the unit is the same as in the overview panel on the same page
> 

This missed the administration Server Status RRD for the root disk, but actually
this needs to be the other way around, I switched to power of two for all memory
graphs in PVE 7.0 and the storage one to power of ten, at least the pve storages,
the root HD one in the node summary is still wrong (will fix).

Re: [pbs-devel] [PATCH proxmox-backup 4/5] ui: datastore/Summary: change storage axis to power of two

[Dominik Csapak]

On 5/17/22 10:09, Thomas Lamprecht wrote:
> Am 5/17/22 um 08:52 schrieb Dominik Csapak:
>> so that the unit is the same as in the overview panel on the same page
>>
> 
> This missed the administration Server Status RRD for the root disk, but actually
> this needs to be the other way around, I switched to power of two for all memory
> graphs in PVE 7.0 and the storage one to power of ten, at least the pve storages,
> the root HD one in the node summary is still wrong (will fix).
> 
> 

yeah you're right, i forgot about the administration rrd panels.

any reason why we use different units for disk vs memory?
imho storage also often uses binary units.

(`lvs`/`lsblk` uses that in it's default output, we use it
for vm/ct disks, etc.)

Re: [pbs-devel] [PATCH proxmox-backup 4/5] ui: datastore/Summary: change storage axis to power of two

[Thomas Lamprecht]

Am 5/17/22 um 10:20 schrieb Dominik Csapak:
> On 5/17/22 10:09, Thomas Lamprecht wrote:
>> Am 5/17/22 um 08:52 schrieb Dominik Csapak:
>>> so that the unit is the same as in the overview panel on the same page
>>>
>>
>> This missed the administration Server Status RRD for the root disk, but actually
>> this needs to be the other way around, I switched to power of two for all memory
>> graphs in PVE 7.0 and the storage one to power of ten, at least the pve storages,
>> the root HD one in the node summary is still wrong (will fix).
>>
>>
> 
> yeah you're right, i forgot about the administration rrd panels.
> 
> any reason why we use different units for disk vs memory?

because they're two different things usage wise, due to memory being tied to the CPU
a power of two address and thus size comes just naturally, and the industries tradition
is to advertised and base 10 for storage, where it doesn't really matter from a technical
POV but from a marketing (bigger number for SI)

> imho storage also often uses binary units.

cannot remember to see any disk sold with a advertised size formatted as power of
two, and users may be confused if they see less capacity than the disk promised.

> 
> (`lvs`/`lsblk` uses that in it's default output, we use it
> for vm/ct disks, etc.)

we show SI units for the disk panel, it should rather fit that, also we nowhere
show LVM in PBS, nor will we ever be able to show the same thing that all filesystem/
olume manager techs do, IMO also a bogus goal, what's important is to be consistent,
which we never really had, it was always a weird mix of either, and especially the
base ten for memory, which results in values on axis that just don't line nicely
up has done it for me for the 6.4 release there.

[pbs-devel] applied: [PATCH proxmox-backup 3/5] ui: form/DataStoreSelector: show maintenance mode in selector

[Thomas Lamprecht]

Am 5/17/22 um 08:52 schrieb Dominik Csapak:
> to not having to query the activeTasks everywhere, change the renderer
> to omit the check/spinner when no activeTasks are given
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/Utils.js                  | 17 ++++++++++-------
>  www/form/DataStoreSelector.js |  9 +++++++++
>  2 files changed, 19 insertions(+), 7 deletions(-)
> 
>

applied, thanks! had a two eslint warnings though and while I find that the info *is*
important to have upfront (thought about something like that too a few days ago when
playing around with maintenance), after checking the result out it felt like spending
a lot of the little space we got there just for this, so I reworked it into adding an
icon in the name column, if in maintenance mode.

[pbs-devel] applied: [PATCH proxmox-backup 2/5] ui: form/NamespaceSelector: show proper emptyText

[Thomas Lamprecht]

Am 5/17/22 um 08:52 schrieb Dominik Csapak:
> by filtering out the empty namespace from the api, and putting the
> correct div with xclass around the gettext
> 
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
>  www/form/NamespaceSelector.js | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
>

applied, thanks!