[pve-devel] [PATCH v6 qemu-server] Add API for import wizards
Fabian Grünbichler
f.gruenbichler at proxmox.com
Mon Mar 15 10:25:06 CET 2021
still a few more comments inline - but this is taking up shape (and the
next iteration will be shorter ;)).
On March 9, 2021 11:43 am, Dominic Jäger wrote:
> Extend qm importdisk/importovf functionality to the API.
>
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
>
> ---
> v5->v6:
> More parsing
> Fix regex
> Improve --boot handling
> Move readovf from manager to qemu-server (like CPU)
> Create properties helper for readovf return values
>
> PVE/API2/Qemu.pm | 458 ++++++++++++++++++++++++++++++++++++++++-
> PVE/API2/Qemu/Makefile | 2 +-
> PVE/API2/Qemu/OVF.pm | 68 ++++++
> PVE/QemuServer.pm | 32 ++-
> PVE/QemuServer/OVF.pm | 10 +-
> 5 files changed, 562 insertions(+), 8 deletions(-)
> create mode 100644 PVE/API2/Qemu/OVF.pm
>
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index 6706b55..a689c9e 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);
>
> @@ -4383,4 +4382,461 @@ __PACKAGE__->register_method({
> return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
> }});
>
> +# Raise exception if $format is not supported by $storeid
> +my $check_format_is_supported = sub {
> + my ($format, $storeid, $storecfg) = @_;
> + die "You have to provide storage ID" if !$storeid;
> + die "You have to provide the storage configurration" if !$storecfg;
these two are basic sanity checks - they can only trigger if the code is
wrong. the 'You' is this potentially misleading. also, typo in the
second message ;)
> +
> + return if !$format;
> +
> + my (undef, $valid_formats) = PVE::Storage::storage_default_format($storecfg, $storeid);
> + my $supported = grep { $_ eq $format } @$valid_formats;
> +
> + die "$format is not supported on storage $storeid" if !$supported;
format '$format', else this could read strange.
> +};
> +
> +# storecfg ... PVE::Storage::config()
> +# vmid ... target VM ID
> +# vmconf ... target VM configuration
> +# source ... source image (volid or absolute path)
> +# target ... hash with
> +# storeid => storage ID
> +# format => disk format (optional)
> +# options => string with device options (may or may not contain <storeid>:0)
> +# device => device where the disk is attached (for example, scsi3) (optional)
> +#
> +# returns ... volid of the allocated disk image (e.g. local-lvm:vm-100-disk-2)
> +my $import_disk_image = sub {
> + my ($param) = @_;
you misunderstood my comments here - I really meant
my ($storecfg, $vmid, $conf, $source, $target) = @_;
putting everything into a single param hash is usually a sign of "need
to refactor this", and tricks you into thinking "bolting on
yet-another-parameter is fine" ;)
> + my $storecfg = $param->{storecfg};
> + my $vmid = $param->{vmid};
> + my $vmconf = $param->{vmconf};
> + my $target = $param->{target};
> + my $requested_format = $target->{format};
> + my $storeid = $target->{storeid};
> +
> + die "Source parameter is undefined!" if !defined $param->{source};
> + my $source = PVE::Storage::abs_filesystem_path($storecfg, $param->{source}, 1);
> +
> + eval { PVE::Storage::storage_config($storecfg, $storeid) };
> + die "Error while importing disk image $source: $@\n" if $@;
> +
> + my $src_size = PVE::Storage::file_size_info($source);
> + # Previous abs_filesystem_path performs additional checks
this comment doesn't tell me anything. IF you really want, add a comment
above abs_file_system_path like
# Check foo, bar and baz
abs_file_system_path
# Check storage exists
storage_config
file_size_info
> + die "Could not get file size of $source" if !defined($src_size);
> +
> + $check_format_is_supported->($requested_format, $storeid, $storecfg);
> +
> + my $dst_format = PVE::QemuServer::resolve_dst_disk_format(
> + $storecfg, $storeid, undef, $requested_format);
> + my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storeid,
> + $vmid, $dst_format, undef, $src_size / 1024);
wrong continuation/indentation/wrapping for both
> +
> + print "Importing disk image '$source'...\n";
as '$dst_volid'..
?
> + 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($storecfg,
> + 'sparseinit', $dst_volid);
same indentation issue
> +
> + PVE::Storage::activate_volumes($storecfg, [$dst_volid]);
> + PVE::QemuServer::qemu_img_convert($source, $dst_volid,
> + $src_size, undef, $zeroinit);
same
> + PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]);
> +
> + };
> + if (my $err = $@) {
> + eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) };
> + warn "Cleanup of $dst_volid failed: $@ \n" if $@;
> +
> + die "Importing disk '$source' failed: $err\n" if $err;
> + }
> +
> + my $drive = $dst_volid;
> + if ($target->{device}) {
> + # Attach to target device with options if they are specified
> + if (defined $target->{options}) {
> + # Options string with or without storeid is allowed
> + # => Avoid potential duplicate storeid for update
> + $target->{options} =~ s/$storeid:0,?//; # ? for if only storeid:0 present
> + $drive .= ",$target->{options}" ;
please use parsed options (by passing them in from the call site instead
of the plain string - currently, the same thing gets parsed again and
again, which is not needed).
> + }
> + } else {
> + $target->{device} = PVE::QemuConfig->add_unused_volume($vmconf, $dst_volid);
> + }
> + print "Imported '$source' to $dst_volid\n";
shouldn't this go higher up, e.g. in case adding the unused volume dies?
also, if we mention both paths/volids in the "Importing ..." message,
this can just become a single "Done" or "Success", directly after the
actual import error handling?
> + $update_vm_api->(
> + {
> + node => $target->{node},
why? not mentioned in the comment describing this method, also, this can
by definition only be the current node anyway?
> + vmid => $vmid,
> + $target->{device} => $drive,
> + skiplock => 1,
> + },
> + 1,
> + );
> +
> + return $dst_volid;
> +};
> +
> +__PACKAGE__->register_method ({
> + name => 'importdisk',
> + path => '{vmid}/importdisk',
> + method => 'POST',
> + proxyto => 'node',
> + protected => 1,
> + 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:99/imageToImport.raw) or an absolute path on the server.",
> + type => 'string',
> + },
> + 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,
> + },
> + 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 $source = extract_param($param, 'source');
> + my $digest_param = extract_param($param, 'digest');
> + my $device_options = extract_param($param, 'device_options');
> + my $device = extract_param($param, 'device');
> + my $storeid = extract_param($param, 'storage');
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
> + my $storecfg = PVE::Storage::config();
> + PVE::Storage::storage_config($storecfg, $storeid); # check for errors
check that storage exists?
> +
> + # Format can be set explicitly "--format vmdk"
> + # or as part of device options "--device_options discard=on,format=vmdk"
> + # or not at all, but not both together
> + my $device_options_format;
> + if ($device_options) {
> + # parse device_options string according to disk schema for
> + # validation and to make usage easier
> +
> + # any existing storage ID is OK to get a valid (fake) string for parse_drive
> + my $valid_string = "$storeid:0,$device_options";
> +
> + # member "file" is fake
> + my $drive_full = PVE::QemuServer::Drive::parse_drive($device, $valid_string);
so this might be written back to $device_options, makes the rest of the
code more reliable/usable and allows us to only parse here once..
> + $device_options_format = $drive_full->{format};
no need for this then, it's just $device_options->{format}
> + }
> +
> + my $format = extract_param($param, 'format'); # may be undefined
> + if ($device_options) {
> + if ($format && $device_options_format) {
> + raise_param_exc({format => "Format already specified in device_options!"});
> + } else {
> + $format = $format || $device_options_format; # may still be undefined
> + }
> + }
> +
> + $check_format_is_supported->($format, $storeid, $storecfg);
> +
> + # provide a useful error (in the API response) before forking
> + my $no_lock_conf = PVE::QemuConfig->load_config($vmid);
$conf is fine for this, and a simple
# quick checks before fork + lock
should be enough, it's a common pattern in our API2 modules..
> + PVE::QemuConfig->check_lock($no_lock_conf);
> + PVE::Tools::assert_if_modified($no_lock_conf->{digest}, $digest_param);
> + if ($device && $no_lock_conf->{$device}) {
> + die "Could not import because device $device is already in ".
> + "use in VM $vmid. Choose a different device!";
> + }
> +
> + my $worker = sub {
> + my $conf;
> + PVE::QemuConfig->lock_config($vmid, sub {
> + $conf = PVE::QemuConfig->load_config($vmid);
> + PVE::QemuConfig->check_lock($conf);
> +
> + # Our device-in-use check may be invalid if the new conf is different
> + PVE::Tools::assert_if_modified($conf->{digest}, $no_lock_conf->{digest});
> +
> + PVE::QemuConfig->set_lock($vmid, 'import');
> + });
> +
> + my $target = {
> + node => $node,
> + storeid => $storeid,
> + };
> + # Avoid keys with undef values
why?
> + $target->{format} = $format if defined $format;
> + $target->{device} = $device if defined $device;
> + $target->{options} = $device_options if defined $device_options;
> + $import_disk_image->({
> + storecfg => $storecfg,
> + vmid => $vmid,
> + vmconf => $conf,
> + source => $source,
> + target => $target,
> + });
> +
> + PVE::QemuConfig->remove_lock($vmid, 'import');
> + };
> + 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',
should probably get a proper format, and this can then be a
'device-image-mapping-list' or whatever fancy name you come up with ;)
\0-delimited might be a good idea - else you restrict what kind of paths
can be imported.. (for CLI, the parameter can be passed multiple times
to defined the list IIRC). ping me if you can't find an example for how
to do this!
> + },
> + 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 $diskimages_string = extract_param($param, 'diskimages');
> + my $boot = extract_param($param, 'boot');
> +
> + my $rpcenv = PVE::RPCEnvironment::get();
> + my $authuser = $rpcenv->get_user();
> + my $storecfg = PVE::Storage::config();
> +
> + PVE::Cluster::check_cfs_quorum();
> +
> + # Return true iff $opt is to be imported, that means it is a device
> + # like scsi2 and the special import syntax/zero is specified for it,
> + # for example local-lvm:0 but not local-lvm:5
> + my $is_import = sub {
> + my ($opt) = @_;
> + return 0 if $opt eq 'efidisk0';
> + if (PVE::QemuServer::Drive::is_valid_drivename($opt)) {
> + my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> + return $drive->{file} =~ m/0$/;
and if I want to create 10GB volume? note that we have a NEW_DISK_RE at
the top of this module, maybe we want a IMPORT_DISK_RE next to it?
> + }
> + return 0;
> + };
> +
> + # bus/device like ide0, scsi5 where the imported disk images get attached
> + my $target_devices = [];
> + # List of VM parameters like memory, cpu type, but also disks that are newly created
# everything else
?
why not build the $param hashes directly, instead of indirectly via
lists? e.g., you can just remove the import params and put them in a new
hash, and keep $params for the rest?
> + my $vm_params = [];
> + foreach my $opt (keys %$param) {
> + next if ($opt eq 'start'); # does not belong in config
> + # New function, so we can forbid deprecated
> + raise_param_exc({bootdisk => "Deprecated: Use --boot order= instead"})
> + if $opt eq 'bootdisk';
> +
> + my $list = $is_import->($opt) ? $target_devices : $vm_params;
> + push @$list, $opt;
> + }
> +
> + my $diskimages = {};
> + foreach (split ',', $diskimages_string) {
> + my ($device, $diskimage) = split('=', $_);
avoid using $_ for such things please.
> + $diskimages->{$device} = $diskimage;
> + if (defined $param->{$device}) {
> + my $drive = PVE::QemuServer::parse_drive($device, $param->{$device});
> + $drive->{file} =~ m/(\d)$/;
you define a helper above, but not use it here.. also, the check is
wrong here as well ;) if you split the $import_params from $params above,
then you can just check if $import_params->{$device} is defined, in
which case you know it has the special import syntax.
> + if ($1 != 0) {
> + raise_param_exc({
> + $device => "Each entry of --diskimages must have a ".
> + "corresponding device with special import syntax " .
> + "(e.g. --scsi3 local-lvm:0). $device from " .
> + "--diskimages has a matching device but the size " .
> + "of that is $1 instead of 0!",
> + });
here the question is - which parameter is wrong ;)
"device '$device' not marked for import, but import source '..'
specified"
> + }
> + } else {
> + # It is possible to also create new empty disk images during
> + # import by adding something like scsi2=local:10, therefore
> + # vice-versa check is not required
same message as for the other branch, no comment needed. in fact, this
could just be
die/raise_param_exc ..
if (!defined($param->{$device}) || !$is_import->($param->{$device}));
altogether to replace both branches and the comment and the overly long
messages. by splitting $params early, it then becomes
die/...
if !defined($import_params{$device});
> + raise_param_exc({
> + diskimages => "There must be a matching device for each " .
> + "--diskimages entry that should be imported, but " .
> + "there is no matching device for $device!\n" .
> + " For example, for --diskimages scsi0=/source/path,scsi1=/other/path " .
> + "there must be --scsi0 local-lvm:0,discard=on --scsi1 local:0,cache=unsafe",
> + });
> + }
> + # Dies if $diskimage cannot be found
> + PVE::Storage::abs_filesystem_path($storecfg, $diskimage, 1);
> + }
> + foreach my $device (@$target_devices) {
> + my $drive = PVE::QemuServer::parse_drive($device, $param->{$device});
> + if ($drive->{file} =~ m/0$/) {
> + if (!defined $diskimages->{$device}) {
> + raise_param_exc({
> + $device => "Each device with the special import " .
> + "syntax (the 0) must have a corresponding in " .
> + "--diskimages that specifies the source of the " .
> + "import, but there is no such entry for $device!",
> + });
> + }
so if you extract the import_params instead of using a target_devices
list, you can just map and grep here to compare $diskimages keys with
$import_params keys.. no need to parse again and again
or if you want a loop:
foreach my $import_device (keys %$import_params) {
die/raise_param_exc "device '$import_device' marked for import, but no source given\n"
if !defined($diskimages->{$import_device});
}
> + }
> + }
> +
> + # After devices are ensured to be correct
why? could also go before, you don't even differentiate between import
or create below? also, we don't have such a check when creating a VM, do
we? just when updating. maybe it would make sense to unify the handling?
this is not related to import at all, so could also be split out for a
follow-up.
> + if ($boot) {
> + my $new_bootcfg = PVE::JSONSchema::parse_property_string('pve-qm-boot', $boot);
> + if ($new_bootcfg->{order}) {
> + my @devs = PVE::Tools::split_list($new_bootcfg->{order});
> + for my $dev (@devs) {
> + my $will_be_imported = grep (/^$dev$/, @$target_devices);
> + my $will_be_created = grep (/^$dev$/, @$vm_params);
> + if ( !($will_be_imported || $will_be_created)) {
> + raise_param_exc({boot => "$dev will be neither imported " .
> + "nor created, so it cannot be a boot device!"});
> + }
> + }
> + } else {
> + raise_param_exc({boot => "Deprecated: Use --boot order= instead"});
couldn't this go above then next to the bootdisk check?
> + }
> + }
> +
> + eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, 'import') };
> + die "Unable to create config for VM import: $@" if $@;
> +
> + my $worker = sub {
> + my $get_conf = sub {
reload_conf might be a better name
> + my ($vmid) = @_;
> + PVE::QemuConfig->lock_config($vmid, sub {
why? you don't update the config here? an flock is needed for *writing*
the config (or *blocking writes* by others for a short period)..
> + my $conf = PVE::QemuConfig->load_config($vmid);
> + if (PVE::QemuConfig->has_lock($conf, 'import')) {
> + return $conf;
> + } else {
> + die "import lock in VM $vmid config file missing!";
> + }
> + });
> + };
> +
> + $get_conf->($vmid); # quick check for lock
> +
> + my $short_update = {
this is a misleading name - this contains all the disks that are not
imported, which can take a while (not as long as importing, but
still..). and it's not needed if we split the import_params out of
$params above anyway..
> + node => $node,
> + vmid => $vmid,
> + skiplock => 1,
> + };
> + foreach ( @$vm_params ) {
> + $short_update->{$_} = $param->{$_};
> + }
> + $update_vm_api->($short_update, 1); # writes directly in config file
yeah, so this does an flock internally, but to guarantee it still is the
right config you need to set the digest in $short_update.. same for
other calls to update_vm_api - you reload the config, check your config
lock is still there, then you have a valid digest to pass to
update_vm_api, which obtains an flock (== no other writes possible),
checks the digest (== no writes happened since our config reload before
calling update_vm_api) and then does the config modification and write
before releasing the flock. of course, now the digest and our $conf is
invalid, so we need to reload (and now we have a digest again for the
next call ;)).
> +
> + my $conf = $get_conf->($vmid);
> +
> + # When all short updates were succesfull, then the long imports
> + my @imported_successfully = ();
> + eval { foreach my $device (@$target_devices) {
> + my $param_parsed = PVE::QemuServer::parse_drive($device, $param->{$device});
we already did that, so please reuse the result from earlier..
> + die "Parsing $param->{$device} failed" if !$param_parsed;
> + my $storeid = PVE::Storage::parse_volume_id($param_parsed->{file});
> +
> + my $imported = $import_disk_image->({
> + storecfg => $storecfg,
> + vmid => $vmid,
> + vmconf => $conf,
> + source => $diskimages->{$device},
> + target => {
> + storeid => $storeid,
> + format => $param_parsed->{format},
> + options => $param->{$device},
and pass the parsed options here..
> + device => $device,
> + },
> + });
> + push @imported_successfully, $imported;
> + }};
> + my $err = $@;
> + if ($err) {
> + foreach my $volid (@imported_successfully) {
> + 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: $!";
at this point we'd need a proper VM removal - disks might have been
allocated, and who knows what else..
> + };
> + warn $@ if $@;
> +
> + die "Import aborted: $err";
> + }
> + if (!$boot) {
> + $conf = $get_conf->($vmid); # import_disk_image changed config file directly
> + my $bootdevs = PVE::QemuServer::get_default_bootdevices($conf);
> + $boot = PVE::QemuServer::print_bootorder($bootdevs);
> + }
> + $update_vm_api->(
> + {
> + node => $node,
> + vmid => $vmid,
> + boot => $boot,
> + skiplock => 1,
> + },
> + 1,
> + );
> +
> + 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/API2/Qemu/Makefile b/PVE/API2/Qemu/Makefile
> index 5d4abda..bdd4762 100644
> --- a/PVE/API2/Qemu/Makefile
> +++ b/PVE/API2/Qemu/Makefile
> @@ -1,4 +1,4 @@
> -SOURCES=Agent.pm CPU.pm Machine.pm
> +SOURCES=Agent.pm CPU.pm Machine.pm OVF.pm
>
> .PHONY: install
> install:
> diff --git a/PVE/API2/Qemu/OVF.pm b/PVE/API2/Qemu/OVF.pm
> new file mode 100644
> index 0000000..bd6e90b
> --- /dev/null
> +++ b/PVE/API2/Qemu/OVF.pm
> @@ -0,0 +1,68 @@
> +package PVE::API2::Qemu::OVF;
> +
> +use strict;
> +use warnings;
> +
> +use PVE::RESTHandler;
> +use PVE::JSONSchema qw(get_standard_option);
> +use PVE::QemuServer::OVF;
> +
> +use base qw(PVE::RESTHandler);
> +
> +__PACKAGE__->register_method ({
> + name => 'index',
> + path => '',
> + method => 'GET',
> + proxyto => 'node',
> + description => "Read an .ovf manifest.",
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + manifest => {
> + description => ".ovf manifest",
> + type => 'string',
> + },
> + },
> + },
> + returns => {
> + description => "VM config according to .ovf manifest and digest of manifest",
> + type => "object",
> + },
> + returns => {
> + type => 'object',
> + additionalProperties => 1,
> + properties => PVE::QemuServer::json_ovf_properties({
> + name => {
> + type => 'string',
> + optional => 1,
> + },
> + cores => {
> + type => 'integer',
> + optional => 1,
> + },
> + memory => {
> + type => 'integer',
> + optional => 1,
> + },
> + }),
> + },
> + code => sub {
> + my ($param) = @_;
> +
> + my $manifest = $param->{manifest};
> + die "$manifest: non-existent or non-regular file\n" if (! -f $manifest);
> +
> + my $parsed = PVE::QemuServer::OVF::parse_ovf($manifest, 0, 1);
> + my $result;
> + $result->{cores} = $parsed->{qm}->{cores};
> + $result->{name} = $parsed->{qm}->{name};
> + $result->{memory} = $parsed->{qm}->{memory};
> + my $disks = $parsed->{disks};
> + foreach my $disk (@$disks) {
> + $result->{$disk->{disk_address}} = $disk->{backing_file};
> + }
> + return $result;
> + }});
> +
> +1;
> \ No newline at end of file
> diff --git a/PVE/QemuServer.pm b/PVE/QemuServer.pm
> index 1410ecb..d4017b7 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)],
can be fine - not 100% convinced yet that we need to treat this as
separate from 'create' ;)
> },
> 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') {
then maybe turn it around? it makes more sense to have a base format and
then an extension IMHO. else now someone has to keep in mind that there
is a filter here when updating the other format, which is easily
missed..
> + 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,
> @@ -2030,6 +2042,22 @@ sub json_config_properties {
> return $prop;
> }
>
> +# Properties that we can read from an OVF file
> +sub json_ovf_properties {
> + my $prop = shift;
> +
> + foreach my $device ( PVE::QemuServer::Drive::valid_drive_names()) {
> + $prop->{$device} = {
> + type => 'string',
> + format => 'pve-volume-id-or-qm-path',
wrong format?
> + description => "Disk image that gets imported to $device",
> + optional => 1,
> + };
> + }
> +
> + return $prop;
> +}
> +
> # return copy of $confdesc_cloudinit to generate documentation
> sub cloudinit_config_properties {
>
> @@ -6722,7 +6750,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
-b $src_volid
the comment is not needed (and wrong) - '-b' already states that we
check for block devices, and this is not LVM specific at all, it also
handles direct references to zvols, mapped rbd images, physical disks,
loopback devices, or, well, any other block device ;)
> $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) = @_;
maybe something like $manifest_only - we might want to skip other
futures stuff as well, not just reading the file 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";
> }
doesn't this already fail?
>
> - 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;
is 0 a good default here? wouldn't undef make more sense?
> + 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