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

["Dominik Rusovac" <d.rusovac@proxmox.com>]

thx for taking the time 

will send a v2 

On Tue May 12, 2026 at 11:07 AM CEST, Daniel Kral wrote:
> 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.
>

ACK

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

ACK

>> +                    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});
>     }
>

ACK

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

will update the description

>> -    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);
>
>

ACK

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