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.eb...@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@pve.proxmox.com
> https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
>
_______________________________________________
pve-devel mailing list
pve-devel@pve.proxmox.com
https://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
