[pve-devel] [PATCH SERIES docs/ha-manager/manager] close #6144: add ui button + api for node maintenance mode

[pve-devel] [PATCH SERIES docs/ha-manager/manager] close #6144: add ui button + api for node maintenance mode

[Thomas Skinner]

Closes #6144. Adds an API endpoint for checking/setting maintenance mode for a
host. Adds a button in the HA State panel UI for enabling/disabling maintenance
mode for a node. Adds docs for the feature.

docs:

Thomas Skinner (1):
  add docs for maintenance mode buttons in UI

 ha-manager.adoc | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

 
ha-manager:

Thomas Skinner (2):
  add additional api field for lrm_mode in status check
  add api getter/setter for node maintenance mode

 debian/pve-ha-manager.install |   1 +
 src/PVE/API2/HA/Makefile      |   2 +-
 src/PVE/API2/HA/Nodes.pm      | 176 ++++++++++++++++++++++++++++++++++
 src/PVE/API2/HA/Status.pm     |   7 ++
 4 files changed, 185 insertions(+), 1 deletion(-)
 create mode 100644 src/PVE/API2/HA/Nodes.pm

 
manager:

Thomas Skinner (2):
  add api path map for node HA endpoints
  add UI for node maintenance enable/disable

 PVE/API2/HAConfig.pm          | 12 ++++-
 www/manager6/ha/StatusView.js | 85 +++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 1 deletion(-)

 
-- 
2.47.2


_______________________________________________
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] add additional api field for lrm_mode in status check

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 src/PVE/API2/HA/Status.pm | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/PVE/API2/HA/Status.pm b/src/PVE/API2/HA/Status.pm
index 6e13c2c..f168637 100644
--- a/src/PVE/API2/HA/Status.pm
+++ b/src/PVE/API2/HA/Status.pm
@@ -104,6 +104,11 @@ __PACKAGE__->register_method({
                     type => "integer",
                     optional => 1,
                 },
+                lrm_mode => {
+                    description => "For type 'lrm'. Mode as reported by the LRM.",
+                    type => "string",
+                    optional => 1,
+                },
                 crm_state => {
                     description => "For type 'service'. Service state as seen by the CRM.",
                     type => "string",
@@ -216,6 +221,7 @@ __PACKAGE__->register_method({
                         type => 'lrm',
                         node => $node,
                         status => "$node (unable to read lrm status)",
+                        lrm_mode => 'unknown',
                     };
             } else {
                 my $status_str = &$timestamp_to_status($ctime, $lrm_status->{timestamp});
@@ -246,6 +252,7 @@ __PACKAGE__->register_method({
                         node => $node,
                         status => $status_text,
                         timestamp => $lrm_status->{timestamp},
+                        lrm_mode => $lrm_mode // "unknown",
                     };
             }
         }
-- 
2.47.2


_______________________________________________
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] add api path map for node HA endpoints

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 PVE/API2/HAConfig.pm | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/PVE/API2/HAConfig.pm b/PVE/API2/HAConfig.pm
index d29211fb..489cbf72 100644
--- a/PVE/API2/HAConfig.pm
+++ b/PVE/API2/HAConfig.pm
@@ -12,6 +12,7 @@ use PVE::JSONSchema qw(get_standard_option);
 use PVE::Exception qw(raise_param_exc);
 use PVE::API2::HA::Resources;
 use PVE::API2::HA::Groups;
+use PVE::API2::HA::Nodes;
 use PVE::API2::HA::Rules;
 use PVE::API2::HA::Status;
 
@@ -37,6 +38,11 @@ __PACKAGE__->register_method({
     path => 'status',
 });
 
+__PACKAGE__->register_method({
+    subclass => "PVE::API2::HA::Nodes",
+    path => 'nodes',
+});
+
 __PACKAGE__->register_method({
     name => 'index',
     path => '',
@@ -63,7 +69,11 @@ __PACKAGE__->register_method({
         my ($param) = @_;
 
         my $res = [
-            { id => 'status' }, { id => 'resources' }, { id => 'groups' }, { id => 'rules' },
+            { id => 'status' },
+            { id => 'resources' },
+            { id => 'groups' },
+            { id => 'rules' },
+            { id => 'nodes' },
         ];
 
         return $res;
-- 
2.47.2


_______________________________________________
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 docs for maintenance mode buttons in UI

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 ha-manager.adoc | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/ha-manager.adoc b/ha-manager.adoc
index f16cfbb..b41a73b 100644
--- a/ha-manager.adoc
+++ b/ha-manager.adoc
@@ -1115,7 +1115,19 @@ maintenance mode is disabled and the node is back online.
 Currently you can enabled or disable the maintenance mode using the ha-manager
 CLI tool.
 
+Maintenance mode can be enabled or disabled using the buttons on the HA pane in
+the {pve} web UI or by using the ha-manager CLI tool.
+
 .Enabling maintenance mode for a node
+
+_Using the web UI_
+
+Navigate to the Datacenter view, then select HA from the menu to show the HA
+Status pane. Select the lrm node that you want to enable maintenance mode for,
+then click the Enable Maintenance Mode button from the toolbar.
+
+_Using the CLI_
+
 ----
 # ha-manager crm-command node-maintenance enable NODENAME
 ----
@@ -1142,6 +1154,15 @@ but only if it is either manually deactivated using the `ha-manager` CLI or if
 the manager-status is manually cleared.
 
 .Disabling maintenance mode for a node
+
+_Using the web UI_
+
+Navigate to the Datacenter view, then select HA from the menu to show the HA
+Status pane. Select the lrm node that you want to disable maintenance mode for,
+then click the Disable Maintenance Mode button from the toolbar.
+
+_Using the CLI_
+
 ----
 # ha-manager crm-command node-maintenance disable NODENAME
 ----
-- 
2.47.2


_______________________________________________
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] add api getter/setter for node maintenance mode

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 debian/pve-ha-manager.install |   1 +
 src/PVE/API2/HA/Makefile      |   2 +-
 src/PVE/API2/HA/Nodes.pm      | 176 ++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+), 1 deletion(-)
 create mode 100644 src/PVE/API2/HA/Nodes.pm

diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
index 2e6b7d5..ec85037 100644
--- a/debian/pve-ha-manager.install
+++ b/debian/pve-ha-manager.install
@@ -15,6 +15,7 @@
 /usr/share/man/man8/pve-ha-crm.8.gz
 /usr/share/man/man8/pve-ha-lrm.8.gz
 /usr/share/perl5/PVE/API2/HA/Groups.pm
+/usr/share/perl5/PVE/API2/HA/Nodes.pm
 /usr/share/perl5/PVE/API2/HA/Resources.pm
 /usr/share/perl5/PVE/API2/HA/Rules.pm
 /usr/share/perl5/PVE/API2/HA/Status.pm
diff --git a/src/PVE/API2/HA/Makefile b/src/PVE/API2/HA/Makefile
index 86c1013..45d714b 100644
--- a/src/PVE/API2/HA/Makefile
+++ b/src/PVE/API2/HA/Makefile
@@ -1,4 +1,4 @@
-SOURCES=Resources.pm Groups.pm Rules.pm Status.pm
+SOURCES=Resources.pm Groups.pm Nodes.pm Rules.pm Status.pm
 
 .PHONY: install
 install:
diff --git a/src/PVE/API2/HA/Nodes.pm b/src/PVE/API2/HA/Nodes.pm
new file mode 100644
index 0000000..2afc3a3
--- /dev/null
+++ b/src/PVE/API2/HA/Nodes.pm
@@ -0,0 +1,176 @@
+package PVE::API2::HA::Nodes::Nodeinfo;
+
+use strict;
+use warnings;
+
+use PVE::SafeSyslog;
+use PVE::Tools qw(extract_param);
+use PVE::Cluster qw(cfs_read_file cfs_write_file);
+use PVE::HA::Config;
+use HTTP::Status qw(:constants);
+use Storable qw(dclone);
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::RPCEnvironment;
+
+use PVE::RESTHandler;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method({
+    name => 'index',
+    path => '',
+    method => 'GET',
+    permissions => { user => 'all' },
+    description => "Node index.",
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            node => get_standard_option('pve-node'),
+        },
+    },
+    returns => {
+        type => 'array',
+        items => {
+            type => "object",
+            properties => {},
+        },
+        links => [{ rel => 'child', href => "{name}" }],
+    },
+    code => sub {
+        my ($param) = @_;
+
+        my $result = [
+            { name => 'maintenance' },
+        ];
+
+        return $result;
+    },
+});
+
+__PACKAGE__->register_method({
+    name => 'maintenance_state',
+    path => 'maintenance',
+    method => 'GET',
+    permissions => { user => 'all' },
+    description => "Get the node maintenance state.",
+    permissions => {
+ check => ['perm', '/', [ 'Sys.Audit' ]],
+    },
+    parameters => {
+        additionalProperties => 0,
+        properties => {
+            node => get_standard_option('pve-node'),
+        },
+    },
+    returns => {
+        type => "object",
+        properties => {},
+    },
+    code => sub {
+        my ($param) = @_;
+
+ my $status = PVE::HA::Config::read_manager_status();
+
+ my $data = {
+            request => $status->{node_request}->{$param->{node}},
+            status => $status->{node_status}->{$param->{node}},
+        };
+
+ return $data;
+    },
+});
+
+__PACKAGE__->register_method ({
+    name => 'maintenance_set',
+    protected => 1,
+    path => 'maintenance',
+    method => 'POST',
+    description => "Change the node-maintenance request state.",
+    permissions => {
+ check => ['perm', '/', [ 'Sys.Console' ]],
+    },
+    parameters => {
+ additionalProperties => 0,
+ properties => {
+     node => get_standard_option('pve-node'),
+     disable => {
+ description => "Requests disabling or enabling maintenance-mode.",
+ type => 'boolean',
+     },
+ },
+    },
+    returns => { type => 'null' },
+    code => sub {
+ my ($param) = @_;
+
+ PVE::Cluster::check_node_exists($param->{node});
+
+ my $cmd = $param->{disable} ? 'disable-node-maintenance' : 'enable-node-maintenance';
+ PVE::HA::Config::queue_crm_commands("$cmd $param->{node}");
+
+ return undef;
+    }
+});
+
+package PVE::API2::HA::Nodes;
+
+use strict;
+use warnings;
+
+use PVE::RESTHandler;
+use PVE::JSONSchema qw(get_standard_option);
+use PVE::RPCEnvironment;
+use PVE::Cluster;
+
+use base qw(PVE::RESTHandler);
+
+__PACKAGE__->register_method({
+    subclass => "PVE::API2::HA::Nodes::Nodeinfo",
+    path => '{node}',
+});
+
+__PACKAGE__->register_method({
+    name => 'index',
+    path => '',
+    method => 'GET',
+    permissions => { user => 'all' },
+    description => "Cluster HA node index.",
+    parameters => {
+        additionalProperties => 0,
+        properties => {},
+    },
+    returns => {
+        type => 'array',
+        items => {
+            type => "object",
+            properties => {
+                node => get_standard_option('pve-node'),
+            },
+        },
+        links => [{ rel => 'child', href => "{node}" }],
+    },
+    code => sub {
+        my ($param) = @_;
+
+        my $rpcenv = PVE::RPCEnvironment::get();
+        my $authuser = $rpcenv->get_user();
+
+        my $clinfo = PVE::Cluster::get_clinfo();
+        my $res = [];
+
+        my $nodelist = PVE::Cluster::get_nodelist();
+        my $members = PVE::Cluster::get_members();
+        my $rrd = PVE::Cluster::rrd_dump();
+
+        foreach my $node (@$nodelist) {
+            my $can_audit = $rpcenv->check($authuser, "/nodes/$node", ['Sys.Audit'], 1);
+            my $entry = { node => $node };
+
+            push @$res, $entry;
+        }
+
+        return $res;
+    },
+});
+
+1;
-- 
2.47.2


_______________________________________________
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] add UI for node maintenance enable/disable

[Thomas Skinner]

Signed-off-by: Thomas Skinner <thomas@atskinner.net>
---
 www/manager6/ha/StatusView.js | 85 +++++++++++++++++++++++++++++++++++
 1 file changed, 85 insertions(+)

diff --git a/www/manager6/ha/StatusView.js b/www/manager6/ha/StatusView.js
index 50ad8e84..79e12df5 100644
--- a/www/manager6/ha/StatusView.js
+++ b/www/manager6/ha/StatusView.js
@@ -41,12 +41,58 @@ Ext.define(
                 },
             });
 
+            let sm = Ext.create('Ext.selection.RowModel', {});
+
+            let caps = Ext.state.Manager.get('GuiCap');
+
+            var node_maintenance_disable = function (disable) {
+                let rec = sm.getSelection()[0];
+                if (!rec || rec.data.type !== "lrm") {
+                    return;
+                }
+                let nodename = rec.get('node');
+                let enableText = disable ? 'Disable' : 'Enable';
+                let msg = Ext.String.format(gettext("{0} maintenance mode on node '{1}'?"), enableText, nodename);
+                Ext.Msg.confirm(gettext('Confirm'), msg, (btn) => {
+                    if (btn === 'yes') {
+                        Proxmox.Utils.API2Request({
+                            params: { disable: disable ? 1 : 0 },
+                            url: '/cluster/ha/nodes/' + nodename + '/maintenance',
+                            method: 'POST',
+                            waitMsgTarget: me,
+                            failure: function (response, opts) {
+                                Ext.Msg.alert(gettext('Error'), response.htmlStatus);
+                            },
+                        });
+                    }
+                });
+            };
+
             Ext.apply(me, {
                 store: store,
+                selModel: sm,
                 stateful: false,
                 viewConfig: {
                     trackOver: false,
                 },
+                tbar: [
+                    {
+                        text: gettext('Enable Maintenance Mode'),
+                        itemId: 'enableMaintBtn',
+                        disabled: true,
+                        handler: function () {
+                            node_maintenance_disable(false);
+                        },
+                    },
+                    {
+                        text: gettext('Disable Maintenance Mode'),
+                        itemId: 'disableMaintBtn',
+                        disabled: true,
+                        handler: function () {
+                            node_maintenance_disable(true);
+                        },
+                    },
+                ],
                 columns: [
                     {
                         header: gettext('Type'),
@@ -60,12 +106,50 @@ Ext.define(
                         dataIndex: 'status',
                     },
                 ],
+                listeners: {
+                    beforeselect: function (tree, record, index, eopts) {
+                        if (!caps.nodes['Sys.Console']) {
+                            return;
+                        }
+                        let enableMaintBtnDisable = true;
+                        let disableMaintBtnDisable = true;
+                        if (record && record.data.type === "lrm") {
+                            if (record.data.lrm_mode && record.data.lrm_mode === 'maintenance') {
+                                disableMaintBtnDisable = false;
+                            } else {
+                                enableMaintBtnDisable = false;
+                            }
+                        }
+                        me.down('#enableMaintBtn').setDisabled(enableMaintBtnDisable);
+                        me.down('#disableMaintBtn').setDisabled(disableMaintBtnDisable);
+                    },
+                }
             });
 
             me.callParent();
 
             me.on('activate', me.rstore.startUpdate);
             me.on('destroy', me.rstore.stopUpdate);
+
+            me.mon(me.rstore, 'load', function (curstore, results) {
+                let rec = sm.getSelection()[0];
+                if (!rec || rec.data.type !== "lrm") {
+                    return;
+                }
+                for (const { data } of results) {
+                    switch (data.type) {
+                        case 'lrm':
+                            if (rec.data.node === data.node) {
+                                let inMaint = rec.data.lrm_mode === 'maintenance';
+                                me.down('#enableMaintBtn').setDisabled(inMaint);
+                                me.down('#disableMaintBtn').setDisabled(!inMaint);
+                            }
+                            break;
+                        default:
+                            break;
+                    }
+                }
+            });
         },
     },
     function () {
@@ -88,6 +172,7 @@ Ext.define(
                 'type',
                 'crm_state',
                 'request_state',
+                'lrm_mode',
                 {
                     name: 'vname',
                     convert: function (value, record) {
-- 
2.47.2


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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Daniel Kral]

Thanks for working on this!

I have added some comments inline. Please run `make tidy` on this patch
(and other patches for that sake) to automatically reformat the code.

See [0] for more information.

[0] https://git.proxmox.com/?p=proxmox-perltidy.git;a=summary

On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  debian/pve-ha-manager.install |   1 +
>  src/PVE/API2/HA/Makefile      |   2 +-
>  src/PVE/API2/HA/Nodes.pm      | 176 ++++++++++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+), 1 deletion(-)
>  create mode 100644 src/PVE/API2/HA/Nodes.pm
>
> diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
> index 2e6b7d5..ec85037 100644
> --- a/debian/pve-ha-manager.install
> +++ b/debian/pve-ha-manager.install
> @@ -15,6 +15,7 @@
>  /usr/share/man/man8/pve-ha-crm.8.gz
>  /usr/share/man/man8/pve-ha-lrm.8.gz
>  /usr/share/perl5/PVE/API2/HA/Groups.pm
> +/usr/share/perl5/PVE/API2/HA/Nodes.pm
>  /usr/share/perl5/PVE/API2/HA/Resources.pm
>  /usr/share/perl5/PVE/API2/HA/Rules.pm
>  /usr/share/perl5/PVE/API2/HA/Status.pm
> diff --git a/src/PVE/API2/HA/Makefile b/src/PVE/API2/HA/Makefile
> index 86c1013..45d714b 100644
> --- a/src/PVE/API2/HA/Makefile
> +++ b/src/PVE/API2/HA/Makefile
> @@ -1,4 +1,4 @@
> -SOURCES=Resources.pm Groups.pm Rules.pm Status.pm
> +SOURCES=Resources.pm Groups.pm Nodes.pm Rules.pm Status.pm
>  
>  .PHONY: install
>  install:
> diff --git a/src/PVE/API2/HA/Nodes.pm b/src/PVE/API2/HA/Nodes.pm
> new file mode 100644
> index 0000000..2afc3a3
> --- /dev/null
> +++ b/src/PVE/API2/HA/Nodes.pm
> @@ -0,0 +1,176 @@
> +package PVE::API2::HA::Nodes::Nodeinfo;

Why use two packages in this file here? AFAICT this seems like it is
copy-pasted from PVE::API2::Nodes and could very well be a single
package as that's much cleaner than having two packages in one file.

Also it seems like we do that seldomly:

# rg -o -c -g "*.pm" "^package" . | awk -F: '{if ($2 > 1){print $1}}'
./pve-guest-common/src/PVE/ReplicationConfig.pm
./pve-buildpkg/PVE/BuildPKG/Tools.pm
./pve-storage/src/PVE/Storage/ESXiPlugin.pm
./pve-manager/PVE/API2/Nodes.pm
./pve-manager/PVE/CLI/pve_network_interface_pinning.pm
./pve-firewall/src/PVE/API2/Firewall/VM.pm
./pve-firewall/src/PVE/API2/Firewall/Rules.pm
./pve-firewall/src/PVE/API2/Firewall/IPSet.pm
./pve-firewall/src/PVE/API2/Firewall/Aliases.pm

> +
> +use strict;
> +use warnings;
> +
> +use PVE::SafeSyslog;
> +use PVE::Tools qw(extract_param);
> +use PVE::Cluster qw(cfs_read_file cfs_write_file);
> +use PVE::HA::Config;
> +use HTTP::Status qw(:constants);
> +use Storable qw(dclone);
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RPCEnvironment;

are extract_param, cfs_{read,write}_file, dclone used?

nit: either way, use statements should follow the dependencies style
guide [1], i.e. something like the following:


use HTTP::Status qw(:constants);
use Storable qw(dclone);

use PVE::Cluster qw(cfs_read_file cfs_write_file);
use PVE::JSONSchema qw(get_standard_option);
use PVE::RPCEnvironment;
use PVE::SafeSyslog;
use PVE::Tools qw(extract_param);

use PVE::HA::Config;


[1] https://pve.proxmox.com/wiki/Perl_Style_Guide#Dependencies

> +
> +use PVE::RESTHandler;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    permissions => { user => 'all' },
> +    description => "Node index.",
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {
> +            node => get_standard_option('pve-node'),
> +        },
> +    },
> +    returns => {
> +        type => 'array',
> +        items => {
> +            type => "object",
> +            properties => {},
> +        },
> +        links => [{ rel => 'child', href => "{name}" }],
> +    },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        my $result = [
> +            { name => 'maintenance' },
> +        ];
> +
> +        return $result;
> +    },
> +});

Hm, we do that differently around the API endpoints, but at least for
the HA-related API endpoints we don't list the subdirectories, but
output the information in the entity itself, e.g. for HA resources:


+ ha
|
+-+ resources     - GET    List HA resources.
  +               - POST   Create a new HA resource.
  |
  +-+ {sid}       - GET    Read resource configuration.
    +             - PUT    Update resource configuration.
    +             - DELETE Delete resource configuration.
    |
    +-+ migrate   - POST   Request resource migration to another node.
      + relocate  - POST   Request resource relocation to another node.


Of course, it doesn't make sense for a POST/DELETE API endpoint for
nodes, but it could still be useful to make the
GET /cluster/ha/nodes/{node} route informational, e.g., output the
current maintenance state and possibly other information about nodes
there.

So something like this:

+ ha
|
+-+ nodes         - GET    List HA nodes.
  |
  +-+ {node}      - GET    Read HA node information.

...

> +
> +__PACKAGE__->register_method({
> +    name => 'maintenance_state',
> +    path => 'maintenance',
> +    method => 'GET',
> +    permissions => { user => 'all' },
> +    description => "Get the node maintenance state.",
> +    permissions => {
> + check => ['perm', '/', [ 'Sys.Audit' ]],
> +    },
> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {
> +            node => get_standard_option('pve-node'),
> +        },
> +    },
> +    returns => {
> +        type => "object",
> +        properties => {},
> +    },
> +    code => sub {
> +        my ($param) = @_;
> +
> + my $status = PVE::HA::Config::read_manager_status();
> +
> + my $data = {
> +            request => $status->{node_request}->{$param->{node}},
> +            status => $status->{node_status}->{$param->{node}},
> +        };
> +
> + return $data;
> +    },
> +});

I think the GET /cluster/ha/nodes/{node}/maintenance API endpoint should
be integrated into the GET /cluster/ha/nodes/{node} instead as putting a
node in/out of maintenance mode is only an action, but the state could
be more than just the node being in maintenance mode.

On second thought, is this API endpoint even used at all in the patch
series?

> +
> +__PACKAGE__->register_method ({
> +    name => 'maintenance_set',
> +    protected => 1,
> +    path => 'maintenance',
> +    method => 'POST',
> +    description => "Change the node-maintenance request state.",
> +    permissions => {
> + check => ['perm', '/', [ 'Sys.Console' ]],
> +    },
> +    parameters => {
> + additionalProperties => 0,
> + properties => {
> +     node => get_standard_option('pve-node'),
> +     disable => {
> + description => "Requests disabling or enabling maintenance-mode.",
> + type => 'boolean',

Hm, I see that that part is from the CLI handler, but there `disable` is
only used for two different subcommands `enable` and `disable`, so
wouldn't it be neater to have two API endpoints for that instead? Like
for example the API endpoints for VM/container start/stop/..., e.g.

- POST enable_maintenance
- POST disable_maintenance

Either way, the CLI command should then use this API handler instead of
the CLI handler to have the logic at one place only, i.e. in
ha_manager.pm

'node-maintenance' => {
    enable => [__PACKAGE__, 'node-maintenance-set', ['node'], { disable => 0 }],
    disable => [__PACKAGE__, 'node-maintenance-set', ['node'], { disable => 1 }],
},

should then be changed to:

'node-maintenance' => {
    enable => ['PVE::API2::HA::Nodes', 'enable_maintenance', ['node']],
    disable => ['PVE::API2::HA::Nodes', 'disable_maintenance', ['node']],
},


> +     },
> + },
> +    },
> +    returns => { type => 'null' },
> +    code => sub {
> + my ($param) = @_;
> +
> + PVE::Cluster::check_node_exists($param->{node});
> +
> + my $cmd = $param->{disable} ? 'disable-node-maintenance' : 'enable-node-maintenance';
> + PVE::HA::Config::queue_crm_commands("$cmd $param->{node}");
> +
> + return undef;
> +    }
> +});
> +
> +package PVE::API2::HA::Nodes;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::RESTHandler;
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::RPCEnvironment;
> +use PVE::Cluster;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method({
> +    subclass => "PVE::API2::HA::Nodes::Nodeinfo",
> +    path => '{node}',
> +});
> +
> +__PACKAGE__->register_method({
> +    name => 'index',
> +    path => '',
> +    method => 'GET',
> +    permissions => { user => 'all' },
> +    description => "Cluster HA node index.",

nit: for consistency's sake with the HA API endpoints, this could e.g.
be "List HA nodes.".. even though they don't really belong to the HA..

> +    parameters => {
> +        additionalProperties => 0,
> +        properties => {},
> +    },
> +    returns => {
> +        type => 'array',
> +        items => {
> +            type => "object",
> +            properties => {
> +                node => get_standard_option('pve-node'),
> +            },
> +        },
> +        links => [{ rel => 'child', href => "{node}" }],
> +    },
> +    code => sub {
> +        my ($param) = @_;
> +
> +        my $rpcenv = PVE::RPCEnvironment::get();
> +        my $authuser = $rpcenv->get_user();
> +
> +        my $clinfo = PVE::Cluster::get_clinfo();
> +        my $res = [];
> +
> +        my $nodelist = PVE::Cluster::get_nodelist();
> +        my $members = PVE::Cluster::get_members();
> +        my $rrd = PVE::Cluster::rrd_dump();
> +
> +        foreach my $node (@$nodelist) {
> +            my $can_audit = $rpcenv->check($authuser, "/nodes/$node", ['Sys.Audit'], 1);

$can_audit is never used here and the check has $noerr set, so it
doesn't do anything..

The more I think about it, I'm somewhat inclined to register the whole
subclass under /nodes/{node}/ha/... instead of /cluster/ha/nodes/... to
not duplicate the information of a nodelist, which is provided by
pve-cluster and there would be something wrong with the HA Manager
anyway if those are out-of-sync and all the HA-related information can
then live in a node-specific "ha" subdirectory.

Then this API handler wouldn't be needed anymore at all.

> +            my $entry = { node => $node };
> +
> +            push @$res, $entry;
> +        }
> +
> +        return $res;
> +    },
> +});
> +
> +1;



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

Re: [pve-devel] [PATCH manager 1/2] add api path map for node HA endpoints

[Daniel Kral]

On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  PVE/API2/HAConfig.pm | 12 +++++++++++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/PVE/API2/HAConfig.pm b/PVE/API2/HAConfig.pm
> index d29211fb..489cbf72 100644
> --- a/PVE/API2/HAConfig.pm
> +++ b/PVE/API2/HAConfig.pm
> @@ -12,6 +12,7 @@ use PVE::JSONSchema qw(get_standard_option);
>  use PVE::Exception qw(raise_param_exc);
>  use PVE::API2::HA::Resources;
>  use PVE::API2::HA::Groups;
> +use PVE::API2::HA::Nodes;
>  use PVE::API2::HA::Rules;
>  use PVE::API2::HA::Status;
>  
> @@ -37,6 +38,11 @@ __PACKAGE__->register_method({
>      path => 'status',
>  });
>  
> +__PACKAGE__->register_method({
> +    subclass => "PVE::API2::HA::Nodes",
> +    path => 'nodes',
> +});
> +

As stated in the ha-manager #2 patch, I'm inclined to register this at
/nodes/{node}/ha/... instead of /cluster/ha/nodes/{nodes}/..., because
then we wouldn't need to duplicate the nodelist in the HA endpoints in
the latter, because it is already provided at GET /nodes. We could still
provide information on GET /nodes/{node}/ha for each node, e.g. the LRM
state, mode, etc.

>  __PACKAGE__->register_method({
>      name => 'index',
>      path => '',
> @@ -63,7 +69,11 @@ __PACKAGE__->register_method({
>          my ($param) = @_;
>  
>          my $res = [
> -            { id => 'status' }, { id => 'resources' }, { id => 'groups' }, { id => 'rules' },
> +            { id => 'status' },
> +            { id => 'resources' },
> +            { id => 'groups' },
> +            { id => 'rules' },
> +            { id => 'nodes' },
>          ];
>  
>          return $res;


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

Re: [pve-devel] [PATCH docs 1/1] add docs for maintenance mode buttons in UI

[Daniel Kral]

On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  ha-manager.adoc | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>
> diff --git a/ha-manager.adoc b/ha-manager.adoc
> index f16cfbb..b41a73b 100644
> --- a/ha-manager.adoc
> +++ b/ha-manager.adoc
> @@ -1115,7 +1115,19 @@ maintenance mode is disabled and the node is back online.
>  Currently you can enabled or disable the maintenance mode using the ha-manager
>  CLI tool.
>  
> +Maintenance mode can be enabled or disabled using the buttons on the HA pane in
> +the {pve} web UI or by using the ha-manager CLI tool.

s/pane/panel/?

I understand that pane might be more correct here, but I'd guess that
users are more familiar with 'panel' here.

Also, I think it would be nicer to integrate that into the former
sentence, for example:

Currently you can enabled or disable the maintenance mode in the HA
status panel in the web UI or using the ha-manager CLI tool.

> +
>  .Enabling maintenance mode for a node
> +
> +_Using the web UI_
> +
> +Navigate to the Datacenter view, then select HA from the menu to show the HA
> +Status pane. Select the lrm node that you want to enable maintenance mode for,
> +then click the Enable Maintenance Mode button from the toolbar.
> +
> +_Using the CLI_
> +
>  ----
>  # ha-manager crm-command node-maintenance enable NODENAME
>  ----
> @@ -1142,6 +1154,15 @@ but only if it is either manually deactivated using the `ha-manager` CLI or if
>  the manager-status is manually cleared.
>  
>  .Disabling maintenance mode for a node
> +
> +_Using the web UI_
> +
> +Navigate to the Datacenter view, then select HA from the menu to show the HA
> +Status pane. Select the lrm node that you want to disable maintenance mode for,
> +then click the Disable Maintenance Mode button from the toolbar.
> +
> +_Using the CLI_
> +
>  ----
>  # ha-manager crm-command node-maintenance disable NODENAME
>  ----



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

Re: [pve-devel] [PATCH manager 2/2] add UI for node maintenance enable/disable

[Daniel Kral]

Hm, the buttons might be a little ambiguous that these are only for LRM
entries... But I think it's a good start as there isn't a dedicated list
for the LRMs which gives more room for action buttons that can be done
on all items. But let's wait for other feedback.

On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> ---
>  www/manager6/ha/StatusView.js | 85 +++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/www/manager6/ha/StatusView.js b/www/manager6/ha/StatusView.js
> index 50ad8e84..79e12df5 100644
> --- a/www/manager6/ha/StatusView.js
> +++ b/www/manager6/ha/StatusView.js
> @@ -41,12 +41,58 @@ Ext.define(
>                  },
>              });
>  
> +            let sm = Ext.create('Ext.selection.RowModel', {});
> +
> +            let caps = Ext.state.Manager.get('GuiCap');
> +
> +            var node_maintenance_disable = function (disable) {

var mustn't be used for new code anymore [0], and new variable names
should be in camelCase [1].

[0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
[1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

this could be an arrow function and the function's variable name is
rather fragile as 'disable' can be set and then does a rather different
action to the node maintenance.. Maybe just "setNodeMaintenance"?

> +                let rec = sm.getSelection()[0];
> +                if (!rec || rec.data.type !== "lrm") {
> +                    return;
> +                }
> +                let nodename = rec.get('node');
> +                let enableText = disable ? 'Disable' : 'Enable';
> +                let msg = Ext.String.format(gettext("{0} maintenance mode on node '{1}'?"), enableText, nodename);
> +                Ext.Msg.confirm(gettext('Confirm'), msg, (btn) => {
> +                    if (btn === 'yes') {
> +                        Proxmox.Utils.API2Request({
> +                            params: { disable: disable ? 1 : 0 },
> +                            url: '/cluster/ha/nodes/' + nodename + '/maintenance',
> +                            method: 'POST',
> +                            waitMsgTarget: me,
> +                            failure: function (response, opts) {
> +                                Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> +                            },
> +                        });
> +                    }
> +                });
> +            };
> +
>              Ext.apply(me, {
>                  store: store,
> +                selModel: sm,
>                  stateful: false,
>                  viewConfig: {
>                      trackOver: false,
>                  },
> +                tbar: [
> +                    {
> +                        text: gettext('Enable Maintenance Mode'),
> +                        itemId: 'enableMaintBtn',
> +                        disabled: true,
> +                        handler: function () {
> +                            node_maintenance_disable(false);
> +                        },

nit: use an arrow function instead

    handler: () => node_maintenance_disable(false),

> +                    },
> +                    {
> +                        text: gettext('Disable Maintenance Mode'),
> +                        itemId: 'disableMaintBtn',
> +                        disabled: true,
> +                        handler: function () {
> +                            node_maintenance_disable(true);

nit: same here

> +                        },
> +                    },
> +                ],
>                  columns: [
>                      {
>                          header: gettext('Type'),
> @@ -60,12 +106,50 @@ Ext.define(
>                          dataIndex: 'status',
>                      },
>                  ],
> +                listeners: {
> +                    beforeselect: function (tree, record, index, eopts) {
> +                        if (!caps.nodes['Sys.Console']) {
> +                            return;
> +                        }
> +                        let enableMaintBtnDisable = true;
> +                        let disableMaintBtnDisable = true;
> +                        if (record && record.data.type === "lrm") {
> +                            if (record.data.lrm_mode && record.data.lrm_mode === 'maintenance') {
> +                                disableMaintBtnDisable = false;
> +                            } else {
> +                                enableMaintBtnDisable = false;
> +                            }
> +                        }
> +                        me.down('#enableMaintBtn').setDisabled(enableMaintBtnDisable);
> +                        me.down('#disableMaintBtn').setDisabled(disableMaintBtnDisable);
> +                    },
> +                }
>              });
>  
>              me.callParent();
>  
>              me.on('activate', me.rstore.startUpdate);
>              me.on('destroy', me.rstore.stopUpdate);
> +
> +            me.mon(me.rstore, 'load', function (curstore, results) {
> +                let rec = sm.getSelection()[0];
> +                if (!rec || rec.data.type !== "lrm") {
> +                    return;
> +                }
> +                for (const { data } of results) {
> +                    switch (data.type) {
> +                        case 'lrm':
> +                            if (rec.data.node === data.node) {
> +                                let inMaint = rec.data.lrm_mode === 'maintenance';
> +                                me.down('#enableMaintBtn').setDisabled(inMaint);
> +                                me.down('#disableMaintBtn').setDisabled(!inMaint);
> +                            }
> +                            break;
> +                        default:
> +                            break;
> +                    }
> +                }
> +            });
>          },
>      },
>      function () {
> @@ -88,6 +172,7 @@ Ext.define(
>                  'type',
>                  'crm_state',
>                  'request_state',
> +                'lrm_mode',
>                  {
>                      name: 'vname',
>                      convert: function (value, record) {



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

Re: [pve-devel] [PATCH manager 1/2] add api path map for node HA endpoints

[Thomas Lamprecht]

On 26/08/2025 11:39, Daniel Kral wrote:
> As stated in the ha-manager #2 patch, I'm inclined to register this at
> /nodes/{node}/ha/... instead of /cluster/ha/nodes/{nodes}/..., because

+1

> then we wouldn't need to duplicate the nodelist in the HA endpoints in
> the latter, because it is already provided at GET /nodes. We could still
> provide information on GET /nodes/{node}/ha for each node, e.g. the LRM
> state, mode, etc.

Just to be sure: do you mean for "each"–as in separately for the respective
node–or for "all"–as in always for all nodes independent on which {node}?
The latter should rather be returned by /cluster/resources, if needed.


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

Re: [pve-devel] [PATCH manager 1/2] add api path map for node HA endpoints

[Daniel Kral]

On Tue Aug 26, 2025 at 12:26 PM CEST, Thomas Lamprecht wrote:
> On 26/08/2025 11:39, Daniel Kral wrote:
>> then we wouldn't need to duplicate the nodelist in the HA endpoints in
>> the latter, because it is already provided at GET /nodes. We could still
>> provide information on GET /nodes/{node}/ha for each node, e.g. the LRM
>> state, mode, etc.
>
> Just to be sure: do you mean for "each"–as in separately for the respective
> node–or for "all"–as in always for all nodes independent on which {node}?
> The latter should rather be returned by /cluster/resources, if needed.

AFAICT the informational endpoints might not be needed anymore for this
patch series anyway (as the lrm_mode is now returned by the HA's status
endpoint), but I meant the former case where GET /nodes/{node}/ha would
have returned the {node}'s LRM state, mode, etc.

On second thought, most users would probably like to know about the
status of all LRMs at once instead of needing to do one-by-one requests,
but that's what GET /cluster/ha/status/current is already here for.


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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Thomas Skinner]

On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.kral@proxmox.com> wrote:
>
> Thanks for working on this!
>
> I have added some comments inline. Please run `make tidy` on this patch
> (and other patches for that sake) to automatically reformat the code.
>
> See [0] for more information.
>
> [0] https://git.proxmox.com/?p=proxmox-perltidy.git;a=summary

I thought I did this on all of them, but I will do it again just in case.

>
> On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  debian/pve-ha-manager.install |   1 +
> >  src/PVE/API2/HA/Makefile      |   2 +-
> >  src/PVE/API2/HA/Nodes.pm      | 176 ++++++++++++++++++++++++++++++++++
> >  3 files changed, 178 insertions(+), 1 deletion(-)
> >  create mode 100644 src/PVE/API2/HA/Nodes.pm
> >
> > diff --git a/debian/pve-ha-manager.install b/debian/pve-ha-manager.install
> > index 2e6b7d5..ec85037 100644
> > --- a/debian/pve-ha-manager.install
> > +++ b/debian/pve-ha-manager.install
> > @@ -15,6 +15,7 @@
> >  /usr/share/man/man8/pve-ha-crm.8.gz
> >  /usr/share/man/man8/pve-ha-lrm.8.gz
> >  /usr/share/perl5/PVE/API2/HA/Groups.pm
> > +/usr/share/perl5/PVE/API2/HA/Nodes.pm
> >  /usr/share/perl5/PVE/API2/HA/Resources.pm
> >  /usr/share/perl5/PVE/API2/HA/Rules.pm
> >  /usr/share/perl5/PVE/API2/HA/Status.pm
> > diff --git a/src/PVE/API2/HA/Makefile b/src/PVE/API2/HA/Makefile
> > index 86c1013..45d714b 100644
> > --- a/src/PVE/API2/HA/Makefile
> > +++ b/src/PVE/API2/HA/Makefile
> > @@ -1,4 +1,4 @@
> > -SOURCES=Resources.pm Groups.pm Rules.pm Status.pm
> > +SOURCES=Resources.pm Groups.pm Nodes.pm Rules.pm Status.pm
> >
> >  .PHONY: install
> >  install:
> > diff --git a/src/PVE/API2/HA/Nodes.pm b/src/PVE/API2/HA/Nodes.pm
> > new file mode 100644
> > index 0000000..2afc3a3
> > --- /dev/null
> > +++ b/src/PVE/API2/HA/Nodes.pm
> > @@ -0,0 +1,176 @@
> > +package PVE::API2::HA::Nodes::Nodeinfo;
>
> Why use two packages in this file here? AFAICT this seems like it is
> copy-pasted from PVE::API2::Nodes and could very well be a single
> package as that's much cleaner than having two packages in one file.

Yes, I did use that file as the source for this package. It could very
well be one package, especially if it was placed elsewhere in the API.
I was trying to mirror the behavior of the existing nodes dir in the
API.

> Also it seems like we do that seldomly:
>
> # rg -o -c -g "*.pm" "^package" . | awk -F: '{if ($2 > 1){print $1}}'
> ./pve-guest-common/src/PVE/ReplicationConfig.pm
> ./pve-buildpkg/PVE/BuildPKG/Tools.pm
> ./pve-storage/src/PVE/Storage/ESXiPlugin.pm
> ./pve-manager/PVE/API2/Nodes.pm
> ./pve-manager/PVE/CLI/pve_network_interface_pinning.pm
> ./pve-firewall/src/PVE/API2/Firewall/VM.pm
> ./pve-firewall/src/PVE/API2/Firewall/Rules.pm
> ./pve-firewall/src/PVE/API2/Firewall/IPSet.pm
> ./pve-firewall/src/PVE/API2/Firewall/Aliases.pm
>
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use PVE::SafeSyslog;
> > +use PVE::Tools qw(extract_param);
> > +use PVE::Cluster qw(cfs_read_file cfs_write_file);
> > +use PVE::HA::Config;
> > +use HTTP::Status qw(:constants);
> > +use Storable qw(dclone);
> > +use PVE::JSONSchema qw(get_standard_option);
> > +use PVE::RPCEnvironment;
>
> are extract_param, cfs_{read,write}_file, dclone used?
>
> nit: either way, use statements should follow the dependencies style
> guide [1], i.e. something like the following:
>
>
> use HTTP::Status qw(:constants);
> use Storable qw(dclone);
>
> use PVE::Cluster qw(cfs_read_file cfs_write_file);
> use PVE::JSONSchema qw(get_standard_option);
> use PVE::RPCEnvironment;
> use PVE::SafeSyslog;
> use PVE::Tools qw(extract_param);
>
> use PVE::HA::Config;
>
>
> [1] https://pve.proxmox.com/wiki/Perl_Style_Guide#Dependencies

I will clean these up in a v2 patch.

> > +
> > +use PVE::RESTHandler;
> > +
> > +use base qw(PVE::RESTHandler);
> > +
> > +__PACKAGE__->register_method({
> > +    name => 'index',
> > +    path => '',
> > +    method => 'GET',
> > +    permissions => { user => 'all' },
> > +    description => "Node index.",
> > +    parameters => {
> > +        additionalProperties => 0,
> > +        properties => {
> > +            node => get_standard_option('pve-node'),
> > +        },
> > +    },
> > +    returns => {
> > +        type => 'array',
> > +        items => {
> > +            type => "object",
> > +            properties => {},
> > +        },
> > +        links => [{ rel => 'child', href => "{name}" }],
> > +    },
> > +    code => sub {
> > +        my ($param) = @_;
> > +
> > +        my $result = [
> > +            { name => 'maintenance' },
> > +        ];
> > +
> > +        return $result;
> > +    },
> > +});
>
> Hm, we do that differently around the API endpoints, but at least for
> the HA-related API endpoints we don't list the subdirectories, but
> output the information in the entity itself, e.g. for HA resources:
>
>
> + ha
> |
> +-+ resources     - GET    List HA resources.
>   +               - POST   Create a new HA resource.
>   |
>   +-+ {sid}       - GET    Read resource configuration.
>     +             - PUT    Update resource configuration.
>     +             - DELETE Delete resource configuration.
>     |
>     +-+ migrate   - POST   Request resource migration to another node.
>       + relocate  - POST   Request resource relocation to another node.
>
>
> Of course, it doesn't make sense for a POST/DELETE API endpoint for
> nodes, but it could still be useful to make the
> GET /cluster/ha/nodes/{node} route informational, e.g., output the
> current maintenance state and possibly other information about nodes
> there.
>
> So something like this:
>
> + ha
> |
> +-+ nodes         - GET    List HA nodes.
>   |
>   +-+ {node}      - GET    Read HA node information.
>
> ...
>
> > +
> > +__PACKAGE__->register_method({
> > +    name => 'maintenance_state',
> > +    path => 'maintenance',
> > +    method => 'GET',
> > +    permissions => { user => 'all' },
> > +    description => "Get the node maintenance state.",
> > +    permissions => {
> > + check => ['perm', '/', [ 'Sys.Audit' ]],
> > +    },
> > +    parameters => {
> > +        additionalProperties => 0,
> > +        properties => {
> > +            node => get_standard_option('pve-node'),
> > +        },
> > +    },
> > +    returns => {
> > +        type => "object",
> > +        properties => {},
> > +    },
> > +    code => sub {
> > +        my ($param) = @_;
> > +
> > + my $status = PVE::HA::Config::read_manager_status();
> > +
> > + my $data = {
> > +            request => $status->{node_request}->{$param->{node}},
> > +            status => $status->{node_status}->{$param->{node}},
> > +        };
> > +
> > + return $data;
> > +    },
> > +});
>
> I think the GET /cluster/ha/nodes/{node}/maintenance API endpoint should
> be integrated into the GET /cluster/ha/nodes/{node} instead as putting a
> node in/out of maintenance mode is only an action, but the state could
> be more than just the node being in maintenance mode.
>
> On second thought, is this API endpoint even used at all in the patch
> series?

The API isn't specifically used here, but we have found it useful in
our local branch to have it so that we can query the transition of the
state of a host directly. I figured someone else might find it useful
as well, so I left it in here. If it should be removed, I can remove
it in the v2 series.

> > +
> > +__PACKAGE__->register_method ({
> > +    name => 'maintenance_set',
> > +    protected => 1,
> > +    path => 'maintenance',
> > +    method => 'POST',
> > +    description => "Change the node-maintenance request state.",
> > +    permissions => {
> > + check => ['perm', '/', [ 'Sys.Console' ]],
> > +    },
> > +    parameters => {
> > + additionalProperties => 0,
> > + properties => {
> > +     node => get_standard_option('pve-node'),
> > +     disable => {
> > + description => "Requests disabling or enabling maintenance-mode.",
> > + type => 'boolean',
>
> Hm, I see that that part is from the CLI handler, but there `disable` is
> only used for two different subcommands `enable` and `disable`, so
> wouldn't it be neater to have two API endpoints for that instead? Like
> for example the API endpoints for VM/container start/stop/..., e.g.
>
> - POST enable_maintenance
> - POST disable_maintenance

For this, I was mirroring the existing underlying API calls as listed
below, but I agree that enable/disable would be more clear. I'll put
that in the v2 series.

> Either way, the CLI command should then use this API handler instead of
> the CLI handler to have the logic at one place only, i.e. in
> ha_manager.pm
>
> 'node-maintenance' => {
>     enable => [__PACKAGE__, 'node-maintenance-set', ['node'], { disable => 0 }],
>     disable => [__PACKAGE__, 'node-maintenance-set', ['node'], { disable => 1 }],
> },
>
> should then be changed to:
>
> 'node-maintenance' => {
>     enable => ['PVE::API2::HA::Nodes', 'enable_maintenance', ['node']],
>     disable => ['PVE::API2::HA::Nodes', 'disable_maintenance', ['node']],
> },

Will update this in the v2 series.

>
> > +     },
> > + },
> > +    },
> > +    returns => { type => 'null' },
> > +    code => sub {
> > + my ($param) = @_;
> > +
> > + PVE::Cluster::check_node_exists($param->{node});
> > +
> > + my $cmd = $param->{disable} ? 'disable-node-maintenance' : 'enable-node-maintenance';
> > + PVE::HA::Config::queue_crm_commands("$cmd $param->{node}");
> > +
> > + return undef;
> > +    }
> > +});
> > +
> > +package PVE::API2::HA::Nodes;
> > +
> > +use strict;
> > +use warnings;
> > +
> > +use PVE::RESTHandler;
> > +use PVE::JSONSchema qw(get_standard_option);
> > +use PVE::RPCEnvironment;
> > +use PVE::Cluster;
> > +
> > +use base qw(PVE::RESTHandler);
> > +
> > +__PACKAGE__->register_method({
> > +    subclass => "PVE::API2::HA::Nodes::Nodeinfo",
> > +    path => '{node}',
> > +});
> > +
> > +__PACKAGE__->register_method({
> > +    name => 'index',
> > +    path => '',
> > +    method => 'GET',
> > +    permissions => { user => 'all' },
> > +    description => "Cluster HA node index.",
>
> nit: for consistency's sake with the HA API endpoints, this could e.g.
> be "List HA nodes.".. even though they don't really belong to the HA..
>
> > +    parameters => {
> > +        additionalProperties => 0,
> > +        properties => {},
> > +    },
> > +    returns => {
> > +        type => 'array',
> > +        items => {
> > +            type => "object",
> > +            properties => {
> > +                node => get_standard_option('pve-node'),
> > +            },
> > +        },
> > +        links => [{ rel => 'child', href => "{node}" }],
> > +    },
> > +    code => sub {
> > +        my ($param) = @_;
> > +
> > +        my $rpcenv = PVE::RPCEnvironment::get();
> > +        my $authuser = $rpcenv->get_user();
> > +
> > +        my $clinfo = PVE::Cluster::get_clinfo();
> > +        my $res = [];
> > +
> > +        my $nodelist = PVE::Cluster::get_nodelist();
> > +        my $members = PVE::Cluster::get_members();
> > +        my $rrd = PVE::Cluster::rrd_dump();
> > +
> > +        foreach my $node (@$nodelist) {
> > +            my $can_audit = $rpcenv->check($authuser, "/nodes/$node", ['Sys.Audit'], 1);
>
> $can_audit is never used here and the check has $noerr set, so it
> doesn't do anything..
>
> The more I think about it, I'm somewhat inclined to register the whole
> subclass under /nodes/{node}/ha/... instead of /cluster/ha/nodes/... to
> not duplicate the information of a nodelist, which is provided by
> pve-cluster and there would be something wrong with the HA Manager
> anyway if those are out-of-sync and all the HA-related information can
> then live in a node-specific "ha" subdirectory.
>
> Then this API handler wouldn't be needed anymore at all.

If this is how Proxmox would like it, that makes it much easier IMO. I
wasn't sure which one to include it under, so I put it under the
cluster path since a cluster must be created to even have the HA
featureset. I'm good with either, I just need to know so that I know
what to update for a v2 series.

> > +            my $entry = { node => $node };
> > +
> > +            push @$res, $entry;
> > +        }
> > +
> > +        return $res;
> > +    },
> > +});
> > +
> > +1;
>
>

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

Re: [pve-devel] [PATCH docs 1/1] add docs for maintenance mode buttons in UI

[Thomas Skinner]

On Tue, Aug 26, 2025 at 4:44 AM Daniel Kral <d.kral@proxmox.com> wrote:
>
> On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  ha-manager.adoc | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> >
> > diff --git a/ha-manager.adoc b/ha-manager.adoc
> > index f16cfbb..b41a73b 100644
> > --- a/ha-manager.adoc
> > +++ b/ha-manager.adoc
> > @@ -1115,7 +1115,19 @@ maintenance mode is disabled and the node is back online.
> >  Currently you can enabled or disable the maintenance mode using the ha-manager
> >  CLI tool.
> >
> > +Maintenance mode can be enabled or disabled using the buttons on the HA pane in
> > +the {pve} web UI or by using the ha-manager CLI tool.
>
> s/pane/panel/?
>
> I understand that pane might be more correct here, but I'd guess that
> users are more familiar with 'panel' here.

Yep, that makes sense to me. Will fix up for a v2 patch.

> Also, I think it would be nicer to integrate that into the former
> sentence, for example:
>
> Currently you can enabled or disable the maintenance mode in the HA
> status panel in the web UI or using the ha-manager CLI tool.

I actually meant for that one to be gone and replaced with the added
sentence instead. I must've missed the removal on my local branch
patch application. Is the replacement sentence okay or should it be
removed and the existing sentence updated?

> > +
> >  .Enabling maintenance mode for a node
> > +
> > +_Using the web UI_
> > +
> > +Navigate to the Datacenter view, then select HA from the menu to show the HA
> > +Status pane. Select the lrm node that you want to enable maintenance mode for,
> > +then click the Enable Maintenance Mode button from the toolbar.
> > +
> > +_Using the CLI_
> > +
> >  ----
> >  # ha-manager crm-command node-maintenance enable NODENAME
> >  ----
> > @@ -1142,6 +1154,15 @@ but only if it is either manually deactivated using the `ha-manager` CLI or if
> >  the manager-status is manually cleared.
> >
> >  .Disabling maintenance mode for a node
> > +
> > +_Using the web UI_
> > +
> > +Navigate to the Datacenter view, then select HA from the menu to show the HA
> > +Status pane. Select the lrm node that you want to disable maintenance mode for,
> > +then click the Disable Maintenance Mode button from the toolbar.
> > +
> > +_Using the CLI_
> > +
> >  ----
> >  # ha-manager crm-command node-maintenance disable NODENAME
> >  ----
>
>

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

Re: [pve-devel] [PATCH manager 2/2] add UI for node maintenance enable/disable

[Thomas Skinner]

On Tue, Aug 26, 2025 at 5:00 AM Daniel Kral <d.kral@proxmox.com> wrote:
>
> Hm, the buttons might be a little ambiguous that these are only for LRM
> entries... But I think it's a good start as there isn't a dedicated list
> for the LRMs which gives more room for action buttons that can be done
> on all items. But let's wait for other feedback.

I wasn't sure where the best place for buttons to go would be, but
Thomas Lamprecht suggested in [1] to place in the HA overview, so
that's where I figured it would fit best. I agree with his logic on
needing to check HA status before enabling/disabling the maintenance.
Selectable grid was an easy implementation there and the status was
already being polled.

[1]: https://lore.proxmox.com/pve-devel/90de8fa6-0e31-4273-adf3-ad337ec39446@proxmox.com/

> On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
> > ---
> >  www/manager6/ha/StatusView.js | 85 +++++++++++++++++++++++++++++++++++
> >  1 file changed, 85 insertions(+)
> >
> > diff --git a/www/manager6/ha/StatusView.js b/www/manager6/ha/StatusView.js
> > index 50ad8e84..79e12df5 100644
> > --- a/www/manager6/ha/StatusView.js
> > +++ b/www/manager6/ha/StatusView.js
> > @@ -41,12 +41,58 @@ Ext.define(
> >                  },
> >              });
> >
> > +            let sm = Ext.create('Ext.selection.RowModel', {});
> > +
> > +            let caps = Ext.state.Manager.get('GuiCap');
> > +
> > +            var node_maintenance_disable = function (disable) {
>
> var mustn't be used for new code anymore [0], and new variable names
> should be in camelCase [1].
>
> [0] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Variables
> [1] https://pve.proxmox.com/wiki/Javascript_Style_Guide#Casing

Some copypasta got the best of me here. Will update for v2.

> this could be an arrow function and the function's variable name is
> rather fragile as 'disable' can be set and then does a rather different
> action to the node maintenance.. Maybe just "setNodeMaintenance"?
>
> > +                let rec = sm.getSelection()[0];
> > +                if (!rec || rec.data.type !== "lrm") {
> > +                    return;
> > +                }
> > +                let nodename = rec.get('node');
> > +                let enableText = disable ? 'Disable' : 'Enable';
> > +                let msg = Ext.String.format(gettext("{0} maintenance mode on node '{1}'?"), enableText, nodename);
> > +                Ext.Msg.confirm(gettext('Confirm'), msg, (btn) => {
> > +                    if (btn === 'yes') {
> > +                        Proxmox.Utils.API2Request({
> > +                            params: { disable: disable ? 1 : 0 },
> > +                            url: '/cluster/ha/nodes/' + nodename + '/maintenance',
> > +                            method: 'POST',
> > +                            waitMsgTarget: me,
> > +                            failure: function (response, opts) {
> > +                                Ext.Msg.alert(gettext('Error'), response.htmlStatus);
> > +                            },
> > +                        });
> > +                    }
> > +                });
> > +            };
> > +
> >              Ext.apply(me, {
> >                  store: store,
> > +                selModel: sm,
> >                  stateful: false,
> >                  viewConfig: {
> >                      trackOver: false,
> >                  },
> > +                tbar: [
> > +                    {
> > +                        text: gettext('Enable Maintenance Mode'),
> > +                        itemId: 'enableMaintBtn',
> > +                        disabled: true,
> > +                        handler: function () {
> > +                            node_maintenance_disable(false);
> > +                        },
>
> nit: use an arrow function instead
>
>     handler: () => node_maintenance_disable(false),

Oh, I didn't know that was possible. Will fix up this one and the next.

> > +                    },
> > +                    {
> > +                        text: gettext('Disable Maintenance Mode'),
> > +                        itemId: 'disableMaintBtn',
> > +                        disabled: true,
> > +                        handler: function () {
> > +                            node_maintenance_disable(true);
>
> nit: same here
>
> > +                        },
> > +                    },
> > +                ],
> >                  columns: [
> >                      {
> >                          header: gettext('Type'),
> > @@ -60,12 +106,50 @@ Ext.define(
> >                          dataIndex: 'status',
> >                      },
> >                  ],
> > +                listeners: {
> > +                    beforeselect: function (tree, record, index, eopts) {
> > +                        if (!caps.nodes['Sys.Console']) {
> > +                            return;
> > +                        }
> > +                        let enableMaintBtnDisable = true;
> > +                        let disableMaintBtnDisable = true;
> > +                        if (record && record.data.type === "lrm") {
> > +                            if (record.data.lrm_mode && record.data.lrm_mode === 'maintenance') {
> > +                                disableMaintBtnDisable = false;
> > +                            } else {
> > +                                enableMaintBtnDisable = false;
> > +                            }
> > +                        }
> > +                        me.down('#enableMaintBtn').setDisabled(enableMaintBtnDisable);
> > +                        me.down('#disableMaintBtn').setDisabled(disableMaintBtnDisable);
> > +                    },
> > +                }
> >              });
> >
> >              me.callParent();
> >
> >              me.on('activate', me.rstore.startUpdate);
> >              me.on('destroy', me.rstore.stopUpdate);
> > +
> > +            me.mon(me.rstore, 'load', function (curstore, results) {
> > +                let rec = sm.getSelection()[0];
> > +                if (!rec || rec.data.type !== "lrm") {
> > +                    return;
> > +                }
> > +                for (const { data } of results) {
> > +                    switch (data.type) {
> > +                        case 'lrm':
> > +                            if (rec.data.node === data.node) {
> > +                                let inMaint = rec.data.lrm_mode === 'maintenance';
> > +                                me.down('#enableMaintBtn').setDisabled(inMaint);
> > +                                me.down('#disableMaintBtn').setDisabled(!inMaint);
> > +                            }
> > +                            break;
> > +                        default:
> > +                            break;
> > +                    }
> > +                }
> > +            });
> >          },
> >      },
> >      function () {
> > @@ -88,6 +172,7 @@ Ext.define(
> >                  'type',
> >                  'crm_state',
> >                  'request_state',
> > +                'lrm_mode',
> >                  {
> >                      name: 'vname',
> >                      convert: function (value, record) {
>
>

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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Thomas Lamprecht]

On 26/08/2025 18:02, Thomas Skinner wrote:
>> The more I think about it, I'm somewhat inclined to register the whole
>> subclass under /nodes/{node}/ha/... instead of /cluster/ha/nodes/... to
>> not duplicate the information of a nodelist, which is provided by
>> pve-cluster and there would be something wrong with the HA Manager
>> anyway if those are out-of-sync and all the HA-related information can
>> then live in a node-specific "ha" subdirectory.
>>
>> Then this API handler wouldn't be needed anymore at all.
> If this is how Proxmox would like it, that makes it much easier IMO. I
> wasn't sure which one to include it under, so I put it under the
> cluster path since a cluster must be created to even have the HA
> featureset. I'm good with either, I just need to know so that I know
> what to update for a v2 series.

We might be already all on the same page, so just to be sure and a
bit more context to avoid making this to HA specific:

Reporting the current (maintenance) status should go into either (or
both of) the /cluster/ha/status and/or the /cluster/resources API
endpoint.

As there are some light-weight plans to provide maintenance mode
for non-ha services, I'd indeed place that outside of a ha specific
path now already, might be even fine to use a dedicated
/nodes/{node}/maintenance API endpoint. For now the description should
state that it's only affecting HA, the non-HA parts are probably not
very complicated work, but got quite a few edge cases and design
decisions that are better done in a separate series building on top
of such API infrastructure you add here.


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

Re: [pve-devel] [PATCH docs 1/1] add docs for maintenance mode buttons in UI

[Daniel Kral]

On Tue Aug 26, 2025 at 6:05 PM CEST, Thomas Skinner wrote:
> On Tue, Aug 26, 2025 at 4:44 AM Daniel Kral <d.kral@proxmox.com> wrote:
>> On Mon Aug 25, 2025 at 6:11 AM CEST, Thomas Skinner wrote:
>> > Signed-off-by: Thomas Skinner <thomas@atskinner.net>
>> > ---
>> >  ha-manager.adoc | 21 +++++++++++++++++++++
>> >  1 file changed, 21 insertions(+)
>> >
>> > diff --git a/ha-manager.adoc b/ha-manager.adoc
>> > index f16cfbb..b41a73b 100644
>> > --- a/ha-manager.adoc
>> > +++ b/ha-manager.adoc
>> > @@ -1115,7 +1115,19 @@ maintenance mode is disabled and the node is back online.
>> >  Currently you can enabled or disable the maintenance mode using the ha-manager
>> >  CLI tool.
>> >
>> > +Maintenance mode can be enabled or disabled using the buttons on the HA pane in
>> > +the {pve} web UI or by using the ha-manager CLI tool.
>> Also, I think it would be nicer to integrate that into the former
>> sentence, for example:
>>
>> Currently you can enabled or disable the maintenance mode in the HA
>> status panel in the web UI or using the ha-manager CLI tool.
>
> I actually meant for that one to be gone and replaced with the added
> sentence instead. I must've missed the removal on my local branch
> patch application. Is the replacement sentence okay or should it be
> removed and the existing sentence updated?

Ah I see, then both are fine even though I'd replace the 'using the
buttons on the HA panel' with just 'in the HA status panel' as you
describe how it's done later already, but that's just nit-picking on my
side ;-).


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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Daniel Kral]

On Tue Aug 26, 2025 at 6:00 PM CEST, Thomas Skinner wrote:
> On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.kral@proxmox.com> wrote:
>> I think the GET /cluster/ha/nodes/{node}/maintenance API endpoint should
>> be integrated into the GET /cluster/ha/nodes/{node} instead as putting a
>> node in/out of maintenance mode is only an action, but the state could
>> be more than just the node being in maintenance mode.
>>
>> On second thought, is this API endpoint even used at all in the patch
>> series?
>
> The API isn't specifically used here, but we have found it useful in
> our local branch to have it so that we can query the transition of the
> state of a host directly. I figured someone else might find it useful
> as well, so I left it in here. If it should be removed, I can remove
> it in the v2 series.

I see, the only thing I'm still worried about is that in the current
state the API endpoint might expose more internal structure than
necessary and should at least be documented in the API schema and/or
abstracted away.

@Thomas do you have an opinion on this?

>
>> > +
>> > +__PACKAGE__->register_method ({
>> > +    name => 'maintenance_set',
>> > +    protected => 1,
>> > +    path => 'maintenance',
>> > +    method => 'POST',
>> > +    description => "Change the node-maintenance request state.",
>> > +    permissions => {
>> > + check => ['perm', '/', [ 'Sys.Console' ]],
>> > +    },
>> > +    parameters => {
>> > + additionalProperties => 0,
>> > + properties => {
>> > +     node => get_standard_option('pve-node'),
>> > +     disable => {
>> > + description => "Requests disabling or enabling maintenance-mode.",
>> > + type => 'boolean',
>>
>> Hm, I see that that part is from the CLI handler, but there `disable` is
>> only used for two different subcommands `enable` and `disable`, so
>> wouldn't it be neater to have two API endpoints for that instead? Like
>> for example the API endpoints for VM/container start/stop/..., e.g.
>>
>> - POST enable_maintenance
>> - POST disable_maintenance
>
> For this, I was mirroring the existing underlying API calls as listed
> below, but I agree that enable/disable would be more clear. I'll put
> that in the v2 series.

Hm, I've only looked at the PVE API now, but one similar feature is the
PBS's maintenance mode for datastores [0], which are set by a single,
enum property 'maintenance-mode' in a PUT request.

Maybe we could add the same PUT method to @Thomas' proposed
/nodes/{node}/maintenance API endpoint, because AFAICT for non-HA use
cases we'd need some way to enable and disable maintenance mode there
too. That's also cleaner than introducing two very similar endpoints
with quite long, weirdly-cased names.

[0] https://pbs.proxmox.com/docs/api-viewer/index.html#/config/datastore/{name}


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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Thomas Lamprecht]

On 27/08/2025 10:26, Daniel Kral wrote:
> On Tue Aug 26, 2025 at 6:00 PM CEST, Thomas Skinner wrote:
>> On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.kral@proxmox.com> wrote:
>>> I think the GET /cluster/ha/nodes/{node}/maintenance API endpoint should
>>> be integrated into the GET /cluster/ha/nodes/{node} instead as putting a
>>> node in/out of maintenance mode is only an action, but the state could
>>> be more than just the node being in maintenance mode.
>>>
>>> On second thought, is this API endpoint even used at all in the patch
>>> series?
>>
>> The API isn't specifically used here, but we have found it useful in
>> our local branch to have it so that we can query the transition of the
>> state of a host directly. I figured someone else might find it useful
>> as well, so I left it in here. If it should be removed, I can remove
>> it in the v2 series.
> 
> I see, the only thing I'm still worried about is that in the current
> state the API endpoint might expose more internal structure than
> necessary and should at least be documented in the API schema and/or
> abstracted away.
> 
> @Thomas do you have an opinion on this?
I'd leave it out for now, that transition could be conveyed in the HA
status in any case and also for the node entries in /cluster/resources,
it could be argued that doing the latter only makes really sense once
the maintenance mode is really generic, but not _that_ hard feelings
here; in any case, with the status there we can cheaply show a
maintenance icon in the web UI's resource tree.


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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Thomas Skinner]

On Wed, Aug 27, 2025 at 3:25 AM Daniel Kral <d.kral@proxmox.com> wrote:
>
> On Tue Aug 26, 2025 at 6:00 PM CEST, Thomas Skinner wrote:
> > On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.kral@proxmox.com> wrote:
> >> I think the GET /cluster/ha/nodes/{node}/maintenance API endpoint should
> >> be integrated into the GET /cluster/ha/nodes/{node} instead as putting a
> >> node in/out of maintenance mode is only an action, but the state could
> >> be more than just the node being in maintenance mode.
> >>
> >> On second thought, is this API endpoint even used at all in the patch
> >> series?
> >
> > The API isn't specifically used here, but we have found it useful in
> > our local branch to have it so that we can query the transition of the
> > state of a host directly. I figured someone else might find it useful
> > as well, so I left it in here. If it should be removed, I can remove
> > it in the v2 series.
>
> I see, the only thing I'm still worried about is that in the current
> state the API endpoint might expose more internal structure than
> necessary and should at least be documented in the API schema and/or
> abstracted away.
>
> @Thomas do you have an opinion on this?
>
> >
> >> > +
> >> > +__PACKAGE__->register_method ({
> >> > +    name => 'maintenance_set',
> >> > +    protected => 1,
> >> > +    path => 'maintenance',
> >> > +    method => 'POST',
> >> > +    description => "Change the node-maintenance request state.",
> >> > +    permissions => {
> >> > + check => ['perm', '/', [ 'Sys.Console' ]],
> >> > +    },
> >> > +    parameters => {
> >> > + additionalProperties => 0,
> >> > + properties => {
> >> > +     node => get_standard_option('pve-node'),
> >> > +     disable => {
> >> > + description => "Requests disabling or enabling maintenance-mode.",
> >> > + type => 'boolean',
> >>
> >> Hm, I see that that part is from the CLI handler, but there `disable` is
> >> only used for two different subcommands `enable` and `disable`, so
> >> wouldn't it be neater to have two API endpoints for that instead? Like
> >> for example the API endpoints for VM/container start/stop/..., e.g.
> >>
> >> - POST enable_maintenance
> >> - POST disable_maintenance
> >
> > For this, I was mirroring the existing underlying API calls as listed
> > below, but I agree that enable/disable would be more clear. I'll put
> > that in the v2 series.
>
> Hm, I've only looked at the PVE API now, but one similar feature is the
> PBS's maintenance mode for datastores [0], which are set by a single,
> enum property 'maintenance-mode' in a PUT request.
>
> Maybe we could add the same PUT method to @Thomas' proposed
> /nodes/{node}/maintenance API endpoint, because AFAICT for non-HA use
> cases we'd need some way to enable and disable maintenance mode there
> too. That's also cleaner than introducing two very similar endpoints
> with quite long, weirdly-cased names.
>
> [0] https://pbs.proxmox.com/docs/api-viewer/index.html#/config/datastore/{name}
>

If the API for maintenance mode is under the /nodes/{node}/maintenance
as @Thomas proposed, does it still make sense to put the code for the
API into the ha-manager package instead of the manager package? I like
the idea of the enum under this endpoint e.g. something like PUT
/nodes/{node}/maintenance --maintenance-mode enable|disable. To be
more specific to HA, could use something like --lrm-maintenance
enable|disable that way the maintenance-mode enum could stay free if
it was wanted for the more generic maintenance mode and it would
convey to the user that it was specific to HA only.

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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Thomas Skinner]

On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.kral@proxmox.com> wrote:
>
> Thanks for working on this!
>
> I have added some comments inline. Please run `make tidy` on this patch
> (and other patches for that sake) to automatically reformat the code.
>
> See [0] for more information.
>
> [0] https://git.proxmox.com/?p=proxmox-perltidy.git;a=summary

The tidy command as written does not currently include untracked
files, so that's why my newly added file did not get caught by the
formatter. Looking at git ls-files docs, adding the -co (tracked and
untracked) and the --exclude-standard (use standard exclusions)
parameters would handle targeting untracked files as well.

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

Re: [pve-devel] [PATCH ha-manager 2/2] add api getter/setter for node maintenance mode

[Daniel Kral]

On Fri Aug 29, 2025 at 10:13 PM CEST, Thomas Skinner wrote:
> On Wed, Aug 27, 2025 at 3:25 AM Daniel Kral <d.kral@proxmox.com> wrote:
>> Hm, I've only looked at the PVE API now, but one similar feature is the
>> PBS's maintenance mode for datastores [0], which are set by a single,
>> enum property 'maintenance-mode' in a PUT request.
>>
>> Maybe we could add the same PUT method to @Thomas' proposed
>> /nodes/{node}/maintenance API endpoint, because AFAICT for non-HA use
>> cases we'd need some way to enable and disable maintenance mode there
>> too. That's also cleaner than introducing two very similar endpoints
>> with quite long, weirdly-cased names.
>>
>> [0] https://pbs.proxmox.com/docs/api-viewer/index.html#/config/datastore/{name}
>>
>
> If the API for maintenance mode is under the /nodes/{node}/maintenance
> as @Thomas proposed, does it still make sense to put the code for the
> API into the ha-manager package instead of the manager package? I like
> the idea of the enum under this endpoint e.g. something like PUT
> /nodes/{node}/maintenance --maintenance-mode enable|disable. To be
> more specific to HA, could use something like --lrm-maintenance
> enable|disable that way the maintenance-mode enum could stay free if
> it was wanted for the more generic maintenance mode and it would
> convey to the user that it was specific to HA only.

If it is under /nodes/{node}/maintenance, then yes, I also think it
should go in the pve-manager package.

IMO for the maintenance-mode parameter it's fine to make it generic
already but stating in the description that it's only affecting the HA
for now, as introducing `lrm-maintenance` might cause confusion later on
when users want to put the whole node under maintenance.. Except if
there are use cases to only 'partially' put the node in maintenance
through the API.

Then it should be rather straight-forward to implement the functionality
with issuing the CRM commands to the HA Manager with

    run_command(['ha-manager', 'crm-command', 'node-maintenance', $maintenance_mode]);

where $maintenance_mode is either 'enable' or 'disable'.


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