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

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

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