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

[Thomas Skinner <thomas@atskinner.net>]

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