[pve-devel] [PATCH pve-manager 1/1] Close #2443: gui: qemu/NetworkEdit: add network mtu advanced option

[Thomas Lamprecht]

First, thanks for the patch!
Most seems to be adapted from surrounding code, which is OK.
Some comments inline.

Also, a pve-docs patch about this would be nice, to clear up things like:

* MTU cannot be bigger than those used by the host in it's chain to the
  outer network.
* All components of the network need to support this MTU
* one can test it under linux with `ping -M do -s $[9000 - 28] <addr>`
  (where 9000 is the MTU and 28 is the ICMP echo packet overhead)
* That only VirtIO-net supports this at the moment (even if the UI makes it
  somewhat clear it's good to state that it's by design and not a bug were
  we just forgot to enable it for e1000e or the like :) )
* Intra-bridge communications can have 64k MTU if the bridge is configured
  to it

I.e., doesn't have to be much, but a few short sentences would be good to have
for a new feature which can be a bit environment picky :)

On 12/11/19 6:03 PM, Paul Shepel wrote:
> Signed-off-by: Paul Shepel <tacid at tacid.kiev.ua>
> ---
>  www/manager6/Parser.js           |  5 +++++
>  www/manager6/qemu/NetworkEdit.js | 36 +++++++++++++++++++++++++++++---
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/www/manager6/Parser.js b/www/manager6/Parser.js
> index 0790e683..6a9cbb78 100644
> --- a/www/manager6/Parser.js
> +++ b/www/manager6/Parser.js
> @@ -131,6 +131,8 @@ Ext.define('PVE.Parser', { statics: {
>  		res.disconnect = match_res[1];
>  	    } else if ((match_res = p.match(/^queues=(\d+)$/)) !== null) {
>  		res.queues = match_res[1];
> +	    } else if ((match_res = p.match(/^mtu=(\d+)$/)) !== null) {
> +		res.mtu = match_res[1];
>  	    } else if ((match_res = p.match(/^trunks=(\d+(?:-\d+)?(?:;\d+(?:-\d+)?)*)$/)) !== null) {
>  		res.trunks = match_res[1];
>  	    } else {
> @@ -167,6 +169,9 @@ Ext.define('PVE.Parser', { statics: {
>  	if (net.queues) {
>  	    netstr += ",queues=" + net.queues;
>  	}
> +	if (net.mtu) {
> +	    netstr += ",mtu=" + net.mtu;
> +	}
>  	if (net.disconnect) {
>  	    netstr += ",link_down=" + net.disconnect;
>  	}
> diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
> index abce4903..c66da59c 100644
> --- a/www/manager6/qemu/NetworkEdit.js
> +++ b/www/manager6/qemu/NetworkEdit.js
> @@ -26,6 +26,12 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  	    delete me.network.rate;
>  	}
>  
> +	if (values.mtu) {
> +	    me.network.mtu = values.mtu;
> +	} else {
> +	    delete me.network.mtu;
> +	}
> +
>  	var params = {};
>  
>  	params[me.confid] = PVE.Parser.printQemuNetwork(me.network);
> @@ -92,7 +98,19 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  		xtype: 'proxmoxcheckbox',
>  		fieldLabel: gettext('Disconnect'),
>  		name: 'disconnect'
> -	    }
> +	    },
> +	    {
> +		xtype: 'numberfield',
> +		fieldLabel: gettext('MTU'),
> +		minValue: 576,
> +		maxValue: 65535,
> +		value: '',
> +		emptyText: '1500',
> +		name: 'mtu',
> +		disabled: true,
> +		hidden: true,
> +		allowBlank: true
> +            }
>  	];
>  
>  	if (me.insideWizard) {
> @@ -111,7 +129,8 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  			    'model',
>  			    'macaddr',
>  			    'rate',
> -			    'queues'
> +			    'queues',
> +			    'mtu',
>  			];
>  			fields.forEach(function(fieldname) {
>  			    me.down('field[name='+fieldname+']').setDisabled(value);
> @@ -150,7 +169,18 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
>  		maxValue: 10*1024,
>  		value: '',
>  		emptyText: 'unlimited',
> -		allowBlank: true
> +		allowBlank: true,
> +		listeners: {
> +                    change: function(f, value) {
> +                        if (!me.rendered) {
> +                            return;
> +                        }
> +                        var mtuField = me.down('field[name=mtu]');
> +                        var isVirtioNet = ( value == 'virtio' );
> +                        mtuField.setDisabled(!isVirtioNet);
> +                        mtuField.setHidden(!isVirtioNet);
> +                    }
> +                }

1. Indentation is completely off
2. This is on the Ratelimit numberfield, how can this possibly work?
3. I'd rather go for a viewModel + binding approach, i.e., the following
   change on top of this patch (I ommited the hiding on purpose, we seldom do
   this):

----8<----
diff --git a/www/manager6/qemu/NetworkEdit.js b/www/manager6/qemu/NetworkEdit.js
index c66da59c..b3801419 100644
--- a/www/manager6/qemu/NetworkEdit.js
+++ b/www/manager6/qemu/NetworkEdit.js
@@ -3,6 +3,14 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
     alias: 'widget.pveQemuNetworkInputPanel',
     onlineHelp: 'qm_network_device',
 
+    viewModel: {
+       data: {
+           cardModel: PVE.qemu.OSDefaults.generic.networkCard,
+       },
+       formulas: {
+           cardIsVirtIO: (get) => get('cardModel') === 'virtio',
+       }
+    },
     insideWizard: false,
 
     onGetValues: function(values) {
@@ -107,8 +115,9 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
                value: '',
                emptyText: '1500',
                name: 'mtu',
-               disabled: true,
-               hidden: true,
+               bind: {
+                   disabled: '{!cardIsVirtIO}',
+               },
                allowBlank: true
             }
        ];
@@ -150,6 +159,9 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
                name: 'model',
                fieldLabel: gettext('Model'),
                value: PVE.qemu.OSDefaults.generic.networkCard,
+               bind: {
+                   value: '{cardModel}',
+               },
                allowBlank: false
            },
            {
@@ -170,17 +182,6 @@ Ext.define('PVE.qemu.NetworkInputPanel', {
                value: '',
                emptyText: 'unlimited',
                allowBlank: true,
-               listeners: {
-                    change: function(f, value) {
-                        if (!me.rendered) {
-                            return;
-                        }
-                        var mtuField = me.down('field[name=mtu]');
-                        var isVirtioNet = ( value == 'virtio' );
-                        mtuField.setDisabled(!isVirtioNet);
-                        mtuField.setHidden(!isVirtioNet);
-                    }
-                }
            },
            {
                xtype: 'proxmoxintegerfield',
---