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 EBCFA6183C for ; Mon, 17 Jan 2022 16:39:15 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id E7ACD1D039 for ; Mon, 17 Jan 2022 16:39:15 +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 31E201D02E for ; Mon, 17 Jan 2022 16:39:14 +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 0A2C645619 for ; Mon, 17 Jan 2022 16:39:14 +0100 (CET) Date: Mon, 17 Jan 2022 16:39:06 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220113100831.34113-1-f.ebner@proxmox.com> <20220113100831.34113-8-f.ebner@proxmox.com> In-Reply-To: <<20220113100831.34113-8-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1642423098.w1kkxetong.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.218 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 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: Mon, 17 Jan 2022 15:39:16 -0000 On January 13, 2022 11:08 am, Fabian Ebner wrote: > From: Dominic J=C3=A4ger >=20 > Extend qm importdisk functionality to the API. >=20 > Co-authored-by: Fabian Gr=C3=BCnbichler > Co-authored-by: Dominic J=C3=A4ger > Signed-off-by: Fabian Ebner > --- >=20 > Changes from v9: >=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. 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. >=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 > * Don't iterate over unused disks in create_disks() >=20 > 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. >=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 > * Drop format supported check >=20 > Instead rely on resolve_dst_disk_format (via do_import) to pick > the most appropriate format. >=20 > 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(-) >=20 > 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 =3D sub { > } else { > PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $volid); > } > + > + if (my $source_image =3D $drive->{'import-from'}) { > + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $source_image); > + } > }); > =20 > $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['D= atastore.AllocateSpace']) > @@ -162,6 +167,9 @@ my $create_disks =3D sub { > my $volid =3D $disk->{file}; > my ($storeid, $volname) =3D PVE::Storage::parse_volume_id($volid, 1); > =20 > + die "'import-from' requires special volume ID - use :0,impo= rt-from=3D\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=20 to 0? otherwise users might expect something like -scsi0 STORAGE:128,import-from:/some/small/image.raw to import and resize on the fly ;) > if (!$volid || $volid eq 'none' || $volid eq 'cdrom') { > delete $disk->{size}; > $res->{$ds} =3D PVE::QemuServer::print_drive($disk); > @@ -190,28 +198,52 @@ my $create_disks =3D sub { > } elsif ($volid =3D~ $NEW_DISK_RE) { > my ($storeid, $size) =3D ($2 || $default_storage, $3); > die "no storage ID specified (and no default storage)\n" if !$store= id; > - my $defformat =3D PVE::Storage::storage_default_format($storecfg, $= storeid); > - my $fmt =3D $disk->{format} || $defformat; > - > - $size =3D PVE::Tools::convert_size($size, 'gb' =3D> 'kb'); # vdisk_= alloc uses kb > - > - my $volid; > - if ($ds eq 'efidisk0') { > - my $smm =3D PVE::QemuServer::Machine::machine_type_is_q35($conf); > - ($volid, $size) =3D 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 =3D PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTATE_DI= SK_SIZE, 'b' =3D> 'kb'); > - $volid =3D PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "raw"= , undef, $size); > + > + if (my $source =3D delete $disk->{'import-from'}) { > + $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); nit: missing '\n'? > + > + 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 { > - $volid =3D PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $fmt,= undef, $size); > + my $defformat =3D PVE::Storage::storage_default_format($storecfg, $sto= reid); > + my $fmt =3D $disk->{format} || $defformat; > + > + $size =3D PVE::Tools::convert_size($size, 'gb' =3D> 'kb'); # vdisk_all= oc uses kb > + > + my $volid; > + if ($ds eq 'efidisk0') { > + my $smm =3D PVE::QemuServer::Machine::machine_type_is_q35($conf); > + ($volid, $size) =3D 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 =3D PVE::Tools::convert_size(PVE::QemuServer::Drive::TPMSTAT= E_DISK_SIZE, 'b' =3D> 'kb'); > + $volid =3D PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, "= raw", undef, $size); > + } else { > + $volid =3D PVE::Storage::vdisk_alloc($storecfg, $storeid, $vmid, $= fmt, undef, $size); > + } > + push @$vollist, $volid; > + $disk->{file} =3D $volid; > + $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); > } > - push @$vollist, $volid; > - $disk->{file} =3D $volid; > - $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); > } else { > =20 > PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $volid); > @@ -639,11 +671,11 @@ __PACKAGE__->register_method({ > =20 > foreach my $opt (keys %$param) { > if (PVE::QemuServer::is_valid_drivename($opt)) { > - my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}); > + my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1= ); > raise_param_exc({ $opt =3D> "unable to parse drive options" }) if = !$drive; > =20 > PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive); > - $param->{$opt} =3D PVE::QemuServer::print_drive($drive); > + $param->{$opt} =3D PVE::QemuServer::print_drive($drive, 1); > } > } > =20 > @@ -1207,11 +1239,11 @@ my $update_vm_api =3D sub { > foreach my $opt (keys %$param) { > if (PVE::QemuServer::is_valid_drivename($opt)) { > # cleanup drive path > - my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}); > + my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1)= ; > raise_param_exc({ $opt =3D> "unable to parse drive options" }) if != $drive; > PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive); > $check_replication->($drive); > - $param->{$opt} =3D PVE::QemuServer::print_drive($drive); > + $param->{$opt} =3D PVE::QemuServer::print_drive($drive, 1); > } elsif ($opt =3D~ m/^net(\d+)$/) { > # add macaddr > my $net =3D PVE::QemuServer::parse_net($param->{$opt}); > @@ -1288,7 +1320,7 @@ my $update_vm_api =3D sub { > =20 > my $check_drive_perms =3D sub { > my ($opt, $val) =3D @_; > - my $drive =3D PVE::QemuServer::parse_drive($opt, $val); > + my $drive =3D 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 =3D sub { > # default legacy boot order implies all cdroms anyway > if (@bootorder) { > # append new CD drives to bootorder to mark them bootable > - my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}); > + my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); > if (PVE::QemuServer::drive_is_cdrom($drive, 1) && !grep(/^$opt$/, @bo= otorder)) { > push @bootorder, $opt; > $conf->{pending}->{boot} =3D PVE::QemuServer::print_bootorder(\@b= ootorder); > 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 =3D { 'file' =3D> $volume_string }; > } else { > - $volume =3D PVE::QemuServer::Drive::parse_drive($key, $volume_string); > + $volume =3D PVE::QemuServer::Drive::parse_drive($key, $volume_string, 1= ); > } > =20 > 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 =3D { > %efitype_fmt, > }; > =20 > +my %import_from_fmt =3D ( > + 'import-from' =3D> { > + type =3D> 'string', > + format =3D> 'pve-volume-id-or-absolute-path', > + format_description =3D> 'source volume', > + description =3D> "When creating a new disk, import from this source.", > + optional =3D> 1, > + }, > +); > +my $alldrive_fmt_with_alloc =3D { > + %$alldrive_fmt, > + %import_from_fmt, > +}; > + > my $unused_fmt =3D { > volume =3D> { alias =3D> 'file' }, > file =3D> { > @@ -436,6 +450,8 @@ my $desc_with_alloc =3D sub { > =20 > my $new_desc =3D dclone($desc); > =20 > + $new_desc->{format}->{'import-from'} =3D $import_from_fmt{'import-fr= om'}; > + > my $extra_note =3D ''; > if ($type eq 'efidisk') { > $extra_note =3D " Note that SIZE_IN_GiB is ignored here and that the de= fault EFI vars are ". > @@ -445,7 +461,8 @@ my $desc_with_alloc =3D sub { > } > =20 > $new_desc->{description} .=3D "Use the special syntax STORAGE_ID:SIZ= E_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."; > =20 > $with_alloc_desc_cache->{$type} =3D $new_desc; > =20 > @@ -547,7 +564,7 @@ sub drive_is_read_only { > # [,iothread=3Don][,serial=3Dserial][,model=3Dmodel] > =20 > sub parse_drive { > - my ($key, $data) =3D @_; > + my ($key, $data, $with_alloc) =3D @_; > =20 > my ($interface, $index); > =20 > @@ -558,12 +575,14 @@ sub parse_drive { > return; > } > =20 > - if (!defined($drivedesc_hash->{$key})) { > + my $desc_hash =3D $with_alloc ? $drivedesc_hash_with_alloc : $drived= esc_hash; > + > + if (!defined($desc_hash->{$key})) { > warn "invalid drive key: $key\n"; > return; > } > =20 > - my $desc =3D $drivedesc_hash->{$key}->{format}; > + my $desc =3D $desc_hash->{$key}->{format}; > my $res =3D eval { PVE::JSONSchema::parse_property_string($desc, $da= ta) }; > return if !$res; > $res->{interface} =3D $interface; > @@ -623,9 +642,10 @@ sub parse_drive { > } > =20 > sub print_drive { > - my ($drive) =3D @_; > + my ($drive, $with_alloc) =3D @_; > my $skip =3D [ 'index', 'interface' ]; > - return PVE::JSONSchema::print_property_string($drive, $alldrive_fmt,= $skip); > + my $fmt =3D $with_alloc ? $alldrive_fmt_with_alloc : $alldrive_fmt; > + return PVE::JSONSchema::print_property_string($drive, $fmt, $skip); > } > =20 > 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, 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 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20