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

[Thomas Skinner <thomas@atskinner.net>]

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