Re: [pve-devel] [PATCH qemu-server 1/2] fix #3502: VM start timeout config parameter

[Daniel Tschlatscher <d.tschlatscher@proxmox.com>]

On 6/21/22 17:52, Thomas Lamprecht wrote:
> fix #3502: add start timeout config parameter
> 
> Am 21/06/2022 um 17:20 schrieb Daniel Tschlatscher:
>> It was already possible to set the timeout parameter for the VM config
>> via the API. However, the value was not considered when the function
>> config_aware_timeout() was called.
> 
> but you only add the timeout param now?! IOW. it was never possible to set
> a timeout config, but one could set the parameter for a single start.
> Please reword it, as of is its confusing.
> 
>> Now, if the timeout parameter is set, it will override the heuristic
>> calculation of the VM start timeout.
> 
> you mean if a timeout config property/option is set, as the API parameter
> was already honored in vm_start_nolock:
> 

Yes, I misunderstood part of the code handling the timeout parameter
when passed via the API call (so, when it is not set in the config)
I took another look and AFAICT, the code for setting the timeout in the
config should work as intended though (i.e. the code in this patch)

Still, I will definitely reword it to make it clear what I actually meant!

> my $start_timeout = $params->{timeout} // config_aware_timeout($conf, $resume);
> 
>>
>> During testing I found a problem where really big values (10^20)+
>> would be converted to scientific notation, which means they no longer
>> pass the integer type check. To get around this, I set the maximum
>> value for the timeout to 2,680,000 seconds, which is around 31 days.
>> This I'd wager, is an upper limit in which nobody should realistically
>> run into.
> 
> 24h is already really big enough, so please use 86000, better to go lower
> (but still reasonable) first, we can always easily relax it, but not really
> make it stricter without breaking existing setups.
> 

Alright!

> Also, did you test HA for the case when a really long timeout is set and
> triggers (by faking it with some sleep added somewhere)?
> 

No, I was not aware of this potential issue. I will look into it

>>
>> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
>> ---
>>  PVE/API2/Qemu.pm          | 1 +
>>  PVE/QemuServer.pm         | 7 +++++++
>>  PVE/QemuServer/Helpers.pm | 3 +++
>>  3 files changed, 11 insertions(+)
>>
>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>> index a824657..d0a4eaa 100644
>> --- a/PVE/API2/Qemu.pm
>> +++ b/PVE/API2/Qemu.pm
>> @@ -2494,6 +2494,7 @@ __PACKAGE__->register_method({
>>  		description => "Wait maximal timeout seconds.",
>>  		type => 'integer',
>>  		minimum => 0,
>> +		maximum => 2680000,
>>  		default => 'max(30, vm memory in GiB)',
>>  		optional => 1,
>>  	    },
>> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
>> index e9aa248..81a7f6d 100644
>> --- a/PVE/QemuServer.pm
>> +++ b/PVE/QemuServer.pm
>> @@ -713,6 +713,13 @@ EODESCR
>>  	description => "Some (read-only) meta-information about this guest.",
>>  	optional => 1,
>>      },
>> +    timeout => {
> 
> "timeout what"? in the API it's clear as there it's for an specific actions, but
> as config options one cannot have any idea about what this timeout actually does
> with such an overly generic name.
> 
> I'd maybe put it behind a 'start-options' format string, then it could keep the
> simple name and would be still telling, it would also allow to put any future
> startup related options in there, so not bloating config line amount up too much.
> 
> start-options: timeout=12345
> 
>> +	optional => 1,
>> +	type => 'integer',
>> +	description => 'The maximum timeout to wait for a VM to start',
> 
> Please describe what is done when this isn't passed. FWIW, there's the
> "verbose_description" schema property for very long ones, but I think
> a few sentences describing config_aware_timeout could still fit in the
> normal "description" one.
> 
>> +	minimum => 0,
>> +	maximum => 2680000,
>> +    }
>>  };
>>  
>>  my $cicustom_fmt = {
>> diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
>> index c10d842..c26d0dc 100644
>> --- a/PVE/QemuServer/Helpers.pm
>> +++ b/PVE/QemuServer/Helpers.pm
>> @@ -142,6 +142,9 @@ sub version_cmp {
>>  
>>  sub config_aware_timeout {
>>      my ($config, $is_suspended) = @_;
>> +
>> +    return $config->{timeout} if defined($config->{timeout});
>> +
>>      my $memory = $config->{memory};
>>      my $timeout = 30;
>>  
> 

Thanks for taking a look. I will look into it and especially, reword the
commits and increase the description verbosity