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

[pve-devel] [PATCH guest-common/container/qemu-server/manager v3 0/5] 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: We add a new optional `overrule-shutdown` parameter to VM/CT `/stop`
  endpoints. If it is 1, we query for active `qmshutdown`/`vzshutdown` tasks for
  the given VMID that are visible for the user, and abort them before attempting
  to stop the VM/CT. Users/tokens with `Sys.Modify` can abort any guest task,
  users can abort their own guest tasks (including the ones by their tokens),
  and tokens can only abort their own guest tasks.
- 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 v2 -> v3:
- dropped pve-container patch that was already applied in v2
- incorporated suggestions by Thomas, see individual patches for details

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 abort active guest tasks of a certain
    type

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


container:

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

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


qemu-server:

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

 PVE/API2/Qemu.pm | 19 ++++++++++++++++++-
 1 file changed, 18 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 | 79 ++++++++++++++++++++++++++++++++
 7 files changed, 105 insertions(+), 9 deletions(-)
 create mode 100644 www/manager6/window/GuestStop.js


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

-- 
Generated by git-murpp 0.5.0

[pve-devel] [PATCH guest-common v3 1/5] guest helpers: add helper to abort active guest tasks of a certain type

[Friedrich Weber]

Given a `(type, user, vmid)` tuple, the helper aborts all tasks of the
given `type` for guest `vmid` that `user` is allowed to abort:

- If `user` has `Sys.Modify` on the node, they can abort any task
- If `user` is an API token, it can abort any task it started itself
- If `user` is a user, they can abort any task started by themselves
  or one of their API tokens.

The helper is used to overrule any active qmshutdown/vzshutdown tasks
when attempting to stop a VM/CT (if requested).

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

Notes:
    As the computation of `$can_abort_task` essentially duplicates logic
    from PVE/API2/Tasks.pm, I considered reusing that, but this would have
    required moving it to one of the dependencies of pve-guest-common
    (Thomas suggested pve-access-control off-list). Seeing that the logic
    boils down to 4 lines in `abort_guest_tasks`, I didn't consider it
    worth the trouble in the end. Happy to reconsider, though.
    
    changes v2 -> v3:
    - improved readability: renamed subroutine to describe what it does,
      renamed return value, added comment, clarified commit message (thx
      Thomas)
    - better align logic with current permission model for stopping tasks:
      - allow users with Sys.Modify to abort *any* task (thx Thomas)
      - allow users to abort tasks of their tokens
    
    no changes v1 -> v2

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

diff --git a/src/PVE/GuestHelpers.pm b/src/PVE/GuestHelpers.pm
index 961a7b8..c9fe147 100644
--- a/src/PVE/GuestHelpers.pm
+++ b/src/PVE/GuestHelpers.pm
@@ -416,4 +416,39 @@ sub check_vnet_access {
 	if !($tag || $trunks);
 }
 
+sub abort_guest_tasks {
+    my ($rpcenv, $type, $vmid) = @_;
+
+    my $authuser = $rpcenv->get_user();
+    my $node = PVE::INotify::nodename();
+    my $can_abort_all = $rpcenv->check($authuser, "/nodes/$node", [ 'Sys.Modify' ], 1);
+
+    my $active_tasks = PVE::INotify::read_file('active');
+    my $aborted_tasks = [];
+    for my $task (@$active_tasks) {
+	if (!$task->{saved}
+	    && $task->{type} eq $type
+	    && $task->{id} eq $vmid
+	) {
+	    my $can_abort_task;
+	    # tasks started by a token can be aborted by the token or token owner,
+	    # tasks started by a user can be aborted by the user
+	    if (PVE::AccessControl::pve_verify_tokenid($task->{user}, 1)) {
+		my $full_tokenid = $task->{user};
+		my ($task_username, undef) = PVE::AccessControl::split_tokenid($full_tokenid);
+		$can_abort_task = $authuser eq $task_username || $authuser eq $full_tokenid;
+	    } else {
+		$can_abort_task = $authuser eq $task->{user};
+	    }
+
+	    if ($can_abort_all || $can_abort_task) {
+	       # passing `1` for parameter $killit aborts the task
+	       PVE::RPCEnvironment->check_worker($task->{upid}, 1);
+	       push @$aborted_tasks, $task->{upid};
+	   }
+	}
+    }
+    return $aborted_tasks;
+}
+
 1;
-- 
2.39.2

[pve-devel] [PATCH container v3 2/5] 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 for the same CT (which are
visible to the user/token) 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 v2 -> v3:
    - avoid printing empty list of overruled tasks
    - rephrased parameter description/commit to reflect changed logic
    - adapt to rename to `abort_guest_tasks`
    
    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 | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/src/PVE/API2/LXC/Status.pm b/src/PVE/API2/LXC/Status.pm
index 7741374..08e23b6 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 and visible 'vzshutdown' tasks 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,14 @@ __PACKAGE__->register_method({
 	    my $realcmd = sub {
 		my $upid = shift;
 
+		if ($overrule_shutdown) {
+		    my $overruled_tasks = PVE::GuestHelpers::abort_guest_tasks(
+			$rpcenv, 'vzshutdown', $vmid);
+		    my $overruled_tasks_list = join(", ", $overruled_tasks->@*);
+		    print "overruled vzshutdown tasks: $overruled_tasks_list\n"
+			if @$overruled_tasks;
+		};
+
 		PVE::LXC::Config->lock_config($vmid, sub {
 		    syslog('info', "stopping CT $vmid: $upid\n");
 
-- 
2.39.2

[pve-devel] [PATCH qemu-server v3 3/5] 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 for the same VM (which are
visible to the user/token) are aborted before attempting to stop the
VM.

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

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

Notes:
    changes v2 -> v3:
    - broke over-long lines
    - avoid printing empty list of overruled tasks
    - removed Wolfgang's Acked-by because patch changed
    - rephrased parameter description/commit to reflect changed logic
    - adapt to rename to `abort_guest_tasks`
    
    no changes v1 -> v2

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

diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
index 497987f..c18b6bd 100644
--- a/PVE/API2/Qemu.pm
+++ b/PVE/API2/Qemu.pm
@@ -3034,7 +3034,13 @@ __PACKAGE__->register_method({
 		type => 'boolean',
 		optional => 1,
 		default => 0,
-	    }
+	    },
+	    'overrule-shutdown' => {
+		description => "Abort any active and visible 'qmshutdown' tasks before stopping",
+		optional => 1,
+		type => 'boolean',
+		default => 0,
+	    },
 	},
     },
     returns => {
@@ -3061,10 +3067,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;
@@ -3084,6 +3093,14 @@ __PACKAGE__->register_method({
 
 		syslog('info', "stop VM $vmid: $upid\n");
 
+		if ($overrule_shutdown) {
+		    my $overruled_tasks = PVE::GuestHelpers::abort_guest_tasks(
+			$rpcenv, 'qmshutdown', $vmid);
+		    my $overruled_tasks_list = join(", ", $overruled_tasks->@*);
+		    print "overruled qmshutdown tasks: $overruled_tasks_list\n"
+			if @$overruled_tasks;
+		};
+
 		PVE::QemuServer::vm_stop($storecfg, $vmid, $skiplock, 0,
 					 $param->{timeout}, 0, 1, $keepActive, $migratedfrom);
 		return;
-- 
2.39.2

[pve-devel] [PATCH manager v3 4/5] 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:
    changes v2 -> v3:
    * no changes
    
    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 v3 5/5] 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 that is
visible to 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 v2 -> v3:
    - aligned permission checks with changed backend logic
    - replace access of `this.vm` with `me.vm`
    
    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 | 79 ++++++++++++++++++++++++++++++++
 6 files changed, 104 insertions(+), 8 deletions(-)
 create mode 100644 www/manager6/window/GuestStop.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index c756cae6..30c56bb9 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..16d05a70
--- /dev/null
+++ b/www/manager6/window/GuestStop.js
@@ -0,0 +1,79 @@
+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`;
+
+	let caps = Ext.state.Manager.get('GuiCap');
+	let hasSysModify = caps.nodes['Sys.Modify'];
+
+	// offer to overrule if there is at least one matching shutdown task
+	// and the guest is not HA-enabled.
+	// allow users to abort tasks started by one of their API tokens
+	let shutdownTaskIdx = Ext.getStore('pve-cluster-tasks')?.findBy(task =>
+	    (hasSysModify || 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, me.vm.vmid),
+	    buttons: Ext.Msg.YESNO,
+	    callback: btn => me.handler(btn),
+	};
+	me.callParent([cfg]);
+    },
+});
-- 
2.39.2

[pve-devel] applied: [PATCH guest-common v3 1/5] guest helpers: add helper to abort active guest tasks of a certain type

[Thomas Lamprecht]

Am 12/04/2024 um 16:15 schrieb Friedrich Weber:
> Given a `(type, user, vmid)` tuple, the helper aborts all tasks of the
> given `type` for guest `vmid` that `user` is allowed to abort:
> 
> - If `user` has `Sys.Modify` on the node, they can abort any task
> - If `user` is an API token, it can abort any task it started itself
> - If `user` is a user, they can abort any task started by themselves
>   or one of their API tokens.
> 
> The helper is used to overrule any active qmshutdown/vzshutdown tasks
> when attempting to stop a VM/CT (if requested).
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     As the computation of `$can_abort_task` essentially duplicates logic
>     from PVE/API2/Tasks.pm, I considered reusing that, but this would have
>     required moving it to one of the dependencies of pve-guest-common
>     (Thomas suggested pve-access-control off-list). Seeing that the logic
>     boils down to 4 lines in `abort_guest_tasks`, I didn't consider it
>     worth the trouble in the end. Happy to reconsider, though.
>     
>     changes v2 -> v3:
>     - improved readability: renamed subroutine to describe what it does,
>       renamed return value, added comment, clarified commit message (thx
>       Thomas)
>     - better align logic with current permission model for stopping tasks:
>       - allow users with Sys.Modify to abort *any* task (thx Thomas)
>       - allow users to abort tasks of their tokens
>     
>     no changes v1 -> v2
> 
>  src/PVE/GuestHelpers.pm | 35 +++++++++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
>

applied, with some (very) tiny efficiency improvement as follow-up, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH container v3 2/5] fix #4474: lxc api: add overrule-shutdown parameter to stop endpoint

[Thomas Lamprecht]

Am 12/04/2024 um 16:15 schrieb Friedrich Weber:
> The new `overrule-shutdown` parameter is boolean and defaults to 0. If
> it is 1, all active `vzshutdown` tasks for the same CT (which are
> visible to the user/token) 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 v2 -> v3:
>     - avoid printing empty list of overruled tasks
>     - rephrased parameter description/commit to reflect changed logic
>     - adapt to rename to `abort_guest_tasks`
>     
>     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 | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
>

applied, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

[pve-devel] applied: [PATCH qemu-server v3 3/5] fix #4474: qemu api: add overrule-shutdown parameter to stop endpoint

[Thomas Lamprecht]

Am 12/04/2024 um 16:15 schrieb Friedrich Weber:
> The new `overrule-shutdown` parameter is boolean and defaults to 0. If
> it is 1, all active `qmshutdown` tasks for the same VM (which are
> visible to the user/token) are aborted before attempting to stop the
> VM.
> 
> Passing `overrule-shutdown=1` is forbidden for HA resources.
> 
> Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
> ---
> 
> Notes:
>     changes v2 -> v3:
>     - broke over-long lines
>     - avoid printing empty list of overruled tasks
>     - removed Wolfgang's Acked-by because patch changed
>     - rephrased parameter description/commit to reflect changed logic
>     - adapt to rename to `abort_guest_tasks`
>     
>     no changes v1 -> v2
> 
>  PVE/API2/Qemu.pm | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
>

applied, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Lamprecht]

Am 12/04/2024 um 16:15 schrieb 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:
>     changes v2 -> v3:
>     * no changes
>     
>     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(-)
> 
>

applied, thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Dominik Csapak]

some minor nits inline, aside from those

Reviewed-by: Dominik Csapak <d.csapak@proxmox.com>


On 4/12/24 16:15, Friedrich Weber wrote:
> Implement a new "guest stop" confirmation message box which first
> checks if there is an active shutdown task for the same guest that is
> visible to 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 v2 -> v3:
>      - aligned permission checks with changed backend logic
>      - replace access of `this.vm` with `me.vm`
>      
>      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 | 79 ++++++++++++++++++++++++++++++++
>   6 files changed, 104 insertions(+), 8 deletions(-)
>   create mode 100644 www/manager6/window/GuestStop.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index c756cae6..30c56bb9 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..16d05a70
> --- /dev/null
> +++ b/www/manager6/window/GuestStop.js
> @@ -0,0 +1,79 @@
> +Ext.define('PVE.GuestStop', {
> +    extend: 'Ext.window.MessageBox',
> +

nit: imho a short high level description why we extend the messagebox instead
of e.g. our edit/safedestroy window would be nice

also maybe we could rewrite this a bit more generic so that the safedestroy
window users could this instead? (not necessary for now though)

> +    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`;
> +
> +	let caps = Ext.state.Manager.get('GuiCap');
> +	let hasSysModify = caps.nodes['Sys.Modify'];
> +
> +	// offer to overrule if there is at least one matching shutdown task
> +	// and the guest is not HA-enabled.
> +	// allow users to abort tasks started by one of their API tokens
> +	let shutdownTaskIdx = Ext.getStore('pve-cluster-tasks')?.findBy(task =>
> +	    (hasSysModify || 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({

nit: i guess this comes from the messagebox class?
a short comment what it is would be nice

> +	    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,

nit: if the property and the variable have the same name you can simply write

params,

in the list.

> +		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, me.vm.vmid),
> +	    buttons: Ext.Msg.YESNO,
> +	    callback: btn => me.handler(btn),

if we'd want to make this generic, we'd want to wrap any given callback
and call it instead of overwriting it with our own handler

> +	};
> +	me.callParent([cfg]);
> +    },
> +});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Lamprecht]

Am 12/04/2024 um 16:15 schrieb Friedrich Weber:
> Implement a new "guest stop" confirmation message box which first
> checks if there is an active shutdown task for the same guest that is
> visible to 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 v2 -> v3:
>     - aligned permission checks with changed backend logic
>     - replace access of `this.vm` with `me.vm`
>     
>     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 | 79 ++++++++++++++++++++++++++++++++
>  6 files changed, 104 insertions(+), 8 deletions(-)
>  create mode 100644 www/manager6/window/GuestStop.js
> 
>

applied, with some smaller style nits squashed in directly and a follow-up
that changed this to be shown more often, mostly because the UI task state
can be dated (slightly more rationale in the commit message), thanks!


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel

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

[Thomas Lamprecht]

Am 19/04/2024 um 12:17 schrieb Dominik Csapak:
> nit: imho a short high level description why we extend the messagebox instead
> of e.g. our edit/safedestroy window would be nice
> 
> also maybe we could rewrite this a bit more generic so that the safedestroy
> window users could this instead? (not necessary for now though)
> 

Yeah, I'd do the following:
- create a new proxmoxPrompt / Proxmox.window.Prompt that is a generic variant
  of the SafeDestroy button, allowing to override the confirm button text, API
  call HTTP method, adding extra items both inside the message container and
  below that, providing basic plumbing to easily integrate extra validation
  to allow confirmation and infra to show (various) hints. As both are things
  that are quite common in prompts (at least should be) it's IMO worth it to
  have native support for that functionality directly in such a general widget
  even if a widget that extends from this could add it themselves.
 

- move over SafeDestroy to this new widget as base, it probably should be one
  or two dozens lines of (mostly declarative) code then.

FWIW, I started the first part here, but it's still pretty bare bones, and I'm
not sure if I get around to finish it, especially with good polishing, in the
near future so just come to me if you, or someone else, want's to continue on
this. IMO this would be something quite worth doing, as our prompts are pretty
rough, often lacking even conveying the basic implications of (destructive)
actions, and having a good component that makes it easy to show hints or add
extra (non-intrusive) confirmation validation could help here.


_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel