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

["Daniel Kral" <d.kral@proxmox.com>]

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