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

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

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 :-/

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 ;)

> 
> 
> [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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>