[pve-devel] [PATCH qemu-server] Use crm-command stop to allow shutdown with timeout and hard stop for HA

[Fabian Ebner]

On 11/13/19 9:55 AM, Thomas Lamprecht wrote:
> On 11/12/19 11:03 AM, Fabian Ebner wrote:
>> The minimum value for timeout in vm_shutdown is changed from 0 to 1, since a
>> value of 0 would trigger a hard stop for HA managed VMs. Like this the API
>> description stays valid for all cases.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>
>> In vm_shutdown we'd like to pass along the timeout parameter to the HA stack,
>> but the value 0 triggers a special behavior there, namely a hard stop.
>> So either the description needs to be changed to mention this behavior or
>> we just don't allow a timeout of 0 in vm_shutdown. Since a shutdown with
>> timeout 0 doesn't really make sense anyways, I opted for the latter.
>> It's the same situation for containers.
>>
> 
> timeout == 0 just means instant stop, I did not checked this, but as
> both CTs and VMs allow to pass 0 it really smells like it was done by
> design.. ^^
> 
> Also, limiting API value ranges could be seen as API breakage, and that
> should be avoided if possible..
> 
> What was the behaviour of passing this for a non-HA VM/CT?
> 

If I interpreted the code correctly:

* For 'qm shutdown --timeout=0':
** For non-HA managed VMs it leads to (depending on whether guest agent 
is enabled) a qmp command 'guest_shutdown' or 'system_powerdown'.
Then (since timeout is 0 this happens immediately) depending on --force 
we either kill with SIGTERM or die with "got timeout".
** For HA managed VMs 'qm shutdown --timeout=0' turns into a vm_stop 
issuing a qmp command 'quit' and we actually wait 60 seconds for that 
command.


* For 'pct shutdown --timeout=0':
** For non-HA managed containers we always give an extra 5 seconds of 
timeout, so 5 seconds here.
** For HA managed containers it will result in an immediate kill.

I mean it probably doesn't really matter, there was a discrepancy 
between what happens for HA managed and non-HA managed VM/CT already.
I can send a v2 without the new minima if you like.

>>   PVE/API2/Qemu.pm | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index c31dd1d..16a7a04 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -2111,7 +2111,7 @@ __PACKAGE__->register_method({
>>   
>>   		print "Requesting HA stop for VM $vmid\n";
>>   
>> -		my $cmd = ['ha-manager', 'set',  "vm:$vmid", '--state', 'stopped'];
>> +		my $cmd = ['ha-manager', 'crm-command', 'stop',  "vm:$vmid", '0'];
>>   		PVE::Tools::run_command($cmd);
>>   		return;
>>   	    };
>> @@ -2204,7 +2204,7 @@ __PACKAGE__->register_method({
>>   	    timeout => {
>>   		description => "Wait maximal timeout seconds.",
>>   		type => 'integer',
>> -		minimum => 0,
>> +		minimum => 1,
>>   		optional => 1,
>>   	    },
>>   	    forceStop => {
>> @@ -2267,12 +2267,13 @@ __PACKAGE__->register_method({
>>   
>>   	if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
>>   
>> +	    my $timeout = $param->{timeout} // 60;
>>   	    my $hacmd = sub {
>>   		my $upid = shift;
>>   
>>   		print "Requesting HA stop for VM $vmid\n";
>>   
>> -		my $cmd = ['ha-manager', 'set', "vm:$vmid", '--state', 'stopped'];
>> +		my $cmd = ['ha-manager', 'crm-command', 'stop', "vm:$vmid", "$timeout"];
>>   		PVE::Tools::run_command($cmd);
>>   		return;
>>   	    };
>>
>