[pve-devel] [PATCH manager 0/2] Show container ip in summary and network tab

[pve-devel] [PATCH manager 0/2] Show container ip in summary and network tab

[Gabriel Goller]

Show the ip/hwaddress of the network interfaces of containers in the summary
tab of the container and in the network tab on a per-interface basis.

This series was originally by Leo Nunner: 
https://lore.proxmox.com/pve-devel/20230615094333.66179-1-l.nunner@proxmox.com/

The pve-container patch has already been applied, so only the ui changes are missing.

manager:

Gabriel Goller (2):
  lxc: show dynamically assigned IPs in network tab
  lxc: show IPs in summary view

 www/manager6/Makefile                 |   1 +
 www/manager6/lxc/ContainerIPView.js   | 194 ++++++++++++++++++++++++++
 www/manager6/lxc/Network.js           |  57 ++++++--
 www/manager6/panel/GuestStatusView.js |  12 +-
 www/manager6/panel/GuestSummary.js    |   2 +-
 5 files changed, 249 insertions(+), 17 deletions(-)
 create mode 100644 www/manager6/lxc/ContainerIPView.js


Summary over all repositories:
  5 files changed, 249 insertions(+), 17 deletions(-)

-- 
Generated by git-murpp 0.7.1


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Gabriel Goller]

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@proxmox.com>
Signed-off-by: Gabriel Goller <g.goller@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);
+			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;
+				}
+
+				records.push(net);
+			    }
+			}
+
+			me.store.loadData(records);
+			me.down('button[name=addButton]').setDisabled(records.length >= 32);
+		    },
+		});
 	    },
 	});
     },
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Gabriel Goller]

modelled after the QEMU Guest Agent UI. We only show the first
non-loopback IP on the summary page itself.

Originally-by: Leo Nunner <l.nunner@proxmox.com>
    [GG: increase status panel height]
Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
---
 www/manager6/Makefile                 |   1 +
 www/manager6/lxc/ContainerIPView.js   | 194 ++++++++++++++++++++++++++
 www/manager6/panel/GuestStatusView.js |  12 +-
 www/manager6/panel/GuestSummary.js    |   2 +-
 4 files changed, 207 insertions(+), 2 deletions(-)
 create mode 100644 www/manager6/lxc/ContainerIPView.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index c94a5cdfbf70..203a9d19cefc 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -201,6 +201,7 @@ JSSRC= 							\
 	lxc/ResourceEdit.js				\
 	lxc/Resources.js				\
 	lxc/MultiMPEdit.js				\
+	lxc/ContainerIPView.js				\
 	menu/MenuItem.js				\
 	menu/TemplateMenu.js				\
 	ceph/CephInstallWizard.js			\
