On March 9, 2022 11:09 am, Fabian Ebner wrote:
> From: Dominic Jäger <d.jae...@proxmox.com>
>
> Extend qm importdisk functionality to the API.
>
> Co-authored-by: Fabian Grünbichler <f.gruenbich...@proxmox.com>
> Co-authored-by: Dominic Jäger <d.jae...@proxmox.com>
> Signed-off-by: Fabian Ebner <f.eb...@proxmox.com>
> ---
>
> 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.
>
> PVE/API2/Qemu.pm | 229 ++++++++++++++++++++++++++++++-----
> PVE/QemuServer/Drive.pm | 34 +++++-
> PVE/QemuServer/ImportDisk.pm | 2 +-
> 3 files changed, 230 insertions(+), 35 deletions(-)
>
> 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 = sub {
> }
> };
>
> +# Used in import-enabled API endpoints. Parses drives using the extended
> '_with_alloc' schema.
> +my $foreach_volume_with_alloc = sub {
> + my ($param, $func) = @_;
> +
> + for my $opt (sort keys $param->%*) {
> + next if !PVE::QemuServer::is_valid_drivename($opt);
> +
> + my $drive = PVE::QemuServer::Drive::parse_drive($opt, $param->{$opt},
> 1);
> + next if !$drive;
> +
> + $func->($opt, $drive);
> + }
> +};
> +
> +my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
> +
> my $check_drive_param = sub {
> my ($param, $storecfg, $extra_checks) = @_;
>
> for my $opt (sort keys $param->%*) {
> next 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);
technically belongs into the previous patch, our non-alloc schema is
just tolerant enough because it doesn't look at the volids too closely
and accepts the NEW_DISK_RE syntax as potential existing volid..
> raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
>
> + if ($drive->{'import-from'}) {
> + die "'import-from' requires special syntax - use <storage
> ID>:0,import-from=<source>\n"
> + if $drive->{file} !~ $NEW_DISK_RE || $3 != 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);
>
> $extra_checks->($drive) if $extra_checks;
>
> - $param->{$opt} = PVE::QemuServer::print_drive($drive);
> + $param->{$opt} = PVE::QemuServer::print_drive($drive, 1);
> }
> };
>
> -my $NEW_DISK_RE = qr!^(([^/:\s]+):)?(\d+(\.\d+)?)$!;
> my $check_storage_access = sub {
> my ($rpcenv, $authuser, $storecfg, $vmid, $settings, $default_storage) =
> @_;
>
> - PVE::QemuConfig->foreach_volume($settings, sub {
> + $foreach_volume_with_alloc->($settings, sub {
> my ($ds, $drive) = @_;
>
> my $isCDROM = PVE::QemuServer::drive_is_cdrom($drive);
> @@ -106,6 +137,20 @@ my $check_storage_access = sub {
> } else {
> PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg,
> $vmid, $volid);
> }
> +
> + if (my $src_image = $drive->{'import-from'}) {
> + my $src_vmid;
> + my ($src_storeid) = 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
will return undef when $noerr is set.
> + $src_vmid = (PVE::Storage::parse_volname($storecfg,
> $src_image))[2]
is there some case where we expect parse_volume_id to work, but the
volume to not have an associated guest? because perl doesn't mind us
accessing the resulting array at arbitrary indices, so this doesn't fail
if $src_vmid is undef..
these should probably also check some more stuff (at least the volume
type?) - else we get strange errors when attempting to import
non-image-volumes (some of which even have owners, for example backup
archives..), and what exactly gets caught where is basically up to the
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);
> + }
> + }
> });
>
> $rpcenv->check($authuser, "/storage/$settings->{vmstatestorage}",
> ['Datastore.AllocateSpace'])
> @@ -164,6 +209,87 @@ my $check_storage_access_migrate = sub {
> if !$scfg->{content}->{images};
> };
>
> +my $import_from_volid = sub {
> + my ($storecfg, $src_volid, $dest_info, $vollist) = @_;
> +
> + die "cannot import from cloudinit disk\n"
> + if PVE::QemuServer::Drive::drive_is_cloudinit({ file => $src_volid });
> +
> + my ($src_storeid, $src_volname) =
> PVE::Storage::parse_volume_id($src_volid);
technically this is already implied by the sub's name, we checked it
already outside, but we need the store id for the bwlimit below..
> + my $src_vmid = (PVE::Storage::parse_volname($storecfg, $src_volid))[2];
> +
> + my $src_vm_state = sub {
> + my $exists = $src_vmid &&
> PVE::Cluster::get_vmlist()->{ids}->{$src_vmid} ? 1 : 0;
> +
> + my $runs = 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 = PVE::QemuServer::Helpers::vm_running_locally($src_vmid) ||
> 0;
> + }
> +
> + return ($exists, $runs);
> + };
> +
> + my ($src_vm_exists, $running) = $src_vm_state->();
> +
> + die "cannot import from '$src_volid' - full clone feature is not
> supported\n"
> + if !PVE::Storage::volume_has_feature($storecfg, 'copy', $src_volid,
> undef, $running);
> +
> + my $clonefn = sub {
> + my ($src_vm_exists_now, $running_now) = $src_vm_state->();
> +
> + die "owner VM $src_vmid changed state unexpectedly\n"
> + if $src_vm_exists_now != $src_vm_exists || $running_now != $running;
> +
> + my $src_conf = $src_vm_exists_now ?
> PVE::QemuConfig->load_config($src_vmid) : {};
> +
> + my $src_drive = { file => $src_volid };
> + my $src_drivename;
> + PVE::QemuConfig->foreach_volume($src_conf, sub {
> + my ($ds, $drive) = @_;
> +
> + return if $src_drivename;
> +
> + if ($drive->{file} eq $src_volid) {
> + $src_drive = $drive;
> + $src_drivename = $ds;
> + }
> + });
> +
> + my $source_info = {
> + vmid => $src_vmid,
> + running => $running_now,
> + drivename => $src_drivename,
> + drive => $src_drive,
> + snapname => 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_info->{storage}]),
> + );
> + };
> +
> + my $cloned;
> + if ($running) {
> + $cloned = PVE::QemuConfig->lock_config_full($src_vmid, 30, $clonefn);
> + } elsif ($src_vmid) {
> + $cloned = PVE::QemuConfig->lock_config_shared($src_vmid, 30, $clonefn);
> + } else {
> + $cloned = $clonefn->();
> + }
> +
> + return $cloned->@{qw(file size)};
> +};
> +
> # Note: $pool is only needed when creating a VM, because pool permissions
> # are automatically inherited if VM already exists inside a pool.
> my $create_disks = sub {
> @@ -207,28 +333,75 @@ 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'}) {
> + my $dst_volid;
> + my ($src_storeid) = 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
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 = {
> + vmid => $vmid,
> + drivename => $ds,
> + storage => $storeid,
> + format => $disk->{format},
> + };
> +
> + $dest_info->{efisize} =
> PVE::QemuServer::get_efivars_size($conf, $disk)
> + if $ds eq 'efidisk0';
> +
> + ($dst_volid, $size) = eval {
> + $import_from_volid->($storecfg, $source, $dest_info,
> $vollist);
> + };
> + die "cannot import from '$source' - $@" if $@;
> + } else {
> + $source = PVE::Storage::abs_filesystem_path($storecfg,
> $source, 1);
> + $size = PVE::Storage::file_size_info($source);
> + die "could not get file size of $source\n" if !$size;
> +
> + (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} = $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);
>
> @@ -242,7 +415,7 @@ my $create_disks = sub {
> }
> };
>
> - eval { PVE::QemuConfig->foreach_volume($settings, $code); };
> + eval { $foreach_volume_with_alloc->($settings, $code); };
>
> # free allocated images on error
> if (my $err = $@) {
> @@ -1285,7 +1458,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);
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 = 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);
same
> 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/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 = {
> %efitype_fmt,
> };
>
> +my %import_from_fmt = (
> + 'import-from' => {
> + type => 'string',
> + format => 'pve-volume-id-or-absolute-path',
> + format_description => 'source volume',
> + description => "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
> actively used by ".
> + "another process during the import!",
> + optional => 1,
> + },
> +);
> +my $alldrive_fmt_with_alloc = {
> + %$alldrive_fmt,
> + %import_from_fmt,
> +};
> +
> my $unused_fmt = {
> volume => { alias => 'file' },
> file => {
> @@ -436,6 +452,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 +463,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 STORAGE_ID:0 and the 'import-from' parameter
> to import from an ".
> + "existing volume.";
>
> $with_alloc_desc_cache->{$type} = $new_desc;
>
> @@ -547,7 +566,7 @@ sub drive_is_read_only {
> # [,iothread=on][,serial=serial][,model=model]
>
> sub parse_drive {
> - my ($key, $data) = @_;
> + my ($key, $data, $with_alloc) = @_;
technically previous patch, same as all the other changes in this file
below this change
>
> my ($interface, $index);
>
> @@ -558,12 +577,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 +644,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'};
should probably be added to the comment at start, even if it has a
speaking name ;) skiplock is missing as well.
> };
> 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
