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

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

[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.
Now, if the timeout parameter is set, it will override the heuristic
calculation of the VM start timeout.

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.

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 => {
+	optional => 1,
+	type => 'integer',
+	description => 'The maximum timeout to wait for a VM to start',
+	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;
 
-- 
2.30.2

[pve-devel] [PATCH manager 2/2] VM start Timeout "Options" parameter in the GUI

[Daniel Tschlatscher]

The qemu config in the backend already allows specifying a timeout
value. This patch introduces a text field in the Options submenu when
a VM is selected, through which the VM start timeout can be set in the
config.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 www/manager6/qemu/Options.js | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
index a1def4bb..c565066a 100644
--- a/www/manager6/qemu/Options.js
+++ b/www/manager6/qemu/Options.js
@@ -338,6 +338,24 @@ Ext.define('PVE.qemu.Options', {
 		    },
 		} : undefined,
 	    },
+	    timeout: {
+		header: gettext('VM start timeout'),
+		defaultValue: Proxmox.Utils.defaultText,
+		renderer: val => val,
+		editor: caps.vms['VM.Config.Options'] ? {
+		    xtype: 'proxmoxWindowEdit',
+		    subject: gettext('VM start timeout'),
+		    items: {
+			xtype: 'proxmoxintegerfield',
+			name: 'timeout',
+			minValue: 0,
+			maxValue: 2680000,
+			fieldLabel: gettext('Timeout (sec)'),
+			emptyText: Proxmox.Utils.defaultText,
+			deleteEmpty: true,
+		    },
+		} : undefined,
+	    },
 	    hookscript: {
 		header: gettext('Hookscript'),
 	    },
-- 
2.30.2

[pve-devel] [PATCH qemu-server 1/1] make the timeout value editable when the VM is locked

[Daniel Tschlatscher]

In some cases the VM could no longer start when the timeout value was
set to a very low value and afterwards, for example, hibernated. In
this case the VM is somewhat soft locked, because the API would not
allow changing the timeout value anymore. (The only way out here would
be to change the value manually in the config)

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
To make it possible to change the timeout config value when the VM
is locked I created a function to tell whether the config should
be editable or not. This reduces clutter in the already rather long
invoking function. I also tried to keep it extensible for possible
existing values that could be included here/are added in the future.

Nonetheless, I am open to suggestions for a possibly more elegant
solution with this function.
Also, I am not quite sure about storing the "allowed" fields hardcoded
in the function. But I also didn't find where a good place to store 
such a "constant" in this repository would be.

 PVE/API2/Qemu.pm | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index db21fd8..f37baa4 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -8,6 +8,7 @@ use POSIX;
 use IO::Socket::IP;
 use URI::Escape;
 use Crypt::OpenSSL::Random;
+use List::Util qw(first);
 
 use PVE::Cluster qw (cfs_read_file cfs_write_file);;
 use PVE::RRD;
@@ -625,6 +626,28 @@ my $check_vm_modify_config_perm = sub {
     return 1;
 };
 
+# Certain fields should still be editable when the VM is locked 
+# For now, this means that only if those certain fields are edited/deleted will the result be true
+sub skiplock_for_allowed_fields {
+    my ($param, @deleted) = @_;
+    
+    my @allowed = qw"timeout";
+    my $skiplock = 1;
+
+    my @to_check = @deleted;
+    for (keys %$param) {
+	push(@to_check, $_);
+    }
+
+    my $idx = 0;
+    while ($skiplock && $idx < keys @to_check) {
+	$skiplock = first { $_ eq $to_check[$idx] } @allowed;
+	$idx++;
+    }
+
+    return $skiplock;
+}
+
 __PACKAGE__->register_method({
     name => 'vmlist',
     path => '',
@@ -1455,7 +1478,9 @@ my $update_vm_api  = sub {
 	    push @delete, 'runningcpu' if $conf->{runningcpu};
 	}
 
-	PVE::QemuConfig->check_lock($conf) if !$skiplock;
+	if (!$skiplock && !skiplock_for_allowed_fields($param, @delete)) {
+	    PVE::QemuConfig->check_lock($conf);
+	}
 
 	foreach my $opt (keys %$revert) {
 	    if (defined($conf->{$opt})) {
-- 
2.30.2

Re: [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@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;
>  

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

[Thomas Lamprecht]

Am 21/06/2022 um 17:52 schrieb Thomas Lamprecht:
> 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.

Obv. meant 86400 here, sorry.

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

[Daniel Tschlatscher]

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