On Tue, Aug 26, 2025 at 4:38 AM Daniel Kral <d.k...@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 <tho...@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
