[pve-devel] [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[pve-devel] [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[Daniel Tschlatscher]

The option to set the mtu parameter for lxc containers already exists
in the backend. It just has to be exposed in the web UI as well.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v1:
* fieldLabel for MTU textfield no longer uses gettext()
* text field has an emptyText now
* the minimum value for the MTU is 576 in the frontend now

 www/manager6/lxc/Network.js | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
index 7b6437c5..c6c40934 100644
--- a/www/manager6/lxc/Network.js
+++ b/www/manager6/lxc/Network.js
@@ -282,6 +282,18 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
 	    },
 	];
 
+	me.advancedColumn1 = [
+	    {
+		xtype: 'proxmoxintegerfield',
+		fieldLabel: 'MTU',
+		emptyText: gettext('Same as bridge'),
+		name: 'mtu',
+		value: cdata.mtu,
+		minValue: 576,
+		maxValue: 65535,
+	    },
+	];
+
 	me.callParent();
     },
 });
@@ -519,6 +531,11 @@ Ext.define('PVE.lxc.NetworkView', {
 			}
 		    },
 		},
+		{
+		    header: gettext('MTU'),
+		    width: 80,
+		    dataIndex: 'mtu',
+		},
 	    ],
 	    listeners: {
 		activate: me.load,
@@ -543,6 +560,7 @@ Ext.define('PVE.lxc.NetworkView', {
 	    'gw6',
 	    'tag',
 	    'firewall',
+	    'mtu',
 	],
     });
 });
-- 
2.30.2

[pve-devel] [PATCH manager v2 2/2] gui: move rate limit field to advanced section

[Daniel Tschlatscher]

The new MTU field and the rate limit field are now in the advanced
section of the NetworkInputPanel to parallel the layout of the
NetworkEdit for VMs.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v1
* This patch now only moves the rate limit text field

 www/manager6/lxc/Network.js | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
index c6c40934..85033bd8 100644
--- a/www/manager6/lxc/Network.js
+++ b/www/manager6/lxc/Network.js
@@ -122,16 +122,6 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
 		name: 'tag',
 		value: cdata.tag,
 	    },
-	    {
-		xtype: 'numberfield',
-		name: 'rate',
-		fieldLabel: gettext('Rate limit') + ' (MB/s)',
-		minValue: 0,
-		maxValue: 10*1024,
-		value: cdata.rate,
-		emptyText: 'unlimited',
-		allowBlank: true,
-	    },
 	    {
 		xtype: 'proxmoxcheckbox',
 		fieldLabel: gettext('Firewall'),
@@ -294,6 +284,19 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
 	    },
 	];
 
+	me.advancedColumn2 = [
+	    {
+		xtype: 'numberfield',
+		name: 'rate',
+		fieldLabel: gettext('Rate limit') + ' (MB/s)',
+		minValue: 0,
+		maxValue: 10*1024,
+		value: cdata.rate,
+		emptyText: 'unlimited',
+		allowBlank: true,
+	    },
+	];
+
 	me.callParent();
     },
 });
-- 
2.30.2

[pve-devel] [PATCH container v2] better parsing for lxc networking mtu setting

[Daniel Tschlatscher]

This patch reworks some mtu settings for LXC containers in the backend
Namely, introducing an absolute maximum for the MTU field of 65535 and
asserting that the MTU setting isn't bigger than the bridge's MTU size

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v1:
* New patch

The functionality of checking whether the config option for 'mtu' is
valid is implemented somewhat redundant here. This is due to
'update_lxc_config' handling the VM start check and 'update_pct_config'
handling the general configuration check.
As far as I can tell, there is no location in the code, that could
handle both cases centrally and elegantly (at least not without major
restructuring, which seem very overkill for this feature)
Of course, open for suggestions though

 src/PVE/LXC.pm        | 10 +++++++++-
 src/PVE/LXC/Config.pm |  9 +++++++++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/PVE/LXC.pm b/src/PVE/LXC.pm
