[pve-devel] [PATCH v4 qemu-server] Add API for disk & VM import
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Feb 10 10:40:56 CET 2021
high level review: this is starting to take shape :)
I wonder whether it doesn't make sense to add proper permissions
already? most of it is handled in PVE::Storage::check_volume_access
or check_storage_access in this API module already.. the latter could be
extended to handle the new special syntax similary to NEW_DISK_RE, and
we'd basically have almost no special casing left..
I'm also a bit on the fence whether we really want a separate API call
or just a new parameter for the importvm feature. with the switch to a
single parameter for the key to source mapping and some of the changes
below it might be rather straight-forward - I might try to take your
next version and refactor it to be in-line with the regular create_vm to
see which variant looks better. just as a heads-up ;)
On February 5, 2021 11:04 am, Dominic Jäger wrote:
> Extend qm importdisk/importovf functionality to the API.
> qm can be adapted to use this later.
>
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
> Biggest v3->v4 changes:
> * New code instead of bloating update_vm_api
> * Don't change anything in the existing schema, use new parameter "diskimages"
> * Because this can happen later:
> - Only root can use this
> - Don't touch qm (yet)
>
>
> PVE/API2/Qemu.pm | 375 +++++++++++++++++++++++++++++++++++++++++-
> PVE/QemuServer.pm | 16 +-
> PVE/QemuServer/OVF.pm | 10 +-
> 3 files changed, 394 insertions(+), 7 deletions(-)
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 3571f5e..1ed763b 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -45,7 +45,6 @@ BEGIN {
> }
> }
>
> -use Data::Dumper; # fixme: remove
>
> use base qw(PVE::RESTHandler);
>
> @@ -4325,4 +4324,378 @@ __PACKAGE__->register_method({
> return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
> }});
>
> +# Raise exception if $format is not supported by $storageid
> +my $check_format_is_supported = sub {
> + my ($format, $storageid) = @_;
> +
> + return if !$format;
> +
> + my $store_conf = PVE::Storage::config();
it probably makes sense to pass this in as parameter, all call sites
probably have it already ;)
> + my (undef, $valid_formats) = PVE::Storage::storage_default_format($store_conf, $storageid);
> + my $supported = grep { $_ eq $format } @$valid_formats;
> +
> + if (!$supported) {
> + raise_param_exc({format => "$format is not supported on storage $storageid"});
I think a regular die here makes more sense (one of the two call-sites
you introduce can have a different parameter than 'format' as source of
the value being checked - that would be rather confusing..). it might
make it usable for other call-sites as well then. where applicable, you
can always raise a param exception with $@ as message ;)
> + }
> +};
> +
> +# paths are returned as is
> +# volids are returned as paths
> +#
> +# Also checks if $original actually exists
> +my $convert_to_path = sub {
> + my ($original) = @_;
> + my $volid_as_path = eval { # Nonempty iff $original_source is a volid
> + PVE::Storage::path(PVE::Storage::config(), $original);
> + };
> + my $result = $volid_as_path || $original ;
> + if (!-e $result) {
> + die "Could not import because source '$original' does not exist!";
> + }
> + return $result;
> +};
what does this do that PVE::Storage::abs_filesystem_path doesn't already
do (except having 'import' in the error message ;))?
ah, further down below I see that -b might be missing for raw block
devices.. maybe it makes sense to allow those in that helper as well?
optionally? call-sites would need to be checked..
> +
> +# vmid ... target VM ID
> +# source ... absolute path of the source image (volid must be converted before)
that restriction is not actually needed, see below
> +# storage ... target storage for the disk image
> +# format ... target format for the disk image (optional)
> +#
> +# returns ... volid of the allocated disk image (e.g. local-lvm:vm-100-disk-2)
> +my $import_disk_image = sub {
> + my ($param) = @_;
> + my $vmid = $param->{vmid};
> + my $requested_format = $param->{format};
> + my $storage = $param->{storage};
> + my $source = $param->{source};
> +
> + my $vm_conf = PVE::QemuConfig->load_config($vmid);
could be passed in, the call sites already have it.. and it might allow
to pull in some of the surrounding codes at the call sites that is
similar (config updating, property string building) into this helper
> + my $store_conf = PVE::Storage::config();
could also be passed in here
> + if (!$source) {
> + die "It is necessary to pass the source parameter";
> + }
just a check for definedness
> + if ($source !~ m!^/!) {
> + die "source must be an absolute path but is $source";
> + }
> + if (!-e $source) {
> + die "Could not import because source $source does not exist!";
> + }
and another call to abs_filesystem_path would do all these checks, and
support passing in regular volumes as source as well.
> + if (!$storage) {
> + die "It is necessary to pass the storage parameter";
> + }
call PVE::Storage::storage_config() here to get both that and the check
that the storage exists..
> +
> + print "Importing disk image '$source'...\n";
> +
> + my $src_size = PVE::Storage::file_size_info($source);
> + if (!defined($src_size)) {
> + die "Could not get file size of $source";
> + } elsif (!$src_size) {
> + die "Size of file $source is 0";
> + } elsif ($src_size==1) {
> + die "Cannot import a directory";
can't happen if abs_filesystem_path was used above - that only works for
volumes or files ;)
> + }
> +
> + $check_format_is_supported->($requested_format, $storage);
> +
> + my $dst_format = PVE::QemuServer::resolve_dst_disk_format(
> + $store_conf, $storage, undef, $requested_format);
> + my $dst_volid = PVE::Storage::vdisk_alloc($store_conf, $storage,
> + $vmid, $dst_format, undef, $src_size / 1024);
> +
> + eval {
> + local $SIG{INT} =
> + local $SIG{TERM} =
> + local $SIG{QUIT} =
> + local $SIG{HUP} =
> + local $SIG{PIPE} = sub { die "Interrupted by signal $!\n"; };
> +
> + my $zeroinit = PVE::Storage::volume_has_feature($store_conf,
> + 'sparseinit', $dst_volid);
> +
> + PVE::Storage::activate_volumes($store_conf, [$dst_volid]);
> + PVE::QemuServer::qemu_img_convert($source, $dst_volid,
> + $src_size, undef, $zeroinit);
> + PVE::Storage::deactivate_volumes($store_conf, [$dst_volid]);
> +
> + };
> + if (my $err = $@) {
> + eval { PVE::Storage::vdisk_free($store_conf, $dst_volid) };
> + warn "Cleanup of $dst_volid failed: $@ \n" if $@;
> +
> + die "Importing disk '$source' failed: $err\n" if $err;
> + }
> +
> + return $dst_volid;
> +};
> +
> +__PACKAGE__->register_method ({
> + name => 'importdisk',
> + path => '{vmid}/importdisk',
> + method => 'POST',
> + protected => 1, # for worker upid file
huh? no - because we need elevated privileges to allocate disks on
storages..
> + proxyto => 'node',
> + description => "Import an external disk image into a VM. The image format ".
> + "has to be supported by qemu-img.",
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + vmid => get_standard_option('pve-vmid',
> + {completion => \&PVE::QemuServer::complete_vmid}),
> + source => {
> + description => "Disk image to import. Can be a volid ".
> + "(local-lvm:vm-104-disk-0), an image on a PVE storage ".
> + "(local:104/toImport.raw) or (for root only) an absolute ".
how does the second work here? and the whole thing is root-only, so that
qualifier at the end is redundant ;)
> + "path on the server.",
> + type => 'string',
> + format => 'pve-volume-id-or-absolute-path',
> + },
> + device => {
> + type => 'string',
> + description => "Bus/Device type of the new disk (e.g. 'ide0', ".
> + "'scsi2'). Will add the image as unused disk if omitted.",
> + enum => [PVE::QemuServer::Drive::valid_drive_names()],
> + optional => 1,
> + },
> + device_options => {
> + type => 'string',
> + description => "Options to set for the new disk ".
> + "(e.g. 'discard=on,backup=0')",
> + optional => 1,
> + requires => 'device',
> + },
> + storage => get_standard_option('pve-storage-id', {
> + description => "The storage to which the image will be imported to.",
> + completion => \&PVE::QemuServer::complete_storage,
> + }),
> + format => {
> + type => 'string',
> + description => 'Target format.',
> + enum => [ 'raw', 'qcow2', 'vmdk' ],
> + optional => 1,
> + },
not directly related to this series, but this enum is copied all over
the place and might be defined in one place as standard option
(qm-new-disk-format ?)
> + digest => get_standard_option('pve-config-digest'),
> + },
> + },
> + returns => { type => 'string'},
> + code => sub {
> + my ($param) = @_;
> + my $vmid = extract_param($param, 'vmid');
> + my $node = extract_param($param, 'node');
> + my $original_source = extract_param($param, 'source');
not sure why this gets this name, it's just passed once and not used
afterwards.. also, see comments above about how to make this more
flexible..
> + my $digest = extract_param($param, 'digest');
> + my $device_options = extract_param($param, 'device_options');
IMHO this one needs to be parsed - e.g., by adding a fake volume with
the syntax used in importvm at the front and parsing it according to the
disk schema..
both to catch bogus stuff early, and to make the handling below more
robust w.r.t. future changes..
> + my $device = extract_param($param, 'device');
> + my $storecfg = PVE::Storage::config();
> + my $storeid = extract_param($param, 'storage');
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
> +
> + my $format_explicit = extract_param($param, 'format');
> + my $format_device_option;
> + if ($device_options) {
> + $device_options =~ m/format=([^,]*)/;
> + $format_device_option = $1;
e.g. here we'd just check $device_options->{format}
> + if ($format_explicit && $format_device_option) {
> + raise_param_exc({format => "Disk format may be specified only once!"});
format already specified in device_options?
> + }
> + }
> + my $format = $format_explicit || $format_device_option;
since both of those are not needed after this point, this could just be
returned / set by the above if, dropping the extra variables in the
outer scope..
> + $check_format_is_supported->($format, $storeid);
> +
> + my $locked = sub {
> + my $conf = PVE::QemuConfig->load_config($vmid);
> + PVE::Tools::assert_if_modified($conf->{digest}, $digest);
> +
> + if ($device && $conf->{$device}) {
> + die "Could not import because device $device is already in ".
> + "use in VM $vmid. Choose a different device!";
both of these checks might make sense outside of the worker already to
give immediate feedback in the API response..
> + }
> +
> + my $imported_volid = $import_disk_image->({
> + vmid => $vmid,
> + source => $convert_to_path->($original_source),
> + storage => $storeid,
> + format => $format,
> + });
so this could become
my $target = {
storage => $storeid,
format => $format,
options => $device_options,
device => $device,
};
$import_disk_image($storecfg, $vmid, $conf, $source, $target)
> +
> + my $volid = $imported_volid;
> + if ($device) {
> + # Attach with specified options
> + $volid .= ",${device_options}" if $device_options;
> + } else {
> + # Add as unused to config
> + $device = PVE::QemuConfig->add_unused_volume($conf, $imported_volid);
> + }
> + $update_vm_api->({
> + node => $node,
> + vmid => $vmid,
> + $device => $volid,
> + });
and all of that could move inside the helper
> + };
> + my $worker = sub {
> + PVE::QemuConfig->lock_config_full($vmid, 1, $locked);
> + };
> + return $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker);
> + }});
> +
> +__PACKAGE__->register_method({
> + name => 'importvm',
> + path => '{vmid}/importvm',
> + method => 'POST',
> + description => "Import a VM from existing disk images.",
> + protected => 1,
> + proxyto => 'node',
> + parameters => {
> + additionalProperties => 0,
> + properties => PVE::QemuServer::json_config_properties(
> + {
> + node => get_standard_option('pve-node'),
> + vmid => get_standard_option('pve-vmid', { completion => \&PVE::Cluster::complete_next_vmid }),
> + diskimages => {
> + description => "Mapping of devices to disk images." .
> + "For example, scsi0:/mnt/nfs/image1.vmdk,scsi1:/mnt/nfs/image2",
> + type => 'string',
> + },
> + start => {
> + optional => 1,
> + type => 'boolean',
> + default => 0,
> + description => "Start VM after it was imported successfully.",
> + },
> + }),
> + },
> + returns => {
> + type => 'string',
> + },
> + code => sub {
> + my ($param) = @_;
> + my $node = extract_param($param, 'node');
> + my $vmid = extract_param($param, 'vmid');
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
> + my $storecfg = PVE::Storage::config();
> +
> + PVE::Cluster::check_cfs_quorum();
> +
> + my $diskimages_string = extract_param($param, 'diskimages');
> + my @diskimage_pairs = split(',', $diskimages_string);
> +
> + my $use_import = sub {
> + my ($opt) = @_;
> + return 0 if $opt eq 'efidisk0';
> + return PVE::QemuServer::Drive::is_valid_drivename($opt);
> + };
> +
> + my $msg = "There must be exactly as many devices specified as there " .
> + " are devices in the diskimage parameter.\n For example for " .
> + "--scsi0 local-lvm:0,discard=on --scsi1 local:0,cache=unsafe " .
> + "there must be --diskimages scsi0=/source/path,scsi1=/other/path";
> + my $device_count = grep { $use_import->($_) } keys %$param;
why though? isn't it sufficient to say there must be a matching device for
each diskimages entry and vice-versa?
while doing the matching you could also check that the file exists (as
per the comments above ;))
> +
> + my $diskimages_count = @diskimage_pairs;
> + if ($device_count != $diskimages_count) {
> + raise_param_exc({diskimages => $msg});
> + }
> +
> + my $diskimages = {};
> + foreach ( @diskimage_pairs ) {
> + my ($device, $diskimage) = split('=', $_);
> + $diskimages->{$device} = $diskimage;
> + }
> +
> + my $worker = sub {
> + eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, 'import') };
> + die "Unable to create config for VM import: $@" if $@;
should happen outside the worker (so that a VMID clash is detected
early). inside the worker you just lock and load, then check that the
'import' lock is still there..
we could also filter out disk parameters, and call update_vm_api with
the rest here (those should be fast, but potentially in the future could
run into permission issues that would be nice to return early before
doing a massive disk conversion that has to be undone afterwards..
> +
> + my @volids_of_imported = ();
> + eval { foreach my $opt (keys %$param) {
this could then just iterate over the disk parameters
> + next if ($opt eq 'start');
> +
> + my $updated_value;
> + if ($use_import->($opt)) {
> + # $opt is bus/device like ide0, scsi5
> +
> + my $device = PVE::QemuServer::parse_drive($opt, $param->{$opt});
$drive? $device is used in this very patch for something else ;)
> + raise_param_exc({ $opt => "Unable to parse drive options" })
> + if !$device;
> +
> + my $source_path = $convert_to_path->($diskimages->{$opt});
see other comments regarding this
> +
> + $param->{$opt} =~ m/format=([^,]*)/;
> + my $format = $1;
parse!
also, if we allow mixing imports and newly-allocated empty or
pre-existing volumes, check whether it's actually one we want to import
here ;)
> +
> + my $imported_volid = $import_disk_image->({
> + vmid => $vmid,
> + source => $source_path,
> + device => $opt,
> + storage => (split ':', $device->{file})[0],
parse!
> + format => $format,
> + });
also see comment for the other call site about this helper's signature..
> + push @volids_of_imported, $imported_volid;
> +
> + # $param->{opt} has all required options but also dummy
> + # import 0 instead of the image
> + # for example, local-lvm:0,discard=on,mbps_rd=100
> + my $volid = $param->{$opt};
> + # Replace 0 with allocated volid, for example
> + # local-lvm:vm-100-disk-2,discard=on,mbps_rd=100
> + $volid =~ s/^.*?,/$imported_volid,/;
parse/print, move into helper..
> +
> + $updated_value = $volid;
> + } else {
> + $updated_value = $param->{$opt};
> + }
> + $update_vm_api->(
> + {
> + node => $node,
> + vmid => $vmid,
> + $opt => $updated_value,
> + skiplock => 1,
could just pass in the whole disk parameters instead, if we do the rest
early..
> + },
> + 1, # avoid nested workers that only do a short operation
> + );
> + }};
> +
> + my $conf = PVE::QemuConfig->load_config($vmid);
> + my $bootdevs = PVE::QemuServer::get_default_bootdevices($conf);
> + $update_vm_api->(
> + {
> + node => $node,
> + vmid => $vmid,
> + boot => PVE::QemuServer::print_bootorder($bootdevs),
> + skiplock => 1,
> + },
> + 1,
> + );
should only be done if boot is not already set..
> +
> + my $err = $@;
> + if ($err) {
> + foreach my $volid (@volids_of_imported) {
> + eval { PVE::Storage::vdisk_free($storecfg, $volid) };
> + warn $@ if $@;
> + }
> +
> + eval {
> + my $conffile = PVE::QemuConfig->config_file($vmid);
> + unlink($conffile) or die "Failed to remove config file: $!\n";
> + };
> + warn $@ if $@;
> +
> + die $err;
> + }
> +
> + eval { PVE::QemuConfig->remove_lock($vmid, 'import') };
> + warn $@ if $@;
> +
> + if ($param->{start}) {
> + PVE::QemuServer::vm_start($storecfg, $vmid);
> + }
> + };
> +
> + return $rpcenv->fork_worker('importvm', $vmid, $authuser, $worker);
> + }});
> +
> +
> 1;
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 9c65d76..c02f5eb 100644
> --- a/PVE/QemuServer.pm
> +++ b/PVE/QemuServer.pm
> @@ -300,7 +300,7 @@ my $confdesc = {
> optional => 1,
> type => 'string',
> description => "Lock/unlock the VM.",
> - enum => [qw(backup clone create migrate rollback snapshot snapshot-delete suspending suspended)],
> + enum => [qw(backup clone create migrate rollback snapshot snapshot-delete suspending suspended import)],
> },
> cpulimit => {
> optional => 1,
> @@ -998,6 +998,18 @@ sub verify_volume_id_or_qm_path {
> return $volid;
> }
>
> +PVE::JSONSchema::register_format('pve-volume-id-or-absolute-path', \&verify_volume_id_or_absolute_path);
> +sub verify_volume_id_or_absolute_path {
> + my ($volid, $noerr) = @_;
> +
> + # Exactly these 2 are allowed in id_or_qm_path but should not be allowed here
> + if ($volid eq 'none' || $volid eq 'cdrom') {
> + return undef if $noerr;
> + die "Invalid format! Should be volume ID or absolute path.";
> + }
> + return verify_volume_id_or_qm_path($volid, $noerr);
> +}
> +
> my $usb_fmt = {
> host => {
> default_key => 1,
> @@ -6659,7 +6671,7 @@ sub qemu_img_convert {
> $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname);
> $src_is_iscsi = ($src_path =~ m|^iscsi://|);
> $cachemode = 'none' if $src_scfg->{type} eq 'zfspool';
> - } elsif (-f $src_volid) {
> + } elsif (-f $src_volid || -b _) { # -b required to import from LVM images
> $src_path = $src_volid;
> if ($src_path =~ m/\.($PVE::QemuServer::Drive::QEMU_FORMAT_RE)$/) {
> $src_format = $1;
> diff --git a/PVE/QemuServer/OVF.pm b/PVE/QemuServer/OVF.pm
> index c76c199..36b7fff 100644
> --- a/PVE/QemuServer/OVF.pm
> +++ b/PVE/QemuServer/OVF.pm
> @@ -87,7 +87,7 @@ sub id_to_pve {
>
> # returns two references, $qm which holds qm.conf style key/values, and \@disks
> sub parse_ovf {
> - my ($ovf, $debug) = @_;
> + my ($ovf, $debug, $ignore_size) = @_;
>
> my $dom = XML::LibXML->load_xml(location => $ovf, no_blanks => 1);
>
> @@ -220,9 +220,11 @@ ovf:Item[rasd:InstanceID='%s']/rasd:ResourceType", $controller_id);
> die "error parsing $filepath, file seems not to exist at $backing_file_path\n";
> }
>
> - my $virtual_size;
> - if ( !($virtual_size = PVE::Storage::file_size_info($backing_file_path)) ) {
> - die "error parsing $backing_file_path, size seems to be $virtual_size\n";
> + my $virtual_size = 0;
> + if (!$ignore_size) { # Not possible if manifest is uploaded in web gui
> + if ( !($virtual_size = PVE::Storage::file_size_info($backing_file_path)) ) {
> + die "error parsing $backing_file_path: Could not get file size info: $@\n";
> + }
> }
>
> $pve_disk = {
> --
> 2.20.1
>
>
> _______________________________________________
> 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