[pve-devel] [PATCH v5 series 0/3] Add GUI for disk reassignment

[pve-devel] [PATCH v5 series 0/3] Add GUI for disk reassignment

[Aaron Lauterer]

This patch series adds the GUI to the recent patch series [0] which
enables the reassignment of disks between VMs.

For this to work, the backend patch series [0] needs to be applied and
installed.

v4 -> v5:
* rebased
* removed whitespace patch as these issues have been addressed in the
  meantime

v3 -> v4:
* rebased
* added check to not add template VMs to dropdown
* renamed disk parameter to drive_name

v2 -> v3:
* fixed check if same VMID for dropdown
* renamed disk parameter to drive_key

v1 -> v2: linter fixes

[0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046479.html


widget-toolkit: Aaron Lauterer (1):
  window/edit: add option to disable reset button

 src/window/Edit.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

manager: Aaron Lauterer (2):
  ui: utils: add method to get VM data from resource store
  ui: qemu: Add disk reassign dialog

 www/manager6/Makefile             |  1 +
 www/manager6/Utils.js             | 13 +++++
 www/manager6/qemu/HDReassign.js   | 80 +++++++++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js | 30 ++++++++++++
 4 files changed, 124 insertions(+)
 create mode 100644 www/manager6/qemu/HDReassign.js


-- 
2.20.1

[pve-devel] [PATCH v5 widget-toolkit 1/3] window/edit: add option to disable reset button

[Aaron Lauterer]

Sometimes the reset button does not make sense and the isCreate option
does not fit as well because with it, the submit button will be enabled
right away instead of waiting for the form to be valid.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

v1 -> v2 -> v3 -> v4 -> v5: nothing changed

This helps to reuse the PVE.window.Edit component in a modern viewModel
and controller way instead of building it all by hand the old way with a
large initComponent method.

A possible candidate for refactoring after this would be the
qemu/HDMove.js component.


 src/window/Edit.js | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/window/Edit.js b/src/window/Edit.js
index e952362..548beed 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -25,6 +25,9 @@ Ext.define('Proxmox.window.Edit', {
     // set to true if you want an Remove button (instead of Create)
     isRemove: false,
 
+    // set to false, if you don't want the reset button present
+    showReset: true,
+
     // custom submitText
     submitText: undefined,
 
@@ -350,7 +353,7 @@ Ext.define('Proxmox.window.Edit', {
 	    me.title = Proxmox.Utils.dialog_title(me.subject, me.isCreate, me.isAdd);
 	}
 
-	if (me.isCreate) {
+	if (me.isCreate || !me.showReset) {
 		me.buttons = [submitBtn];
 	} else {
 		me.buttons = [submitBtn, resetBtn];
-- 
2.20.1

[pve-devel] applied: [PATCH v5 widget-toolkit 1/3] window/edit: add option to disable reset button

[Thomas Lamprecht]

On 15.02.21 16:26, Aaron Lauterer wrote:
> Sometimes the reset button does not make sense and the isCreate option
> does not fit as well because with it, the submit button will be enabled
> right away instead of waiting for the form to be valid.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> v1 -> v2 -> v3 -> v4 -> v5: nothing changed
> 
> This helps to reuse the PVE.window.Edit component in a modern viewModel
> and controller way instead of building it all by hand the old way with a
> large initComponent method.
> 
> A possible candidate for refactoring after this would be the
> qemu/HDMove.js component.
> 
> 
>  src/window/Edit.js | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
>

applied, thanks!

[pve-devel] [PATCH v5 manager 2/3] ui: utils: add method to get VM data from resource store

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
v3 -> v4i -> v5: rebased
v2 -> v3: nothing changed
v1 -> v2: fixed linter errors


 www/manager6/Utils.js | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
index 7a1a7fb6..a298aac0 100644
--- a/www/manager6/Utils.js
+++ b/www/manager6/Utils.js
@@ -1833,4 +1833,17 @@ Ext.define('PVE.Utils', {
 	});
     },
 
+    getNodeVMs: function(nodename) {
+	let rstore = PVE.data.ResourceStore;
+	let vms = {};
+	rstore.data.items.forEach((item) => {
+	    if (!item.id.startsWith('qemu/')) { return; }
+	    let vmdata = item.data;
+	    if (vmdata.node !== nodename) { return; }
+
+	    vms[vmdata.vmid] = vmdata;
+	});
+	return vms;
+    },
+
 });
-- 
2.20.1

Re: [pve-devel] [PATCH v5 manager 2/3] ui: utils: add method to get VM data from resource store

[Thomas Lamprecht]

On 15.02.21 16:26, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> v3 -> v4i -> v5: rebased
> v2 -> v3: nothing changed
> v1 -> v2: fixed linter errors
> 
> 
>  www/manager6/Utils.js | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 7a1a7fb6..a298aac0 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1833,4 +1833,17 @@ Ext.define('PVE.Utils', {
>  	});
>      },
>  
> +    getNodeVMs: function(nodename) {
> +	let rstore = PVE.data.ResourceStore;
> +	let vms = {};
> +	rstore.data.items.forEach((item) => {

we normally use 'rstore' for a remote store, so I dislike the use when talking about
the in-memory ResourceStore as it's confusing.

FIY: In JS arrow functions the parenthesis can be dropped if there's only on parameter

And it seems like what you actually want is a .filter

let guests = rstore.data.items.filter(item => item.id.startsWith('qemu/') && item.data.node === nodename);

Maybe future proof this and allow to add the type, as we want the feature this
is made for also for CTs.

The whole thing could be reduced a bit with combinators, like (untested):

// type can be 'qemu', 'lxc' or left-out (for both)
// returns an Object with `vmid => guest-data` entries
getNodeGuests: function(nodename, type) {
    return PVE.data.ResourceStore.data
        .items
        .filter(item => (type && item.id.startsWith(`${type}/`)) && item.data.node === nodename)
        .reduce((acc, item) => acc[item.data.vmid] = item.data, {});
}

> +	    if (!item.id.startsWith('qemu/')) { return; }
> +	    let vmdata = item.data;
> +	    if (vmdata.node !== nodename) { return; }

Please avoid squishing early return ifs to one line. If that would be actually required
(see above for a general shorter approach), then it'd be nicer to write it as:

if (!item.id.startsWith('qemu/') || item.data.node !== nodename) {
    return;
}

> +
> +	    vms[vmdata.vmid] = vmdata;
> +	});
> +	return vms;
> +    },
> +
>  });
> 

