public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
From: Stefan Hanreich <s.hanreich@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>,
	Daniel Tschlatscher <d.tschlatscher@proxmox.com>
Subject: Re: [pve-devel] [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI
Date: Fri, 11 Nov 2022 12:14:29 +0100	[thread overview]
Message-ID: <5fd1a4f0-8afc-ad12-db52-6a19d42ffced@proxmox.com> (raw)
In-Reply-To: <20221103153810.690086-1-d.tschlatscher@proxmox.com>

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',
>   	],
>       });
>   });




  parent reply	other threads:[~2022-11-11 11:24 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-03 15:38 Daniel Tschlatscher
2022-11-03 15:38 ` [pve-devel] [PATCH manager v2 2/2] gui: move rate limit field to advanced section Daniel Tschlatscher
2022-11-03 15:38 ` [pve-devel] [PATCH container v2] better parsing for lxc networking mtu setting Daniel Tschlatscher
2022-11-11 11:14 ` Stefan Hanreich [this message]
2022-11-16 19:11   ` [pve-devel] applied: [PATCH manager v2 1/2] fix #3719: gui: expose LXC MTU option in web UI Thomas Lamprecht
2022-11-17 12:52     ` Wolfgang Bumiller
2022-11-17 13:13       ` DERUMIER, Alexandre

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=5fd1a4f0-8afc-ad12-db52-6a19d42ffced@proxmox.com \
    --to=s.hanreich@proxmox.com \
    --cc=d.tschlatscher@proxmox.com \
    --cc=pve-devel@lists.proxmox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal