On January 13, 2022 11:08 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 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 <storage
> ID>:0,import-from=<source>\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 ;)
> 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
