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 741AB1FF0E1 for ; Mon, 13 Jul 2026 14:44:27 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id CB3F7214C3; Mon, 13 Jul 2026 14:44:15 +0200 (CEST) Message-ID: Date: Mon, 13 Jul 2026 14:43:39 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: David Riley Subject: Re: [PATCH manager v2 06/12] ui: ha: node affinity: move node priority selector into separate component To: Daniel Kral , pve-devel@lists.proxmox.com References: <20260602100226.180071-1-d.kral@proxmox.com> <20260602100226.180071-7-d.kral@proxmox.com> Content-Language: en-US In-Reply-To: <20260602100226.180071-7-d.kral@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1783946604650 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.325 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: WGOIQH6YFIBSPBHHSDMCEJLQUKQ5AQ2Y X-Message-ID-Hash: WGOIQH6YFIBSPBHHSDMCEJLQUKQ5AQ2Y X-MailFrom: d.riley@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: Thanks for sending in this patch. comments inline. On 6/2/26 12:02 PM, Daniel Kral wrote: > PVE.form.NodePrioritySelector is a reduced adaption of > PVE.form.VMSelector, which adds error highlights to the node priority > selector and fetches the node list from the API directly. > > Signed-off-by: Daniel Kral > --- > changes since v1: > - new > > www/manager6/Makefile | 1 + > www/manager6/ha/NodePrioritySelector.js | 169 ++++++++++++++++++ > www/manager6/ha/rules/NodeAffinityRuleEdit.js | 136 +------------- > 3 files changed, 177 insertions(+), 129 deletions(-) > create mode 100644 www/manager6/ha/NodePrioritySelector.js > > diff --git a/www/manager6/Makefile b/www/manager6/Makefile > index d4dd3f35..eb0e9d9c 100644 > --- a/www/manager6/Makefile > +++ b/www/manager6/Makefile > @@ -151,6 +151,7 @@ JSSRC= \ > window/DirMapEdit.js \ > window/GuestImport.js \ > ha/Fencing.js \ > + ha/NodePrioritySelector.js \ > ha/ResourceEdit.js \ > ha/Resources.js \ > ha/RuleEdit.js \ > diff --git a/www/manager6/ha/NodePrioritySelector.js b/www/manager6/ha/NodePrioritySelector.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? > + 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? > + > + listeners: { > + selectionchange: function () { > + // to trigger validity and error checks > + this.checkChange(); > + }, > + }, > + > + getSubmitData: function () { > + let me = this; > + let res = {}; > + res[me.name] = me.getValue(); > + return res; > + }, > + > + getValue: function () { > + let me = this; > + > + if (me.savedValue !== undefined) { > + return me.savedValue; > + } > + > + let sm = me.getSelectionModel(); > + let selectedNodeModels = sm.getSelection() ?? []; > + let nodes = selectedNodeModels > + .map(({ data }) => data.node + (data.priority ? `:${data.priority}` : '')) > + .join(','); > + > + return nodes; > + }, > + > + setValueSelection: function (value) { > + let me = this; > + > + let store = me.getStore(); > + let nodes = value.split(',').map((item) => { > + let [node, priority] = item.split(':'); > + > + let record = store.findRecord('node', node, 0, false, true, true); > + if (record) { > + record.set('priority', priority); > + record.commit(); > + } else { > + let addedRecords = store.add({ node, priority }); > + record = 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 > + > + let sm = 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 return [""], which will lead to nodes.length being 1 and therefore would evaluate to true in this if. So it will call sm.select with [""]. > + sm.select(nodes); > + } else { > + sm.deselectAll(); > + } > + > + me.getErrors(); > + }, > + > + setValue: function (value) { > + let me = this; > + > + let store = me.getStore(); > + if (!store.isLoaded()) { > + me.savedValue = 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 = this; > + > + if (!me.isDisabled() && me.allowBlank === false && me.getValue().length === 0) { > + me.addBodyCls(['x-form-trigger-wrap-default', 'x-form-trigger-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. > + } > + > + me.removeBodyCls(['x-form-trigger-wrap-default', 'x-form-trigger-wrap-invalid']); > + > + return []; > + }, Not sure if I like this approach. I tried to look for better solutions but it seems like there is no extjs native way to handle this. > + > + initComponent: function () { > + let me = this; > + > + me.callParent(); > + me.initField(); > + }, > +}); > diff --git a/www/manager6/ha/rules/NodeAffinityRuleEdit.js b/www/manager6/ha/rules/NodeAffinityRuleEdit.js > index b4b6a13c..77da18b1 100644 > --- a/www/manager6/ha/rules/NodeAffinityRuleEdit.js > +++ b/www/manager6/ha/rules/NodeAffinityRuleEdit.js > @@ -4,133 +4,6 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', { > initComponent: function () { > let me = this; > > - /* TODO Node selector should be factored out in its own component */ > - let update_nodefield, update_node_selection; > - > - let sm = Ext.create('Ext.selection.CheckboxModel', { > - mode: 'SIMPLE', > - listeners: { > - selectionchange: function (model, selected) { > - update_nodefield(selected); > - }, > - }, > - }); > - > - let store = Ext.create('Ext.data.Store', { > - fields: ['node', 'mem', 'cpu', 'priority'], > - data: PVE.data.ResourceStore.getNodes(), // use already cached data to avoid an API call > - proxy: { > - type: 'memory', > - reader: { type: 'json' }, > - }, > - sorters: [ > - { > - property: 'node', > - direction: 'ASC', > - }, > - ], > - }); > - > - var nodegrid = Ext.createWidget('grid', { > - store: store, > - border: true, > - height: 300, > - selModel: sm, > - 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, > - listeners: { > - change: function (numberfield, value, old_value) { > - let record = numberfield.getWidgetRecord(); > - record.set('priority', value); > - update_nodefield(sm.getSelection()); > - record.commit(); > - }, > - }, > - }, > - }, > - ], > - }); > - > - let nodefield = Ext.create('Ext.form.field.Hidden', { > - name: 'nodes', > - value: '', > - listeners: { > - change: function (field, value) { > - update_node_selection(value); > - }, > - }, > - isValid: function () { > - let value = this.getValue(); > - return value && value.length !== 0; > - }, > - }); > - > - update_node_selection = function (string) { > - let nodes = string.split(',').map((item) => { > - let [node, priority] = item.split(':'); > - > - let record = store.findRecord('node', node, 0, false, true, true); > - if (record) { > - record.set('priority', priority); > - record.commit(); > - } else { > - let addedRecords = store.add({ node, priority }); > - record = addedRecords[0]; > - } > - > - return record; > - }); > - > - if (nodes.length) { > - sm.select(nodes); > - } else { > - sm.deselectAll(); > - } > - > - nodegrid.reconfigure(store); > - }; > - > - update_nodefield = function (selected) { > - let nodes = selected > - .map(({ data }) => data.node + (data.priority ? `:${data.priority}` : '')) > - .join(','); > - > - // nodefield change listener calls us again, which results in a > - // endless recursion, suspend the event temporary to avoid this > - nodefield.suspendEvent('change'); > - nodefield.setValue(nodes); > - nodefield.resumeEvent('change'); > - }; > - > me.column2 = [ > { > xtype: 'proxmoxcheckbox', > @@ -145,10 +18,15 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', { > uncheckedValue: 0, > defaultValue: 0, > }, > - nodefield, > ]; > > - me.columnB = [nodegrid]; > + me.columnB = [ > + { > + xtype: 'pveNodePrioritySelector', > + name: 'nodes', > + allowBlank: false, > + }, > + ]; > > me.callParent(); > },