From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 2E9801FF380 for ; Fri, 19 Apr 2024 12:17:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D32B66022; Fri, 19 Apr 2024 12:17:50 +0200 (CEST) Message-ID: <07d9544b-bae5-4bfc-b837-63241274595f@proxmox.com> Date: Fri, 19 Apr 2024 12:17:15 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta To: Proxmox VE development discussion , Friedrich Weber References: <20240412141553.430554-1-f.weber@proxmox.com> <20240412141553.430554-6-f.weber@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20240412141553.430554-6-f.weber@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.015 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH manager v3 5/5] fix #4474: ui: guest stop: offer to overrule active shutdown tasks X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: Proxmox VE development discussion Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: pve-devel-bounces@lists.proxmox.com Sender: "pve-devel" some minor nits inline, aside from those Reviewed-by: Dominik Csapak 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 > --- > > 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