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) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 5A64B69455 for ; Fri, 12 Feb 2021 10:04:31 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 489DC232DD for ; Fri, 12 Feb 2021 10:04:01 +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 201D3232D2 for ; Fri, 12 Feb 2021 10:04:00 +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 E2CAA41B47 for ; Fri, 12 Feb 2021 10:03:59 +0100 (CET) Date: Fri, 12 Feb 2021 10:03:50 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Dominic =?iso-8859-1?b?SuRnZXI=?= , Proxmox VE development discussion References: <20210205100442.28163-1-d.jaeger@proxmox.com> <1612945336.hphk4jq6ac.astroid@nora.none> <20210211103223.GA34013@mala> In-Reply-To: <20210211103223.GA34013@mala> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1613119192.onm1x6fg5c.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 v4 qemu-server] Add API for disk & VM import 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: Fri, 12 Feb 2021 09:04:31 -0000 On February 11, 2021 11:32 am, Dominic J=C3=A4ger wrote: > Thank you for looking so carefully! >=20 > On Wed, Feb 10, 2021 at 10:40:56AM +0100, Fabian Gr=C3=BCnbichler wrote: >>=20 >> On February 5, 2021 11:04 am, Dominic J=C3=A4ger wrote: >> > Extend qm importdisk/importovf functionality to the API. >> > =20 >> > @@ -4325,4 +4324,378 @@ __PACKAGE__->register_method({ >> > return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $par= am->{vmid}, $param->{type}); >> > }}); >> > =20 >> > +# Raise exception if $format is not supported by $storageid >> > +my $check_format_is_supported =3D sub { >> > + my ($format, $storageid) =3D @_; >> > + >> > + return if !$format; >> > + >> > + my $store_conf =3D PVE::Storage::config(); >>=20 >> it probably makes sense to pass this in as parameter, all call sites=20 >> probably have it already ;) >=20 > I just noticed that I have it in importdisk, but unused. Does it make a > relevant difference in terms of performance to call ::config() multiple t= imes > instead of passing it as parameter? well, it should be the same and fairly cheap since it comes from a cache=20 after the first request, but if you trigger a cfs_update() in-between=20 you might get two different versions. >=20 >>=20 >> > + } >> > +}; >> > + >> > +# paths are returned as is >> > +# volids are returned as paths >> > +# >> > +# Also checks if $original actually exists >> > +my $convert_to_path =3D sub { >> > + my ($original) =3D @_; >> > + my $volid_as_path =3D eval { # Nonempty iff $original_source is a vo= lid >> > + PVE::Storage::path(PVE::Storage::config(), $original); >> > + }; >> > + my $result =3D $volid_as_path || $original ; >> > + if (!-e $result) { >> > + die "Could not import because source '$original' does not exist!= "; >> > + } >> > + return $result; >> > +}; >>=20 >> what does this do that PVE::Storage::abs_filesystem_path doesn't already= =20 >> do (except having 'import' in the error message ;))? >=20 > Haven't thought about that function in the last time...=20 > I'll have a look at it. >=20 >> ah, further down below I see that -b might be missing for raw block=20 >> devices.. maybe it makes sense to allow those in that helper as well?=20 >> optionally? call-sites would need to be checked.. > Would it make sense to not change helper for the moment, certainly not b= reak > other call-sites and refactor later in a separate patch? optionally supporting block devices should not be able to break any=20 existing call sites ;) but yeah, you can also replace your helper here=20 with a fork of abs_filesystem_path that allows -b, that would allow=20 simply switching it out if we want to handle that in pve-storage.. >>=20 >> > + >> > +# vmid ... target VM ID >> > +# source ... absolute path of the source image (volid must be convert= ed before) >>=20 >> that restriction is not actually needed, see below >>=20 >> > +# 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-10= 0-disk-2) >> > +my $import_disk_image =3D sub { >> > + my ($param) =3D @_; >> > + my $vmid =3D $param->{vmid}; >> > + my $requested_format =3D $param->{format}; >> > + my $storage =3D $param->{storage}; >> > + my $source =3D $param->{source}; >> > + >> > + my $vm_conf =3D PVE::QemuConfig->load_config($vmid); >>=20 >> could be passed in, the call sites already have it.. and it might allow=20 >> to pull in some of the surrounding codes at the call sites that is=20 >> similar (config updating, property string building) into this helper >=20 > It's actually unused. I'll remove it. >>=20 >> > + my $store_conf =3D PVE::Storage::config(); >>=20 >> could also be passed in here > * see below >>=20 >> > + if (!$source) { >> > + die "It is necessary to pass the source parameter"; >> > + } >>=20 >> just a check for definedness > Hmm but if someone does > $import_disk_image->({vmid =3D> 102, source =3D> "", storage =3D> "local-= lvm"}); > then >>=20 >> > + if ($source !~ m!^/!) { >> > + die "source must be an absolute path but is $source"; > will only say "... path but is at /usr/share/..."? no, because that check gets removed as well.. $source could be a volid=20 at this point >=20 >>=20 >> > + if (!$storage) { >> > + die "It is necessary to pass the storage parameter"; >> > + } >>=20 >> call PVE::Storage::storage_config() here to get both that and the check=20 >> that the storage exists.. > Do you mean $store_conf->{ids}->{$storage} ? no, I mean storage_config() ;) it checks whether $storage is defined & non-empty, is an actually=20 defined-in-storage.cfg storage, and returns that storage's config=20 section. >=20 >>=20 >> > + >> > +__PACKAGE__->register_method ({ >> > + name =3D> 'importdisk', >> > + path =3D> '{vmid}/importdisk', >> > + method =3D> 'POST', >> > + protected =3D> 1, # for worker upid file >>=20 >> huh? no - because we need elevated privileges to allocate disks on=20 >> storages.. > Might be left over, I'll check. >>=20 >> > + proxyto =3D> 'node', >> > + description =3D> "Import an external disk image into a VM. The im= age 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-lvm:vm-104-disk-0), an image on a PVE storage ". >> > + "(local:104/toImport.raw) or (for root only) an absolute ". >>=20 >> how does the second work here? and the whole thing is root-only, so that= =20 >> qualifier at the end is redundant ;) >=20 > The description is bad... I guess local:104/toImport.raw is a volid, too? > I could just image it to be useful here to allow special volids like > --source local:99/somedisk.qcow2 it's a volid, but not a valid VM-owned volume that we can do ACL checks=20 on.. so it's important to think about where to use `parse_volid` vs=20 `parse_volname` (the latter gives you all the checks per storage type,=20 and returns type of volume, associated VMID, etc.). for this series/patch, we can accept a proper vdisk volid if the user=20 has access, or any volid/path for root. we can't allow arbitrary volids=20 for unprivileged users, as that would allow reading ANY file on the=20 storage. >=20 >>=20 >> > + format =3D> { >> > + type =3D> 'string', >> > + description =3D> 'Target format.', >> > + enum =3D> [ 'raw', 'qcow2', 'vmdk' ], >> > + optional =3D> 1, >> > + }, >>=20 >> not directly related to this series, but this enum is copied all over=20 >> the place and might be defined in one place as standard option=20 >> (qm-new-disk-format ?) > Makes sense to me. Only >> (qm-new-disk-format ?) > raw, qcow2 and vmdk are not only for new disks? Those are allowed formats= for every directory storage, so maybe something like dir-disk-format? as separate parameter they are only used when creating/importing/.. NEW=20 disks. of course, they are also part of the schema as part of property=20 strings. also dir-disk-format would be wrong - just because=20 non-dir-storages only support raw(/subvol) doesn't make the parameter=20 invalid for them ;) >>=20 >> > + 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 $original_source =3D extract_param($param, 'source'); >>=20 >> not sure why this gets this name, it's just passed once and not used=20 >> afterwards.. > It was useful previously... Changed it now. >>=20 >> > + my $digest =3D extract_param($param, 'digest'); >> > + my $device_options =3D extract_param($param, 'device_options'); >>=20 >> IMHO this one needs to be parsed - e.g., by adding a fake volume with=20 >> the syntax used in importvm at the front and parsing it according to the= =20 >> disk schema.. >>=20 >> both to catch bogus stuff early, and to make the handling below more=20 >> robust w.r.t. future changes.. >=20 > OK. The API currently allows any string. Would it be worth to change that= then? not sure whether that's trivially possible - the options are not uniform=20 across bus / device types? you could have a 'everything optional=20 superset of all disk device types' schema, that would be less strict=20 than the final check but better than nothing? looking at=20 PVE::QemuServer::Drive it almost exists in $alldrive_fmt, so cloning=20 that and removing the file property could work? >> > + if ($format_explicit && $format_device_option) { >> > + raise_param_exc({format =3D> "Disk format may be specified only onc= e!"}); >>=20 >> format already specified in device_options? > Also good for me. >=20 >>=20 >> > + } >> > + } >> > + my $format =3D $format_explicit || $format_device_option; >>=20 >> since both of those are not needed after this point, this could just be=20 >> returned / set by the above if, dropping the extra variables in the=20 >> outer scope.. > Do you mean with a small function? > my $get_format =3D sub { > ... > return $explicit || $device_options > } > my $format =3D $get_format($device_options, extract_param($param, 'format= ')); I think a function is overkill for a single use, just my $format; if (...) { // decide and set $format } >>=20 >> > + $check_format_is_supported->($format, $storeid); >> > + >> > + my $locked =3D sub { >> > + my $conf =3D 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!"; >>=20 >> both of these checks might make sense outside of the worker already to=20 >> give immediate feedback in the API response.. > I wanted to do this but didn't know what to do with the lock. > If I check first and lock later then the config could already have change= d? > Or check twice? > 1. outside the worker, because most times it is OK and gives a nice error= and > 2. inside the worker, to be really sure? >=20 > Or lock outside the worker? I'll have to try what is actually possible... check before, compare digest after fork + lock + reload should handle any=20 changes in-between? then you only need to duplicate the digest check. >=20 >> > + } >>=20 >> > + >> > + my $msg =3D "There must be exactly as many devices specified as ther= e " . >> > + " are devices in the diskimage parameter.\n For example for " . >> > + "--scsi0 local-lvm:0,discard=3Don --scsi1 local:0,cache=3Dunsafe= " . >> > + "there must be --diskimages scsi0=3D/source/path,scsi1=3D/other/= path"; >> > + my $device_count =3D grep { $use_import->($_) } keys %$param; >>=20 >> why though? isn't it sufficient to say there must be a matching device f= or=20 >> each diskimages entry and vice-versa? > That's what I meant, but your formulation is better. it also allows you to give concrete error messages if you do one-to-one=20 mapping: "scsi0 configured for disk import, but no matching import source given" "virtio0 not configured for disk import, but import source '...' given" >=20 >>=20 >> > + } >> > + >> > + my $worker =3D sub { >> > + eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, 'import= ') }; >> > + die "Unable to create config for VM import: $@" if $@; >>=20 >> should happen outside the worker (so that a VMID clash is detected=20 >> early). inside the worker you just lock and load, then check that the=20 >> 'import' lock is still there.. >>=20 >> we could also filter out disk parameters, and call update_vm_api with=20 >> the rest here (those should be fast, but potentially in the future could= =20 >> run into permission issues that would be nice to return early before=20 >> doing a massive disk conversion that has to be undone afterwards.. >>=20 >> > + >> > + my @volids_of_imported =3D (); >> > + eval { foreach my $opt (keys %$param) { >>=20 >> this could then just iterate over the disk parameters > I thought about that, too. But I didn't take future permission issues int= o account. And then having a single loop and no filters seemed like the eas= ier-to-read option to me. >=20 > I'll change it. >>=20 >> > + next if ($opt eq 'start'); >> > + >> > + my $updated_value; >> > + if ($use_import->($opt)) { >> > + # $opt is bus/device like ide0, scsi5 >> > + >> > + my $device =3D PVE::QemuServer::parse_drive($opt, $param->{$opt= }); >>=20 >> $drive? $device is used in this very patch for something else ;) > Right. >=20 >=20 > "OK, will fix" to everything that I haven't mentioned. I certainly read i= t. =