[pve-devel] [PATCH v3 ha-manager 4/9] Add timeout parameter for shutdown

[Fabian Ebner]

On 10/4/19 5:34 PM, Thomas Lamprecht wrote:
> On 10/2/19 11:46 AM, Fabian Ebner wrote:
>> Introduces a timeout parameter for shutting a resource down.
>> If the parameter is 0, we perform a hard stop instead of a shutdown.
>>
>> Signed-off-by: Fabian Ebner <f.ebner at proxmox.com>
>> ---
>>   src/PVE/HA/LRM.pm             |  4 ++--
>>   src/PVE/HA/Resources.pm       |  2 +-
>>   src/PVE/HA/Resources/PVECT.pm | 14 ++++++++++----
>>   src/PVE/HA/Resources/PVEVM.pm | 16 +++++++++++-----
>>   4 files changed, 24 insertions(+), 12 deletions(-)
>>
>> diff --git a/src/PVE/HA/LRM.pm b/src/PVE/HA/LRM.pm
>> index 3b4a572..e5eee94 100644
>> --- a/src/PVE/HA/LRM.pm
>> +++ b/src/PVE/HA/LRM.pm
>> @@ -535,7 +535,7 @@ sub manage_resources {
>>   	my $req_state = $sd->{state};
>>   	next if !defined($req_state);
>>   	next if $req_state eq 'freeze';
>> -	$self->queue_resource_command($sid, $sd->{uid}, $req_state, {'target' => $sd->{target}});
>> +	$self->queue_resource_command($sid, $sd->{uid}, $req_state, {'target' => $sd->{target}, 'timeout' => $sd->{timeout}});
> hmm, why don't we transformed this so that we just can to
>
> self->queue_resource_command($sid, $sd->{uid}, $req_state, $sd->{params});
>
> as this is already long and seeming a bit unwieldy...
I agree with the sentiment, but I don't see how to cleanly change it.
The problem is in 'change_service_state' we also get 'node' in %params
and the 'target' parameter is also used once in 
'recompute_online_node_usage',
so it makes sense to assign it to $sd directly.

We could pass along %params as a whole, but I don't like the duplication
(e.g. $sd->{target} and $sd->{params}->{target}) and if we don't pass along
all of %params then we need special treatment of 'target' and 'timeout' 
again,
so we don't gain anything.
>>       }
>>   
>>       return $self->run_workers();
>> @@ -776,7 +776,7 @@ sub exec_resource_agent {
>>   
>>   	$haenv->log("info", "stopping service $sid");
>>   
>> -	$plugin->shutdown($haenv, $id);
>> +	$plugin->shutdown($haenv, $id, $params->{timeout});
>>   
>>   	$running = $plugin->check_running($haenv, $id);
>>   
>> diff --git a/src/PVE/HA/Resources.pm b/src/PVE/HA/Resources.pm
>> index 7c373bc..835c314 100644
>> --- a/src/PVE/HA/Resources.pm
>> +++ b/src/PVE/HA/Resources.pm
>> @@ -126,7 +126,7 @@ sub start {
>>   }
>>   
>>   sub shutdown {
>> -    my ($class, $haenv, $id) = @_;
>> +    my ($class, $haenv, $id, $timeout) = @_;
>>   
>>       die "implement in subclass";
>>   }
>> diff --git a/src/PVE/HA/Resources/PVECT.pm b/src/PVE/HA/Resources/PVECT.pm
>> index a0f05f4..282f4ef 100644
>> --- a/src/PVE/HA/Resources/PVECT.pm
>> +++ b/src/PVE/HA/Resources/PVECT.pm
>> @@ -74,18 +74,24 @@ sub start {
>>   }
>>   
>>   sub shutdown {
>> -    my ($class, $haenv, $id) = @_;
>> +    my ($class, $haenv, $id, $timeout) = @_;
>>   
>>       my $nodename = $haenv->nodename();
>> -    my $shutdown_timeout = 60; # fixme: make this configurable
>> +    my $shutdown_timeout = $timeout // 60;
>>   
>> +    my $upid;
>>       my $params = {
>>   	node => $nodename,
>>   	vmid => $id,
>> -	timeout => $shutdown_timeout,
>>       };
>>   
>> -    my $upid = PVE::API2::LXC::Status->vm_shutdown($params);
>> +    if ($shutdown_timeout) {
>> +	$params->{timeout} = $shutdown_timeout;
>> +	$upid = PVE::API2::LXC::Status->vm_shutdown($params);
>> +    } else {
>> +	$upid = PVE::API2::LXC::Status->vm_stop($params);
>> +    }
>> +
>>       PVE::HA::Tools::upid_wait($upid, $haenv);
>>   }
>>   
>> diff --git a/src/PVE/HA/Resources/PVEVM.pm b/src/PVE/HA/Resources/PVEVM.pm
>> index 9dcf558..45d87e8 100644
>> --- a/src/PVE/HA/Resources/PVEVM.pm
>> +++ b/src/PVE/HA/Resources/PVEVM.pm
>> @@ -72,19 +72,25 @@ sub start {
>>   }
>>   
>>   sub shutdown {
>> -    my ($class, $haenv, $id) = @_;
>> +    my ($class, $haenv, $id, $timeout) = @_;
>>   
>>       my $nodename = $haenv->nodename();
>> -    my $shutdown_timeout = 60; # fixme: make this configurable
>> +    my $shutdown_timeout = $timeout // 60;
>>   
>> +    my $upid;
>>       my $params = {
>>   	node => $nodename,
>>   	vmid => $id,
>> -	timeout => $shutdown_timeout,
>> -	forceStop => 1,
>>       };
>>   
>> -    my $upid = PVE::API2::Qemu->vm_shutdown($params);
>> +    if ($shutdown_timeout) {
>> +	$params->{timeout} = $shutdown_timeout;
>> +	$params->{forceStop} = 1;
>> +	$upid = PVE::API2::Qemu->vm_shutdown($params);
>> +    } else {
>> +	$upid = PVE::API2::Qemu->vm_stop($params);
>> +    }
>> +
>>       PVE::HA::Tools::upid_wait($upid, $haenv);
>>   }
>>   
>>