[pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF

Fabian Grünbichler f.gruenbichler at proxmox.com
Tue Jan 18 11:19:41 CET 2022


On January 18, 2022 10:08 am, Fabian Ebner wrote:
> Am 17.01.22 um 16:43 schrieb Fabian Grünbichler:
>> On January 13, 2022 11:08 am, Fabian Ebner wrote:
>>> Extend qm importdisk/importovf functionality to the API.
>>>
>>>
>>> Used Dominic's latest version[0] as a starting point. GUI part still
>>> needs to be rebased/updated, so it's not included here.
>>>
>>>
>>> Changes from v9:
>>>
>>> * Split patch into smaller parts
>>>
>>> * Some minor (style) fixes/improvements (see individual patches)
>>>
>>> * Drop $manifest_only parameter for parse_ovf
>>>
>>>      Instead, untaint the path when calling file_size_info, which makes
>>>      the call also work via API which uses the -T switch. If we do want
>>>      to keep $manifest_only, I'd argue that it should also skip the
>>>      file existence check and not only the file size check. Opinions?
>>>
>>> * Re-use do_import function
>>>
>>>      Rather than duplicating most of it. The down side is the need to
>>>      add a new parameter for skipping configuration update. But I
>>>      suppose the plan is to have qm import switch to the new API at
>>>      some point, and then do_import can be simplified.
>>>
>>> * Instead of adding an import-sources parameter to the API, use a new
>>>    import-from property for the disk, that's only available with
>>>    import/alloc-enabled API endpoints via its own version of the schema
>>>
>>>      Avoids the split across regular drive key parameters and
>>>      'import_soruces', which avoids quite a bit of cross-checking
>>>      between the two and parsing/passing around the latter.
>>>
>>>      The big downsides are:
>>>      * Schema handling is a bit messy.
>>>      * Need to special case print_drive, because we do intermediate
>>>        parse/print to cleanup drive paths. At a first glance, this
>>>        seems not too easy to avoid without complicating things elsewehere.
>>>      * Using the import-aware parse_drive in parse_volume, because that
>>>        is used via the foreach_volume iterators handling the parameters
>>>        of the import-enabled endpoints. Could be avoided by using for
>>>        loops with the import-aware parse_drive instead of
>>>        foreach_volume.
>>>
>>>      Counter-arguments for using a single schema (citing Fabian G.):
>>>      * docs/schema dump/api docs: shouldn't look like you can put that
>>>        everywhere where we use the config schema
>>>      * shouldn't have nasty side-effects if someone puts it into the
>>>        config
>>>
>>>
>>> After all, the 'import-from' disk property approach turned out to be
>>> a uglier than I hoped it would.
>>>
>>> My problem with the 'import-sources' API parameter approach (see [0]
>>> for details) is that it requires specifying both
>>>      scsi0: <target storage>:-1,<properties>
>>>      import-sources: scsi0=/path/or/volid/for/source
>>> leading to a not ideal user interface and parameter handling needing
>>> cross-checks to verify that the right combination is specified, and
>>> passing both around at the same time.
>>>
>>> Another approach would be adding a new special volid syntax using
>>>      my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>>> allowing for e.g.
>>>      qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=native
>>>      qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backup=0
>>> Yes, it's a hack, but it would avoid the pain points from both other
>>> approaches and be very simple. See the end of the mail for a POC.
>> 
>> the biggest down-side is that this 'invades' the storage plugin-owned
>> volume ID namespace (further than the pre-existing, documented new disk
>> syntax) - a volume ID is defined as anything starting with a storage
>> identifier, followed by ':', followed by an arbitrary string (where the
>> semantics of that string are up to the plugin).
>> 
>> while I don't think there is a storage plugin out there that somehow
>> starts its volume IDs with 'import:' by default, it still doesn't feel
>> very nice.. there might be plugins that are not very strict in their
>> parsing that might accept such an 'import volid' as regular volid, and
>> parse the latter part (which can be a regular volid after all!) instead
>> of the full thing, which would make code paths that should not accept
>> import volids accept them and just use the source without importing,
>> with potentially disastrous consequences :-/
>> 
> 
> Ok, good point!
> 
>> that being said - other then this bigger conceptual question I only
>> found nits/fixup/followup material - so if we want to take the current
>> RFC I'd give this a more in-depth test spin in the next few days and
>> apply with the small stuff adressed ;)
>> 
> 
> Fine by me. I could also re-spin with the import-sources API parameter 
> if that's preferred. If we go with the import-from approach, besides 
> addressing your nits, I'd also not make parse_volume unconditionally 
> parse with the extended schema, but replace the foreach_volume iterators 
> in the appropriate places.

I actually prefer the import-from approach to the old one, even with the 
duplicate schema downside - it makes it more explicit where we want to
allow 'magically' allocating new volumes, which was always a bit 
confusing when you didn't have a clear picture of the code in your 
head..

