[PATCH-SERIES ha-manager 00/16] some sid parsing and LRM speedup improvements

[PATCH-SERIES ha-manager 00/16] some sid parsing and LRM speedup improvements

[Daniel Kral]

It was reported in the enterprise support, that nodes with high counts
of HA resources (250+ HA resources) cause the pve-ha-lrm daemon to
consumes quite a lot of CPU time while being idle. Furthermore,
migration commands for HA resources might take quite a long time to be
actually committed when the max_workers value is not high enough.

This smaller patch series tries to mitigate the former issue first,
which consists of making some performance improvements for the hot LRM
code paths, which consumed the most resources:

- the service config is read and validated very often
- because of that, parse_sid() is called many times as well
- other smaller loops are redundant and are moved out of the hot paths

I started out with making parse_sid() a little lighter, though because
of the latter changes the performance bottleneck of parse_sid() became
rather meaningless. I left the refactoring and changes in though as
having a stricter separation where both VMIDs and SIDs can be used and
where only SIDs can be used is a little nicer now.


The patch series consists of:

PATCH 1-2   make tidy + small cleanup
PATCH 3-9   refactoring and splitting up parse_sid() and its users
PATCH 10-12 prep work to add documentation and rename some methods
PATCH 13    reading the manager status only once per work iter
PATCH 14    reading the service config only once per work iter
PATCH 15-16 factoring out loops which are invariant per work iter


In a test startup of 768 HA resources on a single physical host at the
same time, profiling with Devel::NYTProf 6.14 [0] showed the following:

+---------------------------+---------------+--------------+
|        Subroutine         | Before Change | After Change |
+---------------------------+---------------+--------------+
| check_active_workers      | 327ms         | 998µs        |
| resource_command_finished | 22.7ms        | 50µs         |
| handle_service_exitcode   | 21.1ms        | 12µs         |
+---------------------------+---------------+--------------+

Considering that these are called quite often per LRM cycle, these drops
in execution time per call are significant.



Proposal for LRM worker loop improvements
-----------------------------------------

However, this patch series does only indirectly improve the situation
where HA resources' migration commands (especially if these are
lexicopgraphically at the bottom end) take a lot of time as this is
still pretty much capped by max_workers value even with the nice work by
@Thomas to make the sorting of the worker execution fairer [1].


Currently, the LRM does queue resource commands for the following states
of the HA resources:

    (1) HA resource is in 'started' state
    (2) HA resource is in 'request_stop'/'stopped' state
    (3) HA resource is in 'migrate'/'relocate' state
    (4) HA resource is in 'error' state

For each of those HA resource in such a state, a worker is queued to be
run *once*. However, for the started HA resources (1), these workers are
queued in each HA Manager round. So if there are 768 running HA
resources on a single node, there are 768 workers to check whether the
HA resource is still running.

The datacenter config's $max_workers does limit how many HA resource
workers we can run at a single time. We give these LRM workers 8 seconds
in these $max_workers slots while we wait 1 second each time to
waitpid() them again. This might limit the amount of tasks that can be
processed per LRM cycle severly.


Therefore, I propose to implement a work-stealing scheduling scheme with
a reusable worker thread pool for this, as this would allow the threads
to work on their tasks as soon as they finished the former instead of
exiting and waitpid()'ing on them in the main thread.

Perl has a threads implementation [2] and I'd use the thread-safe
Thread::Queue module [3] in both directions so that the main thread can
queue the resource commands on there and the worker threads can queue
the results back to the main thread.

However, as I don't have much experience with Perl threads and as it's
discouraged because of its heavy weight as noted in its documentation
[2], I'd like some more input on this whether it's a good idea. Though
it seems that the larger part of it being discouraged is because of its
heavy weight, this wouldn't be much of a problem as this proposal uses
reusable worker threads instead of creating new ones repeatedly.

[0] https://metacpan.org/pod/Devel::NYTProf
[1] https://git.proxmox.com/?p=pve-ha-manager.git;a=commitdiff;h=65c1fbac992d8a1f26c401cf49f2d4848bc42080
[2] https://perldoc.perl.org/threads
[3] https://perldoc.perl.org/Thread::Queue


Daniel Kral (16):
  make tidy
  resources: remove commented ipaddr resource type
  tree-wide: use the term vmid instead of name when referring to VMs/CTs
  api: resources: remove unused return values at parse_sid callsites
  make parse_sid always return an array
  config: use early returns in parse_sid
  config: make update_single_resource_config_inplace only allow sids
  introduce separate parse_vmid_or_sid helper subroutine
  drop the unmodified sid return array entry value from parse_sid
  lrm: document and initialize LRM instance properties
  lrm: rename update_lrm_status to flush_lrm_status
  lrm: rename update_service_status to update_lrm_status
  lrm: update the service status once per work iteration
  lrm: read service config once per work iteration
  lrm: compute valid service uids hash set once per work iteration
  lrm: prune non-existent HA resources from results once per work
    iteration

 src/PVE/API2/HA/Resources.pm              | 17 ++---
 src/PVE/CLI/ha_manager.pm                 |  6 +-
 src/PVE/HA/Config.pm                      | 42 +++++------
 src/PVE/HA/Env/PVE2.pm                    |  6 +-
 src/PVE/HA/LRM.pm                         | 85 ++++++++++++++---------
 src/PVE/HA/Manager.pm                     |  6 +-
 src/PVE/HA/Resources.pm                   | 38 ++--------
 src/PVE/HA/Resources/PVECT.pm             |  4 +-
 src/PVE/HA/Resources/PVEVM.pm             |  4 +-
 src/PVE/HA/Sim/Env.pm                     |  4 +-
 src/test/test-cfs-unavailable1/log.expect |  5 --
 src/test/test-group-migrate4/log.expect   | 10 ---
 12 files changed, 104 insertions(+), 123 deletions(-)

-- 
2.47.3

[PATCH ha-manager 01/16] make tidy

[Daniel Kral]

A newer version of proxmox-perltidy doesn't outdent labels anymore.

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

diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 9a4f70c9..5840a76e 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -146,7 +146,7 @@ sub get_active_stationary_movable_resource_bundles {
     my ($ss, $sc, $resource_affinity) = @_;
 
     my $resource_bundles = {};
-OUTER: for my $sid (sort keys %$ss) {
+    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 is not movable
-- 
2.47.3

[PATCH ha-manager 02/16] resources: remove commented ipaddr resource type

[Daniel Kral]

Since 85d36c36 ("remove ipaddr resource type") the ipaddr resource type
has been commented out, which indicates that it is not intended to be
used and should be removed.

Even if it might be used in the future, it would need more rework to fit
in the current HA stack.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Resources.pm | 26 --------------------------
 1 file changed, 26 deletions(-)

diff --git a/src/PVE/HA/Resources.pm b/src/PVE/HA/Resources.pm
index df7c1ff3..e63d78d8 100644
--- a/src/PVE/HA/Resources.pm
+++ b/src/PVE/HA/Resources.pm
@@ -189,30 +189,4 @@ sub get_static_stats_from_config {
     die "implement in subclass";
 }
 
-# package PVE::HA::Resources::IPAddr;
-
-# use strict;
-# use warnings;
-# use PVE::Tools qw($IPV4RE $IPV6RE);
-
-# use base qw(PVE::HA::Resources);
-
-# sub type {
-#     return 'ipaddr';
-# }
-
-# sub verify_name {
-#     my ($class, $name) = @_;
-
-#     die "invalid IP address\n" if $name !~ m!^$IPV6RE|$IPV4RE$!;
-# }
-
-# sub options {
-#     return {
-# 	state => { optional => 1 },
-# 	group => { optional => 1 },
-# 	comment => { optional => 1 },
-#     };
-# }
-
 1;
-- 
2.47.3

[PATCH ha-manager 03/16] tree-wide: use the term vmid instead of name when referring to VMs/CTs

[Daniel Kral]

The variable name $name is a bit generic when referring to the unique
identifier for VMs/CTs, so actually name it after its identifier VMID.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/HA/Resources.pm  | 14 +++++++-------
 src/PVE/CLI/ha_manager.pm     |  4 ++--
 src/PVE/HA/Config.pm          | 20 ++++++++++----------
 src/PVE/HA/Env/PVE2.pm        |  6 +++---
 src/PVE/HA/Resources.pm       | 12 ++++++------
 src/PVE/HA/Resources/PVECT.pm |  4 ++--
 src/PVE/HA/Resources/PVEVM.pm |  4 ++--
 src/PVE/HA/Sim/Env.pm         |  4 ++--
 8 files changed, 34 insertions(+), 34 deletions(-)

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index b96572b9..f786e435 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -198,7 +198,7 @@ __PACKAGE__->register_method({
         PVE::Cluster::check_cfs_quorum();
         mkdir("/etc/pve/ha");
 
-        my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
 
         if (my $param_type = extract_param($param, 'type')) {
             # useless, but do it anyway
@@ -209,9 +209,9 @@ __PACKAGE__->register_method({
             if defined($param->{group}) && PVE::HA::Config::have_groups_been_migrated();
 
         my $plugin = PVE::HA::Resources->lookup($type);
-        $plugin->verify_name($name);
+        $plugin->verify_name($vmid);
 
-        $plugin->exists($name);
+        $plugin->exists($vmid);
 
         my $opts = $plugin->check_config($sid, $param, 1, 1);
 
@@ -253,7 +253,7 @@ __PACKAGE__->register_method({
         my $digest = extract_param($param, 'digest');
         my $delete = extract_param($param, 'delete');
 
-        my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
 
         if (my $param_type = extract_param($param, 'type')) {
             # useless, but do it anyway
@@ -313,7 +313,7 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $purge = extract_param($param, 'purge') // 1;
 
         if (!PVE::HA::Config::service_is_configured($sid)) {
@@ -401,7 +401,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
@@ -503,7 +503,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
diff --git a/src/PVE/CLI/ha_manager.pm b/src/PVE/CLI/ha_manager.pm
index 6625de68..2f89887f 100644
--- a/src/PVE/CLI/ha_manager.pm
+++ b/src/PVE/CLI/ha_manager.pm
@@ -193,8 +193,8 @@ our $cmddef = {
         sub {
             my $res = shift;
             foreach my $rec (sort { $a->{sid} cmp $b->{sid} } @$res) {
-                my ($type, $name) = split(':', $rec->{sid}, 2);
-                print "$type:$name\n";
+                my ($type, $vmid) = split(':', $rec->{sid}, 2);
+                print "$type:$vmid\n";
                 foreach my $k (sort keys %$rec) {
                     next
                         if $k eq 'digest'
diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index bee1ac7d..ec0551e9 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -114,7 +114,7 @@ my sub checked_resources_config {
 
     foreach my $sid (keys %{ $cfg->{ids} }) {
         my $d = $cfg->{ids}->{$sid};
-        my (undef, undef, $name) = parse_sid($sid);
+        my (undef, undef, $vmid) = parse_sid($sid);
         $d->{state} = 'started' if !defined($d->{state});
         $d->{state} = 'started' if $d->{state} eq 'enabled'; # backward compatibility
         $d->{failback} = 1 if !defined($d->{failback});
@@ -123,7 +123,7 @@ my sub checked_resources_config {
         $d->{max_relocate} = 1 if !defined($d->{max_relocate});
 
         if (PVE::HA::Resources->lookup($d->{type})) {
-            if (my $vmd = $vmlist->{ids}->{$name}) {
+            if (my $vmd = $vmlist->{ids}->{$vmid}) {
                 $d->{node} = $vmd->{node};
                 $resources->{$sid} = $d;
             } else {
@@ -148,7 +148,7 @@ sub read_and_check_resources_config {
 my sub update_single_resource_config_inplace {
     my ($cfg, $sid, $param, $delete) = @_;
 
-    ($sid, my $type, my $name) = parse_sid($sid);
+    ($sid, my $type, my $vmid) = parse_sid($sid);
 
     my $scfg = $cfg->{ids}->{$sid}
         || die "no such resource '$sid'\n";
@@ -198,27 +198,27 @@ sub update_resources_config {
 sub parse_sid {
     my ($sid) = @_;
 
-    my ($type, $name);
+    my ($type, $vmid);
 
     if ($sid =~ m/^(\d+)$/) {
-        $name = $1;
+        $vmid = $1;
 
         my $vmlist = PVE::Cluster::get_vmlist();
-        if (defined($vmlist->{ids}->{$name})) {
-            my $vm_type = $vmlist->{ids}->{$name}->{type};
+        if (defined($vmlist->{ids}->{$vmid})) {
+            my $vm_type = $vmlist->{ids}->{$vmid}->{type};
             $type = PVE::HA::Tools::get_ha_resource_type($vm_type);
-            $sid = "$type:$name";
+            $sid = "$type:$vmid";
         } else {
             die "unable to detect SID from VMID - VM/CT $1 does not exist\n";
         }
     } elsif ($sid =~ m/^(\S+):(\S+)$/) {
-        $name = $2;
+        $vmid = $2;
         $type = $1;
     } else {
         die "unable to parse service id '$sid'\n";
     }
 
-    return wantarray ? ($sid, $type, $name) : $sid;
+    return wantarray ? ($sid, $type, $vmid) : $sid;
 }
 
 sub read_rules_config {
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index 782d19db..b91a4933 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -208,11 +208,11 @@ sub exec_fence_agent {
 sub steal_service {
     my ($self, $sid, $current_node, $new_node) = @_;
 
-    my (undef, $type, $name) = PVE::HA::Config::parse_sid($sid);
+    my (undef, $type, $vmid) = PVE::HA::Config::parse_sid($sid);
 
     if (my $plugin = PVE::HA::Resources->lookup($type)) {
-        my $old = $plugin->config_file($name, $current_node);
-        my $new = $plugin->config_file($name, $new_node);
+        my $old = $plugin->config_file($vmid, $current_node);
+        my $new = $plugin->config_file($vmid, $new_node);
         rename($old, $new)
             || die "rename '$old' to '$new' failed - $!\n";
     } else {
diff --git a/src/PVE/HA/Resources.pm b/src/PVE/HA/Resources.pm
index e63d78d8..5a95dfdc 100644
--- a/src/PVE/HA/Resources.pm
+++ b/src/PVE/HA/Resources.pm
@@ -104,7 +104,7 @@ EODESC
 };
 
 sub verify_name {
-    my ($class, $name) = @_;
+    my ($class, $vmid) = @_;
 
     die "implement this in subclass";
 }
@@ -116,27 +116,27 @@ sub private {
 sub format_section_header {
     my ($class, $type, $sectionId) = @_;
 
-    my (undef, $name) = split(':', $sectionId, 2);
+    my (undef, $vmid) = split(':', $sectionId, 2);
 
-    return "$type: $name\n";
+    return "$type: $vmid\n";
 }
 
 sub parse_section_header {
     my ($class, $line) = @_;
 
     if ($line =~ m/^(\S+):\s*(\S+)\s*$/) {
-        my ($type, $name) = (lc($1), $2);
+        my ($type, $vmid) = (lc($1), $2);
         my $errmsg = undef; # set if you want to skip whole section
         eval {
             if (my $plugin = $defaultData->{plugins}->{$type}) {
-                $plugin->verify_name($name);
+                $plugin->verify_name($vmid);
             } else {
                 die "no such resource type '$type'\n";
             }
         };
         $errmsg = $@ if $@;
         my $config = {}; # to return additional attributes
-        return ($type, "$type:$name", $errmsg, $config);
+        return ($type, "$type:$vmid", $errmsg, $config);
     }
     return undef;
 }
diff --git a/src/PVE/HA/Resources/PVECT.pm b/src/PVE/HA/Resources/PVECT.pm
index 177b9070..5121bbe1 100644
--- a/src/PVE/HA/Resources/PVECT.pm
+++ b/src/PVE/HA/Resources/PVECT.pm
@@ -27,9 +27,9 @@ sub type {
 }
 
 sub verify_name {
-    my ($class, $name) = @_;
+    my ($class, $vmid) = @_;
 
-    die "invalid VMID\n" if $name !~ m/^[1-9][0-9]+$/;
+    die "invalid VMID\n" if $vmid !~ m/^[1-9][0-9]+$/;
 }
 
 sub options {
diff --git a/src/PVE/HA/Resources/PVEVM.pm b/src/PVE/HA/Resources/PVEVM.pm
index 87532716..d6595da4 100644
--- a/src/PVE/HA/Resources/PVEVM.pm
+++ b/src/PVE/HA/Resources/PVEVM.pm
@@ -27,9 +27,9 @@ sub type {
 }
 
 sub verify_name {
-    my ($class, $name) = @_;
+    my ($class, $vmid) = @_;
 
-    die "invalid VMID\n" if $name !~ m/^[1-9][0-9]+$/;
+    die "invalid VMID\n" if $vmid !~ m/^[1-9][0-9]+$/;
 }
 
 sub options {
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index 38ac1d03..d3c4a11d 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -231,10 +231,10 @@ sub parse_sid {
     die "unable to parse service id '$sid'\n"
         if !($sid =~ m/^(\S+):(\S+)$/);
 
-    my $name = $2;
+    my $vmid = $2;
     my $type = $1;
 
-    return wantarray ? ($sid, $type, $name) : $sid;
+    return wantarray ? ($sid, $type, $vmid) : $sid;
 }
 
 sub read_fence_config {
-- 
2.47.3

[PATCH ha-manager 04/16] api: resources: remove unused return values at parse_sid callsites

[Daniel Kral]

These seem to be copy-pasted, but not all of the code blocks use all of
the return values. Remove the unused return values so it's easier to see
which return values are actually needed by the specific call sites.

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

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index f786e435..5c2b4551 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -253,7 +253,7 @@ __PACKAGE__->register_method({
         my $digest = extract_param($param, 'digest');
         my $delete = extract_param($param, 'delete');
 
-        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
 
         if (my $param_type = extract_param($param, 'type')) {
             # useless, but do it anyway
@@ -313,7 +313,7 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $purge = extract_param($param, 'purge') // 1;
 
         if (!PVE::HA::Config::service_is_configured($sid)) {
@@ -401,7 +401,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
@@ -503,7 +503,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
-- 
2.47.3

[PATCH ha-manager 05/16] make parse_sid always return an array

[Daniel Kral]

parse_sid() is only called within the pve-ha-manager package and there
are only two callers, which only need the $sid return value.

In preparation of the changes in the following patches and reduce the
clutter introduced by a wantarray, always return an array and adapt its
callers as $sid is already the first value in the returned array.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/HA/Resources.pm | 2 +-
 src/PVE/CLI/ha_manager.pm    | 2 +-
 src/PVE/HA/Config.pm         | 2 +-
 src/PVE/HA/Sim/Env.pm        | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index 5c2b4551..a1fa8856 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -174,7 +174,7 @@ __PACKAGE__->register_method({
         my $cfg = PVE::HA::Config::read_resources_config();
         my $exclude_group_property = PVE::HA::Config::have_groups_been_migrated();
 
-        my $sid = PVE::HA::Config::parse_sid($param->{sid});
+        my ($sid) = PVE::HA::Config::parse_sid($param->{sid});
 
         return &$api_copy_config($cfg, $sid, $exclude_group_property);
     },
diff --git a/src/PVE/CLI/ha_manager.pm b/src/PVE/CLI/ha_manager.pm
index 2f89887f..2bec33cb 100644
--- a/src/PVE/CLI/ha_manager.pm
+++ b/src/PVE/CLI/ha_manager.pm
@@ -104,7 +104,7 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my $sid = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
 
         PVE::HA::Config::service_is_ha_managed($sid);
 
diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index ec0551e9..86574dc2 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -218,7 +218,7 @@ sub parse_sid {
         die "unable to parse service id '$sid'\n";
     }
 
-    return wantarray ? ($sid, $type, $vmid) : $sid;
+    return ($sid, $type, $vmid);
 }
 
 sub read_rules_config {
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index d3c4a11d..311eaad1 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -234,7 +234,7 @@ sub parse_sid {
     my $vmid = $2;
     my $type = $1;
 
-    return wantarray ? ($sid, $type, $vmid) : $sid;
+    return ($sid, $type, $vmid);
 }
 
 sub read_fence_config {
-- 
2.47.3

[PATCH ha-manager 06/16] config: use early returns in parse_sid

[Daniel Kral]

This is in preparation of an upcoming patch, which moves the conditional
vmid parsing and info-fetching branch to its separate helper subroutine.

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

diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 86574dc2..7b49534c 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -198,27 +198,21 @@ sub update_resources_config {
 sub parse_sid {
     my ($sid) = @_;
 
-    my ($type, $vmid);
-
-    if ($sid =~ m/^(\d+)$/) {
-        $vmid = $1;
-
+    if (my ($vmid) = $sid =~ m/^(\d+)$/) {
         my $vmlist = PVE::Cluster::get_vmlist();
         if (defined($vmlist->{ids}->{$vmid})) {
             my $vm_type = $vmlist->{ids}->{$vmid}->{type};
-            $type = PVE::HA::Tools::get_ha_resource_type($vm_type);
+            my $type = PVE::HA::Tools::get_ha_resource_type($vm_type);
             $sid = "$type:$vmid";
+            return ($sid, $type, $vmid);
         } else {
             die "unable to detect SID from VMID - VM/CT $1 does not exist\n";
         }
-    } elsif ($sid =~ m/^(\S+):(\S+)$/) {
-        $vmid = $2;
-        $type = $1;
+    } elsif (my ($type, $vmid) = $sid =~ m/^(\S+):(\S+)$/) {
+        return ($sid, $type, $vmid);
     } else {
         die "unable to parse service id '$sid'\n";
     }
-
-    return ($sid, $type, $vmid);
 }
 
 sub read_rules_config {
-- 
2.47.3

[PATCH ha-manager 07/16] config: make update_single_resource_config_inplace only allow sids

[Daniel Kral]

update_single_resource_config_inplace() has used the $sid return value
from parse_sid(), though all callers already provide the SID format and
do not provide the external VMID at any time.

The subroutine is mainly called internally to persist the HA resource
config state, e.g. when stopping an HA resource or migrating the HA
groups, and is only called externally in
PVE::API2::HA::Resources->update(), where the SID of a provided VMID is
already determined in advance.

Therefore, using the $sid return value is not needed here.

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

diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 7b49534c..77c9737e 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -148,7 +148,7 @@ sub read_and_check_resources_config {
 my sub update_single_resource_config_inplace {
     my ($cfg, $sid, $param, $delete) = @_;
 
-    ($sid, my $type, my $vmid) = parse_sid($sid);
+    (undef, my $type, my $vmid) = parse_sid($sid);
 
     my $scfg = $cfg->{ids}->{$sid}
         || die "no such resource '$sid'\n";
-- 
2.47.3

[PATCH ha-manager 08/16] introduce separate parse_vmid_or_sid helper subroutine

[Daniel Kral]

The separate parse_vmid_or_sid() subroutine helps in stricter checking
that vmid references are only allowed in the API/CLI endpoints, while
the HA stack should always use the proper HA resource id ('type:vmid')
format internally.

This makes the parse_sid() subroutine slightly faster (by about 10%) as
it doesn't unnecessarily need to go through the vmid pattern matching.
The use of qr// was considered but increased the runtime again.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/HA/Resources.pm | 13 +++++++------
 src/PVE/CLI/ha_manager.pm    |  2 +-
 src/PVE/HA/Config.pm         | 14 +++++++++++---
 3 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index a1fa8856..7f910818 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -174,7 +174,7 @@ __PACKAGE__->register_method({
         my $cfg = PVE::HA::Config::read_resources_config();
         my $exclude_group_property = PVE::HA::Config::have_groups_been_migrated();
 
-        my ($sid) = PVE::HA::Config::parse_sid($param->{sid});
+        my ($sid) = PVE::HA::Config::parse_vmid_or_sid($param->{sid});
 
         return &$api_copy_config($cfg, $sid, $exclude_group_property);
     },
@@ -198,7 +198,8 @@ __PACKAGE__->register_method({
         PVE::Cluster::check_cfs_quorum();
         mkdir("/etc/pve/ha");
 
-        my ($sid, $type, $vmid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type, $vmid) =
+            PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
 
         if (my $param_type = extract_param($param, 'type')) {
             # useless, but do it anyway
@@ -253,7 +254,7 @@ __PACKAGE__->register_method({
         my $digest = extract_param($param, 'digest');
         my $delete = extract_param($param, 'delete');
 
-        my ($sid, $type) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid, $type) = PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
 
         if (my $param_type = extract_param($param, 'type')) {
             # useless, but do it anyway
@@ -313,7 +314,7 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
         my $purge = extract_param($param, 'purge') // 1;
 
         if (!PVE::HA::Config::service_is_configured($sid)) {
@@ -401,7 +402,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
@@ -503,7 +504,7 @@ __PACKAGE__->register_method({
 
         my $result = {};
 
-        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
         my $req_node = extract_param($param, 'node');
 
         PVE::HA::Config::service_is_ha_managed($sid);
diff --git a/src/PVE/CLI/ha_manager.pm b/src/PVE/CLI/ha_manager.pm
index 2bec33cb..013742e0 100644
--- a/src/PVE/CLI/ha_manager.pm
+++ b/src/PVE/CLI/ha_manager.pm
@@ -104,7 +104,7 @@ __PACKAGE__->register_method({
     code => sub {
         my ($param) = @_;
 
-        my ($sid) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my ($sid) = PVE::HA::Config::parse_vmid_or_sid(extract_param($param, 'sid'));
 
         PVE::HA::Config::service_is_ha_managed($sid);
 
diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 77c9737e..db86de82 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -198,6 +198,16 @@ sub update_resources_config {
 sub parse_sid {
     my ($sid) = @_;
 
+    if (my ($type, $vmid) = $sid =~ m/^(\S+):(\S+)$/) {
+        return ($sid, $type, $vmid);
+    } else {
+        die "unable to parse service id '$sid'\n";
+    }
+}
+
+sub parse_vmid_or_sid {
+    my ($sid) = @_;
+
     if (my ($vmid) = $sid =~ m/^(\d+)$/) {
         my $vmlist = PVE::Cluster::get_vmlist();
         if (defined($vmlist->{ids}->{$vmid})) {
@@ -208,10 +218,8 @@ sub parse_sid {
         } else {
             die "unable to detect SID from VMID - VM/CT $1 does not exist\n";
         }
-    } elsif (my ($type, $vmid) = $sid =~ m/^(\S+):(\S+)$/) {
-        return ($sid, $type, $vmid);
     } else {
-        die "unable to parse service id '$sid'\n";
+        return parse_sid($sid);
     }
 }
 
-- 
2.47.3

[PATCH ha-manager 09/16] drop the unmodified sid return array entry value from parse_sid

[Daniel Kral]

As all callers of parse_sid() do not use the now unmodified $sid return
value, remove it from the return array. It is only prepended in the case
where parse_vmid_or_sid() is given a correct SID instead of a VMID.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Config.pm   | 8 ++++----
 src/PVE/HA/Env/PVE2.pm | 2 +-
 src/PVE/HA/LRM.pm      | 2 +-
 src/PVE/HA/Manager.pm  | 4 ++--
 src/PVE/HA/Sim/Env.pm  | 2 +-
 5 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index db86de82..9e96a13a 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -114,7 +114,7 @@ my sub checked_resources_config {
 
     foreach my $sid (keys %{ $cfg->{ids} }) {
         my $d = $cfg->{ids}->{$sid};
-        my (undef, undef, $vmid) = parse_sid($sid);
+        my (undef, $vmid) = parse_sid($sid);
         $d->{state} = 'started' if !defined($d->{state});
         $d->{state} = 'started' if $d->{state} eq 'enabled'; # backward compatibility
         $d->{failback} = 1 if !defined($d->{failback});
@@ -148,7 +148,7 @@ sub read_and_check_resources_config {
 my sub update_single_resource_config_inplace {
     my ($cfg, $sid, $param, $delete) = @_;
 
-    (undef, my $type, my $vmid) = parse_sid($sid);
+    my ($type, $vmid) = parse_sid($sid);
 
     my $scfg = $cfg->{ids}->{$sid}
         || die "no such resource '$sid'\n";
@@ -199,7 +199,7 @@ sub parse_sid {
     my ($sid) = @_;
 
     if (my ($type, $vmid) = $sid =~ m/^(\S+):(\S+)$/) {
-        return ($sid, $type, $vmid);
+        return ($type, $vmid);
     } else {
         die "unable to parse service id '$sid'\n";
     }
@@ -219,7 +219,7 @@ sub parse_vmid_or_sid {
             die "unable to detect SID from VMID - VM/CT $1 does not exist\n";
         }
     } else {
-        return parse_sid($sid);
+        return ($sid, parse_sid($sid));
     }
 }
 
diff --git a/src/PVE/HA/Env/PVE2.pm b/src/PVE/HA/Env/PVE2.pm
index b91a4933..7d5b5498 100644
--- a/src/PVE/HA/Env/PVE2.pm
+++ b/src/PVE/HA/Env/PVE2.pm
@@ -208,7 +208,7 @@ sub exec_fence_agent {
 sub steal_service {
     my ($self, $sid, $current_node, $new_node) = @_;
 
-    my (undef, $type, $vmid) = PVE::HA::Config::parse_sid($sid);
+    my ($type, $vmid) = PVE::HA::Config::parse_sid($sid);
 
     if (my $plugin = PVE::HA::Resources->lookup($type)) {
         my $old = $plugin->config_file($vmid, $current_node);
diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index 72e37e69..afe7d6c9 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -947,7 +947,7 @@ sub exec_resource_agent {
 
     my $nodename = $haenv->nodename();
 
-    my (undef, $service_type, $service_name) = $haenv->parse_sid($sid);
+    my ($service_type, $service_name) = $haenv->parse_sid($sid);
 
     my $plugin = PVE::HA::Resources->lookup($service_type);
     if (!$plugin) {
diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
index 5840a76e..12976d33 100644
--- a/src/PVE/HA/Manager.pm
+++ b/src/PVE/HA/Manager.pm
@@ -283,7 +283,7 @@ sub load_balance {
 
     my ($sid, $source, $target) = $migration->@{qw(sid source-node target-node)};
 
-    my (undef, $type, $id) = $haenv->parse_sid($sid);
+    my ($type, $id) = $haenv->parse_sid($sid);
     my $task = $type eq 'vm' ? "migrate" : "relocate";
     my $cmd = "$task $sid $target";
 
@@ -555,7 +555,7 @@ my $fence_recovery_cleanup = sub {
 
     my $haenv = $self->{haenv};
 
-    my (undef, $type, $id) = $haenv->parse_sid($sid);
+    my ($type, $id) = $haenv->parse_sid($sid);
     my $plugin = PVE::HA::Resources->lookup($type);
 
     # should not happen
diff --git a/src/PVE/HA/Sim/Env.pm b/src/PVE/HA/Sim/Env.pm
index 311eaad1..72f494b8 100644
--- a/src/PVE/HA/Sim/Env.pm
+++ b/src/PVE/HA/Sim/Env.pm
@@ -234,7 +234,7 @@ sub parse_sid {
     my $vmid = $2;
     my $type = $1;
 
-    return ($sid, $type, $vmid);
+    return ($type, $vmid);
 }
 
 sub read_fence_config {
-- 
2.47.3

[PATCH ha-manager 10/16] lrm: document and initialize LRM instance properties

[Daniel Kral]

The service_status and node_status properties in the LRM blessed object
reference are updated in the update_service_status() method in each
work() iteration.

No functional changes intended.

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index afe7d6c9..f459591f 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -41,6 +41,13 @@ sub new {
         mode => 'active',
         cluster_state_update => 0,
         active_idle_rounds => 0,
+        # service_status from manager_status in current work() iteration
+        # does contain all HA resources, users must filter for the assigned node
+        service_status => {},
+        # node_status from manager_status in current work() iteration
+        # this is currently only used to check whether a fence for the node is requested
+        # see $PVE::HA::NodeStatus::valid_node_states for possible values
+        node_status => 'unknown',
     }, $class;
 
     $self->set_local_status({ state => 'wait_for_agent_lock' });
-- 
2.47.3

[PATCH ha-manager 11/16] lrm: rename update_lrm_status to flush_lrm_status

[Daniel Kral]

Make the method name similar to PVE::HA::Manager::flush_manager_status
as the term "flush" is more telling about the method writing something
out than the term "update" which could be an update in any direction.

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index f459591f..bdf55d6e 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -156,7 +156,7 @@ sub shutdown_request {
 
     $self->{shutdown_request} = $haenv->get_time();
 
-    eval { $self->update_lrm_status() or die "not quorate?\n"; };
+    eval { $self->flush_lrm_status() or die "not quorate?\n"; };
     if (my $err = $@) {
         $haenv->log('err', "unable to update lrm status file - $err");
     }
@@ -187,7 +187,7 @@ sub set_local_status {
     $self->{status} = $new;
 }
 
-sub update_lrm_status {
+sub flush_lrm_status {
     my ($self) = @_;
 
     my $haenv = $self->{haenv};
@@ -366,7 +366,7 @@ sub work {
     my $haenv = $self->{haenv};
 
     if (!$wrote_lrm_status_at_startup) {
-        if ($self->update_lrm_status()) {
+        if ($self->flush_lrm_status()) {
             $wrote_lrm_status_at_startup = 1;
         } else {
             # do nothing
@@ -491,7 +491,7 @@ sub work {
 
         return 0 if $self->{shutdown_request};
 
-        $self->update_lrm_status();
+        $self->flush_lrm_status();
 
         $haenv->sleep(5);
 
@@ -559,7 +559,7 @@ sub work {
             $haenv->log('err', "got unexpected error - $err");
         }
 
-        $self->update_lrm_status();
+        $self->flush_lrm_status();
 
         return 0 if $shutdown;
 
@@ -629,7 +629,7 @@ sub work {
 
         $self->manage_resources() if !$exit_lrm;
 
-        $self->update_lrm_status();
+        $self->flush_lrm_status();
 
         return 0 if $exit_lrm;
 
-- 
2.47.3

[PATCH ha-manager 12/16] lrm: rename update_service_status to update_lrm_status

[Daniel Kral]

update_lrm_status() is more fitting than update_service_status() here as
the internal LRM state that is updated by the information provided by
manager status does affect the whole LRM's behavior (node_status and
shutdown_request) and not only the LRM's behavior for its HA resources
(service_status).

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index bdf55d6e..920fda52 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -210,7 +210,19 @@ sub flush_lrm_status {
     return 1;
 }
 
-sub update_service_status {
+=head3 $self->update_lrm_status()
+
+Updates the internal LRM status properties to reflect the state given by the
+C<L<manager_status|PVE::HA::Env::read_manager_status()>>.
+
+The internal LRM status properties that are updated are C<node_status>,
+C<service_status>, and C<shutdown_request>.
+
+Returns 1 if the update was successful, otherwise returns undef.
+
+=cut
+
+sub update_lrm_status {
     my ($self) = @_;
 
     my $haenv = $self->{haenv};
@@ -224,7 +236,6 @@ sub update_service_status {
         my $nodename = $haenv->nodename();
         $self->{node_status} = $ms->{node_status}->{$nodename} || 'unknown';
 
-        # FIXME: method name is a bit confusing for doing this, either rename or move
         if (!$self->{shutdown_request}) {
             my $request = $ms->{node_request}->{$nodename} // {};
             if ($request->{maintenance}) {
@@ -378,7 +389,7 @@ sub work {
     my $status = $self->get_local_status();
     my $state = $status->{state};
 
-    $self->update_service_status();
+    $self->update_lrm_status();
 
     my $fence_request = $self->is_fence_requested();
 
@@ -509,7 +520,7 @@ sub work {
 
             # if we could not get the current service status there's no point
             # in doing anything, try again next round.
-            return if !$self->update_service_status();
+            return if !$self->update_lrm_status();
 
             if ($self->{shutdown_request}) {
 
@@ -608,7 +619,7 @@ sub work {
     } elsif ($state eq 'maintenance') {
 
         my $startime = $haenv->get_time();
-        return if !$self->update_service_status();
+        return if !$self->update_lrm_status();
 
         # wait until all active services moved away
         my $service_count = $self->active_service_count();
-- 
2.47.3

[PATCH ha-manager 13/16] lrm: update the service status once per work iteration

[Daniel Kral]

Each invocation of update_service_status() does read the current
manager_status and update several internal LRM state properties.

Reduce it to be run only once at the beginning of the work iteration to
have consistent state across the whole work iteration.

The changes in the test cases reflect the change as test cases with
failing pmxcfs read/write do only output one log message as opposed to
two log messages when read_manager_status() is called.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/LRM.pm                         |  6 +++---
 src/test/test-cfs-unavailable1/log.expect |  5 -----
 src/test/test-group-migrate4/log.expect   | 10 ----------
 3 files changed, 3 insertions(+), 18 deletions(-)

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index 920fda52..1a093e8e 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -389,7 +389,7 @@ sub work {
     my $status = $self->get_local_status();
     my $state = $status->{state};
 
-    $self->update_lrm_status();
+    my $status_update_successful = $self->update_lrm_status();
 
     my $fence_request = $self->is_fence_requested();
 
@@ -520,7 +520,7 @@ sub work {
 
             # if we could not get the current service status there's no point
             # in doing anything, try again next round.
-            return if !$self->update_lrm_status();
+            return if !$status_update_successful;
 
             if ($self->{shutdown_request}) {
 
@@ -619,7 +619,7 @@ sub work {
     } elsif ($state eq 'maintenance') {
 
         my $startime = $haenv->get_time();
-        return if !$self->update_lrm_status();
+        return if !$status_update_successful;
 
         # wait until all active services moved away
         my $service_count = $self->active_service_count();
diff --git a/src/test/test-cfs-unavailable1/log.expect b/src/test/test-cfs-unavailable1/log.expect
index a9bb10f2..4d796c50 100644
--- a/src/test/test-cfs-unavailable1/log.expect
+++ b/src/test/test-cfs-unavailable1/log.expect
@@ -36,27 +36,22 @@ info    120      cmdlist: execute service vm:101 stopped
 err     120    node1/crm: could not read manager status: cfs connection refused - not mounted?
 err     120    node1/crm: got unexpected error - cfs connection refused - not mounted?
 err     121    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     121    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     121    node1/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     140    node1/crm: could not read manager status: cfs connection refused - not mounted?
 err     140    node1/crm: got unexpected error - cfs connection refused - not mounted?
 err     141    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     141    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     141    node1/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     160    node1/crm: could not read manager status: cfs connection refused - not mounted?
 err     160    node1/crm: got unexpected error - cfs connection refused - not mounted?
 err     161    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     161    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     161    node1/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     180    node1/crm: could not read manager status: cfs connection refused - not mounted?
 err     180    node1/crm: got unexpected error - cfs connection refused - not mounted?
 err     181    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     181    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     181    node1/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     200    node1/crm: could not read manager status: cfs connection refused - not mounted?
 err     200    node1/crm: got unexpected error - cfs connection refused - not mounted?
 err     201    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     201    node1/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     201    node1/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 info    220      cmdlist: execute cfs node1 rw work
 info    220    node1/crm: service 'vm:101': state changed from 'started' to 'request_stop'
diff --git a/src/test/test-group-migrate4/log.expect b/src/test/test-group-migrate4/log.expect
index 52b79a3c..2bb3b5a5 100644
--- a/src/test/test-group-migrate4/log.expect
+++ b/src/test/test-group-migrate4/log.expect
@@ -96,53 +96,43 @@ info    420      cmdlist: execute cfs node2 rw fail
 err     422    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     422    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     423    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     423    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     423    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     442    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     442    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     443    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     443    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     443    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     462    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     462    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     463    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     463    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     463    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     482    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     482    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     483    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     483    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     483    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     502    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     502    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     503    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     503    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     503    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 info    520      cmdlist: execute pve-manager-version node2 set 9.0.0
 err     522    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     522    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     523    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     523    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     523    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     542    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     542    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     543    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     543    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     543    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     562    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     562    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     563    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     563    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     563    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     582    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     582    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     583    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     583    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     583    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 err     602    node2/crm: could not read manager status: cfs connection refused - not mounted?
 err     602    node2/crm: got unexpected error - cfs connection refused - not mounted?
 err     603    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
-err     603    node2/lrm: updating service status from manager failed: cfs connection refused - not mounted?
 err     603    node2/lrm: unable to write lrm status file - cfs connection refused - not mounted?
 info    620      cmdlist: execute cfs node2 rw work
 noti    702    node2/crm: start ha group migration...
-- 
2.47.3

[PATCH ha-manager 14/16] lrm: read service config once per work iteration

[Daniel Kral]

Make update_lrm_status() read and parse the HA resources config once per
work() iteration.

This significantly reduces the time spent reading and parsing the HA
resource config every time run_workers() is called and every time a LRM
worker's result is collected with handle_service_exitcode().

This does not change the current behavior as the HA resource config and
vmlist, which are used for the return value of the read_service_config()
in the PVE2 environment, are only updated after every cfs_update().
However, cfs_update() is only called before each work() iteration in
PVE::HA::LRM::do_one_iteration() and in PVE::HA::Env::PVE2::after_fork()
and handle_service_exit_code() is only called from
resource_command_finished(), which in turn is only called from the main
thread and not the worker threads (i.e., after_fork() is not called).

A test startup of 768 HA resources on a single node at the same time
showed ~1,250% less calls to checked_resources_config() (from 10,333 to
82 calls) and ~14,320% less calls to parse_sid() (from 15,639,537 to
109,198 calls).

Additionally, this reduces the inclusive average execution time per call
for several subroutines where the HA resource config was read within:

+---------------------------+---------------+--------------+
|        Subroutine         | Before Change | After Change |
+---------------------------+---------------+--------------+
| check_active_workers      | 327ms         | 11.6ms       |
| resource_command_finished | 22.7ms        | 2.81ms       |
| handle_service_exitcode   | 21.1ms        | 10µs         |
+---------------------------+---------------+--------------+

The profiling was done on a physical host with Devel::NYTProf 6.14 [0].

[0] https://metacpan.org/pod/Devel::NYTProf

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index 1a093e8e..5f77580c 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -41,6 +41,9 @@ sub new {
         mode => 'active',
         cluster_state_update => 0,
         active_idle_rounds => 0,
+        # service_config in current work() iteration
+        # does contain all HA resources, users must filter for the assigned node
+        service_config => {},
         # service_status from manager_status in current work() iteration
         # does contain all HA resources, users must filter for the assigned node
         service_status => {},
@@ -213,10 +216,11 @@ sub flush_lrm_status {
 =head3 $self->update_lrm_status()
 
 Updates the internal LRM status properties to reflect the state given by the
-C<L<manager_status|PVE::HA::Env::read_manager_status()>>.
+C<L<manager_status|PVE::HA::Env::read_manager_status()>> and
+C<L<checked service_config|PVE::HA::Env::read_service_config()>>.
 
 The internal LRM status properties that are updated are C<node_status>,
-C<service_status>, and C<shutdown_request>.
+C<service_config>, C<service_status>, and C<shutdown_request>.
 
 Returns 1 if the update was successful, otherwise returns undef.
 
@@ -232,6 +236,7 @@ sub update_lrm_status {
         $haenv->log('err', "updating service status from manager failed: $err");
         return undef;
     } else {
+        $self->{service_config} = $haenv->read_service_config();
         $self->{service_status} = $ms->{service_status} || {};
         my $nodename = $haenv->nodename();
         $self->{node_status} = $ms->{node_status}->{$nodename} || 'unknown';
@@ -658,13 +663,12 @@ sub work {
 sub run_workers {
     my ($self) = @_;
 
-    my $haenv = $self->{haenv};
+    my ($haenv, $sc) = $self->@{qw(haenv service_config)};
 
     my $starttime = $haenv->get_time();
 
     # number of workers to start, if 0 we exec the command directly without forking
     my $max_workers = $haenv->get_max_workers();
-    my $sc = $haenv->read_service_config();
 
     my $worker = $self->{workers};
     # we only got limited time but want to ensure that every queued worker is scheduled
@@ -909,7 +913,7 @@ sub handle_service_exitcode {
     my $haenv = $self->{haenv};
     my $tries = $self->{restart_tries};
 
-    my $sc = $haenv->read_service_config();
+    my $sc = $self->{service_config};
 
     my $max_restart = 0;
 
-- 
2.47.3

[PATCH ha-manager 15/16] lrm: compute valid service uids hash set once per work iteration

[Daniel Kral]

The $self->{service_status} property is read-only and updated once per
work iteration. This makes the recomputation on every resource command
completion in resource_command_finished() unnecessary.

In a test startup of 768 HA resources on a single node at the same time
showed a ~2.5 speedup for resource_command_finished().

+---------------------------+---------------+--------------+
|        Subroutine         | Before Change | After Change |
+---------------------------+---------------+--------------+
| check_active_workers      | 11.6ms        | 4.09ms       |
| resource_command_finished | 2.81ms        | 1.09ms       |
+---------------------------+---------------+--------------+

The profiling was done on a physical host with Devel::NYTProf 6.14 [0].

[0] https://metacpan.org/pod/Devel::NYTProf

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index 5f77580c..911672cb 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -47,6 +47,8 @@ sub new {
         # service_status from manager_status in current work() iteration
         # does contain all HA resources, users must filter for the assigned node
         service_status => {},
+        # UIDs from the service_status in current work() iteration
+        service_status_uids => {},
         # node_status from manager_status in current work() iteration
         # this is currently only used to check whether a fence for the node is requested
         # see $PVE::HA::NodeStatus::valid_node_states for possible values
@@ -220,7 +222,8 @@ C<L<manager_status|PVE::HA::Env::read_manager_status()>> and
 C<L<checked service_config|PVE::HA::Env::read_service_config()>>.
 
 The internal LRM status properties that are updated are C<node_status>,
-C<service_config>, C<service_status>, and C<shutdown_request>.
+C<service_config>, C<service_status>, C<service_status_uids>, and
+C<shutdown_request>.
 
 Returns 1 if the update was successful, otherwise returns undef.
 
@@ -237,7 +240,9 @@ sub update_lrm_status {
         return undef;
     } else {
         $self->{service_config} = $haenv->read_service_config();
-        $self->{service_status} = $ms->{service_status} || {};
+        my $ss = $ms->{service_status} || {};
+        $self->{service_status} = $ss;
+        $self->{service_status_uids} = { map { $ss->{$_}->{uid} => 1 } keys %$ss };
         my $nodename = $haenv->nodename();
         $self->{node_status} = $ms->{node_status}->{$nodename} || 'unknown';
 
@@ -886,19 +891,9 @@ sub resource_command_finished {
         exit_code => $exit_code,
     };
 
-    my $ss = $self->{service_status};
-
-    # compute hash of valid/existing uids
-    my $valid_uids = {};
-    foreach my $sid (keys %$ss) {
-        my $sd = $ss->{$sid};
-        next if !$sd->{uid};
-        $valid_uids->{ $sd->{uid} } = 1;
-    }
-
     my $results = {};
     foreach my $id (keys %{ $self->{results} }) {
-        next if !$valid_uids->{$id};
+        next if !$self->{service_status_uids}->{$id};
         $results->{$id} = $self->{results}->{$id};
     }
     $self->{results} = $results;
-- 
2.47.3

[PATCH ha-manager 16/16] lrm: prune non-existent HA resources from results once per work iteration

[Daniel Kral]

The $self->{service_status_uids} property is updated once per work
iteration. This makes the pruning and re-creation of the
$self->{results} hash on every resource command completion in
resource_command_finished() unnecessary.

In a test startup of 768 HA resources on a single node at the same time
showed a ~21.8 speedup for resource_command_finished().

+---------------------------+---------------+--------------+
|        Subroutine         | Before Change | After Change |
+---------------------------+---------------+--------------+
| check_active_workers      | 4.09ms        | 998µs        |
| resource_command_finished | 1.09ms        | 50µs         |
+---------------------------+---------------+--------------+

The profiling was done on a physical host with Devel::NYTProf 6.14 [0].

[0] https://metacpan.org/pod/Devel::NYTProf

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

diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
index 911672cb..41a925ff 100644
--- a/src/PVE/HA/LRM.pm
+++ b/src/PVE/HA/LRM.pm
@@ -199,6 +199,15 @@ sub flush_lrm_status {
 
     return 0 if !$haenv->quorate();
 
+    # remove results from non-existing HA resources just before writing it out
+    # to prevent persisting any leftover LRM results
+    my $results = {};
+    for my $id (keys $self->{results}->%*) {
+        next if !$self->{service_status_uids}->{$id};
+        $results->{$id} = $self->{results}->{$id};
+    }
+    $self->{results} = $results;
+
     my $lrm_status = {
         state => $self->{status}->{state},
         mode => $self->{mode},
@@ -890,13 +899,6 @@ sub resource_command_finished {
         state => $w->{state},
         exit_code => $exit_code,
     };
-
-    my $results = {};
-    foreach my $id (keys %{ $self->{results} }) {
-        next if !$self->{service_status_uids}->{$id};
-        $results->{$id} = $self->{results}->{$id};
-    }
-    $self->{results} = $results;
 }
 
 # processes the exit code from a finished resource agent, so that the CRM knows
-- 
2.47.3