diff --git a/www/manager6/lxc/ContainerIPView.js b/www/manager6/lxc/ContainerIPView.js
new file mode 100644
index 000000000000..69b107af3243
--- /dev/null
+++ b/www/manager6/lxc/ContainerIPView.js
@@ -0,0 +1,194 @@
+Ext.define('PVE.window.ContainerIPInfo', {
+    extend: 'Ext.window.Window',
+    width: 600,
+    title: gettext('Container Network Information'),
+    height: 300,
+    layout: {
+	type: 'fit',
+    },
+    modal: true,
+    items: [
+	{
+	    xtype: 'grid',
+	    store: {},
+	    emptyText: gettext('No network information'),
+	    columns: [
+		{
+		    dataIndex: 'name',
+		    text: gettext('Name'),
+		    flex: 2,
+		},
+		{
+		    dataIndex: 'hwaddr',
+		    text: gettext('MAC address'),
+		    width: 140,
+		},
+		{
+		    dataIndex: 'inet',
+		    text: gettext('IPv4 address'),
+		    align: 'right',
+		    flex: 3,
+		},
+		{
+		    dataIndex: 'inet6',
+		    text: gettext('IPv6 address'),
+		    align: 'right',
+		    flex: 4,
+		},
+	    ],
+	},
+    ],
+});
+
+Ext.define('PVE.lxc.IPView', {
+    extend: 'Ext.container.Container',
+    xtype: 'pveContainerIPView',
+
+    layout: {
+	type: 'hbox',
+	align: 'top',
+    },
+
+    items: [
+	{
+	    xtype: 'box',
+	    html: '<i class="fa fa-exchange"></i> IPs',
+	},
+	{
+	    xtype: 'container',
+	    flex: 1,
+	    layout: {
+		type: 'vbox',
+		align: 'right',
+		pack: 'end',
+	    },
+	    items: [
+		{
+		    xtype: 'label',
+		    flex: 1,
+		    itemId: 'ipBox',
+		    style: {
+			'text-align': 'right',
+		    },
+		},
+		{
+		    xtype: 'button',
+		    itemId: 'moreBtn',
+		    hidden: true,
+		    ui: 'default-toolbar',
+		    handler: function(btn) {
+			let view = this.up('pveContainerIPView');
+
+			var win = Ext.create('PVE.window.ContainerIPInfo');
+			win.down('grid').getStore().setData(view.ifaces);
+			win.show();
+		    },
+		    text: gettext('More'),
+		},
+	    ],
+	},
+    ],
+
+    getDefaultIps: function(ifaces) {
+	var me = this;
+	var ips = [];
+	ifaces.forEach(function(iface) {
+	    // We only want to show the first non-loopback interface
+	    if (!ips.length &&
+		iface.data.hwaddr &&
+		iface.data.hwaddr !== '00:00:00:00:00:00' &&
+		iface.data.hwaddr !== '0:0:0:0:0:0') {
+		ips.push(iface.data.inet);
+		ips.push(iface.data.inet6);
+	    }
+	});
+
+	return ips;
+    },
+
+    startIPStore: function(store, records, success) {
+	var me = this;
+	let state = store.getById('status');
+
+	me.running = state && state.data.value === 'running';
+
+	var caps = Ext.state.Manager.get('GuiCap');
+
+	if (!caps.vms['VM.Monitor']) {
+	    var errorText = gettext("Requires '{0}' Privileges");
+	    me.updateStatus(false, Ext.String.format(errorText, 'VM.Monitor'));
+	    return;
+	}
+
+	if (me.running && me.ipStore.isStopped) {
+	    me.ipStore.startUpdate();
+	} else if (me.ipStore.isStopped) {
+	    me.updateStatus();
+	}
+    },
+
+    updateStatus: function(unsuccessful, defaulttext) {
+	var me = this;
+	var text = defaulttext || gettext('No network information');
+	var more = false;
+	if (Ext.isArray(me.ifaces) && me.ifaces.length) {
+	    more = true;
+	    var ips = me.getDefaultIps(me.ifaces);
+	    if (ips.length !== 0) {
+		text = ips.join('<br>');
+	    }
+	}
+
+	var ipBox = me.down('#ipBox');
+	ipBox.update(text);
+
+	var moreBtn = me.down('#moreBtn');
+	moreBtn.setVisible(more);
+    },
+
+    initComponent: function() {
+	var me = this;
+
+	if (!me.rstore) {
+	    throw 'rstore not given';
+	}
+
+	if (!me.pveSelNode) {
+	    throw 'pveSelNode not given';
+	}
+
+	var nodename = me.pveSelNode.data.node;
+	var vmid = me.pveSelNode.data.vmid;
+
+	me.ipStore = Ext.create('Proxmox.data.UpdateStore', {
+	    interval: 10000,
+	    storeid: 'lxc-interfaces-' + vmid,
+	    method: 'GET',
+	    proxy: {
+		type: 'proxmox',
+		url: '/api2/json/nodes/' + nodename + '/lxc/' + vmid + '/interfaces',
+	    },
+	});
+
+	me.callParent();
+
+	me.mon(me.ipStore, 'load', function(store, records, success) {
+	    if (records && records.length) {
+		me.ifaces = records;
+	    } else {
+		me.ifaces = undefined;
+	    }
+	    me.updateStatus(!success);
+	});
+
+	me.on('destroy', me.ipStore.stopUpdate, me.ipStore);
+
+	// if we already have info about the guest, use it immediately
+	if (me.rstore.getCount()) {
+	    me.startIPStore(me.rstore, me.rstore.getData(), false);
+	}
+
+	// check if the guest agent is there on every statusstore load
+	me.mon(me.rstore, 'load', me.startIPStore, me);
+    },
+});
diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
index 6401811c73bb..250c7ed117fa 100644
--- a/www/manager6/panel/GuestStatusView.js
+++ b/www/manager6/panel/GuestStatusView.js
@@ -146,7 +146,7 @@ Ext.define('PVE.panel.GuestStatusView', {
 	    height: 15,
 	},
 	{
-	    itemId: 'ips',
+	    itemId: 'agentIPs',
 	    xtype: 'pveAgentIPView',
 	    cbind: {
 		rstore: '{rstore}',
@@ -155,6 +155,16 @@ Ext.define('PVE.panel.GuestStatusView', {
 		disabled: '{isLxc}',
 	    },
 	},
+	{
+	    itemId: 'ctIPS',
+	    xtype: 'pveContainerIPView',
+	    cbind: {
+		rstore: '{rstore}',
+		pveSelNode: '{pveSelNode}',
+		hidden: '{!isLxc}',
+		disabled: '{!isLxc}',
+	    },
+	},
     ],
 
     updateTitle: function() {
diff --git a/www/manager6/panel/GuestSummary.js b/www/manager6/panel/GuestSummary.js
index 1565db3f658d..2186967f62da 100644
--- a/www/manager6/panel/GuestSummary.js
+++ b/www/manager6/panel/GuestSummary.js
@@ -54,7 +54,7 @@ Ext.define('PVE.guest.Summary', {
 	    items = [
 		{
 		    xtype: 'container',
-		    height: 300,
+		    height: 370,
 		    layout: {
 			type: 'hbox',
 			align: 'stretch',
-- 
2.39.5



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

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

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@proxmox.com>
> Signed-off-by: Gabriel Goller <g.goller@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...)

> +			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

> +
> +				records.push(net);
> +			    }
> +			}
> +
> +			me.store.loadData(records);
> +			me.down('button[name=addButton]').setDisabled(records.length >= 32);
> +		    },
> +		});
>   	    },
>   	});
>       },



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Dominik Csapak]

