[pve-devel] [PATCH common/qemu-server/manager v4] fix #3502: VM start timeout config parameter

[pve-devel] [PATCH common/qemu-server/manager v4] fix #3502: VM start timeout config parameter

[Daniel Tschlatscher]

This patch series adds a an option to specify a timeout value in the
config for starting VMs.
The minimum is 0 seconds, which disables the timeout completely. The
maximum is 86400 seconds, equivalent to 1 day.

The timeout value can also be passed via the CLI, which always
overrides the setting. If no value for timeout is passed, the timeout
will either be read from the config, or if unavailable, chosen
heuristically like before, with a default of 30 seconds in most cases.

For this, a new property string called "startoptions" is added.
Currently only the VM start timeout is configurable with it.


Changes from v3
* Minor code cleanup
* Addressed an edge case concerning the killing of a lingering VM to
  not accidentally kill an unrelated process

Thanks to Fabian for the review and suggestions!


pve-common:

Daniel Tschlatscher (1):
  VM start timeout config parameter

 src/PVE/JSONSchema.pm | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

qemu-server:

Daniel Tschlatscher (4):
  expose VM start timeout config setting in API
  await and kill lingering KVM thread when VM start reaches timeout
  re-check if VM is running and PID for KILL after timeout
  make the timeout value editable when the VM is locked

 PVE/API2/Qemu.pm          | 27 ++++++++++++++++++++++++++
 PVE/QemuServer.pm         | 41 +++++++++++++++++++++++++++++++--------
 PVE/QemuServer/Helpers.pm |  4 ++++
 3 files changed, 64 insertions(+), 8 deletions(-)

pve-manager:

Daniel Tschlatscher (1):
  VM start Timeout "Options" parameter in the GUI

 www/manager6/qemu/Options.js | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

-- 
2.30.2

[pve-devel] [PATCH common v4 1/6] VM start timeout config parameter in backend

[Daniel Tschlatscher]

This allows setting the 'startoptions' property string in the config.
For now this only implements the 'timeout' parameter but should be
rather easily extensible and allow related VM start config options
to be also configurable here.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---

Changes from v3:
* No changes

 src/PVE/JSONSchema.pm | 38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index 527e409..64dc01b 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -640,6 +640,17 @@ sub pve_verify_startup_order {
     die "unable to parse startup options\n";
 }
 
+register_format('start-options', \&pve_verify_startup_options);
+sub pve_verify_startup_options {
+    my ($value, $noerr) = @_;
+
+    return $value if pve_parse_startup_options($value);
+
+    return undef if $noerr;
+
+    die "unable to parse vm start options\n";
+}
+
 my %bwlimit_opt = (
     optional => 1,
     type => 'number', minimum => '0',
@@ -748,6 +759,33 @@ PVE::JSONSchema::register_standard_option('pve-startup-order', {
     typetext => '[[order=]\d+] [,up=\d+] [,down=\d+] ',
 });
 
+sub pve_parse_startup_options {
+    my ($value) = @_;
+
+    return undef if !$value;
+
+    my $res = {};
+
+    foreach my $p (split(/,/, $value)) {
+	next if $p =~ m/^\s*$/;
+
+	if ($p =~ m/^timeout=(\d+)$/ && int($1) <= 86400) {
+	    $res->{timeout} = $1;
+	} else {
+	    return undef;
+	}
+    }
+
+    return $res;
+}
+
+register_standard_option('start-options', {
+    description => "Start up options for the VM. This only allows setting the VM start timeout for now, which is the maximum VM startup timeout in seconds. The maximum value for timeout is 86400, the minimum 0, which disables the timeout completely. If timeout is unset, the timeout will either be the memory of the VM in GiBs or 30, depending on which is higher. If unset and hibernated, the value will at least be 300 seconds, with hugepages at least 150 seconds.",
+    optional => 1,
+    type => 'string', format => 'start-options',
+    typetext => 'timeout=\d+',
+});
+
 register_format('pve-tfa-secret', \&pve_verify_tfa_secret);
 sub pve_verify_tfa_secret {
     my ($key, $noerr) = @_;
-- 
2.30.2

Re: [pve-devel] [PATCH common v4 1/6] VM start timeout config parameter in backend

[Thomas Lamprecht]

for anyone wanting to pick this up:

high level: it should go into pve-guest-common

Am 05/01/2023 um 11:08 schrieb Daniel Tschlatscher:
> This allows setting the 'startoptions' property string in the config.
> For now this only implements the 'timeout' parameter but should be
> rather easily extensible and allow related VM start config options
> to be also configurable here.
> 
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
> 
> Changes from v3:
> * No changes
> 
>  src/PVE/JSONSchema.pm | 38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
> 
> diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
> index 527e409..64dc01b 100644
> --- a/src/PVE/JSONSchema.pm
> +++ b/src/PVE/JSONSchema.pm
> @@ -640,6 +640,17 @@ sub pve_verify_startup_order {
>      die "unable to parse startup options\n";
>  }
>  
> +register_format('start-options', \&pve_verify_startup_options);

would prefer `guest-start-options`, as we have starts for other things (e.g., ceph or
systemd services)

> +sub pve_verify_startup_options {
> +    my ($value, $noerr) = @_;
> +
> +    return $value if pve_parse_startup_options($value);
> +
> +    return undef if $noerr;
> +
> +    die "unable to parse vm start options\n";
> +}
> +
>  my %bwlimit_opt = (
>      optional => 1,
>      type => 'number', minimum => '0',
> @@ -748,6 +759,33 @@ PVE::JSONSchema::register_standard_option('pve-startup-order', {
>      typetext => '[[order=]\d+] [,up=\d+] [,down=\d+] ',
>  });
>  
> +sub pve_parse_startup_options {
> +    my ($value) = @_;
> +
> +    return undef if !$value;
> +
> +    my $res = {};
> +
> +    foreach my $p (split(/,/, $value)) {
> +	next if $p =~ m/^\s*$/;
> +
> +	if ($p =~ m/^timeout=(\d+)$/ && int($1) <= 86400) {
> +	    $res->{timeout} = $1;
> +	} else {
> +	    return undef;
> +	}
> +    }
> +
> +    return $res;
> +}
> +
> +register_standard_option('start-options', {
> +    description => "Start up options for the VM. This only allows setting the VM start timeout for now, which is the maximum VM startup timeout in seconds. The maximum value for timeout is 86400, the minimum 0, which disables the timeout completely. If timeout is unset, the timeout will either be the memory of the VM in GiBs or 30, depending on which is higher. If unset and hibernated, the value will at least be 300 seconds, with hugepages at least 150 seconds.",

please split to multiple lines with 100cc max each

> +    optional => 1,
> +    type => 'string', format => 'start-options',
> +    typetext => 'timeout=\d+',
> +});
> +
>  register_format('pve-tfa-secret', \&pve_verify_tfa_secret);
>  sub pve_verify_tfa_secret {
>      my ($key, $noerr) = @_;

[pve-devel] [PATCH qemu-server v4 2/6] expose VM start timeout config setting in API

[Daniel Tschlatscher]

This patch makes it possible to now set the starting timeout value for
a VM via the config parameter 'startoptions'.
Now, if the timeout parameter is set, it will override the heuristic
calculation of the VM start timeout.

The maximum value for the timeout parameter is 86400 seconds, which is
one day. The minimum value is 0, which disables the timeout.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
No changes from v3

 PVE/API2/Qemu.pm          | 2 ++
 PVE/QemuServer.pm         | 1 +
 PVE/QemuServer/Helpers.pm | 4 ++++
 3 files changed, 7 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index e2a420f..9a6b431 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -541,6 +541,7 @@ my $generaloptions = {
     'reboot' => 1,
     'startdate' => 1,
     'startup' => 1,
+    'startoptions' => 1,
     'tdf' => 1,
     'template' => 1,
 };
@@ -2647,6 +2648,7 @@ __PACKAGE__->register_method({
 		description => "Wait maximal timeout seconds.",
 		type => 'integer',
 		minimum => 0,
+		maximum => 86400,
 		default => 'max(30, vm memory in GiB)',
 		optional => 1,
 	    },
diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 39fc6b0..2a4bc75 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -716,6 +716,7 @@ EODESCR
 	description => "List of host cores used to execute guest processes, for example: 0,5,8-11",
 	optional => 1,
     },
+    startoptions => get_standard_option('start-options'),
 };
 
 my $cicustom_fmt = {
diff --git a/PVE/QemuServer/Helpers.pm b/PVE/QemuServer/Helpers.pm
index e91f906..1fa9011 100644
--- a/PVE/QemuServer/Helpers.pm
+++ b/PVE/QemuServer/Helpers.pm
@@ -144,6 +144,10 @@ sub version_cmp {
 
 sub config_aware_timeout {
     my ($config, $is_suspended) = @_;
+
+    my $startup = PVE::JSONSchema::pve_parse_startup_options($config->{startoptions});
+    return $startup->{timeout} if defined($startup->{timeout});
+
     my $memory = $config->{memory};
     my $timeout = 30;
 
-- 
2.30.2

[pve-devel] [PATCH qemu-server v4 3/6] await and kill lingering KVM thread when VM start reaches timeout

[Daniel Tschlatscher]

In some cases the VM API start method would return before the detached
KVM process would have exited. This is especially problematic with HA,
because the HA manager would think the VM started successfully, later
see that it exited and start it again in an endless loop.

Moreover, another case exists when resuming a hibernated VM. In this
case, the qemu thread will attempt to load the whole vmstate into
memory before exiting.
Depending on vmstate size, disk read speed, and similar factors this
can take quite a while though and it is not possible to start the VM
normally during this time.

To get around this, this patch intercepts the error, looks whether a
corresponding KVM thread is still running, and waits for/kills it,
before continuing.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* Minor code clean up concerning the usage of "$pid" in ifs according
  to Fabian's suggestion

 PVE/QemuServer.pm | 38 +++++++++++++++++++++++++++++++-------
 1 file changed, 31 insertions(+), 7 deletions(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 2a4bc75..549d666 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -5881,15 +5881,39 @@ sub vm_start_nolock {
 		$tpmpid = start_swtpm($storecfg, $vmid, $tpm, $migratedfrom);
 	    }
 
-	    my $exitcode = run_command($cmd, %run_params);
-	    if ($exitcode) {
-		if ($tpmpid) {
-		    warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup error\n";
-		    kill 'TERM', $tpmpid;
+	    eval {
+		my $exitcode = run_command($cmd, %run_params);
+
+		if ($exitcode) {
+		    if ($tpmpid) {
+			log_warn "stopping swtpm instance (pid $tpmpid) due to QEMU startup error\n";
+			kill 'TERM', $tpmpid;
+		    }
+		    die "QEMU exited with code $exitcode\n";
 		}
-		die "QEMU exited with code $exitcode\n";
+	    };
+
+	    if (my $err = $@) {
+		if (my $pid = PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+		    my $count = 0;
+		    my $timeout = 300;
+
+		    print "Waiting $timeout seconds for detached qemu process $pid to exit\n";
+		    while (($count < $timeout) &&
+			PVE::QemuServer::Helpers::vm_running_locally($vmid)) {
+			$count++;
+			sleep(1);
+		    }
+
+		    if ($count >= $timeout) {
+			log_warn "Reached timeout. Terminating now with SIGKILL\n";
+			kill(9, $pid) if PVE::QemuServer::Helpers::vm_running_locally($vmid) eq $pid;
+		    }
+		}
+
+		die $err;
 	    }
-	};
+	}
     };
 
     if ($conf->{hugepages}) {
-- 
2.30.2

[pve-devel] [PATCH qemu-server v4 4/6] re-check if VM is running and PID for KILL after timeout

[Daniel Tschlatscher]

This avoids accidentally killing a potentially completely unrelated
process that was spawned in an edge case between the last check in the
timeout loop and the kill command.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
Changes from v3:
* New patch

 PVE/QemuServer.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
index 549d666..e14b76e 100644
--- a/PVE/QemuServer.pm
+++ b/PVE/QemuServer.pm
@@ -6241,7 +6241,7 @@ sub _do_vm_stop {
 
     if ($count >= $timeout) {
 	warn "VM still running - terminating now with SIGKILL\n";
-	kill 9, $pid;
+	kill(9, $pid) if check_running($vmid, $nocheck) eq $pid;
 	sleep 1;
     }
 
-- 
2.30.2

[pve-devel] [PATCH qemu-server v4 5/6] 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 and afterwards, for example, hibernated. In this case the VM is
soft locked in the GUI, 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)

To avoid unwanted side effects, it is possible to change the value for
the new 'startoptions' parameter, only if the VM is currently locked
with lock 'suspended'.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
No changes from v3

 PVE/API2/Qemu.pm | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 9a6b431..ce5f92b 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -11,6 +11,7 @@ use JSON;
 use URI::Escape;
 use Crypt::OpenSSL::Random;
 use Socket qw(SOCK_STREAM);
+use List::Util qw(first);
 
 use PVE::APIClient::LWP;
 use PVE::CGroup;
@@ -650,6 +651,28 @@ my $check_vm_modify_config_perm = sub {
     return 1;
 };
 
+# Certain parameter fields should still be editable and deletable when the VM is locked
+# Returns true only if all parameter fields to be edited or deleted are defined in @allowed
+sub skiplock_for_allowed_fields {
+    my ($param, @deleted) = @_;
+
+    my @allowed = qw"startoptions";
+    my $skiplock = 1;
+
+    my @to_check = @deleted;
+    for (keys %$param) {
+	push(@to_check, $_);
+    }
+
+    my $idx = 0;
+    while ($skiplock && $idx < keys @to_check) {
+	$skiplock &= defined(first { $_ eq $to_check[$idx] } @allowed);
+	$idx++;
+    }
+
+    return $skiplock;
+}
+
 __PACKAGE__->register_method({
     name => 'vmlist',
     path => '',
@@ -1598,6 +1621,8 @@ my $update_vm_api  = sub {
 	    push @delete, 'runningcpu' if $conf->{runningcpu};
 	}
 
+	$skiplock |= $conf->{lock} eq "suspended" && skiplock_for_allowed_fields($param, @delete);
+
 	PVE::QemuConfig->check_lock($conf) if !$skiplock;
 
 	foreach my $opt (keys %$revert) {
-- 
2.30.2

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

[Daniel Tschlatscher]

This makes it possible to set the newly introduced config parameter
for timeout via the 'startoptions' property string.
For now this only implements setting the timeout value when starting
a VM, though this should be rather easily exentensible to include
other future start options parameters.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
No changes from v3:

 www/manager6/qemu/Options.js | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/www/manager6/qemu/Options.js b/www/manager6/qemu/Options.js
index 7b112400..7f148e18 100644
--- a/www/manager6/qemu/Options.js
+++ b/www/manager6/qemu/Options.js
@@ -76,6 +76,37 @@ Ext.define('PVE.qemu.Options', {
 			onlineHelp: 'qm_startup_and_shutdown',
 		    } : undefined,
 	    },
+	    startoptions: {
+		header: gettext('VM startup options'),
+		defaultValue: Proxmox.Utils.defaultText,
+		renderer: val => val,
+		editor: caps.vms['VM.Config.Options'] ? {
+		    xtype: 'proxmoxWindowEdit',
+		    subject: gettext('VM start timeout'),
+		    setValues: function(values) {
+			Ext.Array.each(this.query('inputpanel'), function(panel) {
+			    panel.setValues(PVE.Parser.parsePropertyString(values.startoptions));
+			});
+		    },
+		    items: {
+			xtype: 'inputpanel',
+			items: {
+			    xtype: 'proxmoxintegerfield',
+			    name: 'timeout',
+			    minValue: 0,
+			    maxValue: 86400,
+			    fieldLabel: gettext('Timeout (sec)'),
+			    emptyText: Proxmox.Utils.defaultText,
+			},
+			onGetValues: function(values) {
+			    if (values === undefined || Object.keys(values).length === 0) {
+				return { 'delete': 'startoptions' };
+			    }
+			    return { 'startoptions': PVE.Parser.printPropertyString(values) };
+			},
+		    },
+		} : undefined,
+	    },
 	    ostype: {
 		header: gettext('OS Type'),
 		editor: caps.vms['VM.Config.Options'] ? 'PVE.qemu.OSTypeEdit' : undefined,
-- 
2.30.2