[pve-devel] [PATCH qemu-server v2] Fix #2412: Missing VMs in pools

[Thomas Lamprecht]

On 10/14/19 1:10 PM, Dominic Jäger wrote:
> Between calling vm_destroy and removing the ID from user.cfg (remove_vm_access)
> creating a new VM with this ID was possible. VMs could go missing from pools as
> a consequence.
> 
> Adding a lock solves this for clones from the same node. Additionally,
> unlinking must happen at the very end of the deletion process to avoid that
> other nodes use the ID in the meanwhile.
> 
> Co-developed-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> v1->v2: Move unlink to the end of the deletion process
> 
>  PVE/API2/Qemu.pm | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 267a08e..f84aca7 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -1492,9 +1492,14 @@ __PACKAGE__->register_method({
>  	    my $upid = shift;
>  
>  	    syslog('info', "destroy VM $vmid: $upid\n");
> -	    PVE::QemuServer::vm_destroy($storecfg, $vmid, $skiplock);

so skiplock is now a dead parameter? I mean that /could/ be OK, but the
commit never mentions anywhere why this is the case. No users, just kept
for backward compat? If so I'd at least adapt the params description, so
people reading man pages or CLI help output can have an idea that this
does nothing.
Or is it in use and still required to be handled? If so, address that.
Either way, just "silently" removing it is not OK.

> -	    PVE::AccessControl::remove_vm_access($vmid);
> -            PVE::Firewall::remove_vmfw_conf($vmid);
> +	    PVE::QemuConfig->lock_config($vmid, sub {
> +		die "VM $vmid is running - destroy failed\n"
> +		    if (PVE::QemuServer::check_running($vmid));
> +		PVE::QemuServer::destroy_vm($storecfg, $vmid, 1, 0);
> +		PVE::AccessControl::remove_vm_access($vmid);
> +		PVE::Firewall::remove_vmfw_conf($vmid);
> +		unlink PVE::QemuConfig->config_file($vmid);

unlink does not errors itself if the removal failed (e.g., pmxcfs went just
inquorate, so please do a
unlink ... or die "final removal of $vmid config failed: $!\n";

to tell an user in such a case.

Looks OK, besides those two things.

> +	    });
>  	};
>  
>  	return $rpcenv->fork_worker('qmdestroy', $vmid, $authuser, $realcmd);
>