[pve-devel] [PATCH ha-manager 0/4] fix #7133 and improve group migration

[pve-devel] [PATCH ha-manager 0/4] fix #7133 and improve group migration

[Daniel Kral]

This small patch series improves the HA groups to rules migration for HA
setups with a lot of HA resources:

- Patch 1 reduces the amount of unnecessary updates and is enough for
  fixing #7133,
- Patch 2 removes redundant failback flag writes to reduce the resource
  config file size, and patch, and
- Patch 3-4 make the resource config in a single write instead of
  individual writes.

Tested this for a PVE 8.4 to PVE 9.1 3-node cluster upgrade.


Daniel Kral (4):
  fix #7133: manager: group migration: skip update for resources without
    group
  manager: group migration: write only non-default failback values
  config, env: allow bulk updates with update_resources_config
  manager: group migration: bulk update changes to resource config

 src/PVE/API2/HA/Resources.pm |  3 +-
 src/PVE/HA/Config.pm         | 57 +++++++++++++++++++++---------------
 src/PVE/HA/Env.pm            |  4 +--
 src/PVE/HA/Env/PVE2.pm       |  4 +--
 src/PVE/HA/Manager.pm        | 11 +++++--
 src/PVE/HA/Sim/Env.pm        |  4 +--
 src/PVE/HA/Sim/Hardware.pm   | 17 ++++++++---
 7 files changed, 62 insertions(+), 38 deletions(-)

-- 
2.47.3



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

[pve-devel] [PATCH ha-manager 1/4] fix #7133: manager: group migration: skip update for resources without group

[Daniel Kral]

The HA group migration is executed if the HA groups config is still
present in the HA config directory and individually updates every HA
resource, even if none of those are part of a HA group.

The call to update_service_config(...) for the HA resources without
group assignments cause unnecessary updates to the config and can become
costly with higher HA resource counts, which might prevent the CRM to
update its watchdog in time, so skip these updates.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Manager.pm | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index f5843dd4..80b29966 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -580,6 +580,9 @@ my $migrate_group_persistently = sub {
 
         PVE::HA::Groups::migrate_groups_to_resources($groups, $resources);
         for my $sid (keys %$resources) {
+            # prevent unnecessary updates for HA resources that do not change
+            next if !defined($resources->{$sid}->{group});
+
             my $param = { failback => $resources->{$sid}->{failback} };
 
             $haenv->update_service_config($sid, $param, 'group');
-- 
2.47.3



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

[pve-devel] [PATCH ha-manager 2/4] manager: group migration: write only non-default failback values

[Daniel Kral]

The failback flag for HA resources is set by default, so it is wasteful
to write the default value in the resources config.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Manager.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 80b29966..c9d1dde5 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -583,7 +583,8 @@ my $migrate_group_persistently = sub {
             # prevent unnecessary updates for HA resources that do not change
             next if !defined($resources->{$sid}->{group});
 
-            my $param = { failback => $resources->{$sid}->{failback} };
+            my $param = {};
+            $param->{failback} = 0 if !$resources->{$sid}->{failback};
 
             $haenv->update_service_config($sid, $param, 'group');
         }
-- 
2.47.3



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

[pve-devel] [PATCH ha-manager 3/4] config, env: allow bulk updates with update_resources_config

[Daniel Kral]

This is used by the next patch to improve the migration of HA groups to
HA node affinity rules.

This is the more straightforward approach instead of duplicating the
logic into another new virtual subroutine and can be reverted back to
the original form if the bulk capability is not needed after the HA
group migration code is removed in PVE 10.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
I left the delete parameter in the signature instead of putting it into
the $changes items too to remain closer to the original method
signature, but that could be moved there too if it's cleaner.

 src/PVE/API2/HA/Resources.pm |  3 +-
 src/PVE/HA/Config.pm         | 57 +++++++++++++++++++++---------------
 src/PVE/HA/Env.pm            |  4 +--
 src/PVE/HA/Env/PVE2.pm       |  4 +--
 src/PVE/HA/Manager.pm        | 10 ++++---
 src/PVE/HA/Sim/Env.pm        |  4 +--
 src/PVE/HA/Sim/Hardware.pm   | 17 ++++++++---
 7 files changed, 60 insertions(+), 39 deletions(-)

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index b95c0e1f..581f4c6a 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -266,7 +266,8 @@ __PACKAGE__->register_method({
 
         check_service_state($sid, $param->{state});
 
-        PVE::HA::Config::update_resources_config($sid, $param, $delete, $digest);
+        my $changes = { $sid => $param };
+        PVE::HA::Config::update_resources_config($changes, $delete, $digest);
 
         return undef;
     },
diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 04e039e0..159798a7 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -136,37 +136,46 @@ sub read_and_check_resources_config {
     return wantarray ? ($conf, $res->{digest}) : $conf;
 }
 
+my sub update_single_resource_config_inplace {
+    my ($cfg, $sid, $param, $delete) = @_;
+
+    ($sid, my $type, my $name) = parse_sid($sid);
+
+    my $scfg = $cfg->{ids}->{$sid}
+        || die "no such resource '$sid'\n";
+
+    my $plugin = PVE::HA::Resources->lookup($scfg->{type});
+    my $opts = $plugin->check_config($sid, $param, 0, 1);
+
+    foreach my $k (%$opts) {
+        $scfg->{$k} = $opts->{$k};
+    }
+
+    if ($delete) {
+        my $options = $plugin->private()->{options}->{$type};
+        foreach my $k (PVE::Tools::split_list($delete)) {
+            my $d = $options->{$k}
+                || die "no such option '$k'\n";
+            die "unable to delete required option '$k'\n"
+                if !$d->{optional};
+            die "unable to delete fixed option '$k'\n"
+                if $d->{fixed};
+            delete $scfg->{$k};
+        }
+    }
+}
+
 sub update_resources_config {
-    my ($sid, $param, $delete, $digest) = @_;
+    my ($changes, $delete, $digest) = @_;
 
     lock_ha_domain(
         sub {
             my $cfg = read_resources_config();
-            ($sid, my $type, my $name) = parse_sid($sid);
-
             PVE::SectionConfig::assert_if_modified($cfg, $digest);
 
-            my $scfg = $cfg->{ids}->{$sid}
-                || die "no such resource '$sid'\n";
-
-            my $plugin = PVE::HA::Resources->lookup($scfg->{type});
-            my $opts = $plugin->check_config($sid, $param, 0, 1);
-
-            foreach my $k (%$opts) {
-                $scfg->{$k} = $opts->{$k};
-            }
-
-            if ($delete) {
-                my $options = $plugin->private()->{options}->{$type};
-                foreach my $k (PVE::Tools::split_list($delete)) {
-                    my $d = $options->{$k}
-                        || die "no such option '$k'\n";
-                    die "unable to delete required option '$k'\n"
-                        if !$d->{optional};
-                    die "unable to delete fixed option '$k'\n"
-                        if $d->{fixed};
-                    delete $scfg->{$k};
-                }
+            for my $sid (keys %$changes) {
+                my $param = $changes->{$sid};
+                update_single_resource_config_inplace($cfg, $sid, $param, $delete);
             }
 
             write_resources_config($cfg);
diff --git a/src/PVE/HA/Env.pm b/src/PVE/HA/Env.pm
index 4282d33f..bfe93f12 100644
--- a/src/PVE/HA/Env.pm
+++ b/src/PVE/HA/Env.pm
@@ -95,9 +95,9 @@ sub read_service_config {
 }
 
 sub update_service_config {
-    my ($self, $sid, $param, $delete) = @_;
+    my ($self, $changes, $delete) = @_;
 
-    return $self->{plug}->update_service_config($sid, $param, $delete);
+    return $self->{plug}->update_service_config($changes, $delete);
 }
 
 sub write_service_config {
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index 9d0dd22b..7bec601d 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -140,9 +140,9 @@ sub read_service_config {
 }
 
 sub update_service_config {
-    my ($self, $sid, $param, $delete) = @_;
+    my ($self, $changes, $delete) = @_;
 
-    return PVE::HA::Config::update_resources_config($sid, $param, $delete);
+    return PVE::HA::Config::update_resources_config($changes, $delete);
 }
 
 sub write_service_config {
diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index c9d1dde5..f55d5870 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -583,10 +583,11 @@ my $migrate_group_persistently = sub {
             # prevent unnecessary updates for HA resources that do not change
             next if !defined($resources->{$sid}->{group});
 
-            my $param = {};
-            $param->{failback} = 0 if !$resources->{$sid}->{failback};
+            my $changes = {};
+            $changes->{$sid} = {};
+            $changes->{$sid}->{failback} = 0 if !$resources->{$sid}->{failback};
 
-            $haenv->update_service_config($sid, $param, 'group');
+            $haenv->update_service_config($changes, 'group');
         }
         $haenv->log('notice', "ha groups migration: migration to resources config successful");
 
@@ -1060,7 +1061,8 @@ sub next_state_started {
                     );
                 }
                 &$change_service_state($self, $sid, 'request_stop', timeout => $timeout);
-                $haenv->update_service_config($sid, { 'state' => 'stopped' });
+                my $changes = { $sid => { state => 'stopped' } };
+                $haenv->update_service_config($changes);
             } else {
                 $haenv->log('err', "unknown command '$cmd' for service '$sid'");
             }
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index 1d70026e..0e7e0e5d 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -212,9 +212,9 @@ sub read_service_config {
 }
 
 sub update_service_config {
-    my ($self, $sid, $param, $delete) = @_;
+    my ($self, $changes, $delete) = @_;
 
-    return $self->{hardware}->update_service_config($sid, $param, $delete);
+    return $self->{hardware}->update_service_config($changes, $delete);
 }
 
 sub write_service_config {
diff --git a/src/PVE/HA/Sim/Hardware.pm b/src/PVE/HA/Sim/Hardware.pm
index 0848d18a..b19a23b4 100644
--- a/src/PVE/HA/Sim/Hardware.pm
+++ b/src/PVE/HA/Sim/Hardware.pm
@@ -114,10 +114,8 @@ sub read_service_config {
     return $conf;
 }
 
-sub update_service_config {
-    my ($self, $sid, $param, $delete) = @_;
-
-    my $conf = $self->read_service_config();
+my sub update_single_service_config_inplace {
+    my ($conf, $sid, $param, $delete) = @_;
 
     my $sconf = $conf->{$sid} || die "no such resource '$sid'\n";
 
@@ -130,6 +128,17 @@ sub update_service_config {
             delete $sconf->{$k};
         }
     }
+}
+
+sub update_service_config {
+    my ($self, $changes, $delete) = @_;
+
+    my $conf = $self->read_service_config();
+
+    for my $sid (keys %$changes) {
+        my $param = $changes->{$sid};
+        update_single_service_config_inplace($conf, $sid, $param, $delete);
+    }
 
     $self->write_service_config($conf);
 }
-- 
2.47.3



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

[pve-devel] [PATCH ha-manager 4/4] manager: group migration: bulk update changes to resource config

[Daniel Kral]

The migration process from HA groups to HA rules might require a lot of
small updates to individual HA resource configs. These updates have been
done per-HA resource, which is quite inefficient and can cause the CRM
to fail to update its watchdog in time.

Even though this is a one-off procedure, it is part of the HA Manager
state machine and must be done in a single action to prevent reading
from an partially updated resource config, i.e., no forking or splitting
the task over multiple HA Manager rounds.

Therefore use the bulk capability of update_resources_config(...) to
remove the group fields and migrate the failback flag for all HA
resources in a single read-update-write operation.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Manager.pm | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index f55d5870..bf368efd 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -579,16 +579,15 @@ my $migrate_group_persistently = sub {
         $haenv->log('notice', "ha groups migration: migration to rules config successful");
 
         PVE::HA::Groups::migrate_groups_to_resources($groups, $resources);
+        my $changes = {};
         for my $sid (keys %$resources) {
             # prevent unnecessary updates for HA resources that do not change
             next if !defined($resources->{$sid}->{group});
 
-            my $changes = {};
             $changes->{$sid} = {};
             $changes->{$sid}->{failback} = 0 if !$resources->{$sid}->{failback};
-
-            $haenv->update_service_config($changes, 'group');
         }
+        $haenv->update_service_config($changes, 'group');
         $haenv->log('notice', "ha groups migration: migration to resources config successful");
 
         $haenv->delete_group_config();
-- 
2.47.3



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