[pve-devel] [PATCH guest-common/container/qemu-server/manager v2 0/6] fix #4474: stop tasks may overrule shutdown tasks

[pve-devel] [PATCH guest-common/container/qemu-server/manager v2 0/6] fix #4474: stop tasks may overrule shutdown tasks

[Friedrich Weber]

As reported in #4474 [0], a user may attempt to shutdown a VM/CT,
realize that it is unresponsive, and decide to stop it instead. If the
shutdown task has not timed out yet, the stop task will fail. The user
needs to manually abort the shutdown task before stopping the VM,
which is inconvenient.

With this patch series, a stop task can optionally overrule active
shutdown tasks.

Summary of changes:

* Backend:
** In the LXC status API handlers, we move config locking from the API
   handlers into workers to avoid blocking, as suggested by Wolfgang on v1.
** We add a new optional `overrule-shutdown` parameter to
   VM/CT `/stop` endpoints. If it is 1, we query for active
   `qmshutdown`/`vzshutdown` tasks by the current user for the given
   VMID, and abort them before attempting to stop the VM/CT.
* Frontend: Issuing a stop command shows a confirmation message box that, if an
  active shutdown task for that guest is detected, shows a default-on checkbox
  that offers to overrule active shutdown tasks. If it is checked, the API
  request is sent with `overrule-shutdown=1`.

Tested with a hung CentOS 7 container and a SeaBIOS VM without a bootable disk.

Changes v1 -> v2:
* fixed patch ordering, as suggested by Wolfgang on v1
* added a pve-container patch that moves config locking from API handlers
  into workers for start/stop/shutdown/suspend, as suggested by Wolfgang on v1
* instead of showing a second modal dialog when stopping a guest, show a "guest
  stop" message box that has an optional checkbox offering to overrule shutdown
  tasks
* split pve-manager patch in two

[0]: https://bugzilla.proxmox.com/show_bug.cgi?id=4474

guest-common:

Friedrich Weber (1):
  guest helpers: add helper to overrule active tasks of a specific type

 src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)


container:

Friedrich Weber (2):
  api: status: move config locking from API handler into worker
  fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint

 src/PVE/API2/LXC/Status.pm | 107 ++++++++++++++++++++-----------------
 1 file changed, 59 insertions(+), 48 deletions(-)


qemu-server:

Friedrich Weber (1):
  fix #4474: qemu api: add overrule-shutdown parameter to stop endpoint

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


manager:

Friedrich Weber (2):
  ui: fix typo to make pve-cluster-tasks store globally available
  fix #4474: ui: guest stop: offer to overrule active shutdown tasks

 www/manager6/Makefile            |  1 +
 www/manager6/dc/Tasks.js         |  2 +-
 www/manager6/lxc/CmdMenu.js      |  8 +++-
 www/manager6/lxc/Config.js       |  8 ++--
 www/manager6/qemu/CmdMenu.js     |  8 +++-
 www/manager6/qemu/Config.js      |  8 ++--
 www/manager6/window/GuestStop.js | 75 ++++++++++++++++++++++++++++++++
 7 files changed, 101 insertions(+), 9 deletions(-)
 create mode 100644 www/manager6/window/GuestStop.js


Summary over all repositories:
  10 files changed, 193 insertions(+), 0 deletions(-)

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Friedrich Weber]

This helper is used to abort any active qmshutdown/vzshutdown tasks
before attempting to stop a VM/CT (if requested).

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    no changes v1 -> v2

 src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index 961a7b8..bd94ed2 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -416,4 +416,22 @@ sub check_vnet_access {
 	if !($tag || $trunks);
 }
 
+sub overrule_tasks {
+    my ($type, $user, $vmid) = @_;
+
+    my $active_tasks = PVE::INotify::read_file('active');
+    my $res = [];
+    for my $task (@$active_tasks) {
+       if (!$task->{saved}
+           && $task->{type} eq $type
+           && $task->{user} eq $user
+           && $task->{id} eq $vmid
+       ) {
+           PVE::RPCEnvironment->check_worker($task->{upid}, 1);
+           push @$res, $task->{upid};
+       }
+    }
+    return $res;
+}
+
 1;
-- 
2.39.2

[pve-devel] [PATCH container v2 2/6] api: status: move config locking from API handler into worker

[Friedrich Weber]

Previously, container start/stop/shutdown/suspend would try to acquire
the config lock in the API handler prior to forking a worker. If the
lock was currently held elsewhere, this would block the API handler
and thus the pvedaemon worker thread until the 10s timeout expired (or
the lock could be acquired).

To avoid blocking the API handler, immediately fork off a worker
process and try to acquire the config lock in that worker.

Patch best viewed with `git show -w`.

Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    The diff is somewhat messy without `-w` -- couldn't come up with a
    better way.
    
    new in v2

 src/PVE/API2/LXC/Status.pm | 91 ++++++++++++++++++--------------------
 1 file changed, 42 insertions(+), 49 deletions(-)

diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index f7e3128..7741374 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -176,32 +176,31 @@ __PACKAGE__->register_method({
 	    my $realcmd = sub {
 		my $upid = shift;
 
-		syslog('info', "starting CT $vmid: $upid\n");
+		PVE::LXC::Config->lock_config($vmid, sub {
+		    syslog('info', "starting CT $vmid: $upid\n");
 
-		my $conf = PVE::LXC::Config->load_config($vmid);
-
-		die "you can't start a CT if it's a template\n"
-		    if PVE::LXC::Config->is_template($conf);
+		    my $conf = PVE::LXC::Config->load_config($vmid);
 
-		if (!$skiplock && !PVE::LXC::Config->has_lock($conf, 'mounted')) {
-		    PVE::LXC::Config->check_lock($conf);
-		}
+		    die "you can't start a CT if it's a template\n"
+			if PVE::LXC::Config->is_template($conf);
 
-		if ($conf->{unprivileged}) {
-		    PVE::LXC::Config->foreach_volume($conf, sub {
-			my ($ms, $mountpoint) = @_;
-			die "Quotas are not supported by unprivileged containers.\n" if $mountpoint->{quota};
-		    });
-		}
+		    if (!$skiplock && !PVE::LXC::Config->has_lock($conf, 'mounted')) {
+			PVE::LXC::Config->check_lock($conf);
+		    }
 
-		PVE::LXC::vm_start($vmid, $conf, $skiplock, $param->{debug});
-	    };
+		    if ($conf->{unprivileged}) {
+			PVE::LXC::Config->foreach_volume($conf, sub {
+			    my ($ms, $mountpoint) = @_;
+			    die "Quotas are not supported by unprivileged containers.\n"
+				if $mountpoint->{quota};
+			});
+		    }
 
-	    my $lockcmd = sub {
-		return $rpcenv->fork_worker('vzstart', $vmid, $authuser, $realcmd);
+		    PVE::LXC::vm_start($vmid, $conf, $skiplock, $param->{debug});
+		});
 	    };
 
-	    return PVE::LXC::Config->lock_config($vmid, $lockcmd);
+	    return $rpcenv->fork_worker('vzstart', $vmid, $authuser, $realcmd);
 	}
     }});
 
@@ -258,21 +257,19 @@ __PACKAGE__->register_method({
 	    my $realcmd = sub {
 		my $upid = shift;
 
-		syslog('info', "stopping CT $vmid: $upid\n");
+		PVE::LXC::Config->lock_config($vmid, sub {
+		    syslog('info', "stopping CT $vmid: $upid\n");
 
-		my $conf = PVE::LXC::Config->load_config($vmid);
-		if (!$skiplock && !PVE::LXC::Config->has_lock($conf, 'mounted')) {
-		    PVE::LXC::Config->check_lock($conf);
-		}
+		    my $conf = PVE::LXC::Config->load_config($vmid);
+		    if (!$skiplock && !PVE::LXC::Config->has_lock($conf, 'mounted')) {
+			PVE::LXC::Config->check_lock($conf);
+		    }
 
-		PVE::LXC::vm_stop($vmid, 1);
+		    PVE::LXC::vm_stop($vmid, 1);
+		});
 	    };
 
-	    my $lockcmd = sub {
-		return $rpcenv->fork_worker('vzstop', $vmid, $authuser, $realcmd);
-	    };
-
-	    return PVE::LXC::Config->lock_config($vmid, $lockcmd);
+	    return $rpcenv->fork_worker('vzstop', $vmid, $authuser, $realcmd);
 	}
     }});
 
@@ -339,19 +336,17 @@ __PACKAGE__->register_method({
 	my $realcmd = sub {
 	    my $upid = shift;
 
-	    syslog('info', "shutdown CT $vmid: $upid\n");
-
-	    my $conf = PVE::LXC::Config->load_config($vmid);
-	    PVE::LXC::Config->check_lock($conf);
+	    PVE::LXC::Config->lock_config($vmid, sub {
+		syslog('info', "shutdown CT $vmid: $upid\n");
 
-	    PVE::LXC::vm_stop($vmid, 0, $timeout, $param->{forceStop});
-	};
+		my $conf = PVE::LXC::Config->load_config($vmid);
+		PVE::LXC::Config->check_lock($conf);
 
-	my $lockcmd = sub {
-	    return $rpcenv->fork_worker('vzshutdown', $vmid, $authuser, $realcmd);
+		PVE::LXC::vm_stop($vmid, 0, $timeout, $param->{forceStop});
+	    });
 	};
 
-	return PVE::LXC::Config->lock_config($vmid, $lockcmd);
+	return $rpcenv->fork_worker('vzshutdown', $vmid, $authuser, $realcmd);
     }});
 
 __PACKAGE__->register_method({
@@ -388,20 +383,18 @@ __PACKAGE__->register_method({
 	my $realcmd = sub {
 	    my $upid = shift;
 
-	    syslog('info', "suspend CT $vmid: $upid\n");
-
-	    my $conf = PVE::LXC::Config->load_config($vmid);
-	    PVE::LXC::Config->check_lock($conf);
+	    PVE::LXC::Config->lock_config($vmid, sub {
+		syslog('info', "suspend CT $vmid: $upid\n");
 
-	    my $cmd = ['lxc-checkpoint', '-n', $vmid, '-s', '-D', '/var/lib/vz/dump'];
-	    run_command($cmd);
-	};
+		my $conf = PVE::LXC::Config->load_config($vmid);
+		PVE::LXC::Config->check_lock($conf);
 
-	my $lockcmd = sub {
-	    return $rpcenv->fork_worker('vzsuspend', $vmid, $authuser, $realcmd);
+		my $cmd = ['lxc-checkpoint', '-n', $vmid, '-s', '-D', '/var/lib/vz/dump'];
+		run_command($cmd);
+	    });
 	};
 
-	return PVE::LXC::Config->lock_config($vmid, $lockcmd);
+	return $rpcenv->fork_worker('vzsuspend', $vmid, $authuser, $realcmd);
     }});
 
 __PACKAGE__->register_method({
-- 
2.39.2

[pve-devel] [PATCH container v2 3/6] fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint

[Friedrich Weber]

The new `overrule-shutdown` parameter is boolean and defaults to 0. If
it is 1, all active `vzshutdown` tasks by the current user for the same
CT are aborted before attempting to stop the CT.

Passing `overrule-shutdown=1` is forbidden for HA resources.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    changes v1 -> v2:
    * move overrule code into worker, as suggested by Wolfgang
    * print to worker stdout instead of syslog

 src/PVE/API2/LXC/Status.pm | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index 7741374..33f449c 100644
--- a/src/PVE/API2/LXC/Status.pm
+++ b/src/PVE/API2/LXC/Status.pm
@@ -220,6 +220,12 @@ __PACKAGE__->register_method({
 	    node => get_standard_option('pve-node'),
 	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
 	    skiplock => get_standard_option('skiplock'),
+	    'overrule-shutdown' => {
+		description => "Abort any active 'vzshutdown' task by the current user for this CT before stopping",
+		optional => 1,
+		type => 'boolean',
+		default => 0,
+	    }
 	},
     },
     returns => {
@@ -237,10 +243,15 @@ __PACKAGE__->register_method({
 	raise_param_exc({ skiplock => "Only root may use this option." })
 	    if $skiplock && $authuser ne 'root@pam';
 
+	my $overrule_shutdown = extract_param($param, 'overrule-shutdown');
+
 	die "CT $vmid not running\n" if !PVE::LXC::check_running($vmid);
 
 	if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
 
+	    raise_param_exc({ 'overrule-shutdown' => "Not applicable for HA resources." })
+		if $overrule_shutdown;
+
 	    my $hacmd = sub {
 		my $upid = shift;
 
@@ -257,6 +268,13 @@ __PACKAGE__->register_method({
 	    my $realcmd = sub {
 		my $upid = shift;
 
+		if ($overrule_shutdown) {
+		    my $overruled_tasks = PVE::GuestHelpers::overrule_tasks(
+			'vzshutdown', $authuser, $vmid);
+		    my $overruled_tasks_list = join(", ", $overruled_tasks->@*);
+		    print "overruled vzshutdown tasks: $overruled_tasks_list\n";
+		};
+
 		PVE::LXC::Config->lock_config($vmid, sub {
 		    syslog('info', "stopping CT $vmid: $upid\n");
 
-- 
2.39.2

[pve-devel] [PATCH qemu-server v2 4/6] fix #4474: qemu api: add overrule-shutdown parameter to stop endpoint

[Friedrich Weber]

The new `overrule-shutdown` parameter is boolean and defaults to 0. If
it is 1, all active `qmshutdown` tasks by the current user for the same
VM are aborted before attempting to stop the VM.

Passing `overrule-shutdown=1` is forbidden for HA resources.

Acked-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    no changes v1 -> v2

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

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index cdc8f7a..e6a7657 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -2964,7 +2964,13 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		optional => 1,
 		default => 0,
-	    }
+	    },
+	    'overrule-shutdown' => {
+		description => "Abort any active 'qmshutdown' task by the current user for this VM before stopping",
+		optional => 1,
+		type => 'boolean',
+		default => 0,
+	    },
 	},
     },
     returns => {
@@ -2991,10 +2997,13 @@ __PACKAGE__->register_method({
 	raise_param_exc({ migratedfrom => "Only root may use this option." })
 	    if $migratedfrom && $authuser ne 'root@pam';
 
+	my $overrule_shutdown = extract_param($param, 'overrule-shutdown');
 
 	my $storecfg = PVE::Storage::config();
 
 	if (PVE::HA::Config::vm_is_ha_managed($vmid) && ($rpcenv->{type} ne 'ha') && !defined($migratedfrom)) {
+	    raise_param_exc({ 'overrule-shutdown' => "Not applicable for HA resources." })
+		if $overrule_shutdown;
 
 	    my $hacmd = sub {
 		my $upid = shift;
@@ -3014,6 +3023,11 @@ __PACKAGE__->register_method({
 
 		syslog('info', "stop VM $vmid: $upid\n");
 
+		if ($overrule_shutdown) {
+		    my $overruled_tasks = PVE::GuestHelpers::overrule_tasks('qmshutdown', $authuser, $vmid);
+		    print "overruled qmshutdown tasks: " . join(", ", $overruled_tasks->@*) . "\n";
+		};
+
 		PVE::QemuServer::vm_stop($storecfg, $vmid, $skiplock, 0,
 					 $param->{timeout}, 0, 1, $keepActive, $migratedfrom);
 		return;
-- 
2.39.2

[pve-devel] [PATCH manager v2 5/6] ui: fix typo to make pve-cluster-tasks store globally available

[Friedrich Weber]

This way, it can be used to retrieve the current list of tasks.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    new in v2:
    * moved fix for pve-cluster-tasks store into its own patch

 www/manager6/dc/Tasks.js | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/www/manager6/dc/Tasks.js b/www/manager6/dc/Tasks.js
index 5344ede4..2001bf76 100644
--- a/www/manager6/dc/Tasks.js
+++ b/www/manager6/dc/Tasks.js
@@ -11,7 +11,7 @@ Ext.define('PVE.dc.Tasks', {
 	let me = this;
 
 	let taskstore = Ext.create('Proxmox.data.UpdateStore', {
-	    storeid: 'pve-cluster-tasks',
+	    storeId: 'pve-cluster-tasks',
 	    model: 'proxmox-tasks',
 	    proxy: {
 		type: 'proxmox',
-- 
2.39.2

[pve-devel] [PATCH manager v2 6/6] fix #4474: ui: guest stop: offer to overrule active shutdown tasks

[Friedrich Weber]

Implement a new "guest stop" confirmation message box which first
checks if there is an active shutdown task for the same guest by the
logged-in user. If there is at least one, the dialog displays an
additional default-on checkbox for overruling active shutdown tasks.
If the user confirms and the checkbox is checked, the UI sends a guest
stop API request with the `overrule-shutdown` parameter set to 1. If
there are no active shutdown tasks, or the checkbox is unchecked, the
UI sends a guest stop API request without `overrule-shutdown`.

To avoid an additional API request for querying active shutdown tasks,
check the UI's current view of cluster tasks instead, which is fetched
from the `pve-cluster-tasks` store.

As the UI might hold an outdated task list, there are some
opportunities for races, e.g., the UI may miss a new shutdown task or
consider a shutdown task active even though it has already terminated.
These races either result in a surviving shutdown task that the user
still needs to abort manually, or a superfluous `override-shutdown=1`
parameter that does not actually abort any tasks. Since "stop
overrules shutdown" is merely a convenience feature, both outcomes
seem bearable.

The confirmation message box is now always marked as dangerous (with a
warning sign icon), whereas previously it was only marked dangerous if
the stop issued from the guest panel, but not when issued from the
resource tree command menu.

Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---

Notes:
    changes v1 -> v2:
    * instead of a second modal dialog that offers to overrule shutdown
      tasks, display an additional checkbox in the "guest stop"
      confirmation dialog if there is a running matching shutdown task
    * spin out pve-cluster-tasks store fix into its own patch

 www/manager6/Makefile            |  1 +
 www/manager6/lxc/CmdMenu.js      |  8 +++-
 www/manager6/lxc/Config.js       |  8 ++--
 www/manager6/qemu/CmdMenu.js     |  8 +++-
 www/manager6/qemu/Config.js      |  8 ++--
 www/manager6/window/GuestStop.js | 75 ++++++++++++++++++++++++++++++++
 6 files changed, 100 insertions(+), 8 deletions(-)
 create mode 100644 www/manager6/window/GuestStop.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index dc3c85b1..a7dc2022 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -131,6 +131,7 @@ JSSRC= 							\
 	window/ScheduleSimulator.js			\
 	window/Wizard.js				\
 	window/GuestDiskReassign.js				\
+	window/GuestStop.js				\
 	window/TreeSettingsEdit.js			\
 	window/PCIMapEdit.js				\
 	window/USBMapEdit.js				\
diff --git a/www/manager6/lxc/CmdMenu.js b/www/manager6/lxc/CmdMenu.js
index b1403fc6..76b39423 100644
--- a/www/manager6/lxc/CmdMenu.js
+++ b/www/manager6/lxc/CmdMenu.js
@@ -66,7 +66,13 @@ Ext.define('PVE.lxc.CmdMenu', {
 		iconCls: 'fa fa-fw fa-stop',
 		disabled: stopped,
 		tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'CT'),
-		handler: () => confirmedVMCommand('stop'),
+		handler: () => {
+		    Ext.create('PVE.GuestStop', {
+			nodename: info.node,
+			vm: info,
+			autoShow: true,
+		    });
+		},
 	    },
 	    {
 		text: gettext('Reboot'),
diff --git a/www/manager6/lxc/Config.js b/www/manager6/lxc/Config.js
index 4516ee8f..082913e1 100644
--- a/www/manager6/lxc/Config.js
+++ b/www/manager6/lxc/Config.js
@@ -77,11 +77,13 @@ Ext.define('PVE.lxc.Config', {
 		{
 		    text: gettext('Stop'),
 		    disabled: !caps.vms['VM.PowerMgmt'],
-		    confirmMsg: Proxmox.Utils.format_task_description('vzstop', vmid),
 		    tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'CT'),
-		    dangerous: true,
 		    handler: function() {
-			vm_command("stop");
+			Ext.create('PVE.GuestStop', {
+			    nodename: nodename,
+			    vm: vm,
+			    autoShow: true,
+			});
 		    },
 		    iconCls: 'fa fa-stop',
 		}],
diff --git a/www/manager6/qemu/CmdMenu.js b/www/manager6/qemu/CmdMenu.js
index 4f59d5f7..834577e7 100644
--- a/www/manager6/qemu/CmdMenu.js
+++ b/www/manager6/qemu/CmdMenu.js
@@ -93,7 +93,13 @@ Ext.define('PVE.qemu.CmdMenu', {
 		iconCls: 'fa fa-fw fa-stop',
 		disabled: stopped,
 		tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'VM'),
-		handler: () => confirmedVMCommand('stop'),
+		handler: () => {
+		    Ext.create('PVE.GuestStop', {
+			nodename: info.node,
+			vm: info,
+			autoShow: true,
+		    });
+		},
 	    },
 	    {
 		text: gettext('Reboot'),
diff --git a/www/manager6/qemu/Config.js b/www/manager6/qemu/Config.js
index 2f9605a4..f28ee67b 100644
--- a/www/manager6/qemu/Config.js
+++ b/www/manager6/qemu/Config.js
@@ -176,11 +176,13 @@ Ext.define('PVE.qemu.Config', {
 		}, {
 		    text: gettext('Stop'),
 		    disabled: !caps.vms['VM.PowerMgmt'],
-		    dangerous: true,
 		    tooltip: Ext.String.format(gettext('Stop {0} immediately'), 'VM'),
-		    confirmMsg: Proxmox.Utils.format_task_description('qmstop', vmid),
 		    handler: function() {
-			vm_command("stop", { timeout: 30 });
+			Ext.create('PVE.GuestStop', {
+			    nodename: nodename,
+			    vm: vm,
+			    autoShow: true,
+			});
 		    },
 		    iconCls: 'fa fa-stop',
 		}, {
diff --git a/www/manager6/window/GuestStop.js b/www/manager6/window/GuestStop.js
new file mode 100644
index 00000000..1fcba17c
--- /dev/null
+++ b/www/manager6/window/GuestStop.js
@@ -0,0 +1,75 @@
+Ext.define('PVE.GuestStop', {
+    extend: 'Ext.window.MessageBox',
+
+    closeAction: 'destroy',
+
+    initComponent: function() {
+	let me = this;
+
+	if (!me.nodename) {
+	    throw "no node name specified";
+	}
+	if (!me.vm) {
+	    throw "no vm specified";
+	}
+
+	let vmid = me.vm.vmid;
+	let vmidString = vmid.toString();
+	let guestType = me.vm.type;
+	let overruleTaskType = guestType === 'qemu' ? 'qmshutdown' : 'vzshutdown';
+
+	me.taskType = guestType === 'qemu' ? 'qmstop' : 'vzstop';
+	me.url = `/nodes/${me.nodename}/${guestType}/${vmid}/status/stop`;
+
+	// offer to overrule if there is at least one matching shutdown task
+	// and the guest is not HA-enabled
+	let shutdownTaskIdx = Ext.getStore('pve-cluster-tasks')?.findBy(task =>
+	    task.data.user === Proxmox.UserName &&
+	    task.data.id === vmidString &&
+	    task.data.status === undefined &&
+	    task.data.type === overruleTaskType,
+	);
+	let haEnabled = me.vm.hastate && me.vm.hastate !== 'unmanaged';
+	me.askOverrule = !haEnabled && shutdownTaskIdx >= 0;
+
+	me.callParent();
+
+	me.promptContainer.add({
+	    xtype: 'proxmoxcheckbox',
+	    name: 'overrule-shutdown',
+	    checked: true,
+	    boxLabel: gettext('Overrule active shutdown tasks'),
+	    hidden: !me.askOverrule,
+	});
+    },
+
+    handler: function(btn) {
+	let me = this;
+	if (btn === 'yes') {
+	    let checkbox = me.promptContainer.down('proxmoxcheckbox[name=overrule-shutdown]');
+	    let overruleShutdown = me.askOverrule && checkbox.getSubmitValue();
+	    let params = overruleShutdown ? { 'overrule-shutdown': 1 } : undefined;
+	    Proxmox.Utils.API2Request({
+		url: me.url,
+		waitMsgTarget: me,
+		method: 'POST',
+		params: params,
+		failure: function(response, opts) {
+		    Ext.Msg.alert('Error', response.htmlStatus);
+		},
+	    });
+	}
+    },
+
+    show: function() {
+	let me = this;
+	let cfg = {
+	    title: gettext('Confirm'),
+	    icon: Ext.Msg.WARNING,
+	    msg: Proxmox.Utils.format_task_description(me.taskType, this.vm.vmid),
+	    buttons: Ext.Msg.YESNO,
+	    callback: btn => me.handler(btn),
+	};
+	me.callParent([cfg]);
+    },
+});
-- 
2.39.2

Re: [pve-devel] [PATCH guest-common/container/qemu-server/manager v2 0/6] fix #4474: stop tasks may overrule shutdown tasks

[Friedrich Weber]

ping -- still applies.

On 30/01/2024 18:10, Friedrich Weber wrote:
> As reported in #4474 [0], a user may attempt to shutdown a VM/CT,
> realize that it is unresponsive, and decide to stop it instead. If the
> shutdown task has not timed out yet, the stop task will fail. The user
> needs to manually abort the shutdown task before stopping the VM,
> which is inconvenient.
> 
> With this patch series, a stop task can optionally overrule active
> shutdown tasks.
> 
> Summary of changes:
> 
> * Backend:
> ** In the LXC status API handlers, we move config locking from the API
>    handlers into workers to avoid blocking, as suggested by Wolfgang on v1.
> ** We add a new optional `overrule-shutdown` parameter to
>    VM/CT `/stop` endpoints. If it is 1, we query for active
>    `qmshutdown`/`vzshutdown` tasks by the current user for the given
>    VMID, and abort them before attempting to stop the VM/CT.
> * Frontend: Issuing a stop command shows a confirmation message box that, if an
>   active shutdown task for that guest is detected, shows a default-on checkbox
>   that offers to overrule active shutdown tasks. If it is checked, the API
>   request is sent with `overrule-shutdown=1`.
> 
> Tested with a hung CentOS 7 container and a SeaBIOS VM without a bootable disk.
> 
> Changes v1 -> v2:
> * fixed patch ordering, as suggested by Wolfgang on v1
> * added a pve-container patch that moves config locking from API handlers
>   into workers for start/stop/shutdown/suspend, as suggested by Wolfgang on v1
> * instead of showing a second modal dialog when stopping a guest, show a "guest
>   stop" message box that has an optional checkbox offering to overrule shutdown
>   tasks
> * split pve-manager patch in two
> 
> [0]: https://bugzilla.proxmox.com/show_bug.cgi?id=4474
> 
> guest-common:
> 
> Friedrich Weber (1):
>   guest helpers: add helper to overrule active tasks of a specific type
> 
>  src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> 
> container:
> 
> Friedrich Weber (2):
>   api: status: move config locking from API handler into worker
>   fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint
> 
>  src/PVE/API2/LXC/Status.pm | 107 ++++++++++++++++++++-----------------
>  1 file changed, 59 insertions(+), 48 deletions(-)
> 
> 
> qemu-server:
> 
> Friedrich Weber (1):
>   fix #4474: qemu api: add overrule-shutdown parameter to stop endpoint
> 
>  PVE/API2/Qemu.pm | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> 
> manager:
> 
> Friedrich Weber (2):
>   ui: fix typo to make pve-cluster-tasks store globally available
>   fix #4474: ui: guest stop: offer to overrule active shutdown tasks
> 
>  www/manager6/Makefile            |  1 +
>  www/manager6/dc/Tasks.js         |  2 +-
>  www/manager6/lxc/CmdMenu.js      |  8 +++-
>  www/manager6/lxc/Config.js       |  8 ++--
>  www/manager6/qemu/CmdMenu.js     |  8 +++-
>  www/manager6/qemu/Config.js      |  8 ++--
>  www/manager6/window/GuestStop.js | 75 ++++++++++++++++++++++++++++++++
>  7 files changed, 101 insertions(+), 9 deletions(-)
>  create mode 100644 www/manager6/window/GuestStop.js
> 
> 
> Summary over all repositories:
>   10 files changed, 193 insertions(+), 0 deletions(-)
> 

Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Thomas Lamprecht]

Am 30/01/2024 um 18:10 schrieb Friedrich Weber:

Maybe start of with "Add a helper to abort all tasks from a specific
(type, user, vmid) tuple. It will be used ...

> This helper is used to abort any active qmshutdown/vzshutdown tasks
> before attempting to stop a VM/CT (if requested).
>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     no changes v1 -> v2
> 
>  src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
> index 961a7b8..bd94ed2 100644
> --- a/src/PVE/GuestHelpers.pm
> +++ b/src/PVE/GuestHelpers.pm
> @@ -416,4 +416,22 @@ sub check_vnet_access {
>  	if !($tag || $trunks);
>  }
>  
> +sub overrule_tasks {

hmm, overruling is the thing you want to do now, but one might
be use this for other reasons, so maybe naming it about what it
does would be a bit better compared to what is used for (now).

From top of my head that could be "abort_guest_tasks", but maybe
you got a better idea.

> +    my ($type, $user, $vmid) = @_;
> +
> +    my $active_tasks = PVE::INotify::read_file('active');
> +    my $res = [];
> +    for my $task (@$active_tasks) {
> +       if (!$task->{saved}
> +           && $task->{type} eq $type
> +           && $task->{user} eq $user

This also means that e.g. root@pam cannot overrule a task started by
apprentice@pam, which might be something admins what to do (they like
overruling users after all ;-P). Or some automation triggering the
shutdown (using a token with a separate user) might be also a good
examples of things I'd like to be able to overrule. 

Maybe make this optional and only enforce it for users that do not
have some more powerful priv?

Or does it even make sense to check this at all?
As long as the user has the rights to execute a stop they probably
should also be able to force it at any time, even for other users?

> +           && $task->{id} eq $vmid
> +       ) {

meh, the pre-existing $killit param is way too subtle for my taste...
Some %param that takes a `kill => "reason"` property for this would be
much more telling.

But changing that is a bit out of scope, a comment would be great for
now though.

> +           PVE::RPCEnvironment->check_worker($task->{upid}, 1);
> +           push @$res, $task->{upid};

renaming $res to $killed_tasks would also help in reading this code out
of further context.

[pve-devel] applied: [PATCH container v2 2/6] api: status: move config locking from API handler into worker

[Thomas Lamprecht]

Am 30/01/2024 um 18:10 schrieb Friedrich Weber:
> Previously, container start/stop/shutdown/suspend would try to acquire
> the config lock in the API handler prior to forking a worker. If the
> lock was currently held elsewhere, this would block the API handler
> and thus the pvedaemon worker thread until the 10s timeout expired (or
> the lock could be acquired).
> 
> To avoid blocking the API handler, immediately fork off a worker
> process and try to acquire the config lock in that worker.
> 
> Patch best viewed with `git show -w`.
> 
> Suggested-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     The diff is somewhat messy without `-w` -- couldn't come up with a
>     better way.
>     
>     new in v2
> 
>  src/PVE/API2/LXC/Status.pm | 91 ++++++++++++++++++--------------------
>  1 file changed, 42 insertions(+), 49 deletions(-)
> 
>

applied this one already, thanks!

btw. in general getting some early timeout sent as response to the initial
request would be good, but blocking the daemon worker is naturally a no-go.
And bringing in Anyevent somehow to make it async-like might be a PITA, so
for now this change is OK.

Oh, and it might be worth mentioning explicitly in the next release notes,
as it's a change in behavior that could theoretically throw up some
tooling that depends on the $action not failing due to locking if the
adapted endpoints returned – albeit IMO a bit silly to assume that (as the
actions can fail in other ways inside the worker anyway).

Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Friedrich Weber]

Thanks for the review!

On 04/04/2024 17:20, Thomas Lamprecht wrote:
> Am 30/01/2024 um 18:10 schrieb Friedrich Weber:
> 
> Maybe start of with "Add a helper to abort all tasks from a specific
> (type, user, vmid) tuple. It will be used ...

Will do.

>> This helper is used to abort any active qmshutdown/vzshutdown tasks
>> before attempting to stop a VM/CT (if requested).
>>
>> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
>> ---
>>
>> Notes:
>>     no changes v1 -> v2
>>
>>  src/PVE/GuestHelpers.pm | 18 ++++++++++++++++++
>>  1 file changed, 18 insertions(+)
>>
>> diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
>> index 961a7b8..bd94ed2 100644
>> --- a/src/PVE/GuestHelpers.pm
>> +++ b/src/PVE/GuestHelpers.pm
>> @@ -416,4 +416,22 @@ sub check_vnet_access {
>>  	if !($tag || $trunks);
>>  }
>>  
>> +sub overrule_tasks {
> 
> hmm, overruling is the thing you want to do now, but one might
> be use this for other reasons, so maybe naming it about what it
> does would be a bit better compared to what is used for (now).
> 
> From top of my head that could be "abort_guest_tasks", but maybe
> you got a better idea.

Yeah, that's true, I'll rename it to `abort_guest_tasks` or some variation.

>> +    my ($type, $user, $vmid) = @_;
>> +
>> +    my $active_tasks = PVE::INotify::read_file('active');
>> +    my $res = [];
>> +    for my $task (@$active_tasks) {
>> +       if (!$task->{saved}
>> +           && $task->{type} eq $type
>> +           && $task->{user} eq $user
> 
> This also means that e.g. root@pam cannot overrule a task started by
> apprentice@pam, which might be something admins what to do (they like
> overruling users after all ;-P). Or some automation triggering the
> shutdown (using a token with a separate user) might be also a good
> examples of things I'd like to be able to overrule. 

Good point. I see the usecase for being able to override other user's
tasks. My original motivation for making the user check that tight was
to avoid privilege escalation.

> Or does it even make sense to check this at all?
> As long as the user has the rights to execute a stop they probably
> should also be able to force it at any time, even for other users?

The usecase sounds reasonable, but would technically amount to a small
privilege escalation: Currently, if user A and user B both have
PVEVMUser and user A starts a `vzshutdown` task, user B must have
`Sys.Modify` to kill the task.

> Maybe make this optional and only enforce it for users that do not
> have some more powerful priv?

We could introduce a similar check as for the DELETE
/nodes/{node}/tasks/{upid} endpoint: Users can always overrule their own
guest tasks, and with `Sys.Modify` they can overrule any guest task (for
guests for which they have the necessary permission).

Still, right now I think the primary motivation for this overruling
feature is to save GUI users some frustration and/or clicks. In this
scenario, a user will overrule only their own tasks, which is possible
with the current check. What do you think about keeping the check as it
is for now, and making it more permissive once the need arises?

>> +           && $task->{id} eq $vmid
>> +       ) {
> 
> meh, the pre-existing $killit param is way too subtle for my taste...
> Some %param that takes a `kill => "reason"` property for this would be
> much more telling.
> 
> But changing that is a bit out of scope, a comment would be great for
> now though.

Makes sense, will add a comment.

> 
>> +           PVE::RPCEnvironment->check_worker($task->{upid}, 1);
>> +           push @$res, $task->{upid};
> 
> renaming $res to $killed_tasks would also help in reading this code out
> of further context.

Makes sense, will rename $res.

Re: [pve-devel] applied: [PATCH container v2 2/6] api: status: move config locking from API handler into worker

[Friedrich Weber]

On 04/04/2024 17:26, Thomas Lamprecht wrote:
> Oh, and it might be worth mentioning explicitly in the next release notes,
> as it's a change in behavior that could theoretically throw up some
> tooling that depends on the $action not failing due to locking if the
> adapted endpoints returned – albeit IMO a bit silly to assume that (as the
> actions can fail in other ways inside the worker anyway).

True, I've made a note to mention this in the release notes.

Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Thomas Lamprecht]

Am 05/04/2024 um 15:13 schrieb Friedrich Weber:
> On 04/04/2024 17:20, Thomas Lamprecht wrote:
>> Or does it even make sense to check this at all?
>> As long as the user has the rights to execute a stop they probably
>> should also be able to force it at any time, even for other users?
> 
> The usecase sounds reasonable, but would technically amount to a small
> privilege escalation: Currently, if user A and user B both have
> PVEVMUser and user A starts a `vzshutdown` task, user B must have
> `Sys.Modify` to kill the task.
> 

Hmm, yeah, this is definitively something one could argue about as
we currently do not have much overruling functionality of tasks triggered
by other users that can manage a specific resource, but not the whole
system. So this is essential new territory w.r.t. semantics, and we can
also adapt new ones, albeit they should certainly not be completely
unexpected.

>> Maybe make this optional and only enforce it for users that do not
>> have some more powerful priv?
> 
> We could introduce a similar check as for the DELETE
> /nodes/{node}/tasks/{upid} endpoint: Users can always overrule their own
> guest tasks, and with `Sys.Modify` they can overrule any guest task (for
> guests for which they have the necessary permission).

Yeah, I think that would be a safe bet for now.

> Still, right now I think the primary motivation for this overruling
> feature is to save GUI users some frustration and/or clicks. In this
> scenario, a user will overrule only their own tasks, which is possible
> with the current check. What do you think about keeping the check as it
> is for now, and making it more permissive once the need arises?

I think that allowing users that hold the respective Sys.Modify and
VM.PowerMgmt to overrule any tasks from the start wouldn't be to much
"speculative future-proofing" but rather something expected while still
safe.

FWIW, you could also drop the $authuser then and just get it from
the RPCEnv singleton available in all API call-paths and then do
the permission check in the helper directly.
This would IMO be also a bit better w.r.t. conveying why we do it this
way.

Re: [pve-devel] [PATCH container v2 3/6] fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint

[Thomas Lamprecht]

Am 30/01/2024 um 18:10 schrieb Friedrich Weber:
> The new `overrule-shutdown` parameter is boolean and defaults to 0. If
> it is 1, all active `vzshutdown` tasks by the current user for the same
> CT are aborted before attempting to stop the CT.
> 
> Passing `overrule-shutdown=1` is forbidden for HA resources.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     changes v1 -> v2:
>     * move overrule code into worker, as suggested by Wolfgang
>     * print to worker stdout instead of syslog
> 
>  src/PVE/API2/LXC/Status.pm | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
> index 7741374..33f449c 100644
> --- a/src/PVE/API2/LXC/Status.pm
> +++ b/src/PVE/API2/LXC/Status.pm
> @@ -220,6 +220,12 @@ __PACKAGE__->register_method({
>  	    node => get_standard_option('pve-node'),
>  	    vmid => get_standard_option('pve-vmid', { completion => \&PVE::LXC::complete_ctid_running }),
>  	    skiplock => get_standard_option('skiplock'),
> +	    'overrule-shutdown' => {
> +		description => "Abort any active 'vzshutdown' task by the current user for this CT before stopping",
> +		optional => 1,
> +		type => 'boolean',
> +		default => 0,
> +	    }
>  	},
>      },
>      returns => {
> @@ -237,10 +243,15 @@ __PACKAGE__->register_method({
>  	raise_param_exc({ skiplock => "Only root may use this option." })
>  	    if $skiplock && $authuser ne 'root@pam';
>  
> +	my $overrule_shutdown = extract_param($param, 'overrule-shutdown');
> +
>  	die "CT $vmid not running\n" if !PVE::LXC::check_running($vmid);
>  
>  	if (PVE::HA::Config::vm_is_ha_managed($vmid) && $rpcenv->{type} ne 'ha') {
>  
> +	    raise_param_exc({ 'overrule-shutdown' => "Not applicable for HA resources." })
> +		if $overrule_shutdown;

This probably could be implemented through a CRM command and would be nice
to have in the long-run. The more HA and non-HA resources can be managed
the same the better, as otherwise it's causing friction to use HA.

The same holds for the qemu-server patch.

But, I'm fine with doing this separately later on, as this on its own is
already an improvement for quite a few users.

Re: [pve-devel] [PATCH guest-common v2 1/6] guest helpers: add helper to overrule active tasks of a specific type

[Friedrich Weber]

On 06/04/2024 10:37, Thomas Lamprecht wrote:
>> Still, right now I think the primary motivation for this overruling
>> feature is to save GUI users some frustration and/or clicks. In this
>> scenario, a user will overrule only their own tasks, which is possible
>> with the current check. What do you think about keeping the check as it
>> is for now, and making it more permissive once the need arises?
> 
> I think that allowing users that hold the respective Sys.Modify and
> VM.PowerMgmt to overrule any tasks from the start wouldn't be to much
> "speculative future-proofing" but rather something expected while still
> safe.

Makes sense.

> FWIW, you could also drop the $authuser then and just get it from
> the RPCEnv singleton available in all API call-paths and then do
> the permission check in the helper directly.
> This would IMO be also a bit better w.r.t. conveying why we do it this
> way.

OK, sounds good! I'll send a v3 then.

Re: [pve-devel] [PATCH container v2 3/6] fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint

[Friedrich Weber]

On 06/04/2024 17:07, Thomas Lamprecht wrote:
> Am 30/01/2024 um 18:10 schrieb Friedrich Weber:
> [...]
>> +	    raise_param_exc({ 'overrule-shutdown' => "Not applicable for HA resources." })
>> +		if $overrule_shutdown;
> 
> This probably could be implemented through a CRM command and would be nice
> to have in the long-run. The more HA and non-HA resources can be managed
> the same the better, as otherwise it's causing friction to use HA.
> 
> The same holds for the qemu-server patch.
> 
> But, I'm fine with doing this separately later on, as this on its own is
> already an improvement for quite a few users.

OK, sounds good. Then I'll keep v3 limited to non-HA guests. I'll either
keep the original bug #4474 [1] open and comment that overruling is not
yet implemented for HA resources, or open a new bug.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4474