index 333286a..ac45fc6 100644
--- a/src/PVE/LXC.pm
+++ b/src/PVE/LXC.pm
@@ -730,7 +730,15 @@ sub update_lxc_config {
 	$raw .= "lxc.net.$ind.veth.pair = veth${vmid}i${ind}\n";
 	$raw .= "lxc.net.$ind.hwaddr = $d->{hwaddr}\n" if defined($d->{hwaddr});
 	$raw .= "lxc.net.$ind.name = $d->{name}\n" if defined($d->{name});
-	$raw .= "lxc.net.$ind.mtu = $d->{mtu}\n" if defined($d->{mtu});
+
+	# Keep container from starting with invalid mtu configuration
+	if (my $mtu = $d->{mtu}) {
+	    my $bridge_mtu = PVE::Network::read_bridge_mtu($d->{bridge});
+	    die "$k: MTU size '$mtu' is bigger than bridge MTU '$bridge_mtu'\n"
+		if ($mtu > $bridge_mtu);
+
+	    $raw .= "lxc.net.$ind.mtu = $mtu\n";
+	}
 
 	# Starting with lxc 4.0, we do not patch lxc to execute our up-scripts.
 	if ($lxc_major >= 4) {
diff --git a/src/PVE/LXC/Config.pm b/src/PVE/LXC/Config.pm
index d1fdd50..4bb27ff 100644
--- a/src/PVE/LXC/Config.pm
+++ b/src/PVE/LXC/Config.pm
@@ -755,6 +755,7 @@ our $netconf_desc = {
 	type => 'integer',
 	description => 'Maximum transfer unit of the interface. (lxc.network.mtu)',
 	minimum => 64, # minimum ethernet frame is 64 bytes
+	maximum => 65535,
 	optional => 1,
     },
     ip => {
@@ -1110,6 +1111,14 @@ sub update_pct_config {
 	    $value = PVE::LXC::verify_searchdomain_list($value);
 	} elsif ($opt eq 'unprivileged') {
 	    die "unable to modify read-only option: '$opt'\n";
+	} elsif ($opt =~ m/^net(\d+)$/) {
+	    my $res = PVE::JSONSchema::parse_property_string($netconf_desc, $value);
+
+	    if (my $mtu = $res->{mtu}) {
+		my $bridge_mtu = PVE::Network::read_bridge_mtu($res->{bridge});
+		die "$opt: MTU size '$mtu' is bigger than bridge MTU '$bridge_mtu'\n"
+		    if ($mtu > $bridge_mtu);
+	    }
 	}
 	$conf->{pending}->{$opt} = $value;
 	$class->remove_from_pending_delete($conf, $opt);
-- 
2.30.2

Re: [pve-devel] [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[Stefan Hanreich]

Tested on an Alpine Linux Container (3.16):

- setting MTU > 65535 or < 576 via UI (doesn't work)
- setting MTU to several values >= 576 and <= 65535 via UI (works)
- setting MTU to >= 64 and < 576 via config then starting (works, but 
this is apparently intended otherwise it would be a backwards breaking 
change)
- setting MTU >= 576 and <= 65535 via Config and then starting the 
container (works)
- setting MTU > 65535 or < 64 via Config and then starting the container 
(doesn't work)

Some Notes:
- Setting the MTU while the container is running, does not update the 
MTU of the running container. If this is intended behavior it might be 
smart to document it somewhere or throw a warning. This also doesn't 
work for the VM patch. Not sure if this is even possible at runtime tbh.
- Adding a network device when the Container is running with a specific, 
valid MTU (e.g. 1234) does add the network device to the container, BUT 
it has a MTU of 1500. Upon reboot the correct MTU is set. Reloading the 
Network config does not change anything. Maybe just an LXC limitation?

Code LGTM - small nit: there is still a gettext('MTU') left in the 
NetworkView, but it has been changed in the NetworkInputPanel.

Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>

On 11/3/22 16:38, Daniel Tschlatscher wrote:
> The option to set the mtu parameter for lxc containers already exists
> in the backend. It just has to be exposed in the web UI as well.
>
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
> Changes from v1:
> * fieldLabel for MTU textfield no longer uses gettext()
> * text field has an emptyText now
> * the minimum value for the MTU is 576 in the frontend now
>
>   www/manager6/lxc/Network.js | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
>
> diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
> index 7b6437c5..c6c40934 100644
> --- a/www/manager6/lxc/Network.js
> +++ b/www/manager6/lxc/Network.js
> @@ -282,6 +282,18 @@ Ext.define('PVE.lxc.NetworkInputPanel', {
>   	    },
>   	];
>   
> +	me.advancedColumn1 = [
> +	    {
> +		xtype: 'proxmoxintegerfield',
> +		fieldLabel: 'MTU',
> +		emptyText: gettext('Same as bridge'),
> +		name: 'mtu',
> +		value: cdata.mtu,
> +		minValue: 576,
> +		maxValue: 65535,
> +	    },
> +	];
> +
>   	me.callParent();
>       },
>   });
> @@ -519,6 +531,11 @@ Ext.define('PVE.lxc.NetworkView', {
>   			}
>   		    },
>   		},
> +		{
> +		    header: gettext('MTU'),
> +		    width: 80,
> +		    dataIndex: 'mtu',
> +		},
>   	    ],
>   	    listeners: {
>   		activate: me.load,
> @@ -543,6 +560,7 @@ Ext.define('PVE.lxc.NetworkView', {
>   	    'gw6',
>   	    'tag',
>   	    'firewall',
> +	    'mtu',
>   	],
>       });
>   });

[pve-devel] applied: [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[Thomas Lamprecht]

Am 11/11/2022 um 12:14 schrieb Stefan Hanreich:
> Some Notes:
> - Setting the MTU while the container is running, does not update the MTU of
>   the running container. If this is intended behavior it might be smart to
>   document it somewhere or throw a warning. This also doesn't
>   work for the VM patch. Not sure if this is even possible at runtime tbh.
> - Adding a network device when the Container is running with a specific,
>   valid MTU (e.g. 1234) does add the network device to the container, BUT it
>   has a MTU of 1500. Upon reboot the correct MTU is set. Reloading the
>   Network config does not change anything. Maybe just an LXC limitation?
> 

good notes which we should also improve (either implementing it or ensuring
that the change stays pending if it cannot be hot-plugged), but both
pre-existing behavior and so not a blocker for this.

> Code LGTM - small nit: there is still a gettext('MTU') left in the
> NetworkView, but it has been changed in the NetworkInputPanel.
> 
> Reviewed-by: Stefan Hanreich <s.hanreich@proxmox.com>
> Tested-by: Stefan Hanreich <s.hanreich@proxmox.com>

with above applied, thanks!

Re: [pve-devel] applied: [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[Wolfgang Bumiller]

On Wed, Nov 16, 2022 at 08:11:22PM +0100, Thomas Lamprecht wrote:
> Am 11/11/2022 um 12:14 schrieb Stefan Hanreich:
> > Some Notes:
> > - Setting the MTU while the container is running, does not update the MTU of
> >   the running container. If this is intended behavior it might be smart to
> >   document it somewhere or throw a warning. This also doesn't
> >   work for the VM patch. Not sure if this is even possible at runtime tbh.

^ This is the same for VMs and the MTU is *sort of* tied to the bridge's
mtu anyway. (Tbh I don't see the point in even having a setting for it
in the first place...)

> > - Adding a network device when the Container is running with a specific,
> >   valid MTU (e.g. 1234) does add the network device to the container, BUT it
> >   has a MTU of 1500. Upon reboot the correct MTU is set. Reloading the

^ not actually 1500, but whatever the *bridge* is configured to

> >   Network config does not change anything. Maybe just an LXC limitation?
> > 
> 
> good notes which we should also improve (either implementing it or ensuring
> that the change stays pending if it cannot be hot-plugged), but both
> pre-existing behavior and so not a blocker for this.

MTUs are a bit of a beast.
I'm not sure we really need to change much here.
On the qemu side we only really check that it's valid, and pass it only
for virtio-net devices via the qemu command line, but do nothing when
changing running vms.
The thing is, when attaching a device to a bridge, it'll inherit the
bridge's mtu, so the host would usually see a different mtu there.
Not sure if - especially for containers - it makes much sense to have a
split view here, so unless someone really needs it, I'd say leave it ;-)

Re: [pve-devel] applied: [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI

[DERUMIER, Alexandre]

Le jeudi 17 novembre 2022 à 13:52 +0100, Wolfgang Bumiller a écrit :
> ^ This is the same for VMs and the MTU is *sort of* tied to the
> bridge's
> mtu anyway. (Tbh I don't see the point in even having a setting for
> it
> in the first place...)

In production, all my proxmox vmbr (vlan-aware),ethX && my physical
switch have mtu 9000~9200 by default.

Only some vms need to use mtu 9000 (mostly storage). Vm with public ip
need to use 1500 to be routed correctly.