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 D09EC69D8A for ; Mon, 14 Mar 2022 16:55:08 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id C42E261CE for ; Mon, 14 Mar 2022 16:55:08 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id D4C0161C3 for ; Mon, 14 Mar 2022 16:55:05 +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 A8BF246357 for ; Mon, 14 Mar 2022 16:55:05 +0100 (CET) Date: Mon, 14 Mar 2022 16:54:56 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20220309100919.31512-1-f.ebner@proxmox.com> <20220309100919.31512-16-f.ebner@proxmox.com> In-Reply-To: <<20220309100919.31512-16-f.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1647267065.5tbtb00mlw.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.137 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 PROLO_LEO3 0.1 Meta Catches all Leo drug variations so far SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pve-devel] [PATCH v12 qemu-server 15/16] 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, 14 Mar 2022 15:55:08 -0000 On March 9, 2022 11:09 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 v11: > * Require relevant parameters to be set explicitly for EFI/TPM > disk import. > * Base calculation of EFI vars size on passed-in parameters. >=20 > PVE/API2/Qemu.pm | 229 ++++++++++++++++++++++++++++++----- > PVE/QemuServer/Drive.pm | 34 +++++- > PVE/QemuServer/ImportDisk.pm | 2 +- > 3 files changed, 230 insertions(+), 35 deletions(-) >=20 > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index 216c326..9220ce2 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; > @@ -63,28 +64,58 @@ my $resolve_cdrom_alias =3D sub { > } > }; > =20 > +# Used in import-enabled API endpoints. Parses drives using the extended= '_with_alloc' schema. > +my $foreach_volume_with_alloc =3D sub { > + my ($param, $func) =3D @_; > + > + for my $opt (sort keys $param->%*) { > + next if !PVE::QemuServer::is_valid_drivename($opt); > + > + my $drive =3D PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt},= 1); > + next if !$drive; > + > + $func->($opt, $drive); > + } > +}; > + > +my $NEW_DISK_RE =3D qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!; > + > my $check_drive_param =3D sub { > my ($param, $storecfg, $extra_checks) =3D @_; > =20 > for my $opt (sort keys $param->%*) { > next if !PVE::QemuServer::is_valid_drivename($opt); > =20 > - my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}); > + my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}, 1); technically belongs into the previous patch, our non-alloc schema is=20 just tolerant enough because it doesn't look at the volids too closely=20 and accepts the NEW_DISK_RE syntax as potential existing volid.. > raise_param_exc({ $opt =3D> "unable to parse drive options" }) if !$dri= ve; > =20 > + if ($drive->{'import-from'}) { > + die "'import-from' requires special syntax - use :0,imp= ort-from=3D\n" > + if $drive->{file} !~ $NEW_DISK_RE || $3 !=3D 0; should probably be a param_exc > + > + if ($opt eq 'efidisk0') { > + for my $required (qw(efitype pre-enrolled-keys)) { > + die "$opt - need to specify '$required' when using 'import-from'\n= " > + if !defined($drive->{$required}); same here > + } > + } elsif ($opt eq 'tpmstate0') { > + die "$opt - need to specify 'version' when using 'import-from'\n" > + if !defined($drive->{version}); and here > + } > + } > + > PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive); > =20 > $extra_checks->($drive) if $extra_checks; > =20 > - $param->{$opt} =3D PVE::QemuServer::print_drive($drive); > + $param->{$opt} =3D PVE::QemuServer::print_drive($drive, 1); > } > }; > =20 > -my $NEW_DISK_RE =3D qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!; > my $check_storage_access =3D sub { > my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage= ) =3D @_; > =20 > - PVE::QemuConfig->foreach_volume($settings, sub { > + $foreach_volume_with_alloc->($settings, sub { > my ($ds, $drive) =3D @_; > =20 > my $isCDROM =3D PVE::QemuServer::drive_is_cdrom($drive); > @@ -106,6 +137,20 @@ my $check_storage_access =3D sub { > } else { > PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $volid); > } > + > + if (my $src_image =3D $drive->{'import-from'}) { > + my $src_vmid; > + my ($src_storeid) =3D PVE::Storage::parse_volume_id($src_image, 1); > + if ($src_storeid) { # PVE-managed volume nit, could be if (PVE::Storage::parse_volume_id($src_image, 1)) { # PVE-managed since we don't actually care about the sid here, and parse_volume_id=20 will return undef when $noerr is set. > + $src_vmid =3D (PVE::Storage::parse_volname($storecfg, $src_image))[2] is there some case where we expect parse_volume_id to work, but the=20 volume to not have an associated guest? because perl doesn't mind us=20 accessing the resulting array at arbitrary indices, so this doesn't fail=20 if $src_vmid is undef.. these should probably also check some more stuff (at least the volume=20 type?) - else we get strange errors when attempting to import=20 non-image-volumes (some of which even have owners, for example backup=20 archives..), and what exactly gets caught where is basically up to the=20 storage plugin via parse_volname and volume_has_feature.. > + } > + > + if ($src_vmid) { # might be actively used by VM and will be copied = via clone_disk() > + $rpcenv->check($authuser, "/vms/${src_vmid}", ['VM.Clone']); > + } else { > + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $vmid= , $src_image); > + } > + } > }); > =20 > $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}", ['D= atastore.AllocateSpace']) > @@ -164,6 +209,87 @@ my $check_storage_access_migrate =3D sub { > if !$scfg->{content}->{images}; > }; > =20 > +my $import_from_volid =3D sub { > + my ($storecfg, $src_volid, $dest_info, $vollist) =3D @_; > + > + die "cannot import from cloudinit disk\n" > + if PVE::QemuServer::Drive::drive_is_cloudinit({ file =3D> $src_volid })= ; > + > + my ($src_storeid, $src_volname) =3D PVE::Storage::parse_volume_id($s= rc_volid); technically this is already implied by the sub's name, we checked it=20 already outside, but we need the store id for the bwlimit below.. > + my $src_vmid =3D (PVE::Storage::parse_volname($storecfg, $src_volid)= )[2]; > + > + my $src_vm_state =3D sub { > + my $exists =3D $src_vmid && PVE::Cluster::get_vmlist()->{ids}->{$src_vm= id} ? 1 : 0; > + > + my $runs =3D 0; > + if ($exists) { > + eval { PVE::QemuConfig::assert_config_exists_on_node($src_vmid); }; > + die "owner VM $src_vmid not on local node\n" if $@; > + $runs =3D PVE::QemuServer::Helpers::vm_running_locally($src_vmid) |= | 0; > + } > + > + return ($exists, $runs); > + }; > + > + my ($src_vm_exists, $running) =3D $src_vm_state->(); > + > + die "cannot import from '$src_volid' - full clone feature is not sup= ported\n" > + if !PVE::Storage::volume_has_feature($storecfg, 'copy', $src_volid, und= ef, $running); > + > + my $clonefn =3D sub { > + my ($src_vm_exists_now, $running_now) =3D $src_vm_state->(); > + > + die "owner VM $src_vmid changed state unexpectedly\n" > + if $src_vm_exists_now !=3D $src_vm_exists || $running_now !=3D $run= ning; > + > + my $src_conf =3D $src_vm_exists_now ? PVE::QemuConfig->load_config($src= _vmid) : {}; > + > + my $src_drive =3D { file =3D> $src_volid }; > + my $src_drivename; > + PVE::QemuConfig->foreach_volume($src_conf, sub { > + my ($ds, $drive) =3D @_; > + > + return if $src_drivename; > + > + if ($drive->{file} eq $src_volid) { > + $src_drive =3D $drive; > + $src_drivename =3D $ds; > + } > + }); > + > + my $source_info =3D { > + vmid =3D> $src_vmid, > + running =3D> $running_now, > + drivename =3D> $src_drivename, > + drive =3D> $src_drive, > + snapname =3D> undef, > + }; > + > + return PVE::QemuServer::clone_disk( > + $storecfg, > + $source_info, > + $dest_info, > + 1, > + $vollist, > + undef, > + undef, > + $src_conf->{agent}, > + PVE::Storage::get_bandwidth_limit('clone', [$src_storeid, $dest_inf= o->{storage}]), > + ); > + }; > + > + my $cloned; > + if ($running) { > + $cloned =3D PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn); > + } elsif ($src_vmid) { > + $cloned =3D PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn= ); > + } else { > + $cloned =3D $clonefn->(); > + } > + > + return $cloned->@{qw(file size)}; > +}; > + > # Note: $pool is only needed when creating a VM, because pool permission= s > # are automatically inherited if VM already exists inside a pool. > my $create_disks =3D sub { > @@ -207,28 +333,75 @@ 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'}) { > + my $dst_volid; > + my ($src_storeid) =3D PVE::Storage::parse_volume_id($source, 1); > + > + if ($src_storeid) { # PVE-managed volume same as above applies here as well, $src_storeid is not used here, so=20 can be shortened. > + die "could not get size of $source\n" > + if !PVE::Storage::volume_size_info($storecfg, $source, 10); this could move into $import_from_volid? > + > + my $dest_info =3D { > + vmid =3D> $vmid, > + drivename =3D> $ds, > + storage =3D> $storeid, > + format =3D> $disk->{format}, > + }; > + > + $dest_info->{efisize} =3D PVE::QemuServer::get_efivars_size($conf,= $disk) > + if $ds eq 'efidisk0'; > + > + ($dst_volid, $size) =3D eval { > + $import_from_volid->($storecfg, $source, $dest_info, $vollist); > + }; > + die "cannot import from '$source' - $@" if $@; > + } else { > + $source =3D PVE::Storage::abs_filesystem_path($storecfg, $source, = 1); > + $size =3D PVE::Storage::file_size_info($source); > + die "could not get file size of $source\n" if !$size; > + > + (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 $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 { > PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, $v= mid, $volid); > =20 > @@ -242,7 +415,7 @@ my $create_disks =3D sub { > } > }; > =20 > - eval { PVE::QemuConfig->foreach_volume($settings, $code); }; > + eval { $foreach_volume_with_alloc->($settings, $code); }; > =20 > # free allocated images on error > if (my $err =3D $@) { > @@ -1285,7 +1458,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); same applies here (move to previous patch?) > # FIXME: cloudinit: CDROM or Disk? > if (PVE::QemuServer::drive_is_cdrom($drive)) { # CDROM > $rpcenv->check_vm_perm($authuser, $vmid, undef, ['VM.Config.CDROM'= ]); > @@ -1391,7 +1564,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); same > 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/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm > index d5d4723..88f013a 100644 > --- a/PVE/QemuServer/Drive.pm > +++ b/PVE/QemuServer/Drive.pm > @@ -409,6 +409,22 @@ 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> "Create a new disk, importing from this source. If the= volume is not ". > + "managed by Proxmox VE, it's up to you to ensure that it's not acti= vely used by ". > + "another process during the import!", > + 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 +452,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 +463,8 @@ my $desc_with_alloc =3D sub { > } > =20 > $new_desc->{description} .=3D " Use the special syntax STORAGE_ID:SI= ZE_IN_GiB to allocate a new ". > - "volume.${extra_note}"; > + "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 +566,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 @_; technically previous patch, same as all the other changes in this file=20 below this change > =20 > my ($interface, $index); > =20 > @@ -558,12 +577,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 +644,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'}; should probably be added to the comment at start, even if it has a=20 speaking name ;) skiplock is missing as well. > }; > 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