Re: [pve-devel] [PATCH v3 manager 1/4] ui: lxc/qemu: add disk reassign and action submenu

[Fabian Ebner <f.ebner@proxmox.com>]

Am 07.03.22 um 11:07 schrieb Aaron Lauterer:
> For the new HDReassign component, we follow the approach of HDMove to
> have one componend for qemu and lxc.
> 
> To avoid button clutter, a new "Disk/Volume action" button is
> introduced. It holds the Move, Reassign and Resize buttons in a submenu.
> 

Some small general things I noticed:
* Since re-assign to/from templates is not possible, we could filter
those out in the selection/disable the action (assuming the information
whether it's a template or not is readily available).
* For such drop-down menus we mostly (always?) have icons too.
* The move disk and reassign windows seem to have a bit much padding.

> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> changes since
> 
> v2:
> * switch from generic to Proxmox Edit window
> * add new submenu for disk/volume specific actions
> * code style improvements
> * simplify some labels, removing "disk" and "volume" as the context
>   already gives this away
> 
> 
> v1: incorporated feedback I got off list
> 
> * use more modern approaches
>     * arrow functions
>     * autoShow
>     * template strings
> * reduce predefined cbind values and use arrow functions in the cbind
>   directly in many cases
> * some code style issues and cleanup
> 
>  www/manager6/Makefile             |   1 +
>  www/manager6/lxc/Resources.js     |  62 +++++--
>  www/manager6/qemu/HDReassign.js   | 274 ++++++++++++++++++++++++++++++
>  www/manager6/qemu/HardwareView.js |  60 ++++++-
>  4 files changed, 378 insertions(+), 19 deletions(-)
>  create mode 100644 www/manager6/qemu/HDReassign.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index e6e01bd1..94a78d89 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -214,6 +214,7 @@ JSSRC= 							\
>  	qemu/HDTPM.js					\
>  	qemu/HDMove.js					\
>  	qemu/HDResize.js				\
> +	qemu/HDReassign.js				\

Nit: not ordered alphabetically

>  	qemu/HardwareView.js				\
>  	qemu/IPConfigEdit.js				\
>  	qemu/KeyboardEdit.js				\
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 15ee3c67..306a4988 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -151,7 +151,8 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    });
>  	};
>  
> -	var run_move = function(b, e, rec) {
> +	var run_move = function() {
> +	    let rec = me.selModel.getSelection()[0];

Style nit: could switch to let instead of var.

>  	    if (!rec) {
>  		return;
>  	    }
> @@ -168,6 +169,24 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    win.on('destroy', me.reload, me);
>  	};
>  
> +	let run_reassign = function() {
> +	    let rec = me.selModel.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    Ext.create('PVE.window.HDReassign', {
> +		disk: rec.data.key,
> +		nodename: nodename,
> +		autoShow: true,
> +		vmid: vmid,
> +		type: 'lxc',
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    });
> +	};
> +
>  	var edit_btn = new Proxmox.button.Button({
>  	    text: gettext('Edit'),
>  	    selModel: me.selModel,
> @@ -182,7 +201,7 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    handler: function() { me.run_editor(); },
>  	});
>  
> -	var resize_btn = new Proxmox.button.Button({
> +	var resize_menuitem = new Ext.menu.Item({

Same here.

>  	    text: gettext('Resize disk'),
>  	    selModel: me.selModel,
>  	    disabled: true,

----8<----

> @@ -265,9 +305,12 @@ Ext.define('PVE.lxc.RessourceView', {
>  	    }
>  	    edit_btn.setDisabled(noedit);
>  
> -	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
> -	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> -	    move_btn.setDisabled(!isDisk || !diskCap);
> +	    volumeaction_btn.setDisabled(!isDisk);
> +	    reassign_menuitem.setDisabled(isRootFS);
> +
> +	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
> +	    resize_menuitem.setDisabled(!isDisk || !diskCap || isUnusedDisk);
> +	    move_menuitem.setDisabled(!isDisk || !diskCap);

Nothing new, but could use '|| isUnusedDisk' too:
cannot move volume 'unused0', only configured volumes can be moved to
another storage

>  	    revert_btn.setDisabled(!pending);
>  
>  	    remove_btn.setText(isUsedDisk ? remove_btn.altText : remove_btn.defaultText);
> @@ -327,8 +370,7 @@ Ext.define('PVE.lxc.RessourceView', {
>  		},
>  		edit_btn,
>  		remove_btn,
> -		resize_btn,
> -		move_btn,
> +		volumeaction_btn,
>  		revert_btn,
>  	    ],
>  	    rows: rows,
> diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
> new file mode 100644
> index 00000000..48bc0b1f
> --- /dev/null
> +++ b/www/manager6/qemu/HDReassign.js
> @@ -0,0 +1,274 @@
> +Ext.define('PVE.window.HDReassign', {
> +    extend: 'Proxmox.window.Edit',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +    showReset: false,
> +    showProgress: true,
> +    method: 'POST',
> +
> +    viewModel: {
> +	data: {
> +	    mpType: '',
> +	},
> +	formulas: {
> +	    mpMaxCount: get => get('mpType') === 'mp'
> +		? PVE.Utils.mp_counts.mps - 1
> +		: PVE.Utils.mp_counts.unused - 1,
> +	},
> +    },
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    vmid: me.vmid,
> +	    disk: me.disk,
> +	    isQemu: me.type === 'qemu',
> +	    nodename: me.nodename,
> +	    url: `/nodes/${me.nodename}/${me.type}/${me.vmid}/`,
> +	};
> +    },
> +
> +    cbind: {
> +	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
> +	submitText: get => get('title'),
> +	qemu: '{isQemu}',
> +	url: '{url}',
> +    },
> +
> +    submitUrl: function(url, values) {
> +	url += this.qemu ? 'move_disk' : 'move_volume';

Why not already construct the full URL above?

> +	return url;
> +    },
> +
> +    getValues: function() {
> +	let me = this;
> +	let values = me.formPanel.getForm().getValues();
> +
> +	let params = {
> +	    vmid: me.vmid,
> +	    'target-vmid': values.targetVmid,
> +	};
> +
> +	params[me.qemu ? 'disk' : 'volume'] = me.disk;
> +
> +	if (me.qemu) {
> +	    params['target-disk'] = `${values.controller}${values.deviceid}`;
> +	} else {
> +	    params['target-volume'] = `${values.mpType}${values.mpId}`;
> +	}
> +	return params;
> +    },
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	initViewModel: function(model) {
> +	    let view = this.getView();
> +	    let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp';
> +	    model.set('mpType', mpTypeValue);
> +	},
> +
> +	onMpTypeChange: function(value) {
> +	    this.getView().getViewModel().set('mpType', value.getValue());
> +	    this.getView().lookup('mpIdSelector').validate();
> +	},
> +
> +	onTargetVMChange: function(f, vmid) {
> +	    let me = this;
> +	    let view = me.getView();
> +	    let diskSelector = view.lookup('diskSelector');
> +	    if (!vmid) {
> +		diskSelector.setVMConfig(null);
> +		me.VMConfig = null;
> +		return;
> +	    }
> +
> +	    let type = view.qemu ? 'qemu' : 'lxc';
> +
> +	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
> +	    Proxmox.Utils.API2Request({
> +		url: url,
> +		method: 'GET',
> +		failure: response => Ext.Msg.alert(gettext('Error'), response.htmlStatus),
> +		success: function(response, options) {
> +		    if (view.qemu) {
> +			diskSelector.setVMConfig(response.result.data);
> +			diskSelector.setDisabled(false);
> +		    } else {
> +			let mpIdSelector = view.lookup('mpIdSelector');
> +			let mpType = view.lookup('mpType');
> +
> +			view.VMConfig = response.result.data;
> +
> +			PVE.Utils.forEachMP((bus, i) => {

Feels hacky and in fact only works in all cases, because the number of
max unused and max mp is the same. The iterator only iterates over mount
points when called like this and you ignore the bus. Using a new helper
akin to PVE.Utils.nextFreeDisk or simply inlining the required loop
here, would be more robust.

> +			    let name = view.getViewModel().get('mpType') + i.toString();
> +			    if (!Ext.isDefined(view.VMConfig[name])) {
> +				mpIdSelector.setValue(i);
> +				return false;
> +			    }
> +			    return undefined;
> +			});
> +
> +			mpType.setDisabled(false);
> +			mpIdSelector.setDisabled(false);
> +			mpIdSelector.validate();
> +		    }
> +		},
> +	    });
> +	},
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'displayfield',
> +		    name: 'sourceDisk',
> +		    fieldLabel: gettext('Source'),
> +		    cbind: {
> +			name: get => get('isQemu') ? 'disk' : 'volume',
> +			value: '{disk}',
> +		    },
> +		    vtype: 'StorageId',

But it's not a storage ID.

> +		    allowBlank: false,
> +		},
> +		{
> +		    xtype: 'vmComboSelector',
> +		    reference: 'targetVMID',
> +		    name: 'targetVmid',
> +		    allowBlank: false,
> +		    fieldLabel: gettext('Target'),
> +		    bind: {
> +			value: '{targetVMID}',
> +		    },
> +		    store: {
> +			model: 'PVEResources',
> +			autoLoad: true,
> +			sorters: 'vmid',
> +			cbind: {}, // for nested cbinds
> +			filters: [
> +			    {
> +				property: 'type',
> +				cbind: {
> +				    value: get => get('isQemu') ? /qemu/ : /lxc/,

Nit: the type can be matched exaclty, or was there a reason for using a
regex?

> +				},
> +			    },
> +			    {
> +				property: 'node',
> +				cbind: {
> +				    value: '{nodename}',
> +				},
> +			    },
> +			    {
> +				property: 'vmid',
> +				operator: '!=',
> +				cbind: {
> +				    value: '{vmid}',
> +				},
> +			    },
> +			],
> +		    },
> +		    listeners: { change: 'onTargetVMChange' },
> +		},
> +		{
> +		    xtype: 'pveControllerSelector',
> +		    reference: 'diskSelector',
> +		    withUnused: true,
> +		    disabled: true,
> +		    cbind: {
> +			hidden: '{!isQemu}',
> +		    },
> +		},
> +		{
> +		    xtype: 'container',
> +		    layout: 'hbox',
> +		    cbind: {
> +			hidden: '{isQemu}',
> +			disabled: '{isQemu}',
> +		    },
> +		    items: [
> +			{
> +			    xtype: 'pmxDisplayEditField',
> +			    cbind: {
> +				editable: get => !get('disk').match(/^unused\d+/),
> +				value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp',
> +			    },
> +			    disabled: true,
> +			    name: 'mpType',
> +			    reference: 'mpType',
> +			    fieldLabel: gettext('Add as'),
> +			    submitValue: true,
> +			    flex: 4,
> +			    editConfig: {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'mpTypeCombo',
> +				reference: 'mpTypeCombo',
> +				deleteEmpty: false,
> +				cbind: {
> +				    hidden: '{isQemu}',
> +				},
> +				comboItems: [
> +				    ['mp', gettext('Mount Point')],
> +				    ['unused', gettext('Unused')],
> +				],
> +				listeners: { change: 'onMpTypeChange' },
> +			    },
> +			},
> +			{
> +			    xtype: 'proxmoxintegerfield',
> +			    name: 'mpId',
> +			    reference: 'mpIdSelector',
> +			    minValue: 0,
> +			    flex: 1,
> +			    allowBlank: false,
> +			    validateOnChange: true,
> +			    disabled: true,
> +			    bind: {
> +				maxValue: '{mpMaxCount}',
> +			    },
> +			    validator: function(value) {
> +				let view = this.up('window');
> +				let type = view.getViewModel().get('mpType');
> +				if (Ext.isDefined(view.VMConfig[type+value])) {

Style nit: could also use `${type}${value}`

> +				    return "Mount point is already in use.";
> +				}
> +				return true;
> +			    },
> +			},
> +		    ],
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +
> +	if (!me.vmid) {
> +	    throw "no VM ID specified";
> +	}
> +
> +	if (!me.type) {
> +	    throw "no type specified";
> +	}
> +
> +	me.callParent();
> +    },
> +});

----8<----