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

[Fabian Ebner]

Am 26.01.22 um 13:42 schrieb Fabian Grünbichler:
> 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 clone, we use qemu_drive_mirror if the VM is running (and not
cloning from a snapshot). I'll think about re-using clone_disk(), then
having a running VM shouldn't be a problem either.

> 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?

Yes, seems like it was a intended for future use, but was never actually
required (well, some external plugin might be using it).

> 
> 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.

Sounds good. We actually only deactivate at the end of full clone if the
clone was to a different target node. Since the new config moves to a
different node then, deactivating the cloned volumes is required of
course, but I /think/ deactivating the source volumes is actually
optional (why should it depend on whether there's a target or not?).

> 
> 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(
>>