From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.gruenbichler@proxmox.com>
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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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 <pve-devel@lists.proxmox.com>; 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?= <f.gruenbichler@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
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 <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=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 <d.jaeger@proxmox.com>
>=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 <st=
oreid>: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
=