On 10/18/2016 08:49 AM, Fabian Grünbichler wrote:
On Mon, Oct 17, 2016 at 05:44:31PM +0200, Thomas Lamprecht wrote:
The get /cluster/nextid API call is not secured against race conditions and
parallel accesses in general.
Users of the API which created multiple CT in parallel or at least very fast
after each other run into problems here: multiple calls got the same VMID
returned.
Fix this by allowing the /cluster/nextid call to virtually reserve VMIDs
temporary.
Also add the possibility that the create_vm calls from CTs and VMs can auto
select a VMID, for convenience (also it was requested as a feature).
This means changes on a few packages (cluster, common, container,
qemu-server)
The first to patches should be quite straight forward and relevant for the
bug fix.
The other three may be seen as RFC.
I think "reserve" is a bit misleading here - if I GET cluster/nextid
with reserve set, it returns an ID. but now anybody can use this ID
(create/restore VM/CT), not just me. this is not what I would expect ;)
That's why its written in the description of the parameter :)
Also our next_unused_port has the same problem.
A reservation is always just virtual if I do not actually take the resource.
In this case that would be creating a VMID.conf and say to the create
call that it can overwrite it if empty.
Even if I make checks API wise or something with tokens a
# touch /etc/pve/qemu-server/$VMID.conf
will overwrite it (if we do not integrate it in the pmxcfs)
I know the purpose is to take it out of the "pool" that the nextid API
call uses, but IMHO for that we could just change the default behaviour
of nextid to always do that (there's no harm, especially if it's
documented and just for 60 seconds) and drop the reserve parameter
altogether (maybe introduce a switch to get the old, non-"reserving"
behaviour back?).
Yeah I had it initially that way, but as its a problem mainly happening
at the API level it let it as opt in.
But yes, I agree here.
If you want to take it a step further into the direction of actual
reservation, you could (optionally, with "reserve" set) return an ID
plus token pair, and only allow usage of that ID (for the set amount of
time) when the given token is also passed to the create/restore API.
All of the above can of course be combined as well:
- default: ID without token, but taken out of pool
- with "reserve" set: ID plus token, taken out of pool
- with "legacy" set: ID, not taken out of pool
The token seems not a bad idea, I'll look into it.
___
pve-devel mailing list
pve-devel@pve.proxmox.com
http://pve.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
