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

[Thomas Lamprecht]

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:

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.

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)?

> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher at 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;
>