Re: [pve-devel] [PATCH v6 manager] ui: vm network: allow to override MTU for virtio devices

[Daniel Tschlatscher <d.tschlatscher@proxmox.com>]

This patch still applies well and works as intended.

There is one UI suggestion inline.


Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

On 5/20/22 10:03, Oguz Bektas wrote:
> we already have the 'mtu' option in the API, so we can just expose
> that option inside the 'Advanced' menu for virtio network interfaces.
> 
> Signed-off-by: Oguz Bektas <o.bektas@proxmox.com>
> ---
>  www/manager6/qemu/NetworkEdit.js | 34 ++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> v5->v6:
> * move the MTU bar to the left column, kept the message tooltip on the
> right one> * minor syntax changes from thomas' review on v5
> 
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index b39cffdc..d9f9b859 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -19,6 +19,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  	me.network.macaddr = values.macaddr;
>  	me.network.disconnect = values.disconnect;
>  	me.network.queues = values.queues;
> +	me.network.mtu = values.mtu;
>  
>  	if (values.rate) {
>  	    me.network.rate = values.rate;
> @@ -33,6 +34,17 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  	return params;
>      },
>  
> +    viewModel: {
> +	data: {
> +	    networkModel: '',
> +	    mtu: '',
> +	},
> +	formulas: {
> +	    isVirtio: get => get('networkModel') === 'virtio',
> +	    showMtuHint: get => get('mtu') === 1,
> +	},
> +    },
> +
>      setNetwork: function(confid, data) {
>  	var me = this;
>  
> @@ -93,6 +105,18 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  		fieldLabel: gettext('Disconnect'),
>  		name: 'disconnect',
>  	    },
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		name: 'mtu',
> +		fieldLabel: 'MTU',
> +		bind: {
> +		    disabled: '{!isVirtio}',
> +		    value: '{mtu}',
> +		},
> +		minValue: 1,
> +		maxValue: 65520,
> +		allowBlank: true,
> +	    },
>  	];
>  
>  	if (me.insideWizard) {
> @@ -112,6 +136,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  			    'macaddr',
>  			    'rate',
>  			    'queues',
> +			    'mtu',
>  			];
>  			fields.forEach(function(fieldname) {
>  			    me.down('field[name='+fieldname+']').setDisabled(value);
> @@ -130,6 +155,7 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  		xtype: 'pveNetworkCardSelector',
>  		name: 'model',
>  		fieldLabel: gettext('Model'),
> +		bind: '{networkModel}',
>  		value: PVE.qemu.OSDefaults.generic.networkCard,
>  		allowBlank: false,
>  	    },
> @@ -161,6 +187,14 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  		value: '',
>  		allowBlank: true,
>  	    },
> +	    {
> +		xtype: 'displayfield',
> +		userCls: 'pmx-hint',
> +		value: gettext("Use the special value '1' to inherit the MTU value from the underlying bridge"),
> +		bind: {
> +		    hidden: '{!showMtuHint}',
> +		},
> +	    },

I think it might make a bit more sense to move this into the
advancedColumn1 as well, as now it is displayed in the opposite column,
which makes the UI flow kind of confusing.
Perhaps an even better option though, would be to move the displayfield
into an advancedColumnB, which would also remove the line width limit
and with it the somewhat ugly line break.

>  	];
>  
>  	me.callParent();