[pve-devel] [PATCH container/docs/ha-manager/manager/qemu-server 0/7] fix #6613: update HA rules upon resource deletion

[pve-devel] [PATCH container/docs/ha-manager/manager/qemu-server 0/7] 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.

But I'm also asking for input here if someone has a different opinion or
suggestions for alternative approaches.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=6613
[1] https://lore.proxmox.com/pve-devel/c004b755-ba3c-46cd-b559-ee2c131d97fc@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         | 15 ++++++++++++++-
 2 files changed, 23 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(-)


pve-manager:

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

 www/manager6/Makefile                      |  1 +
 www/manager6/ha/Resources.js               | 17 +++++++++---
 www/manager6/window/SafeDestroyResource.js | 32 ++++++++++++++++++++++
 3 files changed, 46 insertions(+), 4 deletions(-)
 create mode 100644 www/manager6/window/SafeDestroyResource.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:
  10 files changed, 83 insertions(+), 8 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 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>
---
 src/PVE/HA/Config.pm | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/src/PVE/HA/Config.pm b/src/PVE/HA/Config.pm
index 301c62f..9a7ed0e 100644
--- a/src/PVE/HA/Config.pm
+++ b/src/PVE/HA/Config.pm
@@ -419,17 +419,30 @@ 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);
 
+            my $rules = read_rules_config();
+
+            if ($purge) {
+                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 2/2] api: add purge parameter for resource deletion

[Michael Köppl]

Signed-off-by: Michael Köppl <m.koeppl@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 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>
---
 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 34f615d8..46a6a06d 100644
--- a/src/PVE/API2/Qemu.pm
+++ b/src/PVE/API2/Qemu.pm
@@ -2760,7 +2760,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 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>
---
 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 manager 1/2] ui: add SafeDestroyResource 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/SafeDestroyResource.js | 32 ++++++++++++++++++++++
 2 files changed, 33 insertions(+)
 create mode 100644 www/manager6/window/SafeDestroyResource.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index 85f9268d1..a8bea9413 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -127,6 +127,7 @@ JSSRC= 							\
 	window/Prune.js					\
 	window/Restore.js				\
 	window/SafeDestroyGuest.js			\
+	window/SafeDestroyResource.js			\
 	window/SafeDestroyStorage.js			\
 	window/Settings.js				\
 	window/Snapshot.js				\
diff --git a/www/manager6/window/SafeDestroyResource.js b/www/manager6/window/SafeDestroyResource.js
new file mode 100644
index 000000000..988af7969
--- /dev/null
+++ b/www/manager6/window/SafeDestroyResource.js
@@ -0,0 +1,32 @@
+/*
+ * SafeDestroy window with additional checkboxes for removing resources
+ */
+Ext.define('PVE.window.SafeDestroyResource', {
+    extend: 'Proxmox.window.SafeDestroy',
+    alias: 'widget.pveSafeDestroyResource',
+
+    additionalItems: [
+        {
+            xtype: 'proxmoxcheckbox',
+            name: 'purge',
+            reference: 'purgeCheckbox',
+            boxLabel: gettext('Purge resource from referenced HA rules'),
+            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',
+                ),
+            },
+        },
+    ],
+
+    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 2/2] ui: use SafeDestroyResource 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 | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/www/manager6/ha/Resources.js b/www/manager6/ha/Resources.js
index 65897bed2..8bac671d5 100644
--- a/www/manager6/ha/Resources.js
+++ b/www/manager6/ha/Resources.js
@@ -74,12 +74,21 @@ 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',
+                    handler: function (btn, e, rec) {
+                        Ext.create('PVE.window.SafeDestroyResource', {
+                            url: `/cluster/ha/resources/${rec.data.sid}`,
+                            item: {
+                                id: rec.data.sid,
+                                formattedIdentifier: rec.data.sid,
+                            },
+                            taskName: 'Remove resource',
+                            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 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>
---
 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