Re: [PATCH manager v2 07/12] ui: ha: node affinity: allow setting affinity for node affinity rules

["Daniel Kral" <d.kral@proxmox.com>]

On Mon Jul 13, 2026 at 2:43 PM CEST, David Riley wrote:
> Thanks for the patch.
> comments inline.
>
> On 6/2/26 12:02 PM, Daniel Kral wrote:

[ snip ]

>> @@ -56,6 +66,14 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>>                   minValue: 0,
>>                   maxValue: 1000,
>>                   isFormField: false,
>> +                bind: {
>> +                    hidden: '{!showNodePriority}',
>> +                    disabled: '{!showNodePriority}',
>
> Currently the priority value is not saved/sent to the backend when
> submitting the form. It's always undefined.
> Could it be that you are missing a binding here?
> value: '{record.priority}',
>
> I think that is because you removed the change listener from the
> 'xtype: 'proxmoxintegerfield' (Priority) in Patch 6.
>

Very good catch, thanks! Yeah, I forgot the change listener to add the
priority field to each record as it is changed.

>> +                },
>> +            },
>> +            bind: {
>> +                hidden: '{!showNodePriority}',
>> +                disabled: '{!showNodePriority}',
>>               },
>>           },
>>       ],
>> @@ -74,6 +92,39 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>>           },
>>       },
>>   
>> +    invertCheckboxSelection: function () {
>> +        let me = this;
>> +
>> +        let sm = me.getSelectionModel();
>> +
>> +        let allNodeModels = new Set(sm.getStore().getData().items ?? []);
>
> nit: Would be better to use .getStore().getRange() [0] here instead of accessing the private
> .items field via the collection [1].
> Since getRange() always returns an array you can also remove nullish coalescing
> operator here as well.
>

Nice, will use it in the next revision!

> [0] https://docs.sencha.com/extjs/7.0.0/modern/Ext.data.Store.html#method-getRange
> [1] https://docs.sencha.com/extjs/7.0.0/modern/Ext.util.Collection.html#property-items
>
>> +        let selectedNodeModels = new Set(sm.getSelection() ?? []);
>
> nit: getSelection() should always return an array so '?? []' should not be needed
> here as well [2].
>
> [2] https://docs.sencha.com/extjs/7.0.0/classic/Ext.selection.Model.html#method-getSelection
>

ACK here as well

>> +
>> +        if (!allNodeModels.size || !selectedNodeModels.size) {
>> +            return;
>> +        }
>> +
>> +        sm.deselectAll();
>> +        sm.select([...allNodeModels.difference(selectedNodeModels)]);
>> +    },
>> +
>> +    applyUseNodePriority: function (newValue) {
>> +        let me = this;
>> +
>> +        let oldValue = me.getViewModel().get('showNodePriority');
>> +
>> +        if (newValue !== oldValue) {
>> +            me.getViewModel().set('showNodePriority', newValue);
>> +
>> +            // Prevent inverting the selection during component initialization
>> +            if (oldValue !== null) {
>
> nit: Using if (oldValue != null) here is would be safer because loose inequality catches
> both null and undefined. This prevents potential initialization failures if the ViewModel's
> default behavior ever changes. Currently you set it to null.
>

Seems good to me as well, thanks!

>> +                me.invertCheckboxSelection();
>> +            }
>> +        }
>> +
>> +        return newValue;
>> +    },
>> +
>>       getSubmitData: function () {
>>           let me = this;
>>           let res = {};
>> @@ -91,7 +142,15 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>>           let sm = me.getSelectionModel();
>>           let selectedNodeModels = sm.getSelection() ?? [];
>>           let nodes = selectedNodeModels
>> -            .map(({ data }) => data.node + (data.priority ? `:${data.priority}` : ''))
>> +            .map(({ data }) => {
>> +                let nodeEntry = data.node;
>> +
>> +                if (me.useNodePriority && data.priority) {
>> +                    nodeEntry += `:${data.priority}`;
>> +                }
>> +
>> +                return nodeEntry;
>> +            })
>>               .join(',');
>>   
>>           return nodes;
>> diff --git a/www/manager6/ha/rules/NodeAffinityRuleEdit.js b/www/manager6/ha/rules/NodeAffinityRuleEdit.js
>> index 77da18b1..aecaa276 100644
>> --- a/www/manager6/ha/rules/NodeAffinityRuleEdit.js
>> +++ b/www/manager6/ha/rules/NodeAffinityRuleEdit.js
>> @@ -1,6 +1,15 @@
>>   Ext.define('PVE.ha.rules.NodeAffinityInputPanel', {
>>       extend: 'PVE.ha.RuleInputPanel',
>>   
>> +    viewModel: {
>> +        data: {
>> +            affinity: 'positive',
>> +        },
>> +        formulas: {
>> +            isPositiveNodeAffinity: (get) => get('affinity') === 'positive',
>> +        },
>> +    },
>> +
>>       initComponent: function () {
>>           let me = this;
>>   
>> @@ -18,6 +27,19 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', {
>>                   uncheckedValue: 0,
>>                   defaultValue: 0,
>>               },
>> +            {
>> +                xtype: 'proxmoxKVComboBox',
>> +                name: 'affinity',
>> +                fieldLabel: gettext('Affinity'),
>> +                allowBlank: false,
>> +                comboItems: [
>> +                    ['positive', gettext('Prefer Nodes')],
>> +                    ['negative', gettext('Avoid Nodes')],
>
> The documentation creates a link between the meaning of 'negative' and 'Avoid Nodes'
> but it might be nice to have this link in the WebUI as well because in the CLI one has
> to know what negative and what positive means.
> Example:
> # ha-manager rules add node-affinity ha-rule-negative \
>          --affinity negative --resources ct:200,vm:300 --nodes node3
>
> but no hard feelings as this is well documented in Patch 12/12.
> It might be sufficient to introduce a tooltip for this.
>
> Especially because in the grid it shows up as 'positive' but opening the edit form switches
> over to 'Prefer Nodes'. It might be worth adding a renderer to the affinity column so the
> grid displays 'Prefer Nodes' / 'Avoid Nodes' to match the form.
>

Right, I thought about it as well before, but didn't go for it in the
end. I'll do it now though and will slip in a preceding patch which does
the same for the resource affinity rules to be consistent.

>> +                ],
>> +                bind: {
>> +                    value: '{affinity}',
>> +                },
>> +            },
>>           ];
>>   
>>           me.columnB = [
>> @@ -25,6 +47,9 @@ Ext.define('PVE.ha.rules.NodeAffinityInputPanel', {
>>                   xtype: 'pveNodePrioritySelector',
>>                   name: 'nodes',
>>                   allowBlank: false,
>> +                bind: {
>> +                    useNodePriority: '{isPositiveNodeAffinity}',
>> +                },
>>               },
>>           ];
>>   
>> diff --git a/www/manager6/ha/rules/NodeAffinityRules.js b/www/manager6/ha/rules/NodeAffinityRules.js
>> index 6fc42799..089dece8 100644
>> --- a/www/manager6/ha/rules/NodeAffinityRules.js
>> +++ b/www/manager6/ha/rules/NodeAffinityRules.js
>> @@ -11,6 +11,11 @@ Ext.define('PVE.ha.NodeAffinityRulesView', {
>>       stateId: 'grid-ha-node-affinity-rules',
>>   
>>       columns: [
>> +        {
>> +            header: gettext('Affinity'),
>> +            width: 75,
>> +            dataIndex: 'affinity',
>> +        },
>>           {
>>               header: gettext('Strict'),
>>               width: 75,