[pve-devel] [PATCH manager 1/2] lxc: show dynamically assigned IPs in network tab

[Gabriel Goller]

On 04.12.2024 10:17, Dominik Csapak wrote:
>generally looks good but i have one high level comment/question
>(and some nits inline)
>
>one thing i'd like to see here is to retain the info what is configured,
>so previously the info was either 'dhcp'/'auto' (slaac) or an ip address
>
>now we only show the ip adress
>
>what i mean is something like
>
>'x.y.z.w (dhcp)'
>
>or
>
>'xx00::1 (static)'
>
>etc. so one can still see what mode is configured

This is a nice idea, but it could be a bit tricky. To get the ip info we
execute `ip a` in the container's netns, but for some reason I can't see
the 'dynamic' option which is usually shown on a dynamically acquired
address.
I could use `ip route` and check if the route was inserted by 'dhcp' or
'kernel', but no idea how foolproof this is...

>On 12/2/24 11:46, Gabriel Goller wrote:
>>adds a call to /nodes/{node}/lxc/{vmid}/interfaces and merges the
>>returned data with the existing configuration. This will update the
>>IPv4 and IPv6 address, as well as the interface name (in case the
>>container changed it).
>>
>>Originally-by: Leo Nunner <l.nunner at proxmox.com>
>>Signed-off-by: Gabriel Goller <g.goller at proxmox.com>
>>---
>>  www/manager6/lxc/Network.js | 57 +++++++++++++++++++++++++++----------
>>  1 file changed, 42 insertions(+), 15 deletions(-)
>>
>>diff --git a/www/manager6/lxc/Network.js b/www/manager6/lxc/Network.js
>>index b2cd94109485..41de72f43646 100644
>>--- a/www/manager6/lxc/Network.js
>>+++ b/www/manager6/lxc/Network.js
>>@@ -356,25 +356,52 @@ Ext.define('PVE.lxc.NetworkView', {
>>  	Proxmox.Utils.setErrorMask(me, true);
>>+	let nodename = me.pveSelNode.data.node;
>>+	let vmid = me.pveSelNode.data.vmid;
>>+
>>  	Proxmox.Utils.API2Request({
>>-	    url: me.url,
>>+	    url: `/nodes/${nodename}/lxc/${vmid}/interfaces`,
>>+	    method: 'GET',
>>  	    failure: function(response, opts) {
>>  		Proxmox.Utils.setErrorMask(me, gettext('Error') + ': ' + response.htmlStatus);
>>  	    },
>>-	    success: function(response, opts) {
>>-		Proxmox.Utils.setErrorMask(me, false);
>>-		let result = Ext.decode(response.responseText);
>>-		me.dataCache = result.data || {};
>>-		let records = [];
>>-		for (const [key, value] of Object.entries(me.dataCache)) {
>>-		    if (key.match(/^net\d+/)) {
>>-			let net = PVE.Parser.parseLxcNetwork(value);
>>-			net.id = key;
>>-			records.push(net);
>>-		    }
>>-		}
>>-		me.store.loadData(records);
>>-		me.down('button[name=addButton]').setDisabled(records.length >= 32);
>>+	    success: function(ifResponse, ifOpts) {
>>+		Proxmox.Utils.API2Request({
>>+		    url: me.url,
>>+		    failure: function(response, opts) {
>>+			Proxmox.Utils.setErrorMask(me, gettext('Error') + ': ' + response.htmlStatus);
>>+		    },
>>+		    success: function(confResponse, confOpts) {
>>+			Proxmox.Utils.setErrorMask(me, false);
>>+
>>+			let interfaces = [];
>>+			for (const [, iface] of Object.entries(ifResponse?.result?.data || {})) {
>>+			    interfaces[iface.hwaddr] = iface;
>>+			}
>>+
>>+			let result = Ext.decode(confResponse.responseText);
>
>i know it's pre-existing, but when touching the code we could directly use
>confResponse.result.data, no? AFAICS this is the already decoded info from there
>(no clue why that wasn't used before though...)

Ack.

>>+			me.dataCache = result.data || {};
>>+			let records = [];
>>+			for (const [key, value] of Object.entries(me.dataCache)) {
>>+			    if (key.match(/^net\d+/)) {
>>+				let net = PVE.Parser.parseLxcNetwork(value);
>>+				net.id = key;
>>+
>>+				let iface;
>>+				if ((iface = interfaces[net.hwaddr.toLowerCase()])) {
>>+				    net.name = iface.name;
>>+				    net.ip = iface.inet;
>>+				    net.ip6 = iface.inet6;
>>+				}
>
>this reads a bit odd with the if condition
>i'd rather use something like
>
>let iface = interfaces[net....];
>if (iface) {
>...
>}
>
>this should do the same, but is much easier to read

I agree.

Thanks for the review!