Re: [PATCH ha-manager v3 04/21] next state {stopped,started}: factor out helper to handle motion command

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

On Fri Sep 18, 2026 at 6:08 PM CEST, Fiona Ebner wrote:
> Deduplicate the code before extending it for supporting migration
> options. The only intended functional change is an additional log line
> with the motion command and target in case of next_state_stopped().

Would be nice to add the additional log line in a patch before this so
this patch remains free of functional changes then :)

>
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>
> New in v3.
>
>  src/PVE/HA/Manager.pm                         | 55 +++++++++----------
>  .../test-relocate-to-inactive-node/log.expect |  1 +
>  src/test/test-service-stopped3/log.expect     |  1 +
>  3 files changed, 27 insertions(+), 30 deletions(-)
>
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 5840a76..e0c1858 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -1290,6 +1290,29 @@ sub next_state_migrate_relocate {
>      }
>  }
>  
> +my sub next_state_handle_motion_command {

Hm, "next_state_" is only used for the individual state (transition)
functions, while this is a helper, so I'm unsure to use the prefix here.

Would just "handle{,_resource}_motion_command" work for you too?

> +    my ($self, $cmd, $sid, $sd) = @_;
> +
> +    my $haenv = $self->{haenv};
> +    my $ns = $self->{ns};
> +
> +    my $target = shift @{ $sd->{cmd} };

nit: pre-existing but I'd prefer the postfix $sd->{cmd}->@* here

> +    if (!$ns->node_is_online($target)) {
> +        $haenv->log('err', "ignore service '$sid' $cmd request - node '$target' not online");
> +    } elsif ($sd->{node} eq $target) {
> +        $haenv->log(
> +            'info',
> +            "ignore service '$sid' $cmd request - service already on node '$target'",
> +        );
> +    } else {
> +        $haenv->log('info', "$cmd service '$sid' to node '$target'");
> +        &$change_service_state($self, $sid, $cmd, node => $sd->{node}, target => $target);

nit: pre-existing but this could use the postfix ->

> +        return 1;
> +    }
> +
> +    return;
> +}
> +
>  sub next_state_stopped {
>      my ($self, $sid, $cd, $sd, $lrm_res) = @_;
>  
> @@ -1307,17 +1330,7 @@ sub next_state_stopped {
>          my $cmd = shift @{ $sd->{cmd} };
>  
>          if ($cmd eq 'migrate' || $cmd eq 'relocate') {
> -            my $target = shift @{ $sd->{cmd} };
> -            if (!$ns->node_is_online($target)) {
> -                $haenv->log('err',
> -                    "ignore service '$sid' $cmd request - node '$target' not online");
> -            } elsif ($sd->{node} eq $target) {
> -                $haenv->log(
> -                    'info',
> -                    "ignore service '$sid' $cmd request - service already on node '$target'",
> -                );
> -            } else {
> -                &$change_service_state($self, $sid, $cmd, node => $sd->{node}, target => $target);
> +            if (next_state_handle_motion_command($self, $cmd, $sid, $sd)) {

It would be nice to have a comment here that this calls
change_service_state() with $state = $cmd if the assertions don't fail,
so that it stays easy to enumerate the possible transitions from reading
the code in next_state_stopped() alone.

It could be nice to split this into e.g. assert_valid_resource_motion(),
to check whether to ignore the crm command (for nex_state_started,
next_state_stopped, and update_crm_command), and a
handle_resource_motion_command() to actually make the state transition,
but might be pedantic so no hard feelings.

>                  return;
>              }
>          } elsif ($cmd eq 'stop') {
> @@ -1431,25 +1444,7 @@ sub next_state_started {
>              my $cmd = shift @{ $sd->{cmd} };
>  
>              if ($cmd eq 'migrate' || $cmd eq 'relocate') {
> -                my $target = shift @{ $sd->{cmd} };
> -                if (!$ns->node_is_online($target)) {
> -                    $haenv->log(
> -                        'err',
> -                        "ignore service '$sid' $cmd request - node '$target' not online",
> -                    );
> -                } elsif ($sd->{node} eq $target) {
> -                    $haenv->log(
> -                        'info',
> -                        "ignore service '$sid' $cmd request - service already on node '$target'",
> -                    );
> -                } else {
> -                    $haenv->log('info', "$cmd service '$sid' to node '$target'");
> -                    &$change_service_state(
> -                        $self, $sid, $cmd,
> -                        node => $sd->{node},
> -                        target => $target,
> -                    );
> -                }
> +                next_state_handle_motion_command($self, $cmd, $sid, $sd);

Same here as well

>              } elsif ($cmd eq 'stop') {
>                  my $timeout = shift @{ $sd->{cmd} };
>                  if ($timeout == 0) {
> diff --git a/src/test/test-relocate-to-inactive-node/log.expect b/src/test/test-relocate-to-inactive-node/log.expect
> index 266fb48..8c9d892 100644
> --- a/src/test/test-relocate-to-inactive-node/log.expect
> +++ b/src/test/test-relocate-to-inactive-node/log.expect
> @@ -21,6 +21,7 @@ info     25    node3/lrm: status change wait_for_agent_lock => active
>  info     40    node1/crm: service 'vm:103': state changed from 'request_stop' to 'stopped'
>  info    120      cmdlist: execute service vm:103 relocate node2
>  info    120    node1/crm: got crm command: relocate vm:103 node2
> +info    120    node1/crm: relocate service 'vm:103' to node 'node2'
>  info    120    node1/crm: service 'vm:103': state changed from 'stopped' to 'relocate'  (node = node3, target = node2)
>  info    123    node2/lrm: got lock 'ha_agent_node2_lock'
>  info    123    node2/lrm: status change wait_for_agent_lock => active
> diff --git a/src/test/test-service-stopped3/log.expect b/src/test/test-service-stopped3/log.expect
> index e08b54c..d4737a7 100644
> --- a/src/test/test-service-stopped3/log.expect
> +++ b/src/test/test-service-stopped3/log.expect
> @@ -21,6 +21,7 @@ info     25    node3/lrm: status change wait_for_agent_lock => active
>  info     40    node1/crm: service 'fa:1501': state changed from 'request_stop' to 'stopped'
>  info    120      cmdlist: execute service fa:1501 migrate node2
>  info    120    node1/crm: got crm command: migrate fa:1501 node2
> +info    120    node1/crm: migrate service 'fa:1501' to node 'node2'
>  info    120    node1/crm: service 'fa:1501': state changed from 'stopped' to 'migrate'  (node = node3, target = node2)
>  info    123    node2/lrm: got lock 'ha_agent_node2_lock'
>  info    123    node2/lrm: status change wait_for_agent_lock => active