[pve-devel] avoiding VMID reuse

[Wolfgang Bumiller]

On Wed, Apr 11, 2018 at 03:12:17PM +0300, Lauri Tirkkonen wrote:
> On Tue, Mar 13 2018 11:15:23 +0200, Lauri Tirkkonen wrote:
> > > Sorry if I misunderstood you but VMIDs are already _guaranteed_ to be
> > > unique cluster wide, so also unique per node?
> > 
> > I'll try to clarify: if I create a VM that gets assigned vmid 100, and
> > use zfs for storage, its first disk image is called
> > <zfsstorage>/vm-100-disk-1. If I then later remove vmid 100, and create
> > another new VM, /nextid will suggest that the new vmid should be 100,
> > and its disk image will also be called vm-100-disk-1. We're backing up
> > our disk images using zfs snapshots and sends (on other ZFS hosts too,
> > not just PVE), so it's quite bad if we reuse a name for a completely
> > different dataset - it'll require manual admin intevention. So, we want
> > to avoid using any vmid that has been used in the past.
> > 
> > > Your approach, allowing different nodes from a cluster to alloc
> > > the same VMID also should not work, our clustered configuration
> > > file system (pmxcfs) does not allows this, i.e. no VMID.conf
> > > file can exist multiple times in the qemu-server and lxc folders
> > > ()i.e., the folders holding CT/VM configuration files)
> > 
> > Right. We're not actually running PVE in a cluster configuration, so I
> > might've been a little confused there - if the VMID's are unique in the
> > cluster anyway, then the storage for the counter shouldn't be under
> > "local/", I suppose.
> 
> I took another stab at this, dropping the local/ and making it generally
> less error prone. So to recap, it:
>  - stores next unused vm id in /etc/pve/nextid
>  - returns that stored id in API requests for /cluster/nextid (or
>    highest current existing vmid+1, if nextid is lower and thus out of
>    sync)
>  - PVE::Cluster::alloc_vmid allocates the requested vm id, by storing
>    it into /etc/pve/nextid if it is higher than what there is currently
>    (using lower, non-existing id's is still allowed)
> 
> Thoughts?

Code/style issues aside, in the current shape this is a
convenience-driven upstream API change for a specific use case /
personal preference.

NAK.

I could imagine having an opt-in change to nextid, but it would have to
adhere to our coding style (and use file_get/set_contents, correct
locking to avoid races etc.), and would have to fix the shortcomings
mentioned in the thread already. (One possibility would be to track a
list/bitmap of actually used IDs to avoid the "allocating 99999 once"
issue, and stick to the previous behavior if no such file was
initialized. Another possibility would be making it call out to a helper
script if one exists - not sure what the others think about this - but
the default/main behavior should not be changed.)

(As a side note, if nextid was to get "smarter", my personal next step
would be making it take user permissions into account.)