> 
>>>
>>>
>>> [0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html
>>>
>>>
>>> pve-manager:
>>>
>>> Fabian Ebner (1):
>>>    api: nodes: add readovf endpoint
>>>
>>>   PVE/API2/Nodes.pm | 7 +++++++
>>>   1 file changed, 7 insertions(+)
>>>
>>>
>>> qemu-server:
>>>
>>> Dominic Jäger (1):
>>>    api: support VM disk import
>>>
>>> Fabian Ebner (6):
>>>    schema: add pve-volume-id-or-absolute-path
>>>    parse ovf: untaint path when calling file_size_info
>>>    api: add endpoint for parsing .ovf files
>>>    image convert: allow block device as source
>>>    schema: drive: use separate schema when disk allocation is possible
>>>    api: create disks: factor out common part from if/else
>>>
>>>   PVE/API2/Qemu.pm             | 86 +++++++++++++++++++++++----------
>>>   PVE/API2/Qemu/Makefile       |  2 +-
>>>   PVE/API2/Qemu/OVF.pm         | 55 +++++++++++++++++++++
>>>   PVE/QemuConfig.pm            |  2 +-
>>>   PVE/QemuServer.pm            | 57 +++++++++++++++++++---
>>>   PVE/QemuServer/Drive.pm      | 92 +++++++++++++++++++++++++++---------
>>>   PVE/QemuServer/ImportDisk.pm |  2 +-
>>>   PVE/QemuServer/OVF.pm        |  9 ++--
>>>   8 files changed, 242 insertions(+), 63 deletions(-)
>>>   create mode 100644 PVE/API2/Qemu/OVF.pm
>>>
>>> -- 
>>> 2.30.2
>>>
>>> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
>>> index 6992f6f..6a22899 100644
>>> --- a/PVE/API2/Qemu.pm
>>> +++ b/PVE/API2/Qemu.pm
>>> @@ -21,8 +21,9 @@ use PVE::ReplicationConfig;
>>>   use PVE::GuestHelpers;
>>>   use PVE::QemuConfig;
>>>   use PVE::QemuServer;
>>> -use PVE::QemuServer::Drive;
>>>   use PVE::QemuServer::CPUConfig;
>>> +use PVE::QemuServer::Drive;
>>> +use PVE::QemuServer::ImportDisk;
>>>   use PVE::QemuServer::Monitor qw(mon_cmd);
>>>   use PVE::QemuServer::Machine;
>>>   use PVE::QemuMigrate;
>>> @@ -64,6 +65,7 @@ my $resolve_cdrom_alias = sub {
>>>   };
>>>   
>>>   my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
>>> +my $IMPORT_DISK_RE = qr!^(([^/:\s]+):)import:(.*)$!;
>>>   my $check_storage_access = sub {
>>>      my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) = @_;
>>>   
>>> @@ -86,6 +88,9 @@ my $check_storage_access = sub {
>>>   	    my $scfg = PVE::Storage::storage_config($storecfg, $storeid);
>>>   	    raise_param_exc({ storage => "storage '$storeid' does not support vm images"})
>>>   		if !$scfg->{content}->{images};
>>> +	} elsif (!$isCDROM && ($volid =~ $IMPORT_DISK_RE)) {
>>> +	    warn "check access matched: $2 and $3\n";
>>> +	    warn "TODO implement import access check!\n";
>>>   	} else {
>>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>>   	}
>>> @@ -212,6 +217,29 @@ my $create_disks = sub {
>>>   	    $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b');
>>>   	    delete $disk->{format}; # no longer needed
>>>   	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>> +	} elsif ($volid =~ $IMPORT_DISK_RE) {
>>> +	    my ($storeid, $source) = ($2, $3);
>>> +
>>> +	    $source = PVE::Storage::abs_filesystem_path($storecfg, $source, 1);
>>> +	    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(
>>> +		$source,
>>> +		$vmid,
>>> +		$storeid,
>>> +		{
>>> +		    drive_name => $ds,
>>> +		    format => $disk->{format},
>>> +		    'skip-config-update' => 1,
>>> +		},
>>> +	    );
>>> +
>>> +	    push @$vollist, $dst_volid;
>>> +	    $disk->{file} = $dst_volid;
>>> +	    $disk->{size} = $src_size;
>>> +	    delete $disk->{format}; # no longer needed
>>> +	    $res->{$ds} = PVE::QemuServer::print_drive($disk);
>>>   	} else {
>>>   
>>>   	    PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid);
>>> diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm
>>> index 51ad52e..7557cac 100755
>>> --- a/PVE/QemuServer/ImportDisk.pm
>>> +++ b/PVE/QemuServer/ImportDisk.pm
>>> @@ -71,7 +71,7 @@ sub do_import {
>>>   	PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
>>>   	PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit);
>>>   	PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
>>> -	PVE::QemuConfig->lock_config($vmid, $create_drive);
>>> +	PVE::QemuConfig->lock_config($vmid, $create_drive) if !$params->{'skip-config-update'};
>>>       };
>>>       if (my $err = $@) {
>>>   	eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
>>> -- 
>>> 2.30.2
>>>
>>>
>>> _______________________________________________
>>> pve-devel mailing list
>>> pve-devel at lists.proxmox.com
>>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>>>
>> 
>> 
>> _______________________________________________
>> pve-devel mailing list
>> pve-devel at lists.proxmox.com
>> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 




More information about the pve-devel mailing list