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 <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. 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