[pve-devel] [PATCH v5 manager 3/3] ui: qemu: Add disk reassign dialog

[Aaron Lauterer]

Adds a new button to the hardware panel labeled 'Reassign disk' and
enables a user to reassign a disk to another VM.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

v4 -> v5: rebased

v3 -> v4:
* added check to not show template VMs in dropdown
* renamed disk parameter to `drive_name`

v2 -> v3:
* fixed check to omit the current vmid in the target dropdown
* renamed parameter disk to drive_key
* added missing comma

v1 -> v2: fixed linter errors

This patch needs the previous patch series [0] applied which adds the
backend for disk reassignments.

[0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046479.html

 www/manager6/Makefile             |  1 +
 www/manager6/qemu/HDReassign.js   | 80 +++++++++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js | 30 ++++++++++++
 3 files changed, 111 insertions(+)
 create mode 100644 www/manager6/qemu/HDReassign.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 85f90ecd..21debfcb 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -200,6 +200,7 @@ JSSRC= 							\
 	qemu/HDEdit.js					\
 	qemu/HDEfi.js					\
 	qemu/HDMove.js					\
+	qemu/HDReassign.js				\
 	qemu/HDResize.js				\
 	qemu/HardwareView.js				\
 	qemu/IPConfigEdit.js				\
diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
new file mode 100644
index 00000000..d0aa97f8
--- /dev/null
+++ b/www/manager6/qemu/HDReassign.js
@@ -0,0 +1,80 @@
+Ext.define('PVE.window.HDReassign', {
+    extend: 'Proxmox.window.Edit',
+
+    resizeable: false,
+    title: gettext('Reassign disk'),
+    submitText: gettext('Reassign disk'),
+    showReset: false,
+    method: 'POST',
+    showProgress: true,
+    width: 350,
+
+    viewModel: {
+	data: {
+	    targetVMData: {},
+	    show_running_hint: false,
+	},
+    },
+
+    items: [
+	{
+	    xtype: 'combobox',
+	    name: 'target_vmid',
+	    reference: 'target_vmid',
+	    fieldLabel: gettext('Target VMID'),
+	    bind: {
+		store: '{targetVMData}',
+	    },
+	    queryMode: 'local',
+	    displayField: 'name',
+	    valueField: 'vmid',
+	    anyMatch: true,
+	    emptyText: gettext('Select VM'),
+	},
+	{
+	    xtype: 'displayfield',
+	    padding: '5 0 0 0',
+	    userCls: 'pmx-hint',
+	    value: gettext('This disk cannot be reassigned while the VM is running'),
+	    bind: {
+		hidden: '{!show_running_hint}',
+	    },
+	},
+    ],
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+	init: function(view) {
+	    let me = view;
+	    let vm = me.getViewModel();
+	    let vms = PVE.Utils.getNodeVMs(me.nodename);
+
+	    let show_running_hint = vms[me.vmid].running && !me.drive_name.startsWith('unused');
+	    vm.set('show_running_hint', show_running_hint);
+
+	    if (show_running_hint) {
+	        me.lookup('target_vmid').setDisabled(true);
+	    }
+
+	    let dropdownData = [];
+	    for (const [vmid, data] of Object.entries(vms)) {
+	        if (parseInt(vmid, 10) === parseInt(me.vmid, 10)) { continue; }
+		if (data.template) { continue; }
+	        dropdownData.push({
+		    vmid: vmid,
+		    name: `${vmid} ${data.name}`,
+	        });
+	    }
+
+	    vm.set('targetVMData', { data: dropdownData });
+	},
+    },
+
+    getValues: function() {
+	let me = this;
+	let values = me.callParent(arguments);
+	values.drive_name = me.drive_name;
+
+	return values;
+    },
+});
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 77640e53..d129deda 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -415,6 +415,25 @@ Ext.define('PVE.qemu.HardwareView', {
 	    win.on('destroy', me.reload, me);
 	};
 
+	var run_reassign = function() {
+	    let rec = sm.getSelection()[0];
+	    if (!rec) {
+		return;
+	    }
+
+	    let win = Ext.create('PVE.window.HDReassign', {
+		drive_name: rec.data.key,
+		vmid: vmid,
+		nodename: nodename,
+		title: gettext('Reassign disk') + ': ' + rec.data.key,
+		url: '/api2/extjs/nodes/' + nodename + '/qemu/' + vmid + '/reassign_disk',
+	    });
+
+	    win.show();
+
+	    win.on('destroy', me.reload, me);
+	};
+
 	var edit_btn = new Proxmox.button.Button({
 	    text: gettext('Edit'),
 	    selModel: sm,
@@ -436,6 +455,13 @@ Ext.define('PVE.qemu.HardwareView', {
 	    handler: run_move,
 	});
 
+	var reassign_btn = new Proxmox.button.Button({
+	    text: gettext('Reassign disk'),
+	    selModel: sm,
+	    disabled: true,
+	    handler: run_reassign,
+	});
+
 	var remove_btn = new Proxmox.button.Button({
 	    text: gettext('Remove'),
 	    defaultText: gettext('Remove'),
@@ -578,6 +604,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn.disable();
 		resize_btn.disable();
 		move_btn.disable();
+		reassign_btn.disable();
 		revert_btn.disable();
 		return;
 	    }
@@ -604,6 +631,8 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi) || !diskCap);
 
+	    reassign_btn.setDisabled(pending || !(isUsedDisk || isEfi || isUnusedDisk) || !diskCap);
+
 	    revert_btn.setDisabled(!pending);
 	};
 
@@ -752,6 +781,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn,
 		resize_btn,
 		move_btn,
+		reassign_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
-- 
2.20.1

Re: [pve-devel] [PATCH v5 manager 3/3] ui: qemu: Add disk reassign dialog

[Thomas Lamprecht]

On 15.02.21 16:26, Aaron Lauterer wrote:
> Adds a new button to the hardware panel labeled 'Reassign disk' and
> enables a user to reassign a disk to another VM.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> v4 -> v5: rebased
> 
> v3 -> v4:
> * added check to not show template VMs in dropdown
> * renamed disk parameter to `drive_name`
> 
> v2 -> v3:
> * fixed check to omit the current vmid in the target dropdown
> * renamed parameter disk to drive_key
> * added missing comma
> 
> v1 -> v2: fixed linter errors
> 
> This patch needs the previous patch series [0] applied which adds the
> backend for disk reassignments.
> 
> [0] https://lists.proxmox.com/pipermail/pve-devel/2020-December/046479.html
> 
>  www/manager6/Makefile             |  1 +
>  www/manager6/qemu/HDReassign.js   | 80 +++++++++++++++++++++++++++++++
>  www/manager6/qemu/HardwareView.js | 30 ++++++++++++
>  3 files changed, 111 insertions(+)
>  create mode 100644 www/manager6/qemu/HDReassign.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index 85f90ecd..21debfcb 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -200,6 +200,7 @@ JSSRC= 							\
>  	qemu/HDEdit.js					\
>  	qemu/HDEfi.js					\
>  	qemu/HDMove.js					\
> +	qemu/HDReassign.js				\
>  	qemu/HDResize.js				\
>  	qemu/HardwareView.js				\
>  	qemu/IPConfigEdit.js				\
> diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
> new file mode 100644
> index 00000000..d0aa97f8
> --- /dev/null
> +++ b/www/manager6/qemu/HDReassign.js
> @@ -0,0 +1,80 @@
> +Ext.define('PVE.window.HDReassign', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    resizeable: false,
> +    title: gettext('Reassign disk'),
> +    submitText: gettext('Reassign disk'),
> +    showReset: false,
> +    method: 'POST',
> +    showProgress: true,
> +    width: 350,
> +
> +    viewModel: {
> +	data: {
> +	    targetVMData: {},
> +	    show_running_hint: false,
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'combobox',
> +	    name: 'target_vmid',
> +	    reference: 'target_vmid',
> +	    fieldLabel: gettext('Target VMID'),
> +	    bind: {
> +		store: '{targetVMData}',
> +	    },
> +	    queryMode: 'local',
> +	    displayField: 'name',
> +	    valueField: 'vmid',
> +	    anyMatch: true,
> +	    emptyText: gettext('Select VM'),
> +	},
> +	{
> +	    xtype: 'displayfield',
> +	    padding: '5 0 0 0',
> +	    userCls: 'pmx-hint',
> +	    value: gettext('This disk cannot be reassigned while the VM is running'),
> +	    bind: {
> +		hidden: '{!show_running_hint}',
> +	    },
> +	},
> +    ],
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +	init: function(view) {
> +	    let me = view;
> +	    let vm = me.getViewModel();
> +	    let vms = PVE.Utils.getNodeVMs(me.nodename);
> +
> +	    let show_running_hint = vms[me.vmid].running && !me.drive_name.startsWith('unused');
> +	    vm.set('show_running_hint', show_running_hint);
> +
> +	    if (show_running_hint) {
> +	        me.lookup('target_vmid').setDisabled(true);
> +	    }
> +
> +	    let dropdownData = [];
> +	    for (const [vmid, data] of Object.entries(vms)) {
> +	        if (parseInt(vmid, 10) === parseInt(me.vmid, 10)) { continue; }
> +		if (data.template) { continue; }
> +	        dropdownData.push({
> +		    vmid: vmid,
> +		    name: `${vmid} ${data.name}`,
> +	        });
> +	    }
> +
> +	    vm.set('targetVMData', { data: dropdownData });
> +	},
> +    },
> +
> +    getValues: function() {
> +	let me = this;
> +	let values = me.callParent(arguments);
> +	values.drive_name = me.drive_name;
> +
> +	return values;
> +    },
> +});
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 77640e53..d129deda 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -415,6 +415,25 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    win.on('destroy', me.reload, me);
>  	};
>  
> +	var run_reassign = function() {
> +	    let rec = sm.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    let win = Ext.create('PVE.window.HDReassign', {
> +		drive_name: rec.data.key,
> +		vmid: vmid,
> +		nodename: nodename,
> +		title: gettext('Reassign disk') + ': ' + rec.data.key,
> +		url: '/api2/extjs/nodes/' + nodename + '/qemu/' + vmid + '/reassign_disk',

please use template strings for such things
`/api2/extjs/nodes/${nodename}/qemu/${vmid}/reassign-disk`,

> +	    });
> +
> +	    win.show();
> +
> +	    win.on('destroy', me.reload, me);
> +	};
> +
>  	var edit_btn = new Proxmox.button.Button({
>  	    text: gettext('Edit'),
>  	    selModel: sm,
> @@ -436,6 +455,13 @@ Ext.define('PVE.qemu.HardwareView', {
>  	    handler: run_move,
>  	});
>  
> +	var reassign_btn = new Proxmox.button.Button({
> +	    text: gettext('Reassign disk'),
> +	    selModel: sm,
> +	    disabled: true,
> +	    handler: run_reassign,
> +	});
> +
>  	var remove_btn = new Proxmox.button.Button({
>  	    text: gettext('Remove'),
>  	    defaultText: gettext('Remove'),
> @@ -578,6 +604,7 @@ Ext.define('PVE.qemu.HardwareView', {
>  		edit_btn.disable();
>  		resize_btn.disable();
>  		move_btn.disable();
> +		reassign_btn.disable();

please not yet another button in this already crowed GUI space...

This could actually live under move-disk, making it a split button or re-using
it completely. After all it's just a move-disk, just not to another storage but
to another VM.

>  		revert_btn.disable();
>  		return;
>  	    }
> @@ -604,6 +631,8 @@ Ext.define('PVE.qemu.HardwareView', {
>  
>  	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi) || !diskCap);
>  
> +	    reassign_btn.setDisabled(pending || !(isUsedDisk || isEfi || isUnusedDisk) || !diskCap);
> +
>  	    revert_btn.setDisabled(!pending);
>  	};
>  
> @@ -752,6 +781,7 @@ Ext.define('PVE.qemu.HardwareView', {
>  		edit_btn,
>  		resize_btn,
>  		move_btn,
> +		reassign_btn,
>  		revert_btn,
>  	    ],
>  	    rows: rows,
>