[pbs-devel] [PATCH proxmox-backup 0/9] tape: improve restore ux

[pbs-devel] [PATCH proxmox-backup 0/9] tape: improve restore ux

[Dominik Csapak]

by
* adding a 'generic' restore button that is very obvious
  (in contrast to only the icon)
  this makes it necessary to change the restore window so that
  we can select the media-set there
* also allowing datastores to be 'inline restored'
* expanding the inital media-set list
* renaming the 'action' column to 'restore' to make it more obvious
* using the 'proper' backend code path for when the user selected
  only 'whole' datastores
* fixing some small bugs

Dominik Csapak (9):
  ui: tape/BackupOverview: expand pools by default
  api2/tape: add api call to list media sets
  ui: tape/BackupOverview: rename action column to restore
  ui: tape: add MediaSetSelector
  ui: tape/TapeRestore: add MediaSetSelector
  ui: tape/BackupOverview: add generic 'Restore' button
  ui: tape/TapeRestore: don't send snapshotlist when restoring whole
    datastores
  ui: tape/TapeRestore: allow preselecting a datastore
  ui: tape/TapeRestore: fix some properties

 src/api2/tape/media.rs         |  75 ++++++++++++++
 src/api2/types/tape/media.rs   |  20 ++++
 www/tape/BackupOverview.js     |  18 +++-
 www/tape/window/TapeRestore.js | 179 ++++++++++++++++++++++++++++-----
 4 files changed, 267 insertions(+), 25 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/9] ui: tape/BackupOverview: expand pools by default

[Dominik Csapak]

normally, users will not have many tape media pools,
and are more interested in the actual media-sets, so
expand those nodes by default

if the list gets very long, the user can collapse some pools anyway

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js | 1 +
 1 file changed, 1 insertion(+)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index 1ccbb7f4..16e9a581 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -66,6 +66,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    for (const [pool, media_sets] of Object.entries(data)) {
 		let pool_entry = Ext.create('Ext.data.TreeModel', {
 		    text: pool,
+		    expanded: true,
 		    leaf: false,
 		});
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/9] api2/tape: add api call to list media sets

[Dominik Csapak]

we want a 'media-set' selector in the gui, this makes it
very easy to do and is not as costly as reusing the media list,
since we do not need to iterate over all media (e.g. unassigned)

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 src/api2/tape/media.rs       | 75 ++++++++++++++++++++++++++++++++++++
 src/api2/types/tape/media.rs | 20 ++++++++++
 2 files changed, 95 insertions(+)

diff --git a/src/api2/tape/media.rs b/src/api2/tape/media.rs
index 683c74b8..8351b2be 100644
--- a/src/api2/tape/media.rs
+++ b/src/api2/tape/media.rs
@@ -1,4 +1,5 @@
 use std::path::Path;
+use std::collections::HashSet;
 
 use anyhow::{bail, format_err, Error};
 use serde::{Serialize, Deserialize};
@@ -28,6 +29,7 @@ use crate::{
         CHANGER_NAME_SCHEMA,
         MediaPoolConfig,
         MediaListEntry,
+        MediaSetListEntry,
         MediaStatus,
         MediaContentEntry,
         VAULT_NAME_SCHEMA,
@@ -44,6 +46,74 @@ use crate::{
     },
 };
 
+#[api(
+    returns: {
+        description: "List of media sets.",
+        type: Array,
+        items: {
+            type: MediaSetListEntry,
+        },
+    },
+    access: {
+        description: "List of media sets filtered by Tape.Audit privileges on pool",
+        permission: &Permission::Anybody,
+    },
+)]
+/// List Media sets
+pub async fn list_media_sets(
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Vec<MediaSetListEntry>, Error> {
+    let auth_id: Authid = rpcenv.get_auth_id().unwrap().parse()?;
+    let user_info = CachedUserInfo::new()?;
+
+    let (config, _digest) = config::media_pool::config()?;
+
+    let status_path = Path::new(TAPE_STATUS_DIR);
+
+    let mut media_sets: HashSet<Uuid> = HashSet::new();
+    let mut list = Vec::new();
+
+    for (_section_type, data) in config.sections.values() {
+        let pool_name = match data["name"].as_str() {
+            None => continue,
+            Some(name) => name,
+        };
+
+        let privs = user_info.lookup_privs(&auth_id, &["tape", "pool", pool_name]);
+        if (privs & PRIV_TAPE_AUDIT) == 0  {
+            continue;
+        }
+
+        let config: MediaPoolConfig = config.lookup("pool", pool_name)?;
+
+        let changer_name = None; // assume standalone drive
+        let pool = MediaPool::with_config(status_path, &config, changer_name, true)?;
+
+        for media in pool.list_media() {
+            if let Some(label) = media.media_set_label() {
+                if media_sets.contains(&label.uuid) {
+                    continue;
+                }
+
+                let media_set_uuid = label.uuid.clone();
+                let media_set_ctime = label.ctime;
+                let media_set_name = pool
+                    .generate_media_set_name(&media_set_uuid, config.template.clone())
+                    .unwrap_or_else(|_| media_set_uuid.to_string());
+
+                media_sets.insert(media_set_uuid.clone());
+                list.push(MediaSetListEntry {
+                    media_set_name,
+                    media_set_uuid,
+                    media_set_ctime,
+                    pool: pool_name.to_string(),
+                });
+            }
+        }
+    }
+
+    Ok(list)
+}
 #[api(
     input: {
         properties: {
@@ -546,6 +616,11 @@ const SUBDIRS: SubdirMap = &[
             .get(&API_METHOD_DESTROY_MEDIA)
     ),
     ( "list", &MEDIA_LIST_ROUTER ),
+    (
+        "media-sets",
+        &Router::new()
+        .get(&API_METHOD_LIST_MEDIA_SETS)
+    ),
     (
         "move",
         &Router::new()
diff --git a/src/api2/types/tape/media.rs b/src/api2/types/tape/media.rs
index 554efa7a..b53ce28b 100644
--- a/src/api2/types/tape/media.rs
+++ b/src/api2/types/tape/media.rs
@@ -12,6 +12,26 @@ use crate::api2::types::{
     MediaLocation,
 };
 
+#[api(
+    properties: {
+        "media-set-uuid": {
+            schema: MEDIA_SET_UUID_SCHEMA,
+        },
+    },
+)]
+#[derive(Serialize,Deserialize)]
+#[serde(rename_all = "kebab-case")]
+/// Media Set list entry
+pub struct MediaSetListEntry {
+    /// Media set name
+    pub media_set_name: String,
+    pub media_set_uuid: Uuid,
+    /// MediaSet creation time stamp
+    pub media_set_ctime: i64,
+    /// Media Pool
+    pub pool: String,
+}
+
 #[api(
     properties: {
         location: {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/9] ui: tape/BackupOverview: rename action column to restore

[Dominik Csapak]

to make it clear that this button is for restore and for
now we do not have any plans to add buttons here

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index 16e9a581..e6a78574 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -266,7 +266,7 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    flex: 3,
 	},
 	{
-	    header: gettext('Actions'),
+	    header: gettext('Restore'),
 	    xtype: 'actioncolumn',
 	    items: [
 		{
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/9] ui: tape: add MediaSetSelector

[Dominik Csapak]

so that we can let the user select a media-set

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/window/TapeRestore.js | 41 ++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index 883b8342..1afd57fe 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -666,3 +666,44 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	me.mon(me.store, 'filterchange', () => me.checkChange());
     },
 });
+
+Ext.define('PBS.TapeManagement.MediaSetSelector', {
+    extend: 'Proxmox.form.ComboGrid',
+    alias: 'widget.pbsMediaSetSelector',
+
+    allowBlank: false,
+    displayField: 'media-set-name',
+    valueField: 'media-set-uuid',
+    autoSelect: false,
+
+    store: {
+	proxy: {
+	    type: 'proxmox',
+	    url: '/api2/json/tape/media/media-sets',
+	},
+	autoLoad: true,
+	idProperty: 'media-set-uuid',
+	sorters: ['pool', 'media-set-ctime'],
+    },
+
+    listConfig: {
+	width: 600,
+	columns: [
+	    {
+		text: gettext('Pool'),
+		dataIndex: 'pool',
+		flex: 1,
+	    },
+	    {
+		text: gettext('Name'),
+		dataIndex: 'media-set-name',
+		width: 180,
+	    },
+	    {
+		text: gettext('Media-Set UUID'),
+		dataIndex: 'media-set-uuid',
+		width: 280,
+	    },
+	],
+    },
+});
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/9] ui: tape/TapeRestore: add MediaSetSelector

