Re: [PATCH pve-ha-manager 3/3] fix #7557: introduce 'auto-rebalance' property

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

Very nice test case additions for the new functionality, thanks for the
series!

Only a small nit about a method name and some make tidy notes, but
otherwise it looks good to me, so with those resolved consider this as:

Reviewed-by: Daniel Kral <d.kral@proxmox.com>

On Mon May 11, 2026 at 5:57 PM CEST, Dominik Rusovac wrote:
> Add 'auto-rebalance' property to HA resources config, which gives users
> control over which HA resources may be moved by dynamic CRS during
> automatic rebalancing.
>
> The 'auto-rebalance' flag is set to true by default. Disabling
> 'auto-rebalance' for some HA resource, say vm:100, means that vm:100
> will be disregarded as a migration candidate during auto-rebalancing.
> Any HA resource with a positive affinity for vm:100 will be disregarded
> too.
>
> Tests validate that an entire resource bundle will be disregarded if any
> resource belonging to the bundle has 'auto-rebalance' disabled.
>
> Signed-off-by: Dominik Rusovac <d.rusovac@proxmox.com>
> ---
>  src/PVE/API2/HA/Resources.pm      |   7 ++
>  src/PVE/API2/HA/Status.pm         |   9 +-
>  src/PVE/HA/Config.pm              |   2 +
>  src/PVE/HA/Manager.pm             |  12 ++-
>  src/PVE/HA/Resources.pm           |   6 ++
>  src/PVE/HA/Resources/PVECT.pm     |   1 +
>  src/PVE/HA/Resources/PVEVM.pm     |   1 +
>  src/PVE/HA/Sim/Hardware.pm        |   1 +
>  src/test/test_resource_bundles.pl | 134 +++++++++++++++++++++++++++++-
>  9 files changed, 166 insertions(+), 7 deletions(-)
>
> diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
> index e0690d5..181cdbb 100644
> --- a/src/PVE/API2/HA/Resources.pm
> +++ b/src/PVE/API2/HA/Resources.pm
> @@ -142,6 +142,13 @@ __PACKAGE__->register_method({
>                  optional => 1,
>                  default => 1,
>              },
> +            'auto-rebalance' => {
> +                description => "HA resource may be migrated during"
> +                    . " automatic rebalancing.",

make tidy wants this to be in a single line, but

    description => "HA resource may be migrated during automatic rebalancing.",

seems to work fine.

> +                type => 'boolean',
> +                optional => 1,
> +                default => 1,
> +            },
>              group => get_standard_option('pve-ha-group-id', { optional => 1 }),
>              max_restart => {
>                  description => "Maximal number of tries to restart the service on"
> diff --git a/src/PVE/API2/HA/Status.pm b/src/PVE/API2/HA/Status.pm
> index 4894f3b..c352aa1 100644
> --- a/src/PVE/API2/HA/Status.pm
> +++ b/src/PVE/API2/HA/Status.pm
> @@ -121,6 +121,13 @@ __PACKAGE__->register_method({
>                      optional => 1,
>                      default => 1,
>                  },
> +                'auto-rebalance' => {
> +                    description => "HA resource may be migrated during"
> +                        . " automatic rebalancing.",

same here

> +                    type => 'boolean',
> +                    optional => 1,
> +                    default => 1,
> +                },
>                  max_relocate => {
>                      description => "For type 'service'.",
>                      type => "integer",
> @@ -333,7 +340,7 @@ __PACKAGE__->register_method({
>              # also return common resource attributes
>              if (defined($sc)) {
>                  $data->{request_state} = $sc->{state};
> -                foreach my $key (qw(group max_restart max_relocate failback comment)) {
> +                foreach my $key (qw(group max_restart max_relocate failback comment auto-rebalance)) {

make tidy wants to move the quoted whitespace array to a new line...
Might look just a little bit nicer with:

    my @exported_service_properties =
        qw(group max_restart max_relocate failback comment auto-rebalance);
    for my $key (@exported_service_properties) {
        $data->{$key} = $sc->{$key} if defined($sc->{$key});
    }

>                      $data->{$key} = $sc->{$key} if defined($sc->{$key});
>                  }
>              }
> diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
> index a34a302..54a6503 100644
> --- a/src/PVE/HA/Config.pm
> +++ b/src/PVE/HA/Config.pm
> @@ -118,8 +118,10 @@ sub read_and_check_resources_config {
>          $d->{state} = 'started' if !defined($d->{state});
>          $d->{state} = 'started' if $d->{state} eq 'enabled'; # backward compatibility
>          $d->{failback} = 1 if !defined($d->{failback});
> +        $d->{'auto-rebalance'} = 1 if !defined($d->{'auto-rebalance'});
>          $d->{max_restart} = 1 if !defined($d->{max_restart});
>          $d->{max_relocate} = 1 if !defined($d->{max_relocate});
> +
>          if (PVE::HA::Resources->lookup($d->{type})) {
>              if (my $vmd = $vmlist->{ids}->{$name}) {
>                  $d->{node} = $vmd->{node};
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 2a4b31e..68e3cd6 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -136,12 +136,14 @@ sub update_crs_scheduler_mode {
>  # HA resource in the resource bundle and also the key of each resource bundle
>  # in the returned hash.
>  sub get_active_stationary_resource_bundles {

nit: Hm, might need a better name now that we have an additional
condition on which resource bundles are gathered, e.g.

    get_active_stationary_movable_resource_bundles

though that's rather long. No hard feelings though, but updating the
description would be nice.

> -    my ($ss, $resource_affinity) = @_;
> +    my ($ss, $sc, $resource_affinity) = @_;
>  
>      my $resource_bundles = {};
>  OUTER: for my $sid (sort keys %$ss) {
>          # do not consider non-started resource as 'active' leading resource
>          next if $ss->{$sid}->{state} ne 'started';
> +        # do not consider resource if it may not be moved
> +        next if !$sc->{$sid}->{'auto-rebalance'};
>  
>          my @resources = ($sid);
>          my $nodes = { $ss->{$sid}->{node} => 1 };
> @@ -156,6 +158,8 @@ OUTER: for my $sid (sort keys %$ss) {
>                  next OUTER if $state eq 'migrate' || $state eq 'relocate';
>                  # do not add non-started resource to active bundle
>                  next if $state ne 'started';
> +                # do not consider stationary bundle if a dependent resource may not be moved
> +                next OUTER if !$sc->{$csid}->{'auto-rebalance'};
>  
>                  $nodes->{$node} = 1;
>  

[ ... ]

> diff --git a/src/test/test_resource_bundles.pl b/src/test/test_resource_bundles.pl
> index d38dc51..7df96a9 100755
> --- a/src/test/test_resource_bundles.pl
> +++ b/src/test/test_resource_bundles.pl
> @@ -224,9 +354,9 @@ my $tests = [
>  plan(tests => scalar($tests->@*));
>  
>  for my $case ($get_active_stationary_resource_bundle_tests->@*) {
> -    my ($ss, $resource_affinity) = $case->@{qw(services resource_affinity)};
> +    my ($ss, $sc, $resource_affinity) = $case->@{qw(services service_config resource_affinity)};
>  
> -    my $result = PVE::HA::Manager::get_active_stationary_resource_bundles($ss, $resource_affinity);
> +    my $result = PVE::HA::Manager::get_active_stationary_resource_bundles($ss, $sc, $resource_affinity);

make tidy wants this to be

    my $result =
        PVE::HA::Manager::get_active_stationary_resource_bundles($ss, $sc, $resource_affinity);


>  
>      is_deeply($result, $case->{resource_bundles}, $case->{description});
>  }