high level comments/questions (i know they're not you're patches exactly, but still):

* maybe it would be better to integrate this into the AgentIPView for vms?
   AFAICS the code is very similar and probably just needs a few adaptions
   to work there too (url,parsing, etc.)

   I'm not opposed to have two components, but then we should at least have
   a good reason in the commit message why this was not done, e.g.
   the data structures are too different, or something like that

* IMHO we should keep the columns consistent between VMs and Containers,
   So either we change the AgentIPView to name/mac/ipv4/ipv6 too
   or we combine the ipv4/ipv6 here

  some comments inline:

On 12/2/24 11:46, Gabriel Goller wrote:
> modelled after the QEMU Guest Agent UI. We only show the first
> non-loopback IP on the summary page itself.
> 
> Originally-by: Leo Nunner <l.nunner@proxmox.com>
>      [GG: increase status panel height]
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> ---
>   www/manager6/Makefile                 |   1 +
>   www/manager6/lxc/ContainerIPView.js   | 194 ++++++++++++++++++++++++++
>   www/manager6/panel/GuestStatusView.js |  12 +-
>   www/manager6/panel/GuestSummary.js    |   2 +-
>   4 files changed, 207 insertions(+), 2 deletions(-)
>   create mode 100644 www/manager6/lxc/ContainerIPView.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index c94a5cdfbf70..203a9d19cefc 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -201,6 +201,7 @@ JSSRC= 							\
>   	lxc/ResourceEdit.js				\
>   	lxc/Resources.js				\
>   	lxc/MultiMPEdit.js				\
> +	lxc/ContainerIPView.js				\
>   	menu/MenuItem.js				\
>   	menu/TemplateMenu.js				\
>   	ceph/CephInstallWizard.js			\
> diff --git a/www/manager6/lxc/ContainerIPView.js b/www/manager6/lxc/ContainerIPView.js
> new file mode 100644
> index 000000000000..69b107af3243
> --- /dev/null
> +++ b/www/manager6/lxc/ContainerIPView.js
> @@ -0,0 +1,194 @@
> +Ext.define('PVE.window.ContainerIPInfo', {
> +    extend: 'Ext.window.Window',
> +    width: 600,
> +    title: gettext('Container Network Information'),
> +    height: 300,
> +    layout: {
> +	type: 'fit',
> +    },
> +    modal: true,
> +    items: [
> +	{
> +	    xtype: 'grid',
> +	    store: {},
> +	    emptyText: gettext('No network information'),
> +	    columns: [
> +		{
> +		    dataIndex: 'name',
> +		    text: gettext('Name'),
> +		    flex: 2,
> +		},
> +		{
> +		    dataIndex: 'hwaddr',
> +		    text: gettext('MAC address'),
> +		    width: 140,
> +		},
> +		{
> +		    dataIndex: 'inet',
> +		    text: gettext('IPv4 address'),
> +		    align: 'right',
> +		    flex: 3,
> +		},
> +		{
> +		    dataIndex: 'inet6',
> +		    text: gettext('IPv6 address'),
> +		    align: 'right',
> +		    flex: 4,
> +		},
> +	    ],
> +	},
> +    ],
> +});
> +
> +Ext.define('PVE.lxc.IPView', {
> +    extend: 'Ext.container.Container',
> +    xtype: 'pveContainerIPView',
> +
> +    layout: {
> +	type: 'hbox',
> +	align: 'top',
> +    },
> +
> +    items: [
> +	{
> +	    xtype: 'box',
> +	    html: '<i class="fa fa-exchange"></i> IPs',
> +	},
> +	{
> +	    xtype: 'container',
> +	    flex: 1,
> +	    layout: {
> +		type: 'vbox',
> +		align: 'right',
> +		pack: 'end',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'label',
> +		    flex: 1,
> +		    itemId: 'ipBox',
> +		    style: {
> +			'text-align': 'right',
> +		    },
> +		},
> +		{
> +		    xtype: 'button',
> +		    itemId: 'moreBtn',
> +		    hidden: true,
> +		    ui: 'default-toolbar',
> +		    handler: function(btn) {
> +			let view = this.up('pveContainerIPView');
> +
> +			var win = Ext.create('PVE.window.ContainerIPInfo');
> +			win.down('grid').getStore().setData(view.ifaces);
> +			win.show();
> +		    },
> +		    text: gettext('More'),
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    getDefaultIps: function(ifaces) {
> +	var me = this;
> +	var ips = [];
> +	ifaces.forEach(function(iface) {
> +	    // We only want to show the first non-loopback interface
> +	    if (!ips.length &&
> +		iface.data.hwaddr &&
> +		iface.data.hwaddr !== '00:00:00:00:00:00' &&
> +		iface.data.hwaddr !== '0:0:0:0:0:0') {
> +		ips.push(iface.data.inet);
> +		ips.push(iface.data.inet6);
> +	    }
> +	});
> +
> +	return ips;
> +    },
> +
> +    startIPStore: function(store, records, success) {
> +	var me = this;
> +	let state = store.getById('status');
> +
> +	me.running = state && state.data.value === 'running';
> +
> +	var caps = Ext.state.Manager.get('GuiCap');
> +
> +	if (!caps.vms['VM.Monitor']) {

the api call for getting the interfaces does not really need this permission?
the api only needs 'vm.audit' for this information, so this check should reflect that

> +	    var errorText = gettext("Requires '{0}' Privileges");
> +	    me.updateStatus(false, Ext.String.format(errorText, 'VM.Monitor'));
> +	    return;
> +	}
> +
> +	if (me.running && me.ipStore.isStopped) {
> +	    me.ipStore.startUpdate();
> +	} else if (me.ipStore.isStopped) {
> +	    me.updateStatus();
> +	}
> +    },
> +
> +    updateStatus: function(unsuccessful, defaulttext) {
> +	var me = this;
> +	var text = defaulttext || gettext('No network information');
> +	var more = false;
> +	if (Ext.isArray(me.ifaces) && me.ifaces.length) {
> +	    more = true;
> +	    var ips = me.getDefaultIps(me.ifaces);
> +	    if (ips.length !== 0) {
> +		text = ips.join('<br>');
> +	    }
> +	}
> +
> +	var ipBox = me.down('#ipBox');
> +	ipBox.update(text);
> +
> +	var moreBtn = me.down('#moreBtn');
> +	moreBtn.setVisible(more);
> +    },
> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	if (!me.rstore) {
> +	    throw 'rstore not given';
> +	}
> +
> +	if (!me.pveSelNode) {
> +	    throw 'pveSelNode not given';
> +	}
> +
> +	var nodename = me.pveSelNode.data.node;
> +	var vmid = me.pveSelNode.data.vmid;
> +
> +	me.ipStore = Ext.create('Proxmox.data.UpdateStore', {
> +	    interval: 10000,
> +	    storeid: 'lxc-interfaces-' + vmid,
> +	    method: 'GET',
> +	    proxy: {
> +		type: 'proxmox',
> +		url: '/api2/json/nodes/' + nodename + '/lxc/' + vmid + '/interfaces',
> +	    },
> +	});
> +
> +	me.callParent();
> +
> +	me.mon(me.ipStore, 'load', function(store, records, success) {
> +	    if (records && records.length) {
> +		me.ifaces = records;
> +	    } else {
> +		me.ifaces = undefined;
> +	    }
> +	    me.updateStatus(!success);
> +	});
> +
> +	me.on('destroy', me.ipStore.stopUpdate, me.ipStore);
> +
> +	// if we already have info about the guest, use it immediately
> +	if (me.rstore.getCount()) {
> +	    me.startIPStore(me.rstore, me.rstore.getData(), false);
> +	}
> +
> +	// check if the guest agent is there on every statusstore load
> +	me.mon(me.rstore, 'load', me.startIPStore, me);
> +    },
> +});
> diff --git a/www/manager6/panel/GuestStatusView.js b/www/manager6/panel/GuestStatusView.js
> index 6401811c73bb..250c7ed117fa 100644
> --- a/www/manager6/panel/GuestStatusView.js
> +++ b/www/manager6/panel/GuestStatusView.js
> @@ -146,7 +146,7 @@ Ext.define('PVE.panel.GuestStatusView', {
>   	    height: 15,
>   	},
>   	{
> -	    itemId: 'ips',
> +	    itemId: 'agentIPs',
>   	    xtype: 'pveAgentIPView',
>   	    cbind: {
>   		rstore: '{rstore}',
> @@ -155,6 +155,16 @@ Ext.define('PVE.panel.GuestStatusView', {
>   		disabled: '{isLxc}',
>   	    },
>   	},
> +	{
> +	    itemId: 'ctIPS',
> +	    xtype: 'pveContainerIPView',
> +	    cbind: {
> +		rstore: '{rstore}',
> +		pveSelNode: '{pveSelNode}',
> +		hidden: '{!isLxc}',
> +		disabled: '{!isLxc}',
> +	    },
> +	},
>       ],
>   
>       updateTitle: function() {
> diff --git a/www/manager6/panel/GuestSummary.js b/www/manager6/panel/GuestSummary.js
> index 1565db3f658d..2186967f62da 100644
> --- a/www/manager6/panel/GuestSummary.js
> +++ b/www/manager6/panel/GuestSummary.js
> @@ -54,7 +54,7 @@ Ext.define('PVE.guest.Summary', {
>   	    items = [
>   		{
>   		    xtype: 'container',
> -		    height: 300,
> +		    height: 370,
>   		    layout: {
>   			type: 'hbox',
>   			align: 'stretch',



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [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@proxmox.com>
>>Signed-off-by: Gabriel Goller <g.goller@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!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

On 12/4/24 10:52, Gabriel Goller wrote:
> 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...
> 

but you should have the info what is configured from the confResponse, no?

so something like


if (net.ip === 'dhcp' || ...) {
net.ip = `${iface.inet} (${net.ip})`;
...
}

?

>> 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@proxmox.com>
>>> Signed-off-by: Gabriel Goller <g.goller@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!



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Gabriel Goller]

On 04.12.2024 11:10, Dominik Csapak wrote:
>On 12/4/24 10:52, Gabriel Goller wrote:
>>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...
>>
>
>but you should have the info what is configured from the confResponse, no?
>
>so something like
>
>
>if (net.ip === 'dhcp' || ...) {
>net.ip = `${iface.inet} (${net.ip})`;
>...
>}
>
>?

Oh, yeah didn't notice this, this is nice.
Though I still have to change the api endpoint to return multiple
ip-addresses so that we also support a static ip with a dhcp-enabled
bridge (this will set two addresses on the interface). Then I can match
the static ip from the config with the set ip and find out which one is
static and which one is dynamic.
Thanks!



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Gabriel Goller]

On 04.12.2024 10:25, Dominik Csapak wrote:
>high level comments/questions (i know they're not you're patches exactly, but still):
>
>* maybe it would be better to integrate this into the AgentIPView for vms?
>  AFAICS the code is very similar and probably just needs a few adaptions
>  to work there too (url,parsing, etc.)
>
>  I'm not opposed to have two components, but then we should at least have
>  a good reason in the commit message why this was not done, e.g.
>  the data structures are too different, or something like that
>
>* IMHO we should keep the columns consistent between VMs and Containers,
>  So either we change the AgentIPView to name/mac/ipv4/ipv6 too
>  or we combine the ipv4/ipv6 here

Hmm I had a quick glance at the AgentIPView and while there is a lot of
the same code, I fear that I will have to do a lot of "if container do
this, otherwise do that".

Although we could make one generic panel for both, removing all the
mentions of "Agent" in the name/title though.

Let me know what you prefer!

>>+    startIPStore: function(store, records, success) {
>>+	var me = this;
>>+	let state = store.getById('status');
>>+
>>+	me.running = state && state.data.value === 'running';
>>+
>>+	var caps = Ext.state.Manager.get('GuiCap');
>>+
>>+	if (!caps.vms['VM.Monitor']) {
>
>the api call for getting the interfaces does not really need this permission?
>the api only needs 'vm.audit' for this information, so this check should reflect that

Ack.

>>+	    var errorText = gettext("Requires '{0}' Privileges");
>>+	    me.updateStatus(false, Ext.String.format(errorText, 'VM.Monitor'));
>>+	    return;
>>+	}


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Gabriel Goller]

On 04.12.2024 10:25, Dominik Csapak wrote:
>high level comments/questions (i know they're not you're patches exactly, but still):
>
>* maybe it would be better to integrate this into the AgentIPView for vms?
>  AFAICS the code is very similar and probably just needs a few adaptions
>  to work there too (url,parsing, etc.)
>
>  I'm not opposed to have two components, but then we should at least have
>  a good reason in the commit message why this was not done, e.g.
>  the data structures are too different, or something like that
>
>* IMHO we should keep the columns consistent between VMs and Containers,
>  So either we change the AgentIPView to name/mac/ipv4/ipv6 too
>  or we combine the ipv4/ipv6 here

Oh, and yes I combined ipv4 and ipv6 here + aligned the popup to be
more similar to the AgentView.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 2/2] lxc: show IPs in summary view

[Dominik Csapak]

On 12/5/24 18:28, Gabriel Goller wrote:
> On 04.12.2024 10:25, Dominik Csapak wrote:
>> high level comments/questions (i know they're not you're patches exactly, but still):
>>
>> * maybe it would be better to integrate this into the AgentIPView for vms?
>>  AFAICS the code is very similar and probably just needs a few adaptions
>>  to work there too (url,parsing, etc.)
>>
>>  I'm not opposed to have two components, but then we should at least have
>>  a good reason in the commit message why this was not done, e.g.
>>  the data structures are too different, or something like that
>>
>> * IMHO we should keep the columns consistent between VMs and Containers,
>>  So either we change the AgentIPView to name/mac/ipv4/ipv6 too
>>  or we combine the ipv4/ipv6 here
> 
> Hmm I had a quick glance at the AgentIPView and while there is a lot of
> the same code, I fear that I will have to do a lot of "if container do
> this, otherwise do that".
> 

wouldn't it also be possible to define some callbacks e.g. one for parsing/assigning,
one for privilege checking etc. (depending on what's exactly different)

> Although we could make one generic panel for both, removing all the
> mentions of "Agent" in the name/title though.
> 
> Let me know what you prefer!

If the amount of if/else or callbacks, etc. is not immensely large,
i'd go for unifying them (and since the widget itself is not much
code to begin with, my guess is that it's just a few differences)

OFC if it turns out that some things must be done fundamentally different
between the two, you can keep it separated.

(Sometimes one has to try some options and look at the result
to see what's the better approach, so it's hard to give
a firm recommendation beforehand)

> 
>>> +    startIPStore: function(store, records, success) {
>>> +    var me = this;
>>> +    let state = store.getById('status');
>>> +
>>> +    me.running = state && state.data.value === 'running';
>>> +
>>> +    var caps = Ext.state.Manager.get('GuiCap');
>>> +
>>> +    if (!caps.vms['VM.Monitor']) {
>>
>> the api call for getting the interfaces does not really need this permission?
>> the api only needs 'vm.audit' for this information, so this check should reflect that
> 
> Ack.
> 
>>> +        var errorText = gettext("Requires '{0}' Privileges");
>>> +        me.updateStatus(false, Ext.String.format(errorText, 'VM.Monitor'));
>>> +        return;
>>> +    }



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

Re: [pve-devel] [PATCH manager 0/2] Show container ip in summary and network tab

[Gabriel Goller]

Submitted a v2!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel