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

[Dominik Csapak <d.csapak@proxmox.com>]

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