[Dominik Csapak]

when no uuid/mediaset is given.
we change a bit how we use the uuid by moving it into the viewmodel
(instead of a simple property on the view) so that we can always
use the selected one

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/window/TapeRestore.js | 45 ++++++++++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index 1afd57fe..1987dff4 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -14,6 +14,10 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
     showTaskViewer: true,
     isCreate: true,
 
+    mediaset: undefined,
+    prefilter: undefined,
+    uuid: undefined,
+
     cbindData: function(config) {
 	let me = this;
 	if (me.prefilter !== undefined) {
@@ -27,6 +31,7 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 
     viewModel: {
 	data: {
+	    uuid: "",
 	    singleDatastore: true,
 	},
 	formulas: {
@@ -43,6 +48,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 	    return panel.query('[isFormField]').every(field => field.isValid());
 	},
 
+	changeMediaSet: function(field, value) {
+	    let me = this;
+	    let vm = me.getViewModel();
+	    vm.set('uuid', value);
+	    me.updateSnapshots();
+	},
+
 	checkValidity: function() {
 	    let me = this;
 
@@ -203,10 +215,12 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 	    let me = this;
 	    let view = me.getView();
 	    let grid = me.lookup('snapshotGrid');
+	    let vm = me.getViewModel();
+	    let uuid = vm.get('uuid');
 
 	    Proxmox.Utils.API2Request({
 		waitMsgTarget: view,
-		url: `/tape/media/content?media-set=${view.uuid}`,
+		url: `/tape/media/content?media-set=${uuid}`,
 		success: function(response, opt) {
 		    let datastores = {};
 		    for (const content of response.result.data) {
@@ -229,6 +243,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 	    });
 	},
 
+	init: function(view) {
+	    let me = this;
+	    let vm = me.getViewModel();
+
+	    vm.set('uuid', view.uuid);
+	},
+
 	control: {
 	    '[isFormField]': {
 		change: 'checkValidity',
@@ -283,11 +304,29 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		    },
 
 		    column1: [
+			{
+			    xtype: 'pbsMediaSetSelector',
+			    fieldLabel: gettext('Media-Set'),
+			    width: 350,
+			    submitValue: false,
+			    bind: {
+				value: '{uuid}',
+			    },
+			    cbind: {
+				hidden: '{uuid}',
+				disabled: '{uuid}',
+			    },
+			    listeners: {
+				change: 'changeMediaSet',
+			    },
+			},
 			{
 			    xtype: 'displayfield',
 			    fieldLabel: gettext('Media-Set'),
 			    cbind: {
 				value: '{mediaset}',
+				hidden: '{!uuid}',
+				disabled: '{!uuid}',
 			    },
 			},
 		    ],
@@ -298,8 +337,10 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 			    fieldLabel: gettext('Media-Set UUID'),
 			    name: 'media-set',
 			    submitValue: true,
-			    cbind: {
+			    bind: {
 				value: '{uuid}',
+				hidden: '{!uuid}',
+				disabled: '{!uuid}',
 			    },
 			},
 		    ],
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/9] ui: tape/BackupOverview: add generic 'Restore' button

[Dominik Csapak]

this will open the restore window without anything preselected

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

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index e6a78574..e8273f86 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -17,6 +17,12 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    });
 	},
 
+	restore: function() {
+	    Ext.create('PBS.TapeManagement.TapeRestoreWindow', {
+		autoShow: true,
+	    });
+	},
+
 	restoreBackups: function(view, rI, cI, item, e, rec) {
 	    let me = this;
 
@@ -255,6 +261,10 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 	    text: gettext('New Backup'),
 	    handler: 'backup',
 	},
+	{
+	    text: gettext('Restore'),
+	    handler: 'restore',
+	},
     ],
 
     columns: [
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 7/9] ui: tape/TapeRestore: don't send snapshotlist when restoring whole datastores

[Dominik Csapak]

for the case that the user selects only whole datastores, we do not
want to send and (exhaustive) list of snapshots that get restored,
but we only want to honor the mapping the user gives

this avoids using the backup restore codepath that iterates twice
over the tapes and would generally be slower for a lot of snapshots

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/window/TapeRestore.js | 57 +++++++++++++++++++++++++++++-----
 1 file changed, 50 insertions(+), 7 deletions(-)

diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index 1987dff4..35619957 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -184,9 +184,13 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		values = [];
 	    }
 	    let datastores = {};
-	    values.forEach((snapshot) => {
-		const [datastore] = snapshot.split(':');
-		datastores[datastore] = true;
+	    values.forEach((snapshotOrDatastore) => {
+		let datastore = snapshotOrDatastore;
+		if (snapshotOrDatastore.indexOf(':') !== -1) {
+		    let snapshot = snapshotOrDatastore;
+		    let match = snapshot.split(':');
+		    datastore = match[0];
+		} datastores[datastore] = true;
 	    });
 
 	    me.setDataStores(Object.keys(datastores));
@@ -230,7 +234,7 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		    if (response.result.data.length > 0) {
 			grid.setDisabled(false);
 			grid.setVisible(true);
-			grid.getStore().setData(response.result.data);
+			grid.setData(response.result.data);
 			grid.getSelectionModel().selectAll();
 			// we've shown a big list, center the window again
 			view.center();
@@ -294,10 +298,14 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
 		    onGetValues: function(values) {
 			let me = this;
 
-			if (values.snapshots === 'all') {
-			    delete values.snapshots;
-			} else if (Ext.isString(values.snapshots) && values.snapshots) {
+			if (values !== "all" &&
+			    Ext.isString(values.snapshots) &&
+			    values.snapshots &&
+			    values.snapshots.indexOf(':') !== -1
+			) {
 			    values.snapshots = values.snapshots.split(',');
+			} else {
+			    delete values.snapshots;
 			}
 
 			return values;
@@ -593,6 +601,8 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	let me = this;
 	let snapshots = [];
 
+	let storeCounts = {};
+
 	me.getSelection().forEach((rec) => {
 	    let id = rec.get('id');
 	    let store = rec.data.store;
@@ -600,6 +610,10 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	    // only add if not filtered
 	    if (me.store.findExact('id', id) !== -1) {
 		snapshots.push(`${store}:${snap}`);
+		if (storeCounts[store] === undefined) {
+		    storeCounts[store] = 0;
+		}
+		storeCounts[store]++;
 	    }
 	});
 
@@ -610,6 +624,21 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	    return "all";
 	}
 
+	let wholeStores = [];
+	let wholeStoresSelected = true;
+	for (const [store, count] of Object.entries(storeCounts)) {
+	    if (me.storeCounts[store] === count) {
+		wholeStores.push(store);
+	    } else {
+		wholeStoresSelected = false;
+		break;
+	    }
+	}
+
+	if (wholeStoresSelected) {
+	    return wholeStores;
+	}
+
 	return snapshots;
     },
 
@@ -639,6 +668,20 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	return [];
     },
 
+    setData: function(records) {
+	let me = this;
+	let storeCounts = {};
+	records.forEach((rec) => {
+	    let store = rec.store;
+	    if (storeCounts[store] === undefined) {
+		storeCounts[store] = 0;
+	    }
+	    storeCounts[store]++;
+	});
+	me.storeCounts = storeCounts;
+	me.getStore().setData(records);
+    },
+
     scrollable: true,
     plugins: 'gridfilters',
 
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 8/9] ui: tape/TapeRestore: allow preselecting a datastore

[Dominik Csapak]

for that we need to split the prefilter additions, else
we always filter the snaphots too and giving 'undefined' filters
all snapshots...

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/BackupOverview.js     |  5 +++++
 www/tape/window/TapeRestore.js | 33 ++++++++++++++++++++-------------
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/www/tape/BackupOverview.js b/www/tape/BackupOverview.js
index e8273f86..690836c3 100644
--- a/www/tape/BackupOverview.js
+++ b/www/tape/BackupOverview.js
@@ -161,6 +161,11 @@ Ext.define('PBS.TapeManagement.BackupOverview', {
 			    text: store,
 			    'media-set-uuid': entry['media-set-uuid'],
 			    iconCls: 'fa fa-database',
+			    restore: true,
+			    'media-set': media_set,
+			    prefilter: {
+				store,
+			    },
 			    tapes: {},
 			};
 		    }
diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index 35619957..bc513632 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -732,19 +732,26 @@ Ext.define('PBS.TapeManagement.SnapshotGrid', {
 	let me = this;
 	me.callParent();
 	if (me.prefilter !== undefined) {
-	    me.store.filters.add(
-		{
-		    id: 'x-gridfilter-store',
-		    property: 'store',
-		    operator: 'in',
-		    value: [me.prefilter.store],
-		},
-		{
-		    id: 'x-gridfilter-snapshot',
-		    property: 'snapshot',
-		    value: me.prefilter.snapshot,
-		},
-	    );
+	    if (me.prefilter.store !== undefined) {
+		me.store.filters.add(
+		    {
+			id: 'x-gridfilter-store',
+			property: 'store',
+			operator: 'in',
+			value: [me.prefilter.store],
+		    },
+		);
+	    }
+
+	    if (me.prefilter.snapshot !== undefined) {
+		me.store.filters.add(
+		    {
+			id: 'x-gridfilter-snapshot',
+			property: 'snapshot',
+			value: me.prefilter.snapshot,
+		    },
+		);
+	    }
 	}
 
 	me.mon(me.store, 'filterchange', () => me.checkChange());
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 9/9] ui: tape/TapeRestore: fix some properties

[Dominik Csapak]

remove leftover from when it was an Proxmox.window.Edit, and
add the missing 'modal'

Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
 www/tape/window/TapeRestore.js | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/www/tape/window/TapeRestore.js b/www/tape/window/TapeRestore.js
index bc513632..a6ed179f 100644
--- a/www/tape/window/TapeRestore.js
+++ b/www/tape/window/TapeRestore.js
@@ -11,8 +11,7 @@ Ext.define('PBS.TapeManagement.TapeRestoreWindow', {
     url: '/api2/extjs/tape/restore',
     method: 'POST',
 
-    showTaskViewer: true,
-    isCreate: true,
+    modal: true,
 
     mediaset: undefined,
     prefilter: undefined,
-- 
2.20.1

[pbs-devel] applied-series: [PATCH proxmox-backup 0/9] tape: improve restore ux

[Thomas Lamprecht]

On 26.05.21 15:48, Dominik Csapak wrote:
> by
> * adding a 'generic' restore button that is very obvious
>   (in contrast to only the icon)
>   this makes it necessary to change the restore window so that
>   we can select the media-set there
> * also allowing datastores to be 'inline restored'
> * expanding the inital media-set list
> * renaming the 'action' column to 'restore' to make it more obvious
> * using the 'proper' backend code path for when the user selected
>   only 'whole' datastores
> * fixing some small bugs
> 

With expanding the pools and renaming the action column to the single action
it provides, namely "Restore" would have been already enough to defuse any
confusion about where to trigger a restore. Any how, it is definitively more
prominent this way, so I applied it with some small changes:

* set icones in the tbar button, matching with other uses
* always show the snapshot grid in the wizard to avoid that much empty space

And rather independent of your series: add a bit more context to the tooltip
of the restore action button.

The "Restore from Tape" docs section could do with some expansion and maybe even
a screenshot of the wizard now, then we could set the onlineHelp reference
in the wizard to - maybe we could then also explain there that restoring a whole
set is possibly cheaper to do than restoring a partial selection of snapshots.

thanks!