From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 29FC61FF0E9 for ; Tue, 14 Jul 2026 09:57:10 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 3C11621346; Tue, 14 Jul 2026 09:57:09 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Tue, 14 Jul 2026 09:57:02 +0200 Message-Id: From: "Daniel Kral" To: "David Riley" , Subject: Re: [PATCH manager v2 06/12] ui: ha: node affinity: move node priority selector into separate component X-Mailer: aerc 0.21.0-147-g43ac7b685014-dirty References: <20260602100226.180071-1-d.kral@proxmox.com> <20260602100226.180071-7-d.kral@proxmox.com> In-Reply-To: X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784015806491 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.348 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: B5T7RVPSRGUUSVFIJACO6UROZKMKEELW X-Message-ID-Hash: B5T7RVPSRGUUSVFIJACO6UROZKMKEELW X-MailFrom: d.kral@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Mon Jul 13, 2026 at 2:43 PM CEST, David Riley wrote: > Thanks for sending in this patch. > comments inline. > > On 6/2/26 12:02 PM, Daniel Kral wrote: [ snip ] >> diff --git a/www/manager6/ha/NodePrioritySelector.js b/www/manager6/ha/N= odePrioritySelector.js >> new file mode 100644 >> index 00000000..ec6ac02a >> --- /dev/null >> +++ b/www/manager6/ha/NodePrioritySelector.js >> @@ -0,0 +1,169 @@ >> +Ext.define('PVE.forms.NodePrioritySelector', { >> + extend: 'Ext.grid.Panel', >> + alias: 'widget.pveNodePrioritySelector', >> + >> + mixins: { >> + field: 'Ext.form.field.Field', >> + }, >> + >> + allowBlank: true, >> + selectAll: false, > > Does this select all actually do anything here? > I can still select all nodes in the grid? > Good question, to be honest, I aligned this with the existing PVE.form.VMSelector and similar extensions + mixins from Ext.grid.Panel and Ext.form.field.Field... At least for ExtJS 7.0.0 I cannot find anywhere in our codebase nor the ExtJS codebase where this property is some flag. It only is used as a class method in some components. Will remove that in the next revision and might send a patch which removes the same property from the other components as well. Haven't checked though whether this had any functionality in past versions of ExtJS. >> + isFormField: true, >> + >> + store: { >> + autoLoad: true, >> + fields: ['node', 'cpu', 'mem', 'priority'], >> + proxy: { >> + type: 'proxmox', >> + url: '/api2/json/nodes', >> + }, >> + sorters: [ >> + { >> + property: 'node', >> + direction: 'ASC', >> + }, >> + ], >> + }, >> + >> + columns: [ >> + { >> + header: gettext('Node'), >> + flex: 1, >> + dataIndex: 'node', >> + }, >> + { >> + header: gettext('Memory usage') + ' %', >> + renderer: PVE.Utils.render_mem_usage_percent, >> + sortable: true, >> + width: 150, >> + dataIndex: 'mem', >> + }, >> + { >> + header: gettext('CPU usage'), >> + renderer: Proxmox.Utils.render_cpu, >> + sortable: true, >> + width: 150, >> + dataIndex: 'cpu', >> + }, >> + { >> + header: gettext('Priority'), >> + xtype: 'widgetcolumn', >> + dataIndex: 'priority', >> + sortable: true, >> + stopSelection: true, >> + widget: { >> + xtype: 'proxmoxintegerfield', >> + minValue: 0, >> + maxValue: 1000, >> + isFormField: false, >> + }, >> + }, >> + ], >> + >> + selModel: { >> + selType: 'checkboxmodel', >> + mode: 'SIMPLE', >> + }, >> + >> + checkChangeEvents: ['selectionchange', 'change'], > > Is this change event here even used? > You're right, doesn't seem like it... I'll remove it if I don't see any reason to leave it here. >> + >> + listeners: { >> + selectionchange: function () { >> + // to trigger validity and error checks >> + this.checkChange(); >> + }, >> + }, >> + >> + getSubmitData: function () { >> + let me =3D this; >> + let res =3D {}; >> + res[me.name] =3D me.getValue(); >> + return res; >> + }, >> + >> + getValue: function () { >> + let me =3D this; >> + >> + if (me.savedValue !=3D=3D undefined) { >> + return me.savedValue; >> + } >> + >> + let sm =3D me.getSelectionModel(); >> + let selectedNodeModels =3D sm.getSelection() ?? []; >> + let nodes =3D selectedNodeModels >> + .map(({ data }) =3D> data.node + (data.priority ? `:${data.= priority}` : '')) >> + .join(','); >> + >> + return nodes; >> + }, >> + >> + setValueSelection: function (value) { >> + let me =3D this; >> + >> + let store =3D me.getStore(); >> + let nodes =3D value.split(',').map((item) =3D> { >> + let [node, priority] =3D item.split(':'); >> + >> + let record =3D store.findRecord('node', node, 0, false, tru= e, true); >> + if (record) { >> + record.set('priority', priority); >> + record.commit(); >> + } else { >> + let addedRecords =3D store.add({ node, priority }); >> + record =3D addedRecords[0]; >> + } >> + >> + return record; >> + }); > > Since this maps over a potentially long list of nodes and commits each > record individually, wrapping this block in store.beginUpdate() and > store.endUpdate() would prevent it from sending events for each set/add > operation. [0] > > [0] https://docs.sencha.com/extjs/7.0.0/modern/Ext.data.Store.html#method= -beginUpdate > Nice, thanks for that! Though we don't expect the nodelist to become any longer than 48 nodes as of now, it's good to have this in place already, will use it in the next revision! >> + >> + let sm =3D me.getSelectionModel(); >> + if (nodes.length) { > > I'm wondering if there is bug here. > If you call split on an empty string ("") using .split(',') it will retur= n > [""], which will lead to nodes.length being 1 and therefore would > evaluate to true in this if. So it will call sm.select with [""]. > > Yeah, the whole existing code section assumes that the API never outputs an empty string... Though this would kind of break already at the splitting of the node items then. This should not happen. I could slip in a preceding patch, which filter()s it away, but my gut feeling tells me that it shouldn't ignore wrong API output as it would make it harder to spot that there's something wrong there... I'll think about it. >> + sm.select(nodes); >> + } else { >> + sm.deselectAll(); >> + } >> + >> + me.getErrors(); >> + }, >> + >> + setValue: function (value) { >> + let me =3D this; >> + >> + let store =3D me.getStore(); >> + if (!store.isLoaded()) { >> + me.savedValue =3D value; >> + store.on( >> + 'load', >> + function () { >> + me.setValueSelection(value); >> + delete me.savedValue; >> + }, >> + { single: true }, >> + ); >> + } else { >> + me.setValueSelection(value); >> + } >> + >> + return me.mixins.field.setValue.call(me, value); >> + }, >> + >> + getErrors: function (value) { >> + let me =3D this; >> + >> + if (!me.isDisabled() && me.allowBlank =3D=3D=3D false && me.get= Value().length =3D=3D=3D 0) { >> + me.addBodyCls(['x-form-trigger-wrap-default', 'x-form-trigg= er-wrap-invalid']); >> + return [gettext('No nodes selected')]; > > Not sure if this returned text is visible in the UI. I played around with= it and the grid turned > red if no node is selected, but due to the fact that the "Add" button is = disabled anyways this > message will never appear, but it does not hurt either. > Yeah, this was also taken from the VMSelector component... The nodes field is required here and probably will always be for the node priority selector. >> + } >> + >> + me.removeBodyCls(['x-form-trigger-wrap-default', 'x-form-trigge= r-wrap-invalid']); >> + >> + return []; >> + }, > > > Not sure if I like this approach. I tried to look for better solutions bu= t it seems like > there is no extjs native way to handle this. > What approach are you referring to exactly? >> + >> + initComponent: function () { >> + let me =3D this; >> + >> + me.callParent(); >> + me.initField(); >> + }, >> +}); [ snip ]