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

[Thomas Lamprecht]

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?

> (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,...)
> https://github.com/Telmate/terraform-provider-proxmox/issues/168
> 
> 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:
> https://bugzilla.proxmox.com/show_bug.cgi?id=4689
> 
> 
> 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.

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

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.