Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On January 26, 2022 12:40 pm, Fabian Ebner wrote:
> Am 13.01.22 um 11:08 schrieb Fabian Ebner:
>> +
>> +	    if (my $source = delete $disk->{'import-from'}) {
> 
> I'm adding a comment here in v11, because otherwise it's not clear where 
> volume activation happens:
> +               # abs_filesystem_path also calls activate_volume when 
> $source is a volid
> 
> I'm also adding "The source should not be actively used by another 
> process!" to the description of the import-from parameter in v11.

sounds good

> 
>> +		$source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
> 
> But there are a couple of issues here:
> 
> 1. There's no protection against using a source volume that's actively 
> used by a guest/other operation. While it's not possible to detect in 
> general, I wonder if we should behave more like a full clone and lock 
> the owning VM?
> 
> 1a. we could check if the volume is referenced in the config/snapshots, 
> but migration picks up everything, so it might be preferable not to.
> 
> 1b. the volume might be configured in a VM that doesn't own it...
> 
> 2. Related: avoiding concurrent activation of volumes on a shared LVM.
> 
> 3. Related: cannot deactivate any volumes as the might be used by 
> something else.
> 
> 4. abs_filesystem_path does not work for RBD when krbd=0, because the 
> plugin produces an "rbd:XYZ" path and the -f || -b check doesn't like 
> that. But full clone does work, passing the volid to qemu_img_convert 
> and that's likely what we should do here as well, when we are dealing 
> with an existing volid rather than an absolute path.
> 
> 5. Add your own ;)
> 
> TL;DR: I'd like to behave much more like full clone, when we are dealing 
> with a volid rather than an absolute path.

yeah. it sounds to me like we could do most of that properly by just 
(iff import source is a volume) check whether the owning VM resides on 
the current node, and lock if so (and fail if not?). not sure whether 
we'd want to require it to be stopped as well if the volume is 
referenced in the current config? for full clones we pass the 'running' 
state to the storage layer's volume_has_feature - but that seems to not 
use the information in any way?

that way we can skip deactivation altogether (it's only relevant for 
shared storages that require it for migration, and by locking the owning 
VM and having a requirement for it to be on the same node at import-time, 
no migration can happen in parallel anyway..). or we could deactivate if 
an owning VM exists and is not running, like we do at the end of full 
clones.

1b is 'all bets are off' territory anyway IMHO - there is no sane way to 
handle all the edge cases..

> 
>> +		my $src_size = PVE::Storage::file_size_info($source);
>> +		die "Could not get file size of $source" if !defined($src_size);
>> +
>> +		my (undef, $dst_volid) = PVE::QemuServer::ImportDisk::do_import(
>