Re: [pve-devel] [PATCH-SERIES pve-manager/qemu-server] fix#4689 autofind node with proxyto_callback

["DERUMIER, Alexandre" <alexandre.derumier@groupe-cyllene.com>]

Le samedi 03 juin 2023 à 15:57 +0200, Thomas Lamprecht a écrit :
> Hi!
> 
> Am 01/06/2023 um 00:28 schrieb Alexandre Derumier:
> > Currently, to manage qemu && lxc vms, we always need to specify
> > nodename in uri.
> > 
> > This is a problem with automation tools like terraform, where is
> > node is registered
> > in the state of terraform.
> 
> What do you need here, the whole API, just some operation on existing
> VMs
> like start, stop, migrate, or just that + VM creation?

It's really for all vm operations including modification.
Basicaly, terraform is statefull, than mean than if you create a vm 
with param --node X  , It'll keep the node x  in his state.

if you (or the HA) is moving the vm, terraform don't known about it.
(then if you relaunch terraform, it'll try to recreate the vm again)


> 
> > (That mean, than if we move the vm on another node, terraform don't
> > known it, and try to create the vm
> > again or can't delete the vm,...)
> > 
> > 
> > This can also be a potential problem with race, if we need to query
> > /cluster/ressources to find the node, then another
> > query on the vm.
> > 
> > I have some discussion with fabian about it:
> > 
> > 
> > This patch series, find the nodename with proxyto_callback.
> > > a new api endpoint /guests/  is defined:
> > 
> > /guests/(qemu|lxc)/vmid
> > 
> 
> exposing the same API through multiple points isn't really REST-y
> design,
> could even break things and would def. need special handling to make
> this
> actually visible in the API schema, and thus pvesh and the api
> viewer, among
> other things.

Yes, that's why my v1 was a simple uri rewrite.

> 
> > 
> > This patch series currently implement callback for api2::qemu
> > 
> > I'm not sure how to create vm_create when vmid && nodename is not
> > defined.
> > Currently the callback return localhost, so the vm is created on
> > the called
> > node.
> > 
> > todo (if this patch serie is ok for you):
> 
> 
> ATM I'd rather strongly object this, for one to avoid that more work
> is done that
> then might not get accepted, which would be avoidable waste for all
> of us, and as
> temporary reason: this definitively won't get into Proxmox VE 8.0,
> but as new
> functionality, which doesn't breaks existing stuff, it can be added
> just fine to
> 8.1 or 8.2 – and so I'd like to postpone in-depth review and
> accepting it into
> the source tree until a few weeks after 8.0 Release where I got a bit
> more time
> to calmly think about this – because the base idea of having such a
> feature
> is definitively compelling and I think quite a few admins and devs
> that re-integrate
> our API would like to see this.
> 
yes, no problem.

> Still, some thoughts that I couldn't suppress ;P:
> 
> - the datacenter manager might avoid a lot of such issues already,
> there we
>   need to resolve Guest ID's to nodes anyway. But, it'd require
> having that
>   setup always, which might not be wanted.
> 
> - could putting an adapter between Terraform <-> Proxmox VE API work?
>   Did not really use Terraform, so I'm just guesstimating here.
> 
> - FWIW; the HA stack already is somewhat of a automagic node
> resolver, but
>   naturally only for operative things like start/stop/migrate, but if
> one would
>   just require those actions then it might be a feasible way that
> already exists.
> 
> - We got the Batch-process API calls Endpoint already and while I
> actually planned
>   to remove that for PVE 8 (for other, mostly security reasons), if
> we'd keep (and
>   fix) that, one could potentially also add proxying  and relaying
> support there.
> 
> That said, I have to admit that the solution you choose, while being
> a bit hacky is
> really not a invasive change code-wise; But, and here still assuming
> we go that
> direction in the first place, I  still don't like doing that in such
> a subtle way.
> 
> Rather, I'd add something like a "inherit" property that
> register_method understands
> and then we could re-register API endpoint paths under another path,
> while letting
> the schema and routing actually know about it. 
> 
> I'd would probably do that even in a lightweight way, i.e., resolved
> dynamically and
> then also calling the "actual" original code site, to avoid potential
> bugs w.r.t.
> imports and global variables from more in-depth copies.
> 
> I.e., usage would look something like:
> 
> 
> pve-manager:# cat PVE/API2/Guests/Qemu.pm
> 
> # ...
> 
> use PVE::API2::Qemu; # <- required to ensure the methods we want to
> inherit from got registered
> 
> use base qw(PVE::RESTHandler);
> 
> __PACKAGE__->register_method({
>     name => 'index',
>     path => '{vmid}',
>     # ...
>     code => sub {
>         return [
>             { name => 'config' },
>             # ...
>         ];
> });
> 
> __PACKAGE__->register_method({
>     name => 'set_config',
>     path => '{vmid}',
>     method => 'GET',
>     inherit => 'nodes/{nodes}/qemu/{vmid}/config',
>     proxyto_callback => \&PVE::API2Tools::resolve_vmid_node,
> });
> 
> # ... other api endpoint's we like to expose in a node-independent
> way.
> 
> 1;
> 
> 
> For that to work we'd (probably, I did not check _that_ closely) need
> to adapt the base
> RestHandler's register_method and map_path_to_methods, but for either
> the changes should
> be in reasonable size.
> 
> For starters I'd only allow-list a few properties that can be
> overridden on such a
> inheritance, as e.g., exposing the same thing with different
> privileges might be
> questionable at best and give us some security woes.
> 
> I had something like above approach in my mind for a few other things
> already in the past,
> e.g., in the Proxmox Mail Gateway we have a few places where we
> register the essentially
> same API endpoint a few times, and do that with some adhoc loop and a
> bit of not _that_
> nice code; but mostly that wasn't so bad to require change and thus I
> did not felt that
> such inheritance was required just due to that.
> 
> But, if we do something like you want to have here as end result,
> above would be for me the
> way I'd find the most acceptable for this. Albeit, with the
> disclaimer that without thinking
> this fully through and having my brain somewhat melted by the
> absolutely huge amount of
> packages, and churn it has been through for upcoming PVE 8 ;-P
> 
> IMO it would be nice as it would be really explicit, ensure that we
> can get it easily into
> API schema dumps without having to adapt the code managing that (at
> least not in a big way)
> and one could even easily create a CLI tool for cluster-local
> convenience things like
> "start that VM but I don't care where it currently is"
> 
> ps. sorry for dumping a lot of loosely organized info/questions/idea
> here in this reply,
> but I tried to get as much into it to remember when returning to
> this, after PVE 8.0 release
> and post-release work is done.
>