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

[Dominik Csapak <d.csapak@proxmox.com>]

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