Re: [pve-devel] [PATCH ha-manager 02/18] manager: retranslate rules if nodes are added or removed

["Michael Köppl" <m.koeppl@proxmox.com>]

2 minor suggestions inline

On Thu Aug 21, 2025 at 4:35 PM CEST, Daniel Kral wrote:
> Some rule checks depend on the list of cluster nodes, e.g., to check
> whether a negative resource affinity rule doesn't specify more HA
> resources than cluster nodes.
>
> The HA Manager retranslate rules only in certain conditions to reduce
> unnecessary computations, but lacks a check whether cluster nodes have
> been added or removed, which is different from what users are reported
> through the rules API endpoints and web interface.
>
> Fixes: 6c4c0458 ("rules: add haenv node list to the rules' canonicalization stage")
> Signed-off-by: Daniel Kral <d.kral@proxmox.com>
> ---
>  src/PVE/HA/Manager.pm    |  2 ++
>  src/PVE/HA/NodeStatus.pm | 14 ++++++++++++++
>  2 files changed, 16 insertions(+)
>
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index ba59f642..92a2c05e 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -692,6 +692,7 @@ sub manage {
>      my ($haenv, $ms, $ns, $ss) = ($self->{haenv}, $self->{ms}, $self->{ns}, $self->{ss});
>  
>      my ($node_info) = $haenv->get_node_info();
> +    my $has_changed_nodelist = $ns->check_for_changed_nodelist($node_info);
>      my ($lrm_results, $lrm_modes) = $self->read_lrm_status();
>  
>      $ns->update($node_info, $lrm_modes);
> @@ -745,6 +746,7 @@ sub manage {
>  
>      if (
>          !$self->{rules}
> +        || $has_changed_nodelist
>          || $new_rules->{digest} ne $self->{last_rules_digest}
>          || $self->{groups}->{digest} ne $self->{last_groups_digest}
>          || $services_digest && $services_digest ne $self->{last_services_digest}
> diff --git a/src/PVE/HA/NodeStatus.pm b/src/PVE/HA/NodeStatus.pm
> index 0d04cd53..e2abfd6e 100644
> --- a/src/PVE/HA/NodeStatus.pm
> +++ b/src/PVE/HA/NodeStatus.pm
> @@ -92,6 +92,20 @@ sub list_online_nodes {
>      return $res;
>  }
>  
> +sub check_for_changed_nodelist {
> +    my ($self, $node_info) = @_;
> +
> +    for my $node (sort keys %$node_info) {
> +        return 1 if !$self->{status}->{$node};
> +    }
> +
> +    for my $node (sort keys $self->{status}->%*) {
> +        return 1 if !$node_info->{$node};
> +    }
> +
> +    return 0;
> +}
Is the sort necessary for the loops? It seems to me that the order of keys
doesn't really matter for this.

Also, could it make sense (not necessarily in this series) to move this to
HashTools? It's not used anywhere else now, but I might come in handy
for further extensions of the HA rules implementation, whenever you need
to check if 2 sets match exactly.

> +
>  my $delete_node = sub {
>      my ($self, $node) = @_;
>  



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