From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 2ABA61FF13A for ; Wed, 15 Apr 2026 14:33:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AEF2813634; Wed, 15 Apr 2026 14:33:11 +0200 (CEST) Message-ID: <900084a5-9466-4a98-b653-2c97fc863f38@proxmox.com> Date: Wed, 15 Apr 2026 14:32:36 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH manager v2] ui: ha: add disarm/re-arm button To: Dominik Rusovac , pve-devel@lists.proxmox.com References: <20260415064118.33290-1-d.rusovac@proxmox.com> Content-Language: en-US From: Dominik Csapak In-Reply-To: <20260415064118.33290-1-d.rusovac@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776256279272 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.048 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: G7Y4GK7XWYEGKFPUZQNAP2WFDERHMVTE X-Message-ID-Hash: G7Y4GK7XWYEGKFPUZQNAP2WFDERHMVTE X-MailFrom: d.csapak@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: what i miss with using these is some feedback when i activated them. This is probably more due to the backend decision, but when clicking arm/disarm, there is nothing happening in the gui at first, no spinning icon, no change in state.. I think there are a few possible solutions on this, but i'm not super deep in the ha stack, so sorry in advance if some can't work: * use a worker that waits for the change of the ha state, e.g. via polling. this could directly be shown in the gui * mark the requested state immediately somewhere and return it with the overall ha state, so we see in the gui what is happening * 'fake' the progress until we see a status change (i don't really like this, since it's prone to errors when e.g. one admin disarms, the other arms again, but the gui does not get an updated state in the meantime) other comments inline On 4/15/26 8:40 AM, Dominik Rusovac wrote: > The button to disarm HA in either of the resource modes ('freeze' or > 'ignore') is disabled as long as HA is disarmed. Analogously, the button > to arm HA is disabled as long as HA is not disarmed. > > The icons ('unlink' and 'link') are chosen to emphasize that "Disarm HA" > and "Arm HA" are complements. There may be more suitable pairs of icons > though. > > Signed-off-by: Dominik Rusovac > --- > changes since v1: > * use camel case for function names > * choose clearer parameter: 'comp' -> 'menuItem' > * choose clearer function names: > - 'disarm' -> 'handleDisarmButton' > - 'rearm' -> 'handleArmButton' > * deduplicate translation keys > * use same translation keys as for menu items > * remove obsolete 'params' > * change button text "Re-arm HA" to "Arm HA" > * remove extra new-line > > www/manager6/ha/Status.js | 104 ++++++++++++++++++++++++++++++++++ > www/manager6/ha/StatusView.js | 8 +++ > 2 files changed, 112 insertions(+) > > diff --git a/www/manager6/ha/Status.js b/www/manager6/ha/Status.js > index b0b0feb9..5c65d52d 100644 > --- a/www/manager6/ha/Status.js > +++ b/www/manager6/ha/Status.js > @@ -8,6 +8,105 @@ Ext.define('PVE.ha.Status', { > align: 'stretch', > }, > > + viewModel: { > + data: { > + haDisarmed: false, > + }, > + }, > + > + controller: { > + xclass: 'Ext.app.ViewController', > + > + checkHaStatus: function (isDisarmed) { > + let vm = this.getViewModel(); > + vm.set('haDisarmed', isDisarmed); > + }, > + this method does not 'check' it 'sets' the HA status? IMHO a better name would be: setHaStatus or similar also, this is only used once, so it's probably better to just call the vm.set inline > + handleDisarmButton: function (menuItem) { > + let me = this; > + let view = me.getView(); > + > + Ext.Msg.confirm( > + gettext('Confirm'), > + Ext.String.format( > + gettext("Are you sure you want to disarm HA with resource mode '{0}'?"), > + menuItem.text, > + ), > + function (btn) { > + if (btn !== 'yes') { > + return; > + } > + Proxmox.Utils.API2Request({ > + url: '/cluster/ha/status/disarm-ha', > + params: { 'resource-mode': menuItem.mode }, > + method: 'POST', > + waitMsgTarget: view, > + failure: (response) => Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + }); > + }, > + ); > + }, > + > + handleArmButton: function () { > + let me = this; > + let view = me.getView(); > + > + Ext.Msg.confirm( > + gettext('Confirm'), > + gettext('Are you sure you want to arm HA?'), > + function (btn) { > + if (btn !== 'yes') { > + return; > + } > + Proxmox.Utils.API2Request({ > + url: '/cluster/ha/status/arm-ha', > + method: 'POST', > + waitMsgTarget: view, > + failure: (response) => Ext.Msg.alert(gettext('Error'), response.htmlStatus), > + }); > + }, > + ); > + }, > + }, > + > + dockedItems: [ > + { > + xtype: 'toolbar', > + dock: 'top', > + items: [ it should be able to shorten that to just using 'tbar' tbar: [ { ... }, // button 1 { ... }, // button 2 ], > + { > + text: gettext('Disarm HA'), > + iconCls: 'fa fa-unlink', > + bind: { > + disabled: '{haDisarmed}', > + }, > + menu: [ > + { > + text: gettext('Freeze'), > + iconCls: 'fa fa-snowflake-o', > + mode: 'freeze', > + handler: 'handleDisarmButton', > + }, > + { > + text: gettext('Ignore'), > + iconCls: 'fa fa-eye-slash', > + mode: 'ignore', > + handler: 'handleDisarmButton', > + }, > + ], > + }, > + { > + text: gettext('Arm HA'), > + iconCls: 'fa fa-link', > + bind: { > + disabled: '{!haDisarmed}', > + }, > + handler: 'handleArmButton', > + }, > + ], i'm not totally against having two buttons here, but since there is always only one or the other active, wouldn't it make more sense to hide the button that can't do anything? (though i admit we do most often show the unusable buttons across our gui, so it's fine too) At least i would probably switch the position of these, since the more (for the lack of a better word) 'affirming' action is usually at the beginning (like 'add' or 'start') and the other options comes later (like 'remove' or 'shutdown') so | Arm | Disarm | reads more logical to me than | Disarm | Arm | > + }, > + ], > + > initComponent: function () { > var me = this; > > @@ -30,6 +129,11 @@ Ext.define('PVE.ha.Status', { > border: 0, > collapsible: true, > padding: '0 0 20 0', > + listeners: { > + hastatuschange: function (isDisarmed) { > + me.getController().checkHaStatus(isDisarmed); > + }, > + }, > }, > { > xtype: 'pveHAResourcesView', > diff --git a/www/manager6/ha/StatusView.js b/www/manager6/ha/StatusView.js > index bc2da71f..4cbe85a1 100644 > --- a/www/manager6/ha/StatusView.js > +++ b/www/manager6/ha/StatusView.js > @@ -42,6 +42,13 @@ Ext.define( > }, > }); > > + me.rstore.on('load', function () { > + let fencing = store.findRecord('type', 'fencing'); > + let disarmed = fencing && fencing.get('armed-state') === 'disarmed'; > + > + me.fireEvent('hastatuschange', disarmed); just a remark, not even a nit: the usual convention in extjs is that the first parameter of an event is the component itself, so it would usually be me.fireEvent('foo', me, ); we (and extjs itself) don't always adhere to that, so it's fine here, but wanted to note it anyway > + }); > + > Ext.apply(me, { > store: store, > stateful: false, > @@ -105,6 +112,7 @@ Ext.define( > return PVE.data.ResourceStore.guestName(vmid); > }, > }, > + 'armed-state', > ], > idProperty: 'id', > });