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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 27C7B61A9C for ; Tue, 18 Jan 2022 10:09:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 11F3823498 for ; Tue, 18 Jan 2022 10:08:45 +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 9DC592348C for ; Tue, 18 Jan 2022 10:08:43 +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 62CB046045 for ; Tue, 18 Jan 2022 10:08:43 +0100 (CET) Message-ID: Date: Tue, 18 Jan 2022 10:08:41 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.4.1 Content-Language: en-US To: pve-devel@lists.proxmox.com, =?UTF-8?Q?Fabian_Gr=c3=bcnbichler?= References: <20220113100831.34113-1-f.ebner@proxmox.com> <1642425420.013zr4zixi.astroid@nora.none> From: Fabian Ebner In-Reply-To: <1642425420.013zr4zixi.astroid@nora.none> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.264 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_ASCII_DIVIDERS 0.8 Spam that uses ascii formatting tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.001 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [nodes.pm, importdisk.pm, ovf.pm, qemuserver.pm, qemuconfig.pm, proxmox.com, qemu.pm, drive.pm] 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: Tue, 18 Jan 2022 09:09:15 -0000 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: :-1, >> 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. >> >> >> [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 >> > > > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel