[pve-devel] [RFC v10 qemu-server 6/7] api: support VM disk import
Fabian Ebner
f.ebner at proxmox.com
Tue Jan 18 09:51:54 CET 2022
Am 17.01.22 um 16:39 schrieb Fabian Grünbichler:
> 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 ;)
The error just gives an example, but I agree that's not clear without
any "e.g." in there ;) And by allowing (and ignoring) more patterns now,
adding the "resize on the fly" feature becomes much more difficult in
the future (should we ever want that). I'll make the check more strict
(just adding a check for size 0 avoids the need for a second RE) and
adapt the description below as suggested.
>
>> 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
>>
>
>
> _______________________________________________
> 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