[pve-devel] [PATCH container/docs/ha-manager/manager/widget-toolkit/qemu-server v3 00/11] fix #6613: update HA rules upon resource deletion

[pve-devel] [PATCH container/docs/ha-manager/manager/widget-toolkit/qemu-server v3 00/11] fix #6613: update HA rules upon resource deletion

[Michael Köppl]

This patch series aims to fix #6613 [0]. Although an implementation was
proposed in the past, it was not applied since it was unclear how to
handle the case where the removed resource is the last resource in a
rule.

The approach in this patch series is the following:
- With the purge param activated (default), the
  resource is removed from any rule referencing it.
- Without the purge param, rules are left untouched, making the removal
  of resources from rules "opt-out", as was suggested in the previous
  implementation [1].
- If the resource was the last resource in the rule *and* the purge
  param is set, the rule will also be deleted.
- For guest destruction, the "Purge from job configurations" option will
  also set the purge parameter for removing the HA resource
- The purge param is set by default for resource removal in the GUI and
  CLI

After some back-and-forth, I settled in this approach as the most
intuitive and transparent. Alternative approaches considered were the
following:
- Updating rules even without the purge flag set, but failing and
  showing an error to the user if a rule would be deleted, prompting the
  user to enable the purge flag. This approach seems to be to
  restrictive and IMO doesn't add any UX value.
- By default not setting the purge flag. This would by default leave
  "zombie" rules with non-existent resources. This is IMO intransparent
  if it happens by default. Users might be removing resources, not even
  knowing they're leaving behind rules that still reference said
  resources.

Changes since v2:
- Rebase on latest master
- Split up widget-toolkit patches to make review easier. The intention
  for patches 1/4 to 3/4 of widget-toolkit is to keep the functionality
  of SafeDestroy intact (no changes in behavior for SafeDestroyGuest,
  SafeDestroyStorage, and other SafeDestroy dialogs), only adding
  additional functionality that is then moved to ConfirmRemoveDialog in
  4/4, where the default settings are then changed in
  ConfirmRemoveDialog and SafeDestroy explicitly sets the parameters to
  keep behavior intact.
- Instead of replacing SafeDestroy with ConfirmRemoveDialog and
  requiring additional changes to windows extending SafeDestroy, make
  SafeDestroy extend ConfirmRemoveDialog, encapsulating the specific
  settings of a SafeDestroy dialog. This also makes the original
  follow-up series obsolete [2].

Thanks to @Daniel and @Thomas for their review and feedback on v2!

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=6613
[1] https://lore.proxmox.com/pve-devel/c004b755-ba3c-46cd-b559-ee2c131d97fc@proxmox.com
[2] https://lore.proxmox.com/pve-devel/20250924161714.434858-1-m.koeppl@proxmox.com/

pve-ha-manager:

Michael Köppl (2):
  fix #6613: update rules containing the resource to be deleted
  api: add purge parameter for resource deletion

 src/PVE/API2/HA/Resources.pm | 10 +++++++++-
 src/PVE/HA/Config.pm         | 13 ++++++++++++-
 2 files changed, 21 insertions(+), 2 deletions(-)


qemu-server:

Michael Köppl (1):
  fix #6613: pass purge param to delete_service_from_config

 src/PVE/API2/Qemu.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)


pve-container:

Michael Köppl (1):
  fix #6613: pass purge param to delete_service_from_config

 src/PVE/API2/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)


proxmox-widget-toolkit:

Michael Köppl (4):
  window: refactor construction of SafeDestroy items
  window: introduce dangerous parameter to SafeDestroy
  window: make buttons in SafeDestroy configurable
  window: add more general base dialog, make SafeDestroy concrete

 src/Makefile                      |   1 +
 src/window/ConfirmRemoveDialog.js | 247 ++++++++++++++++++++++++++++++
 src/window/SafeDestroy.js         | 196 ++----------------------
 3 files changed, 257 insertions(+), 187 deletions(-)
 create mode 100644 src/window/ConfirmRemoveDialog.js


pve-manager:

Michael Köppl (2):
  ui: add ConfirmRemoveResource window
  ui: use ConfirmRemoveResource window for removing resources

 www/manager6/Makefile                        |  1 +
 www/manager6/ha/Resources.js                 | 16 ++++++--
 www/manager6/window/ConfirmRemoveResource.js | 41 ++++++++++++++++++++
 3 files changed, 54 insertions(+), 4 deletions(-)
 create mode 100644 www/manager6/window/ConfirmRemoveResource.js


pve-docs:

Michael Köppl (1):
  add notes about effects of purge flag for resource and guest removal

 ha-manager.adoc | 4 ++++
 pct.adoc        | 3 +++
 qm.adoc         | 3 +++
 3 files changed, 10 insertions(+)


Summary over all repositories:
  13 files changed, 346 insertions(+), 195 deletions(-)

-- 
Generated by git-murpp 0.8.0


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

[pve-devel] [PATCH ha-manager v3 1/2] fix #6613: update rules containing the resource to be deleted

[Michael Köppl]

If the purge param is set, the resource that is deleted is also removed
from any rules referencing it. In the case that a resource is removed
that is also the last resource in a rule, the rule is also removed if
the purge option is enabled. This avoids rules remaining in the rules
config if their resources no longer exist.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/HA/Config.pm | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 301c62f..e964a7f 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -419,17 +419,28 @@ sub get_resource_motion_info {
 
 # graceful, as long as locking + cfs_write works
 sub delete_service_from_config {
-    my ($sid) = @_;
+    my ($sid, $purge) = @_;
 
     return 1 if !service_is_configured($sid);
 
     my $res;
     PVE::HA::Config::lock_ha_domain(
         sub {
+
             my $conf = read_resources_config();
             $res = delete $conf->{ids}->{$sid};
             write_resources_config($conf);
 
+            if ($purge) {
+                my $rules = read_rules_config();
+                for my $ruleid (keys $rules->{ids}->%*) {
+                    my $rule_resources = $rules->{ids}->{$ruleid}->{resources} // {};
+
+                    delete $rule_resources->{$sid};
+                    delete $rules->{ids}->{$ruleid} if !%$rule_resources;
+                }
+                write_rules_config($rules);
+            }
         },
         "delete resource failed",
     );
-- 
2.47.3



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

[pve-devel] [PATCH ha-manager v3 2/2] api: add purge parameter for resource deletion

[Michael Köppl]

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/HA/Resources.pm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/HA/Resources.pm b/src/PVE/API2/HA/Resources.pm
index 894fe90..09fe954 100644
--- a/src/PVE/API2/HA/Resources.pm
+++ b/src/PVE/API2/HA/Resources.pm
@@ -284,6 +284,13 @@ __PACKAGE__->register_method({
                 'pve-ha-resource-or-vm-id',
                 { completion => \&PVE::HA::Tools::complete_sid },
             ),
+            purge => {
+                type => 'boolean',
+                description =>
+                    "Remove this resource from rules that reference it, deleting the rule if this resource is the only resource in the rule",
+                optional => 1,
+                default => 1,
+            },
         },
     },
     returns => { type => 'null' },
@@ -291,12 +298,13 @@ __PACKAGE__->register_method({
         my ($param) = @_;
 
         my ($sid, $type, $name) = PVE::HA::Config::parse_sid(extract_param($param, 'sid'));
+        my $purge = extract_param($param, 'purge') // 1;
 
         if (!PVE::HA::Config::service_is_configured($sid)) {
             die "cannot delete service '$sid', not HA managed!\n";
         }
 
-        PVE::HA::Config::delete_service_from_config($sid);
+        PVE::HA::Config::delete_service_from_config($sid, $purge);
 
         return undef;
     },
-- 
2.47.3



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

[pve-devel] [PATCH qemu-server v3 1/1] fix #6613: pass purge param to delete_service_from_config

[Michael Köppl]

This covers the case where users want to delete a VM that is also a HA
resource. If the purge param is set, the HA resource will also be
removed from the affinity rules referencing the resource. If the
affected rule only contains this one resource, the rule is also deleted.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/Qemu.pm | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/PVE/API2/Qemu.pm b/src/PVE/API2/Qemu.pm
index 594c5d48..68775834 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -2762,7 +2762,9 @@ __PACKAGE__->register_method({
                         PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
 
                         if ($ha_managed) {
-                            PVE::HA::Config::delete_service_from_config("vm:$vmid");
+                            PVE::HA::Config::delete_service_from_config(
+                                "vm:$vmid", $param->{purge},
+                            );
                             print "NOTE: removed VM $vmid from HA resource configuration.\n";
                         }
                     }
-- 
2.47.3



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

[pve-devel] [PATCH container v3 1/1] fix #6613: pass purge param to delete_service_from_config

[Michael Köppl]

This covers the case where users want to delete a CT that is also a HA
resource. If the purge param is set, the HA resource will also be
removed from the affinity rules referencing the resource. If the
affected rule only contains this one resource, the rule is also deleted.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
---
 src/PVE/API2/LXC.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/API2/LXC.pm b/src/PVE/API2/LXC.pm
index 37b9d055..dc0ba5b2 100644
--- a/src/PVE/API2/LXC.pm
+++ b/src/PVE/API2/LXC.pm
@@ -879,7 +879,7 @@ __PACKAGE__->register_method({
                 PVE::VZDump::Plugin::remove_vmid_from_backup_jobs($vmid);
 
                 if ($ha_managed) {
-                    PVE::HA::Config::delete_service_from_config("ct:$vmid");
+                    PVE::HA::Config::delete_service_from_config("ct:$vmid", $param->{purge});
                     print "NOTE: removed CT $vmid from HA resource configuration.\n";
                 }
             }
-- 
2.47.3



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

[pve-devel] [PATCH widget-toolkit v3 1/4] window: refactor construction of SafeDestroy items

[Michael Köppl]

This is done to make extending SafeDestroy with additional functionality
easier.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
No functional change intended here. I mostly split the widget-toolkit
patches up into multiple parts to make review of the individual
extensions of the dialog easier. This is the first step that makes later
changes easier to implement and understand.

 src/window/SafeDestroy.js | 175 +++++++++++++++++++++-----------------
 1 file changed, 96 insertions(+), 79 deletions(-)

diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
index d38f085..76096cb 100644
--- a/src/window/SafeDestroy.js
+++ b/src/window/SafeDestroy.js
@@ -107,101 +107,118 @@ Ext.define('Proxmox.window.SafeDestroy', {
         },
     },
 
-    buttons: [
-        {
-            reference: 'removeButton',
-            text: gettext('Remove'),
-            disabled: true,
-        },
-    ],
-
     initComponent: function () {
         let me = this;
 
-        me.items = [
-            {
-                xtype: 'component',
-                cls: [
-                    Ext.baseCSSPrefix + 'message-box-icon',
-                    Ext.baseCSSPrefix + 'message-box-warning',
-                    Ext.baseCSSPrefix + 'dlg-icon',
-                ],
-            },
-            {
+        let body = {
+            xtype: 'container',
+            layout: 'hbox',
+            items: [
+                {
+                    xtype: 'component',
+                    cls: [
+                        Ext.baseCSSPrefix + 'message-box-icon',
+                        Ext.baseCSSPrefix + 'dlg-icon',
+                        Ext.baseCSSPrefix + 'message-box-warning',
+                    ],
+                },
+            ],
+        };
+
+        const itemId = me.getItem().id;
+        if (!Ext.isDefined(itemId)) {
+            throw 'no ID specified';
+        }
+
+        const taskName = me.getTaskName();
+        let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
+
+        let content = {
+            xtype: 'container',
+            layout: 'vbox',
+            items: [
+                {
+                    xtype: 'component',
+                    reference: 'messageCmp',
+                    html: Ext.htmlEncode(
+                        Proxmox.Utils.format_task_description(
+                            taskName,
+                            me.getItem().formattedIdentifier ?? itemId,
+                        ),
+                    ),
+                },
+                {
+                    itemId: 'confirmField',
+                    reference: 'confirmField',
+                    xtype: 'textfield',
+                    name: 'confirm',
+                    padding: '5 0 0 0',
+                    width: 340,
+                    labelWidth: 240,
+                    fieldLabel: label,
+                    hideTrigger: true,
+                    allowBlank: false,
+                },
+            ],
+        };
+
+        if (me.additionalItems && me.additionalItems.length > 0) {
+            content.items.push({
                 xtype: 'container',
+                height: 5,
+            });
+            for (const item of me.additionalItems) {
+                content.items.push(item);
+            }
+        }
+
+        if (Ext.isDefined(me.getNote())) {
+            content.items.push({
+                xtype: 'container',
+                reference: 'noteContainer',
                 flex: 1,
                 layout: {
                     type: 'vbox',
-                    align: 'stretch',
                 },
                 items: [
                     {
                         xtype: 'component',
-                        reference: 'messageCmp',
-                    },
-                    {
-                        itemId: 'confirmField',
-                        reference: 'confirmField',
-                        xtype: 'textfield',
-                        name: 'confirm',
-                        labelWidth: 300,
-                        hideTrigger: true,
-                        allowBlank: false,
+                        reference: 'noteCmp',
+                        userCls: 'pmx-hint',
+                        html: `<span title="${me.getNote()}">${me.getNote()}</span>`,
                     },
-                ]
-                    .concat(me.additionalItems)
-                    .concat([
-                        {
-                            xtype: 'container',
-                            reference: 'noteContainer',
-                            flex: 1,
-                            hidden: true,
-                            layout: {
-                                type: 'vbox',
-                            },
-                            items: [
-                                {
-                                    xtype: 'component',
-                                    reference: 'noteCmp',
-                                    userCls: 'pmx-hint',
-                                },
-                            ],
-                        },
-                    ]),
-            },
-        ];
+                ],
+            });
+        }
 
-        me.callParent();
+        body.items.push(content);
 
-        const itemId = me.getItem().id;
-        if (!Ext.isDefined(itemId)) {
-            throw 'no ID specified';
-        }
+        me.items = [body];
 
-        if (Ext.isDefined(me.getNote())) {
-            me.lookupReference('noteCmp').setHtml(
-                `<span title="${me.getNote()}">${me.getNote()}</span>`,
-            );
-            const noteContainer = me.lookupReference('noteContainer');
-            noteContainer.setHidden(false);
-            noteContainer.setDisabled(false);
-        }
+        let buttons = [
+            {
+                xtype: 'button',
+                reference: 'removeButton',
+                text: gettext('Remove'),
+                disabled: true,
+                width: 75,
+                margin: '0 5 0 0',
+            },
+        ];
 
-        let taskName = me.getTaskName();
-        if (Ext.isDefined(taskName)) {
-            me.lookupReference('messageCmp').setHtml(
-                Ext.htmlEncode(
-                    Proxmox.Utils.format_task_description(
-                        taskName,
-                        me.getItem().formattedIdentifier ?? itemId,
-                    ),
-                ),
-            );
-        } else {
-            throw 'no task name specified';
-        }
+        me.dockedItems = [
+            {
+                xtype: 'container',
+                dock: 'bottom',
+                cls: ['x-toolbar', 'x-toolbar-footer'],
+                layout: {
+                    type: 'hbox',
+                    pack: 'center',
+                },
+                items: buttons,
+            },
+        ];
 
-        let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
-        me.lookupReference('confirmField').setFieldLabel(Ext.htmlEncode(label));
+        me.callParent();
     },
 });
-- 
2.47.3



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

[pve-devel] [PATCH widget-toolkit v3 2/4] window: introduce dangerous parameter to SafeDestroy

[Michael Köppl]

The 'dangerous' parameter determines whether the user will be required to
enter the ID of the item they want to delete to confirm their intent. If
set to true, the input field will be added and a warning icon will be
displayed. If false, there is no ID input field and a question mark will
be displayed.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/window/SafeDestroy.js | 54 ++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 21 deletions(-)

diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
index 76096cb..ffa7d1d 100644
--- a/src/window/SafeDestroy.js
+++ b/src/window/SafeDestroy.js
@@ -1,5 +1,6 @@
-// Pop-up a message window where the user has to manually enter the resource ID to enable the
-// destroy confirmation button to ensure that they got the correct resource selected for.
+// Pop-up a message window where the user has to confirm their intent to delete an item.
+// Optionally, an additional textfield can be added, requiring the user to enter the ID
+// of the given item again to confirm their intent.
 Ext.define('Proxmox.window.SafeDestroy', {
     extend: 'Ext.window.Window',
     alias: 'widget.proxmoxSafeDestroy',
@@ -13,6 +14,11 @@ Ext.define('Proxmox.window.SafeDestroy', {
     defaultFocus: 'confirmField',
     showProgress: false,
 
+    // if set to true, a warning sign will be displayed and entering the ID will
+    // be required before removal is possible. If set to false, a question mark
+    // will be displayed.
+    dangerous: true,
+
     additionalItems: [],
 
     // gets called if we have a progress bar or taskview and it detected that
@@ -110,17 +116,20 @@ Ext.define('Proxmox.window.SafeDestroy', {
     initComponent: function () {
         let me = this;
 
+        let cls = [Ext.baseCSSPrefix + 'message-box-icon', Ext.baseCSSPrefix + 'dlg-icon'];
+        if (me.dangerous) {
+            cls.push(Ext.baseCSSPrefix + 'message-box-warning');
+        } else {
+            cls.push(Ext.baseCSSPrefix + 'message-box-question');
+        }
+
         let body = {
             xtype: 'container',
             layout: 'hbox',
             items: [
                 {
                     xtype: 'component',
-                    cls: [
-                        Ext.baseCSSPrefix + 'message-box-icon',
-                        Ext.baseCSSPrefix + 'dlg-icon',
-                        Ext.baseCSSPrefix + 'message-box-warning',
-                    ],
+                    cls: cls,
                 },
             ],
         };
@@ -131,7 +140,6 @@ Ext.define('Proxmox.window.SafeDestroy', {
         }
 
         const taskName = me.getTaskName();
-        let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
 
         let content = {
             xtype: 'container',
@@ -147,21 +155,25 @@ Ext.define('Proxmox.window.SafeDestroy', {
                         ),
                     ),
                 },
-                {
-                    itemId: 'confirmField',
-                    reference: 'confirmField',
-                    xtype: 'textfield',
-                    name: 'confirm',
-                    padding: '5 0 0 0',
-                    width: 340,
-                    labelWidth: 240,
-                    fieldLabel: label,
-                    hideTrigger: true,
-                    allowBlank: false,
-                },
             ],
         };
 
+        if (me.dangerous) {
+            let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
+            content.items.push({
+                itemId: 'confirmField',
+                reference: 'confirmField',
+                xtype: 'textfield',
+                name: 'confirm',
+                padding: '5 0 0 0',
+                width: 340,
+                labelWidth: 240,
+                fieldLabel: label,
+                hideTrigger: true,
+                allowBlank: false,
+            });
+        }
+
         if (me.additionalItems && me.additionalItems.length > 0) {
             content.items.push({
                 xtype: 'container',
@@ -200,7 +212,7 @@ Ext.define('Proxmox.window.SafeDestroy', {
                 xtype: 'button',
                 reference: 'removeButton',
                 text: gettext('Remove'),
-                disabled: true,
+                disabled: me.dangerous,
                 width: 75,
                 margin: '0 5 0 0',
             },
-- 
2.47.3



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

[pve-devel] [PATCH widget-toolkit v3 3/4] window: make buttons in SafeDestroy configurable

[Michael Köppl]

The SafeDestroy dialog can now feature either 1 or 2 buttons. By
default, "Remove" will be the only button. By overriding the
confirmButtonText and/or declineButtonText variables, the button texts
can be configured and the second button can be added to the dialog. This
enables additional use cases for the dialog.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/window/SafeDestroy.js | 37 ++++++++++++++++++++++++++++---------
 1 file changed, 28 insertions(+), 9 deletions(-)

diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
index ffa7d1d..4193db1 100644
--- a/src/window/SafeDestroy.js
+++ b/src/window/SafeDestroy.js
@@ -19,6 +19,10 @@ Ext.define('Proxmox.window.SafeDestroy', {
     // will be displayed.
     dangerous: true,
 
+    confirmButtonText: gettext('Remove'),
+    // second button will only be displayed if a text is given
+    declineButtonText: undefined,
+
     additionalItems: [],
 
     // gets called if we have a progress bar or taskview and it detected that
@@ -56,21 +60,21 @@ Ext.define('Proxmox.window.SafeDestroy', {
             'field[name=confirm]': {
                 change: function (f, value) {
                     const view = this.getView();
-                    const removeButton = this.lookupReference('removeButton');
+                    const confirmButton = this.lookupReference('confirmButton');
                     if (value === view.getItem().id.toString()) {
-                        removeButton.enable();
+                        confirmButton.enable();
                     } else {
-                        removeButton.disable();
+                        confirmButton.disable();
                     }
                 },
                 specialkey: function (field, event) {
-                    const removeButton = this.lookupReference('removeButton');
-                    if (!removeButton.isDisabled() && event.getKey() === event.ENTER) {
-                        removeButton.fireEvent('click', removeButton, event);
+                    const confirmButton = this.lookupReference('confirmButton');
+                    if (!confirmButton.isDisabled() && event.getKey() === event.ENTER) {
+                        confirmButton.fireEvent('click', confirmButton, event);
                     }
                 },
             },
-            'button[reference=removeButton]': {
+            'button[reference=confirmButton]': {
                 click: function () {
                     const view = this.getView();
                     Proxmox.Utils.API2Request({
@@ -110,6 +114,12 @@ Ext.define('Proxmox.window.SafeDestroy', {
                     });
                 },
             },
+            'button[reference=declineButton]': {
+                click: function () {
+                    const view = this.getView();
+                    view.close();
+                },
+            },
         },
     },
 
@@ -210,14 +220,23 @@ Ext.define('Proxmox.window.SafeDestroy', {
         let buttons = [
             {
                 xtype: 'button',
-                reference: 'removeButton',
-                text: gettext('Remove'),
+                reference: 'confirmButton',
+                text: me.confirmButtonText,
                 disabled: me.dangerous,
                 width: 75,
                 margin: '0 5 0 0',
             },
         ];
 
+        if (me.declineButtonText) {
+            buttons.push({
+                xtype: 'button',
+                reference: 'declineButton',
+                text: me.declineButtonText,
+                width: 75,
+            });
+        }
+
         me.dockedItems = [
             {
                 xtype: 'container',
-- 
2.47.3



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

[pve-devel] [PATCH widget-toolkit v3 4/4] window: add more general base dialog, make SafeDestroy concrete

[Michael Köppl]

Move the functionality of SafeDestroy to ConfirmRemoveDialog and use it
as a base for SafeDestroy. ConfirmRemoveDialog will serve as the general
base implementation of a dialog, offering some customization for remove
dialogs while covering the functionality of the original SafeDestroy
dialog. SafeDestroy is now a special case of ConfirmRemoveDialog. This
also avoids changes for dialog windows extending SafeDestroy.

Also makes ConfirmRemoveDialog a non-dangerous yes/no dialog by default,
setting dangerous = true and the "Remove" in SafeDestroy explicitly.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 src/Makefile                      |   1 +
 src/window/ConfirmRemoveDialog.js | 247 ++++++++++++++++++++++++++++++
 src/window/SafeDestroy.js         | 240 +----------------------------
 3 files changed, 255 insertions(+), 233 deletions(-)
 create mode 100644 src/window/ConfirmRemoveDialog.js

diff --git a/src/Makefile b/src/Makefile
index 0e2b329..05c7b90 100644
--- a/src/Makefile
+++ b/src/Makefile
@@ -83,6 +83,7 @@ JSSRC=					\
 	panel/WebhookEditPanel.js	\
 	window/Edit.js			\
 	window/PasswordEdit.js		\
+	window/ConfirmRemoveDialog.js	\
 	window/SafeDestroy.js		\
 	window/PackageVersions.js	\
 	window/TaskViewer.js		\
diff --git a/src/window/ConfirmRemoveDialog.js b/src/window/ConfirmRemoveDialog.js
new file mode 100644
index 0000000..94f1e84
--- /dev/null
+++ b/src/window/ConfirmRemoveDialog.js
@@ -0,0 +1,247 @@
+// Pop-up a message window where the user has to confirm their intent to delete an item.
+// Optionally, an additional textfield can be added, requiring the user to enter the ID
+// of the given item again to confirm their intent.
+Ext.define('Proxmox.window.ConfirmRemoveDialog', {
+    extend: 'Ext.window.Window',
+    alias: 'widget.proxmoxConfirmRemoveDialog',
+
+    title: gettext('Confirm'),
+    modal: true,
+    buttonAlign: 'center',
+    bodyPadding: 10,
+    width: 450,
+    layout: { type: 'hbox' },
+    defaultFocus: 'confirmField',
+    showProgress: false,
+
+    // if set to true, a warning sign will be displayed and entering the ID will
+    // be required before removal is possible. If set to false, a question mark
+    // will be displayed.
+    dangerous: false,
+
+    confirmButtonText: gettext('Yes'),
+    // second button will only be displayed if a text is given
+    declineButtonText: gettext('No'),
+
+    additionalItems: [],
+
+    // gets called if we have a progress bar or taskview and it detected that
+    // the task finished. function(success)
+    taskDone: Ext.emptyFn,
+
+    // gets called when the api call is finished, right at the beginning
+    // function(success, response, options)
+    apiCallDone: Ext.emptyFn,
+
+    config: {
+        item: {
+            id: undefined,
+        },
+        text: undefined,
+        url: undefined,
+        note: undefined,
+        params: {},
+    },
+
+    getParams: function () {
+        let me = this;
+
+        if (Ext.Object.isEmpty(me.params)) {
+            return '';
+        }
+        return '?' + Ext.Object.toQueryString(me.params);
+    },
+
+    controller: {
+        xclass: 'Ext.app.ViewController',
+
+        control: {
+            'field[name=confirm]': {
+                change: function (f, value) {
+                    const view = this.getView();
+                    const confirmButton = this.lookupReference('confirmButton');
+                    if (value === view.getItem().id.toString()) {
+                        confirmButton.enable();
+                    } else {
+                        confirmButton.disable();
+                    }
+                },
+                specialkey: function (field, event) {
+                    const confirmButton = this.lookupReference('confirmButton');
+                    if (!confirmButton.isDisabled() && event.getKey() === event.ENTER) {
+                        confirmButton.fireEvent('click', confirmButton, event);
+                    }
+                },
+            },
+            'button[reference=confirmButton]': {
+                click: function () {
+                    const view = this.getView();
+                    Proxmox.Utils.API2Request({
+                        url: view.getUrl() + view.getParams(),
+                        method: 'DELETE',
+                        waitMsgTarget: view,
+                        failure: function (response, opts) {
+                            view.apiCallDone(false, response, opts);
+                            view.close();
+                            Ext.Msg.alert('Error', response.htmlStatus);
+                        },
+                        success: function (response, options) {
+                            const hasProgressBar = !!(view.showProgress && response.result.data);
+
+                            view.apiCallDone(true, response, options);
+
+                            if (hasProgressBar) {
+                                // stay around so we can trigger our close events
+                                // when background action is completed
+                                view.hide();
+
+                                const upid = response.result.data;
+                                const win = Ext.create('Proxmox.window.TaskProgress', {
+                                    upid: upid,
+                                    taskDone: view.taskDone,
+                                    listeners: {
+                                        destroy: function () {
+                                            view.close();
+                                        },
+                                    },
+                                });
+                                win.show();
+                            } else {
+                                view.close();
+                            }
+                        },
+                    });
+                },
+            },
+            'button[reference=declineButton]': {
+                click: function () {
+                    const view = this.getView();
+                    view.close();
+                },
+            },
+        },
+    },
+
+    initComponent: function () {
+        let me = this;
+
+        let cls = [Ext.baseCSSPrefix + 'message-box-icon', Ext.baseCSSPrefix + 'dlg-icon'];
+        if (me.dangerous) {
+            cls.push(Ext.baseCSSPrefix + 'message-box-warning');
+        } else {
+            cls.push(Ext.baseCSSPrefix + 'message-box-question');
+        }
+
+        let body = {
+            xtype: 'container',
+            layout: 'hbox',
+            items: [
+                {
+                    xtype: 'component',
+                    cls: cls,
+                },
+            ],
+        };
+
+        const itemId = me.getItem().id;
+        if (!Ext.isDefined(itemId)) {
+            throw 'no ID specified';
+        }
+
+        let content = {
+            xtype: 'container',
+            layout: 'vbox',
+            items: [
+                {
+                    xtype: 'component',
+                    reference: 'messageCmp',
+                    html: Ext.htmlEncode(me.getText()),
+                },
+            ],
+        };
+
+        if (me.dangerous) {
+            let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
+            content.items.push({
+                itemId: 'confirmField',
+                reference: 'confirmField',
+                xtype: 'textfield',
+                name: 'confirm',
+                padding: '5 0 0 0',
+                width: 340,
+                labelWidth: 240,
+                fieldLabel: label,
+                hideTrigger: true,
+                allowBlank: false,
+            });
+        }
+
+        if (me.additionalItems && me.additionalItems.length > 0) {
+            content.items.push({
+                xtype: 'container',
+                height: 5,
+            });
+            for (const item of me.additionalItems) {
+                content.items.push(item);
+            }
+        }
+
+        if (Ext.isDefined(me.getNote())) {
+            content.items.push({
+                xtype: 'container',
+                reference: 'noteContainer',
+                flex: 1,
+                layout: {
+                    type: 'vbox',
+                },
+                items: [
+                    {
+                        xtype: 'component',
+                        reference: 'noteCmp',
+                        userCls: 'pmx-hint',
+                        html: `<span title="${me.getNote()}">${me.getNote()}</span>`,
+                    },
+                ],
+            });
+        }
+
+        body.items.push(content);
+
+        me.items = [body];
+
+        let buttons = [
+            {
+                xtype: 'button',
+                reference: 'confirmButton',
+                text: me.confirmButtonText,
+                disabled: me.dangerous,
+                width: 75,
+                margin: '0 5 0 0',
+            },
+        ];
+
+        if (me.declineButtonText) {
+            buttons.push({
+                xtype: 'button',
+                reference: 'declineButton',
+                text: me.declineButtonText,
+                width: 75,
+            });
+        }
+
+        me.dockedItems = [
+            {
+                xtype: 'container',
+                dock: 'bottom',
+                cls: ['x-toolbar', 'x-toolbar-footer'],
+                layout: {
+                    type: 'hbox',
+                    pack: 'center',
+                },
+                items: buttons,
+            },
+        ];
+
+        me.callParent();
+    },
+});
diff --git a/src/window/SafeDestroy.js b/src/window/SafeDestroy.js
index 4193db1..4ef1389 100644
--- a/src/window/SafeDestroy.js
+++ b/src/window/SafeDestroy.js
@@ -1,255 +1,29 @@
-// Pop-up a message window where the user has to confirm their intent to delete an item.
-// Optionally, an additional textfield can be added, requiring the user to enter the ID
-// of the given item again to confirm their intent.
+// Pop-up a message window where the user has to manually enter the resource ID to enable the
+// destroy confirmation button to ensure that they got the correct resource selected for.
 Ext.define('Proxmox.window.SafeDestroy', {
-    extend: 'Ext.window.Window',
+    extend: 'Proxmox.window.ConfirmRemoveDialog',
     alias: 'widget.proxmoxSafeDestroy',
 
-    title: gettext('Confirm'),
-    modal: true,
-    buttonAlign: 'center',
-    bodyPadding: 10,
-    width: 450,
-    layout: { type: 'hbox' },
-    defaultFocus: 'confirmField',
-    showProgress: false,
-
-    // if set to true, a warning sign will be displayed and entering the ID will
-    // be required before removal is possible. If set to false, a question mark
-    // will be displayed.
     dangerous: true,
 
     confirmButtonText: gettext('Remove'),
     // second button will only be displayed if a text is given
     declineButtonText: undefined,
 
-    additionalItems: [],
-
-    // gets called if we have a progress bar or taskview and it detected that
-    // the task finished. function(success)
-    taskDone: Ext.emptyFn,
-
-    // gets called when the api call is finished, right at the beginning
-    // function(success, response, options)
-    apiCallDone: Ext.emptyFn,
-
     config: {
         item: {
             id: undefined,
             formattedIdentifier: undefined,
         },
-        url: undefined,
-        note: undefined,
         taskName: undefined,
-        params: {},
     },
 
-    getParams: function () {
+    getText: function () {
         let me = this;
 
-        if (Ext.Object.isEmpty(me.params)) {
-            return '';
-        }
-        return '?' + Ext.Object.toQueryString(me.params);
-    },
-
-    controller: {
-        xclass: 'Ext.app.ViewController',
-
-        control: {
-            'field[name=confirm]': {
-                change: function (f, value) {
-                    const view = this.getView();
-                    const confirmButton = this.lookupReference('confirmButton');
-                    if (value === view.getItem().id.toString()) {
-                        confirmButton.enable();
-                    } else {
-                        confirmButton.disable();
-                    }
-                },
-                specialkey: function (field, event) {
-                    const confirmButton = this.lookupReference('confirmButton');
-                    if (!confirmButton.isDisabled() && event.getKey() === event.ENTER) {
-                        confirmButton.fireEvent('click', confirmButton, event);
-                    }
-                },
-            },
-            'button[reference=confirmButton]': {
-                click: function () {
-                    const view = this.getView();
-                    Proxmox.Utils.API2Request({
-                        url: view.getUrl() + view.getParams(),
-                        method: 'DELETE',
-                        waitMsgTarget: view,
-                        failure: function (response, opts) {
-                            view.apiCallDone(false, response, opts);
-                            view.close();
-                            Ext.Msg.alert('Error', response.htmlStatus);
-                        },
-                        success: function (response, options) {
-                            const hasProgressBar = !!(view.showProgress && response.result.data);
-
-                            view.apiCallDone(true, response, options);
-
-                            if (hasProgressBar) {
-                                // stay around so we can trigger our close events
-                                // when background action is completed
-                                view.hide();
-
-                                const upid = response.result.data;
-                                const win = Ext.create('Proxmox.window.TaskProgress', {
-                                    upid: upid,
-                                    taskDone: view.taskDone,
-                                    listeners: {
-                                        destroy: function () {
-                                            view.close();
-                                        },
-                                    },
-                                });
-                                win.show();
-                            } else {
-                                view.close();
-                            }
-                        },
-                    });
-                },
-            },
-            'button[reference=declineButton]': {
-                click: function () {
-                    const view = this.getView();
-                    view.close();
-                },
-            },
-        },
-    },
-
-    initComponent: function () {
-        let me = this;
-
-        let cls = [Ext.baseCSSPrefix + 'message-box-icon', Ext.baseCSSPrefix + 'dlg-icon'];
-        if (me.dangerous) {
-            cls.push(Ext.baseCSSPrefix + 'message-box-warning');
-        } else {
-            cls.push(Ext.baseCSSPrefix + 'message-box-question');
-        }
-
-        let body = {
-            xtype: 'container',
-            layout: 'hbox',
-            items: [
-                {
-                    xtype: 'component',
-                    cls: cls,
-                },
-            ],
-        };
-
-        const itemId = me.getItem().id;
-        if (!Ext.isDefined(itemId)) {
-            throw 'no ID specified';
-        }
-
-        const taskName = me.getTaskName();
-
-        let content = {
-            xtype: 'container',
-            layout: 'vbox',
-            items: [
-                {
-                    xtype: 'component',
-                    reference: 'messageCmp',
-                    html: Ext.htmlEncode(
-                        Proxmox.Utils.format_task_description(
-                            taskName,
-                            me.getItem().formattedIdentifier ?? itemId,
-                        ),
-                    ),
-                },
-            ],
-        };
-
-        if (me.dangerous) {
-            let label = `${gettext('Please enter the ID to confirm')} (${itemId})`;
-            content.items.push({
-                itemId: 'confirmField',
-                reference: 'confirmField',
-                xtype: 'textfield',
-                name: 'confirm',
-                padding: '5 0 0 0',
-                width: 340,
-                labelWidth: 240,
-                fieldLabel: label,
-                hideTrigger: true,
-                allowBlank: false,
-            });
-        }
-
-        if (me.additionalItems && me.additionalItems.length > 0) {
-            content.items.push({
-                xtype: 'container',
-                height: 5,
-            });
-            for (const item of me.additionalItems) {
-                content.items.push(item);
-            }
-        }
-
-        if (Ext.isDefined(me.getNote())) {
-            content.items.push({
-                xtype: 'container',
-                reference: 'noteContainer',
-                flex: 1,
-                layout: {
-                    type: 'vbox',
-                },
-                items: [
-                    {
-                        xtype: 'component',
-                        reference: 'noteCmp',
-                        userCls: 'pmx-hint',
-                        html: `<span title="${me.getNote()}">${me.getNote()}</span>`,
-                    },
-                ],
-            });
-        }
-
-        body.items.push(content);
-
-        me.items = [body];
-
-        let buttons = [
-            {
-                xtype: 'button',
-                reference: 'confirmButton',
-                text: me.confirmButtonText,
-                disabled: me.dangerous,
-                width: 75,
-                margin: '0 5 0 0',
-            },
-        ];
-
-        if (me.declineButtonText) {
-            buttons.push({
-                xtype: 'button',
-                reference: 'declineButton',
-                text: me.declineButtonText,
-                width: 75,
-            });
-        }
-
-        me.dockedItems = [
-            {
-                xtype: 'container',
-                dock: 'bottom',
-                cls: ['x-toolbar', 'x-toolbar-footer'],
-                layout: {
-                    type: 'hbox',
-                    pack: 'center',
-                },
-                items: buttons,
-            },
-        ];
+        let identifier = me.getItem().formattedIdentifier ?? me.getItem().id;
+        me.text = `${Proxmox.Utils.format_task_description(me.getTaskName(), identifier)}`;
 
-        me.callParent();
+        return me.callParent();
     },
 });
-- 
2.47.3



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

[pve-devel] [PATCH manager v3 1/2] ui: add ConfirmRemoveResource window

[Michael Köppl]

Add an extended removal dialog for HA resources, which also allows
setting a purge parameter through the checkbox. By default, the purge
option is enabled, updating rules referencing the resource and deleting
any rules that only have this resource left.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/Makefile                        |  1 +
 www/manager6/window/ConfirmRemoveResource.js | 41 ++++++++++++++++++++
 2 files changed, 42 insertions(+)
 create mode 100644 www/manager6/window/ConfirmRemoveResource.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 85f9268d1..17ae49a24 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -120,6 +120,7 @@ JSSRC= 							\
 	window/BulkAction.js				\
 	window/CephInstall.js				\
 	window/Clone.js					\
+	window/ConfirmRemoveResource.js			\
 	window/FirewallEnableEdit.js			\
 	window/FirewallLograteEdit.js			\
 	window/LoginWindow.js				\
diff --git a/www/manager6/window/ConfirmRemoveResource.js b/www/manager6/window/ConfirmRemoveResource.js
new file mode 100644
index 000000000..110595778
--- /dev/null
+++ b/www/manager6/window/ConfirmRemoveResource.js
@@ -0,0 +1,41 @@
+/*
+ * ConfirmRemoveDialog window with additional checkboxes for removing resources
+ */
+Ext.define('PVE.window.ConfirmRemoveResource', {
+    extend: 'Proxmox.window.ConfirmRemoveDialog',
+    alias: 'widget.pveConfirmRemoveResource',
+
+    additionalItems: [
+        {
+            xtype: 'proxmoxcheckbox',
+            name: 'purge',
+            reference: 'purgeCheckbox',
+            boxLabel: gettext('Purge resource from referenced HA rules'),
+            padding: '5 0 0 0',
+            checked: true,
+            autoEl: {
+                tag: 'div',
+                'data-qtip': gettext(
+                    'Also removes resource from HA rules and removes rule if there are no other resources in it',
+                ),
+            },
+        },
+    ],
+
+    getText: function () {
+        let me = this;
+
+        me.text = `Are you sure you want to remove resource '${me.getItem().id}'?`;
+
+        return me.callParent();
+    },
+
+    getParams: function () {
+        let me = this;
+
+        const purgeCheckbox = me.lookupReference('purgeCheckbox');
+        me.params.purge = purgeCheckbox.checked ? 1 : 0;
+
+        return me.callParent();
+    },
+});
-- 
2.47.3



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

[pve-devel] [PATCH manager v3 2/2] ui: use ConfirmRemoveResource window for removing resources

[Michael Köppl]

This allows users to additionally choose whether they want to purge
referenced rules that only include the resource that is to be deleted.

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
---
 www/manager6/ha/Resources.js | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/www/manager6/ha/Resources.js b/www/manager6/ha/Resources.js
index 65897bed2..621ed3367 100644
--- a/www/manager6/ha/Resources.js
+++ b/www/manager6/ha/Resources.js
@@ -74,12 +74,20 @@ Ext.define('PVE.ha.ResourcesView', {
                     handler: run_editor,
                 },
                 {
-                    xtype: 'proxmoxStdRemoveButton',
+                    xtype: 'proxmoxButton',
+                    text: gettext('Remove'),
                     selModel: sm,
-                    getUrl: function (rec) {
-                        return `/cluster/ha/resources/${rec.get('sid')}`;
+                    itemId: 'removeBtn',
+                    disabled: true,
+                    handler: function (btn, e, rec) {
+                        Ext.create('PVE.window.ConfirmRemoveResource', {
+                            url: `/cluster/ha/resources/${rec.data.sid}`,
+                            item: {
+                                id: rec.data.sid,
+                            },
+                            apiCallDone: () => me.rstore.load(),
+                        }).show();
                     },
-                    callback: () => me.rstore.load(),
                 },
             ],
             columns: [
-- 
2.47.3



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

[pve-devel] [PATCH docs v3 1/1] add notes about effects of purge flag for resource and guest removal

[Michael Köppl]

Signed-off-by: Michael Köppl <m.koeppl@proxmox.com>
Tested-by: Daniel Kral <d.kral@proxmox.com>
Reviewed-by: Daniel Kral <d.kral@proxmox.com>
---
 ha-manager.adoc | 4 ++++
 pct.adoc        | 3 +++
 qm.adoc         | 3 +++
 3 files changed, 10 insertions(+)

diff --git a/ha-manager.adoc b/ha-manager.adoc
index ea477cc0..5c17f115 100644
--- a/ha-manager.adoc
+++ b/ha-manager.adoc
@@ -220,6 +220,10 @@ the following command:
 # ha-manager remove vm:100
 ----
 
+By default, this will also remove the resource from any rule that references it
+and will delete rules that only reference this resource. You can override this
+behavior using '--purge 0'.
+
 NOTE: This does not start or stop the resource.
 
 But all HA related tasks can be done in the GUI, so there is no need to
diff --git a/pct.adoc b/pct.adoc
index d6146ebe..b3695ac4 100644
--- a/pct.adoc
+++ b/pct.adoc
@@ -1044,6 +1044,9 @@ removes the firewall configuration of the container. You have to activate
 '--purge', if you want to additionally remove the container from replication jobs,
 backup jobs and HA resource configurations.
 
+NOTE: Activating purge will also remove the HA resource from any affinity rules
+referencing it and will remove rules that have only this one remaining resource.
+
 ----
 # pct destroy 100 --purge
 ----
diff --git a/qm.adoc b/qm.adoc
index f798043a..e70064b1 100644
--- a/qm.adoc
+++ b/qm.adoc
@@ -2199,6 +2199,9 @@ removes the firewall configuration of the VM. You have to activate
 '--purge', if you want to additionally remove the VM from replication jobs,
 backup jobs and HA resource configurations.
 
+NOTE: Activating purge will also remove the HA resource from any affinity rules
+referencing it and will remove rules that have only this one remaining resource.
+
 ----
 # qm destroy 300 --purge
 ----
-- 
2.47.3



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