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 A03BA619E2 for ; Tue, 18 Jan 2022 09:51:59 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4E56B22F00 for ; Tue, 18 Jan 2022 09:51:59 +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 33EEB22EF5 for ; Tue, 18 Jan 2022 09:51:57 +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 D6D5C45C80 for ; Tue, 18 Jan 2022 09:51:56 +0100 (CET) Message-ID: <76b40413-110e-0701-0696-02f2a4f101b6@proxmox.com> Date: Tue, 18 Jan 2022 09:51:54 +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> <20220113100831.34113-8-f.ebner@proxmox.com> <1642423098.w1kkxetong.astroid@nora.none> From: Fabian Ebner In-Reply-To: <1642423098.w1kkxetong.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. [qemuconfig.pm, proxmox.com, importdisk.pm, drive.pm, qemu.pm] Subject: Re: [pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import 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 08:51:59 -0000 Am 17.01.22 um 16:39 schrieb Fabian Grünbichler: > On January 13, 2022 11:08 am, Fabian Ebner wrote: >> From: Dominic Jäger >> >> Extend qm importdisk functionality to the API. >> >> Co-authored-by: Fabian Grünbichler >> Co-authored-by: Dominic Jäger >> Signed-off-by: Fabian Ebner >> --- >> >> Changes from v9: >> >> * 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. 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 instead. >> >> 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 >> >> * Don't iterate over unused disks in create_disks() >> >> Would need to be its own patch and need to make sure everything >> also works with respect to usual (i.e. non-import) new disk >> creation, etc. >> >> * 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. >> >> * Drop format supported check >> >> Instead rely on resolve_dst_disk_format (via do_import) to pick >> the most appropriate format. >> >> PVE/API2/Qemu.pm | 86 +++++++++++++++++++++++++----------- >> PVE/QemuConfig.pm | 2 +- >> PVE/QemuServer/Drive.pm | 32 +++++++++++--- >> PVE/QemuServer/ImportDisk.pm | 2 +- >> 4 files changed, 87 insertions(+), 35 deletions(-) >> >> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm >> index e6a6cdc..8c74ecc 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; >> @@ -89,6 +90,10 @@ my $check_storage_access = sub { >> } else { >> PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid); >> } >> + >> + if (my $source_image = $drive->{'import-from'}) { >> + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $source_image); >> + } >> }); >> >> $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['Datastore.AllocateSpace']) >> @@ -162,6 +167,9 @@ my $create_disks = sub { >> my $volid = $disk->{file}; >> my ($storeid, $volname) = PVE::Storage::parse_volume_id($volid, 1); >> >> + die "'import-from' requires special volume ID - use :0,import-from=\n" >> + if $disk->{'import-from'} && $volid !~ $NEW_DISK_RE; >> + > > nit: check and message disagree ($NEW_DISK_RE allows more than just '0') > > not sure whether it's worth it to add an $IMPORT_DISK_RE that is limited > to 0? otherwise users might expect something like > > -scsi0 STORAGE:128,import-from:/some/small/image.raw > > to import and resize on the fly ;) The error just gives an example, but I agree that's not clear without any "e.g." in there ;) And by allowing (and ignoring) more patterns now, adding the "resize on the fly" feature becomes much more difficult in the future (should we ever want that). I'll make the check more strict (just adding a check for size 0 avoids the need for a second RE) and adapt the description below as suggested. > >> if (!$volid || $volid eq 'none' || $volid eq 'cdrom') { >> delete $disk->{size}; >> $res->{$ds} = PVE::QemuServer::print_drive($disk); >> @@ -190,28 +198,52 @@ my $create_disks = sub { >> } elsif ($volid =~ $NEW_DISK_RE) { >> my ($storeid, $size) = ($2 || $default_storage, $3); >> die "no storage ID specified (and no default storage)\n" if !$storeid; >> - my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid); >> - my $fmt = $disk->{format} || $defformat; >> - >> - $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb >> - >> - my $volid; >> - if ($ds eq 'efidisk0') { >> - my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf); >> - ($volid, $size) = PVE::QemuServer::create_efidisk( >> - $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm); >> - } elsif ($ds eq 'tpmstate0') { >> - # swtpm can only use raw volumes, and uses a fixed size >> - $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb'); >> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size); >> + >> + if (my $source = delete $disk->{'import-from'}) { >> + $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); > > nit: missing '\n'? > >> + >> + 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 { >> - $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size); >> + my $defformat = PVE::Storage::storage_default_format($storecfg, $storeid); >> + my $fmt = $disk->{format} || $defformat; >> + >> + $size = PVE::Tools::convert_size($size, 'gb' => 'kb'); # vdisk_alloc uses kb >> + >> + my $volid; >> + if ($ds eq 'efidisk0') { >> + my $smm = PVE::QemuServer::Machine::machine_type_is_q35($conf); >> + ($volid, $size) = PVE::QemuServer::create_efidisk( >> + $storecfg, $storeid, $vmid, $fmt, $arch, $disk, $smm); >> + } elsif ($ds eq 'tpmstate0') { >> + # swtpm can only use raw volumes, and uses a fixed size >> + $size = PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DISK_SIZE, 'b' => 'kb'); >> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw", undef, $size); >> + } else { >> + $volid = PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt, undef, $size); >> + } >> + push @$vollist, $volid; >> + $disk->{file} = $volid; >> + $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b'); >> + delete $disk->{format}; # no longer needed >> + $res->{$ds} = PVE::QemuServer::print_drive($disk); >> } >> - push @$vollist, $volid; >> - $disk->{file} = $volid; >> - $disk->{size} = PVE::Tools::convert_size($size, 'kb' => 'b'); >> - delete $disk->{format}; # no longer needed >> - $res->{$ds} = PVE::QemuServer::print_drive($disk); >> } else { >> >> PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid, $volid); >> @@ -639,11 +671,11 @@ __PACKAGE__->register_method({ >> >> foreach my $opt (keys %$param) { >> if (PVE::QemuServer::is_valid_drivename($opt)) { >> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}); >> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); >> raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive; >> >> PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive); >> - $param->{$opt} = PVE::QemuServer::print_drive($drive); >> + $param->{$opt} = PVE::QemuServer::print_drive($drive, 1); >> } >> } >> >> @@ -1207,11 +1239,11 @@ my $update_vm_api = sub { >> foreach my $opt (keys %$param) { >> if (PVE::QemuServer::is_valid_drivename($opt)) { >> # cleanup drive path >> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}); >> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); >> raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive; >> PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive); >> $check_replication->($drive); >> - $param->{$opt} = PVE::QemuServer::print_drive($drive); >> + $param->{$opt} = PVE::QemuServer::print_drive($drive, 1); >> } elsif ($opt =~ m/^net(\d+)$/) { >> # add macaddr >> my $net = PVE::QemuServer::parse_net($param->{$opt}); >> @@ -1288,7 +1320,7 @@ my $update_vm_api = sub { >> >> my $check_drive_perms = sub { >> my ($opt, $val) = @_; >> - my $drive = PVE::QemuServer::parse_drive($opt, $val); >> + my $drive = PVE::QemuServer::parse_drive($opt, $val, 1); >> # FIXME: cloudinit: CDROM or Disk? >> if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM >> $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM']); >> @@ -1384,7 +1416,7 @@ my $update_vm_api = sub { >> # default legacy boot order implies all cdroms anyway >> if (@bootorder) { >> # append new CD drives to bootorder to mark them bootable >> - my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}); >> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); >> if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bootorder)) { >> push @bootorder, $opt; >> $conf->{pending}->{boot} = PVE::QemuServer::print_bootorder(\@bootorder); >> diff --git a/PVE/QemuConfig.pm b/PVE/QemuConfig.pm >> index b993378..76b4314 100644 >> --- a/PVE/QemuConfig.pm >> +++ b/PVE/QemuConfig.pm >> @@ -101,7 +101,7 @@ sub parse_volume { >> } >> $volume = { 'file' => $volume_string }; >> } else { >> - $volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string); >> + $volume = PVE::QemuServer::Drive::parse_drive($key, $volume_string, 1); >> } >> >> die "unable to parse volume\n" if !defined($volume) && !$noerr; >> diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm >> index f024269..828076d 100644 >> --- a/PVE/QemuServer/Drive.pm >> +++ b/PVE/QemuServer/Drive.pm >> @@ -409,6 +409,20 @@ my $alldrive_fmt = { >> %efitype_fmt, >> }; >> >> +my %import_from_fmt = ( >> + 'import-from' => { >> + type => 'string', >> + format => 'pve-volume-id-or-absolute-path', >> + format_description => 'source volume', >> + description => "When creating a new disk, import from this source.", >> + optional => 1, >> + }, >> +); >> +my $alldrive_fmt_with_alloc = { >> + %$alldrive_fmt, >> + %import_from_fmt, >> +}; >> + >> my $unused_fmt = { >> volume => { alias => 'file' }, >> file => { >> @@ -436,6 +450,8 @@ my $desc_with_alloc = sub { >> >> my $new_desc = dclone($desc); >> >> + $new_desc->{format}->{'import-from'} = $import_from_fmt{'import-from'}; >> + >> my $extra_note = ''; >> if ($type eq 'efidisk') { >> $extra_note = " Note that SIZE_IN_GiB is ignored here and that the default EFI vars are ". >> @@ -445,7 +461,8 @@ my $desc_with_alloc = sub { >> } >> >> $new_desc->{description} .= "Use the special syntax STORAGE_ID:SIZE_IN_GiB to allocate a new ". >> - "volume.${extra_note}"; >> + "volume.${extra_note} Use the 'import-from' parameter to import from an existing volume ". >> + "instead (SIZE_IN_GiB is ignored then)."; > > or change the check above, and make this > > + "volume.${extra_note} Use STORAGE_ID:0 and the 'import-from' parameter to import from an existing volume."; > >> >> $with_alloc_desc_cache->{$type} = $new_desc; >> >> @@ -547,7 +564,7 @@ sub drive_is_read_only { >> # [,iothread=on][,serial=serial][,model=model] >> >> sub parse_drive { >> - my ($key, $data) = @_; >> + my ($key, $data, $with_alloc) = @_; >> >> my ($interface, $index); >> >> @@ -558,12 +575,14 @@ sub parse_drive { >> return; >> } >> >> - if (!defined($drivedesc_hash->{$key})) { >> + my $desc_hash = $with_alloc ? $drivedesc_hash_with_alloc : $drivedesc_hash; >> + >> + if (!defined($desc_hash->{$key})) { >> warn "invalid drive key: $key\n"; >> return; >> } >> >> - my $desc = $drivedesc_hash->{$key}->{format}; >> + my $desc = $desc_hash->{$key}->{format}; >> my $res = eval { PVE::JSONSchema::parse_property_string($desc, $data) }; >> return if !$res; >> $res->{interface} = $interface; >> @@ -623,9 +642,10 @@ sub parse_drive { >> } >> >> sub print_drive { >> - my ($drive) = @_; >> + my ($drive, $with_alloc) = @_; >> my $skip = [ 'index', 'interface' ]; >> - return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt, $skip); >> + my $fmt = $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt; >> + return PVE::JSONSchema::print_property_string($drive, $fmt, $skip); >> } >> >> sub get_bootdisks { >> 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