[PATCH-SERIES ha-manager/qemu-server 0/7] fix #7230: acknowledge configured shutdown timeout for HA resources

[PATCH-SERIES ha-manager/qemu-server 0/7] fix #7230: acknowledge configured shutdown timeout for HA resources

[Daniel Kral]

If a shutdown request is issued for HA resources, the vm_shutdown API
handler and the VM-related HA code will always fallback to 60 seconds.

Partially fix #7230 by acknowledging the shutdown timeout value in the
VM config and only fallback to 60 seconds otherwise. This allows users
to at least set a better timeout value through the VM config.



However, this patch series doesn't implement being able to set the value
for --force-stop yet, because similar to #7053 [0], the line-based
crm_commands are not easily extendable without some churn to accomodate
different pve-ha-{crm,lrm} versions across the cluster to add the
--force-stop value to it...

We should make the crm_commands format more extendible as we might run
into more cases where we want to pass relevant parameters through the HA
stack without racy/error-prone code that circles around the problem [1].

Though maybe --force-stop 0 could be the new default behavior in the HA
stack as long as we spawn new 'request_stop' resource commands and
probably introduce some 'max-stop' config for each HA resource so this
doesn't end up in an infinite stop loop if for some reason the HA
resource really cannot be stopped.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=7053
[1] https://lore.proxmox.com/pve-devel/DGYA16ZRKXXY.3CJFSGFKT7K79@proxmox.com/


qemu-server:

Daniel Kral (6):
  rename config_aware_timeout to config_aware_start_timeout
  vm_stop: introduce config_aware_shutdown_timeout helper
  api: qemu: improve wording in timeout parameter descriptions
  api: qemu: add verbose description for shutdown and reboot timeouts
  fix #7230: api: vm_shutdown: make HA resources acknowledge config
    timeout
  api: vm_reboot: initialize the config-aware timeout in API handler

 src/PVE/API2/Qemu.pm          | 33 ++++++++++++++++++++++++++-------
 src/PVE/QemuMigrate.pm        |  4 ++--
 src/PVE/QemuServer.pm         |  9 +++------
 src/PVE/QemuServer/Helpers.pm | 17 +++++++++++++++--
 4 files changed, 46 insertions(+), 17 deletions(-)


ha-manager:

Daniel Kral (1):
  fix #7230: resources: let shutdown api handle timeout fallback value

 src/PVE/HA/Resources/PVECT.pm | 10 +++++-----
 src/PVE/HA/Resources/PVEVM.pm | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)


Summary over all repositories:
  6 files changed, 56 insertions(+), 27 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH qemu-server 1/7] rename config_aware_timeout to config_aware_start_timeout

[Daniel Kral]

The config_aware_timeout() helper is only used to estimate an
appropriate timeout value for VM startups.

This is done in preperation of another patch, which introduces a helper
to compute the shutdown timeout.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/QemuMigrate.pm        | 4 ++--
 src/PVE/QemuServer.pm         | 5 +++--
 src/PVE/QemuServer/Helpers.pm | 4 ++--
 3 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/src/PVE/QemuMigrate.pm b/src/PVE/QemuMigrate.pm
index 8da6f15d..afe0f777 100644
--- a/src/PVE/QemuMigrate.pm
+++ b/src/PVE/QemuMigrate.pm
@@ -31,7 +31,7 @@ use PVE::QemuServer::Agent;
 use PVE::QemuServer::BlockJob;
 use PVE::QemuServer::CPUConfig;
 use PVE::QemuServer::Drive qw(checked_volume_format);
-use PVE::QemuServer::Helpers qw(min_version);
+use PVE::QemuServer::Helpers qw(config_aware_start_timeout min_version);
 use PVE::QemuServer::Machine;
 use PVE::QemuServer::Monitor qw(mon_cmd vm_qmp_peer);
 use PVE::QemuServer::Memory qw(get_current_memory);
@@ -1157,7 +1157,7 @@ sub phase2_start_remote_cluster {
 
     # like regular start but with some overhead accounted for
     my $memory = get_current_memory($self->{vmconf}->{memory});
-    my $timeout = PVE::QemuServer::Helpers::config_aware_timeout($self->{vmconf}, $memory) + 10;
+    my $timeout = config_aware_start_timeout($self->{vmconf}, $memory) + 10;
 
     my $res = PVE::Tunnel::write_tunnel($self->{tunnel}, $timeout, "start", $params);
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 191ae549..5b9f5753 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -57,7 +57,7 @@ use PVE::QemuServer::Blockdev;
 use PVE::QemuServer::BlockJob;
 use PVE::QemuServer::Cfg2Cmd;
 use PVE::QemuServer::Helpers
-    qw(config_aware_timeout get_iscsi_initiator_name get_host_arch min_version kvm_user_version windows_version);
+    qw(config_aware_start_timeout get_iscsi_initiator_name get_host_arch min_version kvm_user_version windows_version);
 use PVE::QemuServer::Cloudinit;
 use PVE::QemuServer::CGroup;
 use PVE::QemuServer::CPUConfig qw(
@@ -5675,7 +5675,8 @@ sub vm_start_nolock {
         );
 
         my $memory = get_current_memory($conf->{memory});
-        $start_timeout = $params->{timeout} // config_aware_timeout($conf, $memory, $resume);
+        $start_timeout = $params->{timeout}
+            // config_aware_start_timeout($conf, $memory, $resume);
 
         push $cmd->@*, $state_cmdline->@*;
 
diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
index dd17eef5..de9bc098 100644
--- a/src/PVE/QemuServer/Helpers.pm
+++ b/src/PVE/QemuServer/Helpers.pm
@@ -15,7 +15,7 @@ use PVE::Tools;
 use base 'Exporter';
 our @EXPORT_OK = qw(
     min_version
-    config_aware_timeout
+    config_aware_start_timeout
     get_iscsi_initiator_name
     kvm_user_version
     parse_number_sets
@@ -261,7 +261,7 @@ sub version_cmp {
     return 0;
 }
 
-sub config_aware_timeout {
+sub config_aware_start_timeout {
     my ($config, $memory, $is_suspended) = @_;
     my $timeout = 30;
 
-- 
2.47.3

[PATCH qemu-server 2/7] vm_stop: introduce config_aware_shutdown_timeout helper

[Daniel Kral]

Factor out the parsing of the shutdown timeout from the VM config in
preparation of a patch, which makes the shutdown timeout of HA resources
consistent with non-HA resources.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/QemuServer.pm         |  5 +----
 src/PVE/QemuServer/Helpers.pm | 13 +++++++++++++
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index 5b9f5753..fefaf36c 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -6241,10 +6241,7 @@ sub _do_vm_stop {
     if (!$nocheck) {
         $conf = PVE::QemuConfig->load_config($vmid);
         PVE::QemuConfig->check_lock($conf) if !$skiplock;
-        if (!defined($timeout) && $shutdown && $conf->{startup}) {
-            my $opts = PVE::JSONSchema::pve_parse_startup_order($conf->{startup});
-            $timeout = $opts->{down} if $opts->{down};
-        }
+        $timeout //= PVE::QemuServer::Helpers::config_aware_shutdown_timeout($conf) if $shutdown;
         PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-stop');
     }
 
diff --git a/src/PVE/QemuServer/Helpers.pm b/src/PVE/QemuServer/Helpers.pm
index de9bc098..81e42854 100644
--- a/src/PVE/QemuServer/Helpers.pm
+++ b/src/PVE/QemuServer/Helpers.pm
@@ -297,6 +297,19 @@ sub config_aware_start_timeout {
     return $timeout;
 }
 
+sub config_aware_shutdown_timeout {
+    my ($conf) = @_;
+
+    my $timeout = 60;
+
+    if ($conf->{startup}) {
+        my $opts = PVE::JSONSchema::pve_parse_startup_order($conf->{startup});
+        $timeout = $opts->{down} if $opts->{down};
+    }
+
+    return $timeout;
+}
+
 sub get_node_pvecfg_version {
     my ($node) = @_;
 
-- 
2.47.3

[PATCH qemu-server 3/7] api: qemu: improve wording in timeout parameter descriptions

[Daniel Kral]

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 28cbb9b0..bf6ade58 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3505,7 +3505,7 @@ __PACKAGE__->register_method({
             },
             targetstorage => get_standard_option('pve-targetstorage'),
             timeout => {
-                description => "Wait maximal timeout seconds.",
+                description => "Maximum timeout in seconds.",
                 type => 'integer',
                 minimum => 0,
                 default => 'max(30, vm memory in GiB)',
@@ -3681,7 +3681,7 @@ __PACKAGE__->register_method({
             skiplock => get_standard_option('skiplock'),
             migratedfrom => get_standard_option('pve-node', { optional => 1 }),
             timeout => {
-                description => "Wait maximal timeout seconds.",
+                description => "Maximum timeout in seconds.",
                 type => 'integer',
                 minimum => 0,
                 optional => 1,
@@ -3856,7 +3856,7 @@ __PACKAGE__->register_method({
             ),
             skiplock => get_standard_option('skiplock'),
             timeout => {
-                description => "Wait maximal timeout seconds.",
+                description => "Maximum timeout in seconds.",
                 type => 'integer',
                 minimum => 0,
                 optional => 1,
@@ -3970,7 +3970,7 @@ __PACKAGE__->register_method({
                 { completion => \&PVE::QemuServer::complete_vmid_running },
             ),
             timeout => {
-                description => "Wait maximal timeout seconds for the shutdown.",
+                description => "Maximum timeout in seconds for the shutdown.",
                 type => 'integer',
                 minimum => 0,
                 optional => 1,
-- 
2.47.3

[PATCH qemu-server 4/7] api: qemu: add verbose description for shutdown and reboot timeouts

[Daniel Kral]

Add documentation for the fallback behavior of the shutdown timeout
parameter for the vm_shutdown and vm_reboot API endpoints.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index bf6ade58..304805dc 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3857,6 +3857,10 @@ __PACKAGE__->register_method({
             skiplock => get_standard_option('skiplock'),
             timeout => {
                 description => "Maximum timeout in seconds.",
+                verbose_description => "Maximum timeout in seconds.\n\n"
+                    . "If the parameter is not provided, the shutdown timeout will fallback"
+                    . " to the shutdown timeout value in the VM config. If none is set there,"
+                    . " the timeout defaults to 60 seconds.",
                 type => 'integer',
                 minimum => 0,
                 optional => 1,
@@ -3971,6 +3975,10 @@ __PACKAGE__->register_method({
             ),
             timeout => {
                 description => "Maximum timeout in seconds for the shutdown.",
+                verbose_description => "Maximum timeout in seconds for the shutdown.\n\n"
+                    . "If the parameter is not provided, the shutdown timeout will fallback"
+                    . " to the shutdown timeout value in the VM config. If none is set there,"
+                    . " the timeout defaults to 60 seconds.",
                 type => 'integer',
                 minimum => 0,
                 optional => 1,
-- 
2.47.3

[PATCH qemu-server 5/7] fix #7230: api: vm_shutdown: make HA resources acknowledge config timeout

[Daniel Kral]

Allow users to configure the shutdown timeout of HA resources by
acknowledging the shutdown timeout value from the VM config.

This does only partially fix #7230 as HA resources are still
force-stopped as the LRM will always set the flag.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 304805dc..2613aea0 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -3899,6 +3899,12 @@ __PACKAGE__->register_method({
         raise_param_exc({ keepActive => "Only root may use this option." })
             if $keepActive && $authuser ne 'root@pam';
 
+        my $timeout = extract_param($param, 'timeout');
+        if (!defined($timeout)) {
+            my $conf = PVE::QemuConfig->load_config($vmid);
+            $timeout = PVE::QemuServer::Helpers::config_aware_shutdown_timeout($conf);
+        }
+
         my $storecfg = PVE::Storage::config();
 
         my $shutdown = 1;
@@ -3916,7 +3922,6 @@ __PACKAGE__->register_method({
 
         if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
 
-            my $timeout = $param->{timeout} // 60;
             my $hacmd = sub {
                 my $upid = shift;
 
@@ -3941,7 +3946,7 @@ __PACKAGE__->register_method({
                     $vmid,
                     $skiplock,
                     0,
-                    $param->{timeout},
+                    $timeout,
                     $shutdown,
                     $param->{forceStop},
                     $keepActive,
-- 
2.47.3

[PATCH qemu-server 6/7] api: vm_reboot: initialize the config-aware timeout in API handler

[Daniel Kral]

As vm_reboot() is the only remaining caller which depends on
_do_vm_stop() to acknowledge the shutdown timeout from the VM config,
move the behavior to the API handler and remove it from _do_vm_stop().

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/Qemu.pm  | 8 +++++++-
 src/PVE/QemuServer.pm | 1 -
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 2613aea0..a380cfc1 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -4002,6 +4002,12 @@ __PACKAGE__->register_method({
         my $node = extract_param($param, 'node');
         my $vmid = extract_param($param, 'vmid');
 
+        my $timeout = extract_param($param, 'timeout');
+        if (!defined($timeout)) {
+            my $conf = PVE::QemuConfig->load_config($vmid);
+            $timeout = PVE::QemuServer::Helpers::config_aware_shutdown_timeout($conf);
+        }
+
         die "VM is paused - cannot shutdown\n" if PVE::QemuServer::vm_is_paused($vmid, 1);
 
         die "VM $vmid not running\n" if !PVE::QemuServer::check_running($vmid);
@@ -4010,7 +4016,7 @@ __PACKAGE__->register_method({
             my $upid = shift;
 
             syslog('info', "requesting reboot of VM $vmid: $upid\n");
-            PVE::QemuServer::vm_reboot($vmid, $param->{timeout});
+            PVE::QemuServer::vm_reboot($vmid, $timeout);
             return;
         };
 
diff --git a/src/PVE/QemuServer.pm b/src/PVE/QemuServer.pm
index fefaf36c..1f2bc424 100644
--- a/src/PVE/QemuServer.pm
+++ b/src/PVE/QemuServer.pm
@@ -6241,7 +6241,6 @@ sub _do_vm_stop {
     if (!$nocheck) {
         $conf = PVE::QemuConfig->load_config($vmid);
         PVE::QemuConfig->check_lock($conf) if !$skiplock;
-        $timeout //= PVE::QemuServer::Helpers::config_aware_shutdown_timeout($conf) if $shutdown;
         PVE::GuestHelpers::exec_hookscript($conf, $vmid, 'pre-stop');
     }
 
-- 
2.47.3

[PATCH ha-manager 7/7] fix #7230: resources: let shutdown api handle timeout fallback value

[Daniel Kral]

PVE::API2::LXC::Status::vm_shutdown() and PVE::API2::Qemu::vm_shutdown()
already fallback to either the configured shutdown timeout or the
default value of 60 seconds. Therefore, let the API handlers decide what
the timeout value should be set to.

This does only partially fix #7230 as HA resources are still
force-stopped as the shutdown timeout runs out.

Signed-off-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Resources/PVECT.pm | 10 +++++-----
 src/PVE/HA/Resources/PVEVM.pm | 10 +++++-----
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/src/PVE/HA/Resources/PVECT.pm b/src/PVE/HA/Resources/PVECT.pm
index 177b9070..ead66e0e 100644
--- a/src/PVE/HA/Resources/PVECT.pm
+++ b/src/PVE/HA/Resources/PVECT.pm
@@ -87,7 +87,6 @@ sub shutdown {
     my ($class, $haenv, $id, $timeout) = @_;
 
     my $nodename = $haenv->nodename();
-    my $shutdown_timeout = $timeout // 60;
 
     my $upid;
     my $params = {
@@ -95,11 +94,12 @@ sub shutdown {
         vmid => $id,
     };
 
-    if ($shutdown_timeout) {
-        $params->{timeout} = $shutdown_timeout;
-        $upid = PVE::API2::LXC::Status->vm_shutdown($params);
-    } else {
+    # stop the CT immediately if $timeout is set to exactly 0
+    if (defined($timeout) && $timeout == 0) {
         $upid = PVE::API2::LXC::Status->vm_stop($params);
+    } else {
+        $params->{timeout} = $timeout;
+        $upid = PVE::API2::LXC::Status->vm_shutdown($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 87532716..d89a0bce 100644
--- a/src/PVE/HA/Resources/PVEVM.pm
+++ b/src/PVE/HA/Resources/PVEVM.pm
@@ -87,7 +87,6 @@ sub shutdown {
     my ($class, $haenv, $id, $timeout) = @_;
 
     my $nodename = $haenv->nodename();
-    my $shutdown_timeout = $timeout // 60;
 
     my $upid;
     my $params = {
@@ -95,12 +94,13 @@ sub shutdown {
         vmid => $id,
     };
 
-    if ($shutdown_timeout) {
-        $params->{timeout} = $shutdown_timeout;
+    # stop the VM immediately if $timeout is set to exactly 0
+    if (defined($timeout) && $timeout == 0) {
+        $upid = PVE::API2::Qemu->vm_stop($params);
+    } else {
+        $params->{timeout} = $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);
-- 
2.47.3