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

[David Riley <d.riley@proxmox.com>]

Thanks for the patch.
comments inline.

On 6/2/26 12:02 PM, Daniel Kral wrote:
> Since the node affinity rules can be either 'positive' or 'negative'
> now, allow users to define either affinity type for the node affinity
> rules in the web interface as well.
>
> Since the priority field does not have any semantic value for negative
> node affinity rules, do not use the field for the negative affinity
> type.
>
> As the affinity type is switched from positive to negative and vice
> versa, the node selection is inverted to make the (lossy) conversion
> easier. The column widths are changed to minimize movement while
> switching between the affinity types.
>
> The inversion is not done if the selection is empty, as preferring all
> nodes is rather uselessly verbose and avoiding all nodes is disallowed
> as this rule would not satisfiable.
>
> Signed-off-by: Daniel Kral<d.kral@proxmox.com>
> ---
> changes since v1:
> - new
>
>   www/manager6/ha/NodePrioritySelector.js       | 65 ++++++++++++++++++-
>   www/manager6/ha/rules/NodeAffinityRuleEdit.js | 25 +++++++
>   www/manager6/ha/rules/NodeAffinityRules.js    |  5 ++
>   3 files changed, 92 insertions(+), 3 deletions(-)
>
> diff --git a/www/manager6/ha/NodePrioritySelector.js b/www/manager6/ha/NodePrioritySelector.js
> index ec6ac02a..b58b0ada 100644
> --- a/www/manager6/ha/NodePrioritySelector.js
> +++ b/www/manager6/ha/NodePrioritySelector.js
> @@ -10,6 +10,16 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>       selectAll: false,
>       isFormField: true,
>   
> +    config: {
> +        useNodePriority: null,
> +    },
> +
> +    publishes: ['useNodePriority'],
> +
> +    viewModel: {
> +        showNodePriority: null,
> +    },
> +
>       store: {
>           autoLoad: true,
>           fields: ['node', 'cpu', 'mem', 'priority'],
> @@ -28,7 +38,7 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>       columns: [
>           {
>               header: gettext('Node'),
> -            flex: 1,
> +            width: 150,
>               dataIndex: 'node',
>           },
>           {
> @@ -42,7 +52,7 @@ Ext.define('PVE.forms.NodePrioritySelector', {
>               header: gettext('CPU usage'),
>               renderer: Proxmox.Utils.render_cpu,
>               sortable: true,
> -            width: 150,
> +            flex: 1,
>               dataIndex: 'cpu',
>           },
>           {
> @@ -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.

> +                },
> +            },
> +            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.

[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

> +
> +        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.

> +                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.

> +                ],
> +                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,