[pve-devel] pve-manager: bug or feature?

[Fabian Grünbichler]

On Tue, Jun 13, 2017 at 11:48:02AM +0200, Michael Rasmussen wrote:
> On Mon, 12 Jun 2017 13:20:13 +0200
> Fabian Grünbichler <f.gruenbichler at proxmox.com> wrote:
> 
> > 
> > you are wrong - vdisk_free is called, but if it fails it is treated as
> > non-fatal error (since we want to destroy the whole VM), and a simple
> > warning is logged. this is correct and intended, because once you delete
> > the first disk, the VM is broken anyhow, even if you can't free the
> > second(/third/fourth/..) disk...
> > 
> You are right. vdisk_free is called but bails before the free_image
> method is called in the storage plugin.
> die "base volume '$volname' is still in use by linked clones\n"
>     if &$volume_is_base_and_used__no_lock($scfg, $storeid, $plugin, $volname);
> 
> So if vdisk_free bails why is the vmid.conf file removed? Bailing here
> means nothing has changed but because vmid.conf is removed the user
> cannot get to the vm anymore through the gui and are left to handle the
> situation from the command line. IMHO the user should not be force to
> go to the command line if there is no reason to do this, command line
> should always be last resort.

see below for the (special) template case. in general, vdisk_free can
fail for any number of reasons. as soon as the first disk is freed, the
config is invalid anyway (and cannot be deleted any more, if a
requirement for deletion is that vdisk_free must not fail like you
propose). so the sane way to handle this is to warn the user that the
destroy operation of the VM was not complete (hence the warning about
failing to free a disk), but continue anyway in order to free as many
disks as possible.

think about the following VM config:

virtio0: somestorage:vm-disk-XXX-1,...
virtio1: storagewhichisoffline:vm-disk-XXX-1,...
virtio2: somestorage:vm-disk-XXX-2,...
virtio3: somestorage:vm-disk-XXX-3,...
scsi0: somestorage:vm-disk-XXX-4,...
scsi1: somestorage:vm-disk-XXX-5,...

you want to destroy this VM. virtio0 gets freed (successfully). virtio1
cannot be freed because the storage is not there (anymore or
temporarily?). now you bail out - either you leave the config as is
(which means it references a disk which has been freed!) or you just
remove virtio0. in both cases, the VM is broken, but there is no
indication for this whatsoever in the config.

by contrast, our current code will free all disks except virtio1, warn
you about (potentially) leaving the volume referenced by virtio1 around,
and successfully remove the VM config. you can now clean up the volume
(if it actually still exists - this is not a given!) via the API or
pvesm, or ignore the warning and (again - potentially!) waste some
space.

> 
> > vdisk_free does a check if the volume is a base volume which is still
> > referenced by a linked clone, which is why you get the warning in the
> > log. it probably makes sense to add an additional check and iterate over
> > all disks and check if they are base and used iff the VM is a template.
> > 
> I don't see the point. If a VM is converted to a template all disks, by
> convention, will be referenced by linked clones if linked clones exists.
> 

in this very specific case - yes. which is why I suggested adding an
extra check for "template && any volume still used in a linked clone",
before iterating over the volumes in order to free them. that way, we
know we are not in the "partially destroyed VM" territory and can
(safely!) bail out with a nice error message.