From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5DCE56184C for ; Mon, 17 Jan 2022 16:44:28 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 449FB1D1BF for ; Mon, 17 Jan 2022 16:43:58 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id A1BD61D1B1 for ; Mon, 17 Jan 2022 16:43:56 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 705DD46BED for ; Mon, 17 Jan 2022 16:43:56 +0100 (CET) Date: Mon, 17 Jan 2022 16:43:48 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220113100831.34113-1-f.ebner@proxmox.com> In-Reply-To: <<20220113100831.34113-1-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1642425420.013zr4zixi.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.217 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [RFC v10 qemu-server/manager] API for disk import and OVF X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 17 Jan 2022 15:44:28 -0000 On January 13, 2022 11:08 am, Fabian Ebner wrote: > Extend qm importdisk/importovf functionality to the API. >=20 >=20 > Used Dominic's latest version[0] as a starting point. GUI part still > needs to be rebased/updated, so it's not included here. >=20 >=20 > Changes from v9: >=20 > * Split patch into smaller parts >=20 > * Some minor (style) fixes/improvements (see individual patches) >=20 > * Drop $manifest_only parameter for parse_ovf >=20 > 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? >=20 > * Re-use do_import function >=20 > 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. >=20 > * 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 >=20 > 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. >=20 > 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. >=20 > 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 >=20 >=20 > After all, the 'import-from' disk property approach turned out to be > a uglier than I hoped it would. >=20 > My problem with the 'import-sources' API parameter approach (see [0] > for details) is that it requires specifying both > scsi0: :-1, > import-sources: scsi0=3D/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. >=20 > Another approach would be adding a new special volid syntax using > my $IMPORT_DISK_RE =3D qr!^(([^/:\s]+):)import:(.*)$!; > allowing for e.g. > qm set 126 -scsi1 rbdkvm:import:myzpool:vm-114-disk-0,aio=3Dnative > qm set 126 -scsi2 rbdkvm:import:/dev/zvol/myzpool/vm-114-disk-1,backu= p=3D0 > 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=20 volume ID namespace (further than the pre-existing, documented new disk=20 syntax) - a volume ID is defined as anything starting with a storage=20 identifier, followed by ':', followed by an arbitrary string (where the=20 semantics of that string are up to the plugin). while I don't think there is a storage plugin out there that somehow=20 starts its volume IDs with 'import:' by default, it still doesn't feel=20 very nice.. there might be plugins that are not very strict in their=20 parsing that might accept such an 'import volid' as regular volid, and=20 parse the latter part (which can be a regular volid after all!) instead=20 of the full thing, which would make code paths that should not accept=20 import volids accept them and just use the source without importing,=20 with potentially disastrous consequences :-/ that being said - other then this bigger conceptual question I only=20 found nits/fixup/followup material - so if we want to take the current=20 RFC I'd give this a more in-depth test spin in the next few days and=20 apply with the small stuff adressed ;) >=20 >=20 > [0]: https://lists.proxmox.com/pipermail/pve-devel/2021-June/048564.html >=20 >=20 > pve-manager: >=20 > Fabian Ebner (1): > api: nodes: add readovf endpoint >=20 > PVE/API2/Nodes.pm | 7 +++++++ > 1 file changed, 7 insertions(+) >=20 >=20 > qemu-server: >=20 > Dominic J=C3=A4ger (1): > api: support VM disk import >=20 > 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 >=20 > 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 >=20 > --=20 > 2.30.2 >=20 > 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 =3D sub { > }; > =20 > my $NEW_DISK_RE =3D qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!; > +my $IMPORT_DISK_RE =3D qr!^(([^/:\s]+):)import:(.*)$!; > my $check_storage_access =3D sub { > my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage= ) =3D @_; > =20 > @@ -86,6 +88,9 @@ my $check_storage_access =3D sub { > my $scfg =3D PVE::Storage::storage_config($storecfg, $storeid); > raise_param_exc({ storage =3D> "storage '$storeid' does not support= vm images"}) > if !$scfg->{content}->{images}; > + } elsif (!$isCDROM && ($volid =3D~ $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, $v= mid, $volid); > } > @@ -212,6 +217,29 @@ my $create_disks =3D sub { > $disk->{size} =3D PVE::Tools::convert_size($size, 'kb' =3D> 'b'); > delete $disk->{format}; # no longer needed > $res->{$ds} =3D PVE::QemuServer::print_drive($disk); > + } elsif ($volid =3D~ $IMPORT_DISK_RE) { > + my ($storeid, $source) =3D ($2, $3); > + > + $source =3D PVE::Storage::abs_filesystem_path($storecfg, $source, 1= ); > + my $src_size =3D PVE::Storage::file_size_info($source); > + die "Could not get file size of $source" if !defined($src_size); > + > + my (undef, $dst_volid) =3D PVE::QemuServer::ImportDisk::do_import( > + $source, > + $vmid, > + $storeid, > + { > + drive_name =3D> $ds, > + format =3D> $disk->{format}, > + 'skip-config-update' =3D> 1, > + }, > + ); > + > + push @$vollist, $dst_volid; > + $disk->{file} =3D $dst_volid; > + $disk->{size} =3D $src_size; > + delete $disk->{format}; # no longer needed > + $res->{$ds} =3D PVE::QemuServer::print_drive($disk); > } else { > =20 > PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $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, und= ef, $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 =3D $@) { > eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) }; > --=20 > 2.30.2 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20