Re: [pve-devel] [PATCH pve-manager 2/7] fabrics: add common interface panel

[Friedrich Weber <f.weber@proxmox.com>]

Hi, one minor comment inline:

On 28/03/2025 18:13, Gabriel Goller wrote:
> Implements a shared interface selector panel for openfabric and ospf fabrics.
> This GridPanel combines data from two sources: the node network interfaces
> (/nodes/<node>/network) and the fabrics section configuration, displaying
> a merged view of both.
> 
> It implements the following warning states:
> - When an interface has an IP address configured in /etc/network/interfaces,
>   we display a warning and disable the input field, prompting users to
>   configure addresses only via the fabrics interface
> - When addresses exist in both /etc/network/interfaces and
>   /etc/network/interfaces.d/sdn, we show a warning without disabling the field,
>   allowing users to remove the SDN interface configuration while preserving
>   the underlying one
> 
> Signed-off-by: Gabriel Goller <g.goller@proxmox.com>
> Co-authored-by: Stefan Hanreich <s.hanreich@proxmox.com>
> ---
>  www/manager6/Makefile              |   1 +
>  www/manager6/sdn/fabrics/Common.js | 285 +++++++++++++++++++++++++++++
>  2 files changed, 286 insertions(+)
>  create mode 100644 www/manager6/sdn/fabrics/Common.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index c94a5cdfbf70..7df96f58eb1f 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -303,6 +303,7 @@ JSSRC= 							\
>  	sdn/zones/SimpleEdit.js				\
>  	sdn/zones/VlanEdit.js				\
>  	sdn/zones/VxlanEdit.js				\
> +	sdn/fabrics/Common.js				\
>  	storage/ContentView.js				\
>  	storage/BackupView.js				\
>  	storage/Base.js					\
> diff --git a/www/manager6/sdn/fabrics/Common.js b/www/manager6/sdn/fabrics/Common.js
> new file mode 100644
> index 000000000000..d71127d9c57f
> --- /dev/null
> +++ b/www/manager6/sdn/fabrics/Common.js
> @@ -0,0 +1,285 @@
> +Ext.define('PVE.sdn.Fabric.InterfacePanel', {
> +    extend: 'Ext.grid.Panel',
> +    mixins: ['Ext.form.field.Field'],
> +
> +    network_interfaces: undefined,
> +    parentClass: undefined,
> +
> +    selectionChange: function(_grid, selection) {
> +	let me = this;
> +	me.value = me.getSelection().map((rec) => {
> +	    let submitValue = structuredClone(rec.data);
> +	    delete submitValue.selected;
> +	    delete submitValue.isDisabled;
> +	    delete submitValue.statusIcon;
> +	    delete submitValue.statusTooltip;
> +	    delete submitValue.type;
> +	    return PVE.Parser.printPropertyString(submitValue);
> +	});
> +	me.checkChange();
> +    },
> +
> +    getValue: function() {
> +        let me = this;
> +        return me.value ?? [];
> +    },
> +
> +    setValue: function(value) {
> +        let me = this;
> +
> +        value ??= [];
> +
> +        me.updateSelectedInterfaces(value);
> +
> +        return me.mixins.field.setValue.call(me, value);
> +    },
> +
> +    addInterfaces: function(fabricInterfaces) {
> +	let me = this;
> +	if (me.network_interfaces) {
> +	    let nodeInterfaces = me.network_interfaces
> +		.map((elem) => {
> +		    const obj = {
> +			name: elem.iface,
> +			type: elem.type,
> +			ip: elem.cidr,
> +			ipv6: elem.cidr6,
> +		    };
> +		    return obj;
> +		});
> +
> +	    if (fabricInterfaces) {
> +		// Map existing node interfaces with fabric data
> +		nodeInterfaces = nodeInterfaces.map(i => {
> +		    let elem = fabricInterfaces.find(j => j.name === i.name);
> +		    if (elem) {
> +			if ((elem.ip && i.ip) || (elem.ipv6 && i.ipv6)) {
> +			    i.statusIcon = 'warning fa-warning';
> +			    i.statusTooltip = gettext('Interface already has an address configured in /etc/network/interfaces');
> +			} else if (i.ip || i.ipv6) {
> +			    i.statusIcon = 'warning fa-warning';
> +			    i.statusTooltip = gettext('Configure the ip-address using the fabrics interface');
> +			    i.isDisabled = true;
> +			}
> +		    }
> +		    return Object.assign(i, elem);
> +		});
> +
> +		// Add any fabric interface that doesn't exist in node_interfaces
> +		for (const fabricIface of fabricInterfaces) {
> +		    if (!nodeInterfaces.some(nodeIface => nodeIface.name === fabricIface.name)) {
> +			nodeInterfaces.push({
> +			    name: fabricIface.name,
> +			    statusIcon: 'warning fa-warning',
> +			    statusTooltip: gettext('Interface not found on node'),
> +			    ...fabricIface,
> +			});
> +		    }
> +		}
> +		let store = me.getStore();
> +		store.setData(nodeInterfaces);
> +	    } else {
> +		let store = me.getStore();
> +		store.setData(nodeInterfaces);
> +	    }
> +	} else if (fabricInterfaces) {
> +	    // We could not get the available interfaces of the node, so we display the configured ones only.
> +	    let interfaces = fabricInterfaces.map((elem) => {
> +		const obj = {
> +		    name: elem.name,
> +		    ...elem,
> +		};
> +		return obj;
> +	    });
> +
> +	    let store = me.getStore();
> +	    store.setData(interfaces);
> +	} else {
> +	    console.warn("no fabric_interfaces and cluster_interfaces available!");
> +	}
> +    },
> +
> +    updateSelectedInterfaces: function(values) {
> +	let me = this;
> +	if (values) {
> +	    let recs = [];
> +	    let store = me.getStore();
> +
> +	    for (const i of values) {
> +		let rec = store.getById(i.name);
> +		if (rec) {
> +		    recs.push(rec);
> +		}
> +	    }
> +	    me.suspendEvent('change');
> +	    me.setSelection();
> +	    me.setSelection(recs);
> +	} else {
> +	    me.suspendEvent('change');
> +	    me.setSelection();
> +	}
> +	me.resumeEvent('change');
> +    },
> +
> +    setNetworkInterfaces: function(network_interfaces) {
> +	this.network_interfaces = network_interfaces;
> +    },
> +
> +    getSubmitData: function() {
> +	let records = this.getSelection().map((record) => {
> +	    let submitData = structuredClone(record.data);
> +	    delete submitData.selected;
> +	    delete submitData.isDisabled;
> +	    delete submitData.statusIcon;
> +	    delete submitData.statusTooltip;
> +	    delete submitData.type;
> +
> +	    // Delete any properties that are null or undefined
> +	    Object.keys(submitData).forEach(function(key) {
> +		if (submitData[key] === null || submitData[key] === undefined || submitData[key] === '') {
> +		    delete submitData[key];
> +		}
> +	    });
> +
> +	    return Proxmox.Utils.printPropertyString(submitData);
> +	});
> +	return {
> +	    'interfaces': records,
> +	};
> +    },
> +
> +    controller: {
> +	onValueChange: function(field, value) {
> +	    let me = this;
> +	    let record = field.getWidgetRecord();
> +	    let column = field.getWidgetColumn();
> +	    if (record) {
> +	        record.set(column.dataIndex, value);
> +	        record.commit();
> +
> +	        me.getView().checkChange();
> +	        me.getView().selectionChange();
> +	    }
> +	},
> +
> +	control: {
> +	    'field': {
> +		change: 'onValueChange',
> +	    },
> +	},
> +    },
> +
> +    selModel: {
> +	type: 'checkboxmodel',
> +	mode: 'SIMPLE',
> +    },
> +
> +    listeners: {
> +	selectionchange: function() {
> +	    this.selectionChange(...arguments);
> +	},
> +    },
> +
> +    commonColumns: [
> +	{
> +	    text: gettext('Status'),
> +	    dataIndex: 'status',
> +	    width: 30,
> +	    renderer: function(value, metaData, record) {
> +		let icon = record.data.statusIcon || '';
> +		let tooltip = record.data.statusTooltip || '';
> +
> +		if (tooltip) {
> +		    metaData.tdAttr = 'data-qtip="' + Ext.htmlEncode(tooltip) + '"';
> +		}

Tooltips need to be double-encoded [1], so one htmlEncode might be missing here.

[1] https://git.proxmox.com/?p=pve-manager.git;a=commit;h=f08f08a042cec0124f73199dcda0d8f882e14507

> +
> +		if (icon) {
> +		    return `<i class="fa ${icon}"></i>`;
> +		}
> +
> +		return value || '';
> +	    },
> +
> +	},
> +	{
> +	    text: gettext('Name'),
> +	    dataIndex: 'name',
> +	    flex: 2,
> +	},
> +	{
> +	    text: gettext('Type'),
> +	    dataIndex: 'type',
> +	    flex: 1,
> +	},
> +	{
> +	    text: gettext('IP'),
> +	    xtype: 'widgetcolumn',
> +	    dataIndex: 'ip',
> +	    flex: 1,
> +
> +	    widget: {
> +		xtype: 'proxmoxtextfield',
> +		isFormField: false,
> +		bind: {
> +		    disabled: '{record.isDisabled}',
> +		},
> +	    },
> +	},
> +    ],
> +
> +    additionalColumns: [],
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	Ext.apply(me, {
> +	    store: Ext.create("Ext.data.Store", {
> +		model: "Pve.sdn.Interface",
> +		sorters: {
> +		    property: 'name',
> +		    direction: 'ASC',
> +		},
> +	    }),
> +	    columns: me.commonColumns.concat(me.additionalColumns),
> +	});
> +
> +	me.callParent();
> +
> +	Proxmox.Utils.monStoreErrors(me, me.getStore(), true);
> +	me.initField();
> +    },
> +});
> +
> +
> +Ext.define('Pve.sdn.Fabric', {
> +    extend: 'Ext.data.Model',
> +    idProperty: 'name',
> +    fields: [
> +	'name',
> +	'type',
> +    ],
> +});
> +
> +Ext.define('Pve.sdn.Node', {
> +    extend: 'Ext.data.Model',
> +    idProperty: 'name',
> +    fields: [
> +	'name',
> +	'fabric',
> +	'type',
> +    ],
> +});
> +
> +Ext.define('Pve.sdn.Interface', {
> +    extend: 'Ext.data.Model',
> +    idProperty: 'name',
> +    fields: [
> +	'name',
> +	'ip',
> +	'ipv6',
> +	'passive',
> +	'hello_interval',
> +	'hello_multiplier',
> +	'csnp_interval',
> +    ],
> +});



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