From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id C37626A668 for ; Mon, 15 Mar 2021 10:25:19 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id AE69B1E3D2 for ; Mon, 15 Mar 2021 10:25:19 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 355291E3C7 for ; Mon, 15 Mar 2021 10:25:17 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id CDB78426CE for ; Mon, 15 Mar 2021 10:25:16 +0100 (CET) Date: Mon, 15 Mar 2021 10:25:06 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox VE development discussion References: <20210309104318.317454-1-d.jaeger@proxmox.com> <20210309104318.317454-2-d.jaeger@proxmox.com> In-Reply-To: <<20210309104318.317454-2-d.jaeger@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1615553780.00blcsy2ca.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v6 qemu-server] Add API for import wizards X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 15 Mar 2021 09:25:19 -0000 still a few more comments inline - but this is taking up shape (and the=20 next iteration will be shorter ;)). On March 9, 2021 11:43 am, Dominic J=C3=A4ger wrote: > Extend qm importdisk/importovf functionality to the API. >=20 > Signed-off-by: Dominic J=C3=A4ger >=20 > --- > 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 >=20 > 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 >=20 > 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 { > } > } > =20 > -use Data::Dumper; # fixme: remove > =20 > use base qw(PVE::RESTHandler); > =20 > @@ -4383,4 +4382,461 @@ __PACKAGE__->register_method({ > return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param-= >{vmid}, $param->{type}); > }}); > =20 > +# Raise exception if $format is not supported by $storeid > +my $check_format_is_supported =3D sub { > + my ($format, $storeid, $storecfg) =3D @_; > + 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=20 wrong. the 'You' is this potentially misleading. also, typo in the=20 second message ;) > + > + return if !$format; > + > + my (undef, $valid_formats) =3D PVE::Storage::storage_default_format(= $storecfg, $storeid); > + my $supported =3D 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 =3D> storage ID > +# format =3D> disk format (optional) > +# options =3D> string with device options (may or may not contain :0) > +# device =3D> device where the disk is attached (for example, scsi3) = (optional) > +# > +# returns ... volid of the allocated disk image (e.g. local-lvm:vm-100-d= isk-2) > +my $import_disk_image =3D sub { > + my ($param) =3D @_; you misunderstood my comments here - I really meant my ($storecfg, $vmid, $conf, $source, $target) =3D @_; putting everything into a single param hash is usually a sign of "need=20 to refactor this", and tricks you into thinking "bolting on=20 yet-another-parameter is fine" ;) > + my $storecfg =3D $param->{storecfg}; > + my $vmid =3D $param->{vmid}; > + my $vmconf =3D $param->{vmconf}; > + my $target =3D $param->{target}; > + my $requested_format =3D $target->{format}; > + my $storeid =3D $target->{storeid}; > + > + die "Source parameter is undefined!" if !defined $param->{source}; > + my $source =3D 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 =3D 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=20 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 =3D PVE::QemuServer::resolve_dst_disk_format( > + $storecfg, $storeid, undef, $requested_format); > + my $dst_volid =3D 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} =3D > + local $SIG{TERM} =3D > + local $SIG{QUIT} =3D > + local $SIG{HUP} =3D > + local $SIG{PIPE} =3D sub { die "Interrupted by signal $!\n"; }; > + > + my $zeroinit =3D 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 =3D $@) { > + 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 =3D $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 > + # =3D> Avoid potential duplicate storeid for update > + $target->{options} =3D~ s/$storeid:0,?//; # ? for if only storeid:0= present > + $drive .=3D ",$target->{options}" ; please use parsed options (by passing them in from the call site instead=20 of the plain string - currently, the same thing gets parsed again and=20 again, which is not needed). > + } > + } else { > + $target->{device} =3D 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?=20 also, if we mention both paths/volids in the "Importing ..." message,=20 this can just become a single "Done" or "Success", directly after the=20 actual import error handling? > + $update_vm_api->( > + { > + node =3D> $target->{node}, why? not mentioned in the comment describing this method, also, this can=20 by definition only be the current node anyway? > + vmid =3D> $vmid, > + $target->{device} =3D> $drive, > + skiplock =3D> 1, > + }, > + 1, > + ); > + > + return $dst_volid; > +}; > + > +__PACKAGE__->register_method ({ > + name =3D> 'importdisk', > + path =3D> '{vmid}/importdisk', > + method =3D> 'POST', > + proxyto =3D> 'node', > + protected =3D> 1, > + description =3D> "Import an external disk image into a VM. The image= format ". > + "has to be supported by qemu-img.", > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> { > + node =3D> get_standard_option('pve-node'), > + vmid =3D> get_standard_option('pve-vmid', > + {completion =3D> \&PVE::QemuServer::complete_vmid}), > + source =3D> { > + description =3D> "Disk image to import. Can be a volid ". > + "(local:99/imageToImport.raw) or an absolute path on the server.", > + type =3D> 'string', > + }, > + device =3D> { > + type =3D> 'string', > + description =3D> "Bus/Device type of the new disk (e.g. 'ide0', ". > + "'scsi2'). Will add the image as unused disk if omitted.", > + enum =3D> [PVE::QemuServer::Drive::valid_drive_names()], > + optional =3D> 1, > + }, > + device_options =3D> { > + type =3D> 'string', > + description =3D> "Options to set for the new disk (e.g. 'discard=3Don,= backup=3D0')", > + optional =3D> 1, > + requires =3D> 'device', > + }, > + storage =3D> get_standard_option('pve-storage-id', { > + description =3D> "The storage to which the image will be imported to."= , > + completion =3D> \&PVE::QemuServer::complete_storage, > + }), > + format =3D> { > + type =3D> 'string', > + description =3D> 'Target format.', > + enum =3D> [ 'raw', 'qcow2', 'vmdk' ], > + optional =3D> 1, > + }, > + digest =3D> get_standard_option('pve-config-digest'), > + }, > + }, > + returns =3D> { type =3D> 'string'}, > + code =3D> sub { > + my ($param) =3D @_; > + my $vmid =3D extract_param($param, 'vmid'); > + my $node =3D extract_param($param, 'node'); > + my $source =3D extract_param($param, 'source'); > + my $digest_param =3D extract_param($param, 'digest'); > + my $device_options =3D extract_param($param, 'device_options'); > + my $device =3D extract_param($param, 'device'); > + my $storeid =3D extract_param($param, 'storage'); > + > + my $rpcenv =3D PVE::RPCEnvironment::get(); > + my $authuser =3D $rpcenv->get_user(); > + my $storecfg =3D 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=3Don,format=3D= 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 pa= rse_drive > + my $valid_string =3D "$storeid:0,$device_options"; > + > + # member "file" is fake > + my $drive_full =3D PVE::QemuServer::Drive::parse_drive($device, $va= lid_string); so this might be written back to $device_options, makes the rest of the=20 code more reliable/usable and allows us to only parse here once.. > + $device_options_format =3D $drive_full->{format}; no need for this then, it's just $device_options->{format} > + } > + > + my $format =3D extract_param($param, 'format'); # may be undefined > + if ($device_options) { > + if ($format && $device_options_format) { > + raise_param_exc({format =3D> "Format already specified in device_optio= ns!"}); > + } else { > + $format =3D $format || $device_options_format; # may still be undefine= d > + } > + } > + > + $check_format_is_supported->($format, $storeid, $storecfg); > + > + # provide a useful error (in the API response) before forking > + my $no_lock_conf =3D 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 =3D sub { > + my $conf; > + PVE::QemuConfig->lock_config($vmid, sub { > + $conf =3D 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 =3D { > + node =3D> $node, > + storeid =3D> $storeid, > + }; > + # Avoid keys with undef values why? > + $target->{format} =3D $format if defined $format; > + $target->{device} =3D $device if defined $device; > + $target->{options} =3D $device_options if defined $device_options; > + $import_disk_image->({ > + storecfg =3D> $storecfg, > + vmid =3D> $vmid, > + vmconf =3D> $conf, > + source =3D> $source, > + target =3D> $target, > + }); > + > + PVE::QemuConfig->remove_lock($vmid, 'import'); > + }; > + return $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker); > + }}); > + > +__PACKAGE__->register_method({ > + name =3D> 'importvm', > + path =3D> '{vmid}/importvm', > + method =3D> 'POST', > + description =3D> "Import a VM from existing disk images.", > + protected =3D> 1, > + proxyto =3D> 'node', > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> PVE::QemuServer::json_config_properties( > + { > + node =3D> get_standard_option('pve-node'), > + vmid =3D> get_standard_option('pve-vmid', { completion =3D> > + \&PVE::Cluster::complete_next_vmid }), > + diskimages =3D> { > + description =3D> "Mapping of devices to disk images. For " . > + "example, scsi0=3D/mnt/nfs/image1.vmdk,scsi1=3D/mnt/nfs/image2", > + type =3D> 'string', should probably get a proper format, and this can then be a=20 '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=20 can be imported.. (for CLI, the parameter can be passed multiple times=20 to defined the list IIRC). ping me if you can't find an example for how=20 to do this! > + }, > + start =3D> { > + optional =3D> 1, > + type =3D> 'boolean', > + default =3D> 0, > + description =3D> "Start VM after it was imported successfully.", > + }, > + }), > + }, > + returns =3D> { > + type =3D> 'string', > + }, > + code =3D> sub { > + my ($param) =3D @_; > + my $node =3D extract_param($param, 'node'); > + my $vmid =3D extract_param($param, 'vmid'); > + my $diskimages_string =3D extract_param($param, 'diskimages'); > + my $boot =3D extract_param($param, 'boot'); > + > + my $rpcenv =3D PVE::RPCEnvironment::get(); > + my $authuser =3D $rpcenv->get_user(); > + my $storecfg =3D PVE::Storage::config(); > + > + PVE::Cluster::check_cfs_quorum(); > + > + # Return true iff $opt is to be imported, that means it is a device=20 > + # 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 =3D sub { > + my ($opt) =3D @_; > + return 0 if $opt eq 'efidisk0'; > + if (PVE::QemuServer::Drive::is_valid_drivename($opt)) { > + my $drive =3D PVE::QemuServer::parse_drive($opt, $param->{$opt}); > + return $drive->{file} =3D~ m/0$/; and if I want to create 10GB volume? note that we have a NEW_DISK_RE at=20 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 attach= ed > + my $target_devices =3D []; > + # 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=20 lists? e.g., you can just remove the import params and put them in a new=20 hash, and keep $params for the rest? > + my $vm_params =3D []; > + 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 =3D> "Deprecated: Use --boot order=3D ins= tead"}) > + if $opt eq 'bootdisk'; > + > + my $list =3D $is_import->($opt) ? $target_devices : $vm_params; > + push @$list, $opt; > + } > + > + my $diskimages =3D {}; > + foreach (split ',', $diskimages_string) { > + my ($device, $diskimage) =3D split('=3D', $_); avoid using $_ for such things please. > + $diskimages->{$device} =3D $diskimage; > + if (defined $param->{$device}) { > + my $drive =3D PVE::QemuServer::parse_drive($device, $param->{$device})= ; > + $drive->{file} =3D~ m/(\d)$/; you define a helper above, but not use it here.. also, the check is=20 wrong here as well ;) if you split the $import_params from $params above,=20 then you can just check if $import_params->{$device} is defined, in=20 which case you know it has the special import syntax. > + if ($1 !=3D 0) { > + raise_param_exc({ > + $device =3D> "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 '..'=20 specified" > + } > + } else { > + # It is possible to also create new empty disk images during > + # import by adding something like scsi2=3Dlocal:10, therefore > + # vice-versa check is not required same message as for the other branch, no comment needed. in fact, this=20 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=20 messages. by splitting $params early, it then becomes die/... if !defined($import_params{$device}); > + raise_param_exc({ > + diskimages =3D> "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=3D/source/path,scsi1=3D/other/p= ath " . > + "there must be --scsi0 local-lvm:0,discard=3Don --scsi1 local:0,cache= =3Dunsafe", > + }); > + } > + # Dies if $diskimage cannot be found > + PVE::Storage::abs_filesystem_path($storecfg, $diskimage, 1); > + } > + foreach my $device (@$target_devices) { > + my $drive =3D PVE::QemuServer::parse_drive($device, $param->{$devic= e}); > + if ($drive->{file} =3D~ m/0$/) { > + if (!defined $diskimages->{$device}) { > + raise_param_exc({ > + $device =3D> "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=20 list, you can just map and grep here to compare $diskimages keys with=20 $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=20 or create below? also, we don't have such a check when creating a VM, do=20 we? just when updating. maybe it would make sense to unify the handling?=20 this is not related to import at all, so could also be split out for a=20 follow-up. > + if ($boot) { > + my $new_bootcfg =3D PVE::JSONSchema::parse_property_string('pve-qm-= boot', $boot); > + if ($new_bootcfg->{order}) { > + my @devs =3D PVE::Tools::split_list($new_bootcfg->{order}); > + for my $dev (@devs) { > + my $will_be_imported =3D grep (/^$dev$/, @$target_devices); > + my $will_be_created =3D grep (/^$dev$/, @$vm_params); > + if ( !($will_be_imported || $will_be_created)) { > + raise_param_exc({boot =3D> "$dev will be neither imported " . > + "nor created, so it cannot be a boot device!"}); > + } > + } > + } else { > + raise_param_exc({boot =3D> "Deprecated: Use --boot order=3D 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 =3D sub { > + my $get_conf =3D sub { reload_conf might be a better name > + my ($vmid) =3D @_; > + PVE::QemuConfig->lock_config($vmid, sub { why? you don't update the config here? an flock is needed for *writing*=20 the config (or *blocking writes* by others for a short period).. > + my $conf =3D 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 =3D { this is a misleading name - this contains all the disks that are not=20 imported, which can take a while (not as long as importing, but=20 still..). and it's not needed if we split the import_params out of=20 $params above anyway.. > + node =3D> $node, > + vmid =3D> $vmid, > + skiplock =3D> 1, > + }; > + foreach ( @$vm_params ) { > + $short_update->{$_} =3D $param->{$_}; > + } > + $update_vm_api->($short_update, 1); # writes directly in config fil= e yeah, so this does an flock internally, but to guarantee it still is the=20 right config you need to set the digest in $short_update.. same for=20 other calls to update_vm_api - you reload the config, check your config=20 lock is still there, then you have a valid digest to pass to=20 update_vm_api, which obtains an flock (=3D=3D no other writes possible),=20 checks the digest (=3D=3D no writes happened since our config reload before= =20 calling update_vm_api) and then does the config modification and write=20 before releasing the flock. of course, now the digest and our $conf is=20 invalid, so we need to reload (and now we have a digest again for the=20 next call ;)). > + > + my $conf =3D $get_conf->($vmid); > + > + # When all short updates were succesfull, then the long imports > + my @imported_successfully =3D (); > + eval { foreach my $device (@$target_devices) { > + my $param_parsed =3D PVE::QemuServer::parse_drive($device, $param->{$d= evice}); we already did that, so please reuse the result from earlier.. > + die "Parsing $param->{$device} failed" if !$param_parsed; > + my $storeid =3D PVE::Storage::parse_volume_id($param_parsed->{file}); > + > + my $imported =3D $import_disk_image->({ > + storecfg =3D> $storecfg, > + vmid =3D> $vmid, > + vmconf =3D> $conf, > + source =3D> $diskimages->{$device}, > + target =3D> { > + storeid =3D> $storeid, > + format =3D> $param_parsed->{format}, > + options =3D> $param->{$device}, and pass the parsed options here.. > + device =3D> $device, > + }, > + }); > + push @imported_successfully, $imported; > + }}; > + my $err =3D $@; > + if ($err) { > + foreach my $volid (@imported_successfully) { > + eval { PVE::Storage::vdisk_free($storecfg, $volid) }; > + warn $@ if $@; > + } > + > + eval { > + my $conffile =3D 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=20 allocated, and who knows what else.. > + }; > + warn $@ if $@; > + > + die "Import aborted: $err"; > + } > + if (!$boot) { > + $conf =3D $get_conf->($vmid); # import_disk_image changed config file = directly > + my $bootdevs =3D PVE::QemuServer::get_default_bootdevices($conf); > + $boot =3D PVE::QemuServer::print_bootorder($bootdevs); > + } > + $update_vm_api->( > + { > + node =3D> $node, > + vmid =3D> $vmid, > + boot =3D> $boot, > + skiplock =3D> 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=3DAgent.pm CPU.pm Machine.pm > +SOURCES=3DAgent.pm CPU.pm Machine.pm OVF.pm > =20 > .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 =3D> 'index', > + path =3D> '', > + method =3D> 'GET', > + proxyto =3D> 'node', > + description =3D> "Read an .ovf manifest.", > + parameters =3D> { > + additionalProperties =3D> 0, > + properties =3D> { > + node =3D> get_standard_option('pve-node'), > + manifest =3D> { > + description =3D> ".ovf manifest", > + type =3D> 'string', > + }, > + }, > + }, > + returns =3D> { > + description =3D> "VM config according to .ovf manifest and digest of ma= nifest", > + type =3D> "object", > + }, > + returns =3D> { > + type =3D> 'object', > + additionalProperties =3D> 1, > + properties =3D> PVE::QemuServer::json_ovf_properties({ > + name =3D> { > + type =3D> 'string', > + optional =3D> 1, > + }, > + cores =3D> { > + type =3D> 'integer', > + optional =3D> 1, > + }, > + memory =3D> { > + type =3D> 'integer', > + optional =3D> 1, > + }, > + }), > + }, > + code =3D> sub { > + my ($param) =3D @_; > + > + my $manifest =3D $param->{manifest}; > + die "$manifest: non-existent or non-regular file\n" if (! -f $manifest)= ; > + > + my $parsed =3D PVE::QemuServer::OVF::parse_ovf($manifest, 0, 1); > + my $result; > + $result->{cores} =3D $parsed->{qm}->{cores}; > + $result->{name} =3D $parsed->{qm}->{name}; > + $result->{memory} =3D $parsed->{qm}->{memory}; > + my $disks =3D $parsed->{disks}; > + foreach my $disk (@$disks) { > + $result->{$disk->{disk_address}} =3D $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 =3D { > optional =3D> 1, > type =3D> 'string', > description =3D> "Lock/unlock the VM.", > - enum =3D> [qw(backup clone create migrate rollback snapshot snapshot-de= lete suspending suspended)], > + enum =3D> [qw(backup clone create migrate rollback snapshot snapshot-de= lete suspending suspended import)], can be fine - not 100% convinced yet that we need to treat this as=20 separate from 'create' ;) > }, > cpulimit =3D> { > optional =3D> 1, > @@ -998,6 +998,18 @@ sub verify_volume_id_or_qm_path { > return $volid; > } > =20 > +PVE::JSONSchema::register_format('pve-volume-id-or-absolute-path', \&ver= ify_volume_id_or_absolute_path); > +sub verify_volume_id_or_absolute_path { > + my ($volid, $noerr) =3D @_; > + > + # Exactly these 2 are allowed in id_or_qm_path but should not be all= owed here > + if ($volid eq 'none' || $volid eq 'cdrom') { then maybe turn it around? it makes more sense to have a base format and=20 then an extension IMHO. else now someone has to keep in mind that there=20 is a filter here when updating the other format, which is easily=20 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 =3D { > host =3D> { > default_key =3D> 1, > @@ -2030,6 +2042,22 @@ sub json_config_properties { > return $prop; > } > =20 > +# Properties that we can read from an OVF file > +sub json_ovf_properties { > + my $prop =3D shift; > + > + foreach my $device ( PVE::QemuServer::Drive::valid_drive_names()) { > + $prop->{$device} =3D { > + type =3D> 'string', > + format =3D> 'pve-volume-id-or-qm-path', wrong format? > + description =3D> "Disk image that gets imported to $device", > + optional =3D> 1, > + }; > + } > + > + return $prop; > +} > + > # return copy of $confdesc_cloudinit to generate documentation > sub cloudinit_config_properties { > =20 > @@ -6722,7 +6750,7 @@ sub qemu_img_convert { > $src_path =3D PVE::Storage::path($storecfg, $src_volid, $snapname); > $src_is_iscsi =3D ($src_path =3D~ m|^iscsi://|); > $cachemode =3D 'none' if $src_scfg->{type} eq 'zfspool'; > - } elsif (-f $src_volid) { > + } elsif (-f $src_volid || -b _) { # -b required to import from LVM i= mages -b $src_volid the comment is not needed (and wrong) - '-b' already states that we=20 check for block devices, and this is not LVM specific at all, it also=20 handles direct references to zvols, mapped rbd images, physical disks,=20 loopback devices, or, well, any other block device ;) > $src_path =3D $src_volid; > if ($src_path =3D~ m/\.($PVE::QemuServer::Drive::QEMU_FORMAT_RE)$/) { > $src_format =3D $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 { > =20 > # returns two references, $qm which holds qm.conf style key/values, and = \@disks > sub parse_ovf { > - my ($ovf, $debug) =3D @_; > + my ($ovf, $debug, $ignore_size) =3D @_; maybe something like $manifest_only - we might want to skip other=20 futures stuff as well, not just reading the file size. > =20 > my $dom =3D XML::LibXML->load_xml(location =3D> $ovf, no_blanks =3D>= 1); > =20 > @@ -220,9 +220,11 @@ ovf:Item[rasd:InstanceID=3D'%s']/rasd:ResourceType",= $controller_id); > die "error parsing $filepath, file seems not to exist at $backing_f= ile_path\n"; > } doesn't this already fail? > =20 > - my $virtual_size; > - if ( !($virtual_size =3D PVE::Storage::file_size_info($backing_file_pat= h)) ) { > - die "error parsing $backing_file_path, size seems to be $virtual_si= ze\n"; > + my $virtual_size =3D 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 =3D PVE::Storage::file_size_info($backing_file= _path)) ) { > + die "error parsing $backing_file_path: Could not get file size info: $= @\n"; > + } > } > =20 > $pve_disk =3D { > --=20 > 2.20.1 >=20 >=20 > _______________________________________________ > pve-devel mailing list > pve-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel >=20 =