[pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import

Fabian Grünbichler f.gruenbichler at proxmox.com
Mon Jan 17 16:39:06 CET 2022


On January 13, 2022 11:08 am, Fabian Ebner wrote:
> From: Dominic Jäger <d.jaeger at proxmox.com>
> 
> Extend qm importdisk functionality to the API.
> 
> Co-authored-by: Fabian Grünbichler <f.gruenbichler at proxmox.com>
> Co-authored-by: Dominic Jäger <d.jaeger at proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner at 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 at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 




More information about the pve-devel mailing list