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

[Aaron Lauterer <a.lauterer@proxmox.com>]

On 3/10/22 11:49, Fabian Ebner wrote:
> 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).

good point, definitely doable

> * For such drop-down menus we mostly (always?) have icons too.

Yes, but since the existing buttons did not have any icons and finding matching and not yet (too widely) used icons in font-awesome is getting tricky, I opted for a menu without icons.
I'd rather have no icons instead of poorly fitting ones. But definitely something that can be discussed and changed quite easily.


> * The move disk and reassign windows seem to have a bit much padding.

thanks for catching that! I'll investigate where that happens.

[...]

>> 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

yep, will fix it

> 
>>   	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.

yeah, I tried to keep it similar to the surrounding, but we could definitely switch it over to let

> 
[...]
>>   
>> -	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

I kept it as is because, and maybe that was also considered originally, getting an error message at some point, explaining why it does not work, helps more than having the button disabled without knowing why.

[...]
>> +    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?

I'll see how to do that. First tests show that if I construct the submitUrl directly, I need to set the 'url' at some point, even if not used because the edit window does check its existence.

[...]

>> +	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.

Yep, going for a PVE.Utils.nextFreeMP implementation

> 
>> +			    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.

yeah, bad copy pasta and AFAICT not even needed here

> 
>> +		    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?

IIRC not really, will change it

[...]

>> +			{
>> +			    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}`

good point