[pve-devel] [PATCH ha-manager 09/13] Add crm command 'stop'

[Fabian Grünbichler]

one comment inline

On September 26, 2019 1:38 pm, Fabian Ebner wrote:
> Not every command parameter is 'target' anymore, so
> it was necessary to modify the parsing of $sd->{cmd}.
> 
> Just changing the state to request_stop is not enough,
> we need to actually update the service configuration as well.
> 
> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
> ---
>  src/PVE/HA/Manager.pm | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
> 
> diff --git a/src/PVE/HA/Manager.pm b/src/PVE/HA/Manager.pm
> index 0b90de4..768baa7 100644
> --- a/src/PVE/HA/Manager.pm
> +++ b/src/PVE/HA/Manager.pm
> @@ -349,6 +349,14 @@ sub update_crm_commands {
>  		$haenv->log('err', "crm command error - no such service: $cmd");
>  	    }
>  
> +	} elsif ($cmd =~ m/^stop\s+(\S+)\s+(\S+)$/) {
> +	    my ($sid, $timeout) = ($1, $2);
> +	    if (my $sd = $ss->{$sid}) {
> +		$haenv->log('info', "got crm command: $cmd");
> +		$ss->{$sid}->{cmd} = [ 'stop', $timeout ];
> +	    } else {
> +		$haenv->log('err', "crm command error - no such service: $cmd");
> +	    }
>  	} else {
>  	    $haenv->log('err', "unable to parse crm command: $cmd");
>  	}
> @@ -562,10 +570,10 @@ sub next_state_stopped {
>      }
>  
>      if ($sd->{cmd}) {
> -	my ($cmd, $target) = @{$sd->{cmd}};
> -	delete $sd->{cmd};
> +	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) {
> @@ -575,9 +583,12 @@ sub next_state_stopped {
>  				       target => $target);
>  		return;
>  	    }
> +	} elsif ($cmd eq 'stop') {
> +		$haenv->log('info', "ignore service '$sid' $cmd request - service already stopped");
>  	} else {
>  	    $haenv->log('err', "unknown command '$cmd' for service '$sid'");
>  	}
> +	delete $sd->{cmd};
>      }
>  
>      if ($cd->{state} eq 'disabled') {
> @@ -639,10 +650,10 @@ sub next_state_started {
>      if ($cd->{state} eq 'started') {
>  
>  	if ($sd->{cmd}) {
> -	    my ($cmd, $target) = @{$sd->{cmd}};
> -	    delete $sd->{cmd};
> +	    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) {
> @@ -651,9 +662,17 @@ sub next_state_started {
>  		    $haenv->log('info', "$cmd service '$sid' to node '$target'");
>  		    &$change_service_state($self, $sid, $cmd, node => $sd->{node}, target => $target);
>  		}
> +	    } elsif ($cmd eq 'stop') {
> +		my $timeout = shift @{$sd->{cmd}};
> +		$haenv->log('info', "$cmd service with timeout '$timeout'");
> +		&$change_service_state($self, $sid, 'request_stop', timeout => $timeout);
> +		$haenv->update_service_config(0, 0, $sid, {'state' => 'stopped'});

this uses the methods introduced in patch #2/3, but is missing the 
locking that the other call site (and the other read/modify/write blocks 
for that config file) have.

either move the locking to PVE::HA::Config::update_resources_config in 
#2, or add locking here, or re-evaluate whether there is an approach 
that can skip modifying the config file ;). there are other calls to 
lock_ha_domain in the CRM command handling flow, so it's probably not 
that bad to have another few instances here..

>  	    } else {
>  		$haenv->log('err', "unknown command '$cmd' for service '$sid'");
>  	    }
> +
> +	    delete $sd->{cmd};
> +
>  	} else {
>  
>  	    my $try_next = 0;
> -- 
> 2.20.1
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel at pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
>