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 18F33694EA for ; Fri, 12 Feb 2021 12:13:35 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0416B248D6 for ; Fri, 12 Feb 2021 12:13:05 +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 2C401248CB for ; Fri, 12 Feb 2021 12:13:03 +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 9D54041EF6 for ; Fri, 12 Feb 2021 12:13:02 +0100 (CET) Date: Fri, 12 Feb 2021 12:13:00 +0100 From: Dominic =?iso-8859-1?Q?J=E4ger?= To: Fabian =?iso-8859-1?Q?Gr=FCnbichler?= Cc: Proxmox VE development discussion Message-ID: <20210212111300.GA40406@mala> References: <1613119192.onm1x6fg5c.astroid@nora.none> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1613119192.onm1x6fg5c.astroid@nora.none> User-Agent: Mutt/1.10.1 (2018-07-13) X-SPAM-LEVEL: Spam detection results: 0 AWL 1.669 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 11:13:35 -0000 On Fri, Feb 12, 2021 at 10:03:50AM +0100, Fabian Grünbichler wrote: > >> > + my $store_conf = PVE::Storage::config(); > >> > >> it probably makes sense to pass this in as parameter, all call sites > >> probably have it already ;) > > > > 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 times > > instead of passing it as parameter? > > well, it should be the same and fairly cheap since it comes from a cache > after the first request, but if you trigger a cfs_update() in-between > you might get two different versions. Makes sense. Fixed it. > >> > +# paths are returned as is > >> > +# volids are returned as paths > >> > +# > >> > +# Also checks if $original actually exists > >> > +my $convert_to_path = sub { > >> > + my ($original) = @_; > >> > + my $volid_as_path = eval { # Nonempty iff $original_source is a volid > >> > + PVE::Storage::path(PVE::Storage::config(), $original); > >> > + }; > >> > + my $result = $volid_as_path || $original ; > >> > + if (!-e $result) { > >> > + die "Could not import because source '$original' does not exist!"; > >> > + } > >> > + return $result; > >> > +}; > >> > >> what does this do that PVE::Storage::abs_filesystem_path doesn't already > >> do (except having 'import' in the error message ;))? > > > > Haven't thought about that function in the last time... > > I'll have a look at it. > > > >> ah, further down below I see that -b might be missing for raw block > >> devices.. maybe it makes sense to allow those in that helper as well? > >> optionally? call-sites would need to be checked.. > > Would it make sense to not change helper for the moment, certainly not break > > other call-sites and refactor later in a separate patch? > > optionally supporting block devices should not be able to break any > existing call sites ;) but yeah, you can also replace your helper here > with a fork of abs_filesystem_path that allows -b, that would allow > simply switching it out if we want to handle that in pve-storage.. > > >> Makes sense, too. I added another parameter to abs_filesystem_path. > > then > >> > >> > + 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 > at this point Thank you. It became clear when I changed all the signatures... > > > > >> > >> > + if (!$storage) { > >> > + die "It is necessary to pass the storage parameter"; > >> > + } > >> > >> call PVE::Storage::storage_config() here to get both that and the check > >> that the storage exists.. > > Do you mean $store_conf->{ids}->{$storage} ? > > no, I mean storage_config() ;) > > it checks whether $storage is defined & non-empty, is an actually > defined-in-storage.cfg storage, and returns that storage's config > section. Sorry, now I see it. Reading is hard... > >> > + source => { > >> > + description => "Disk image to import. Can be a volid ". > >> > + "(local-lvm:vm-104-disk-0), an image on a PVE storage ". > >> > + "(local:104/toImport.raw) or (for root only) an absolute ". > >> > >> how does the second work here? and the whole thing is root-only, so that > >> qualifier at the end is redundant ;) > > > > 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 > on.. so it's important to think about where to use `parse_volid` vs > `parse_volname` (the latter gives you all the checks per storage type, > and returns type of volume, associated VMID, etc.). > > for this series/patch, we can accept a proper vdisk volid if the user > has access, or any volid/path for root. we can't allow arbitrary volids > for unprivileged users, as that would allow reading ANY file on the > storage. OK, will have to give that a closer look. > >> > + format => { > >> > + type => 'string', > >> > + description => 'Target format.', > >> > + enum => [ 'raw', 'qcow2', 'vmdk' ], > >> > + optional => 1, > >> > + }, > >> > >> not directly related to this series, but this enum is copied all over > >> the place and might be defined in one place as standard option > >> (qm-new-disk-format ?) > > 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 > disks. of course, they are also part of the schema as part of property > strings. also dir-disk-format would be wrong - just because > non-dir-storages only support raw(/subvol) doesn't make the parameter > invalid for them ;) Right :D > > >> > >> > + digest => get_standard_option('pve-config-digest'), > >> > + }, > >> > + }, > >> > + returns => { type => 'string'}, > >> > + code => sub { > >> > + my ($param) = @_; > >> > + my $vmid = extract_param($param, 'vmid'); > >> > + my $node = extract_param($param, 'node'); > >> > + my $original_source = extract_param($param, 'source'); > >> > >> not sure why this gets this name, it's just passed once and not used > >> afterwards.. > > It was useful previously... Changed it now. > >> > >> > + my $digest = extract_param($param, 'digest'); > >> > + my $device_options = extract_param($param, 'device_options'); > >> > >> IMHO this one needs to be parsed - e.g., by adding a fake volume with > >> the syntax used in importvm at the front and parsing it according to the > >> disk schema.. > >> > >> both to catch bogus stuff early, and to make the handling below more > >> robust w.r.t. future changes.. > > > > 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 > across bus / device types? you could have a 'everything optional > superset of all disk device types' schema, that would be less strict > than the final check but better than nothing? looking at > PVE::QemuServer::Drive it almost exists in $alldrive_fmt, so cloning > that and removing the file property could work? I'll give it a try. Having some checks sounds better than none at all to me, too. > >> > + } > >> > + my $format = $format_explicit || $format_device_option; > >> > >> since both of those are not needed after this point, this could just be > >> returned / set by the above if, dropping the extra variables in the > >> outer scope.. > > Do you mean with a small function? > > my $get_format = sub { > > ... > > return $explicit || $device_options > > } > > my $format = $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 > } OK. My if-solution so far looks ugly to me, but it avoids those extra variables in outer scope. > >> > + $check_format_is_supported->($format, $storeid); > >> > + > >> > + my $locked = sub { > >> > + my $conf = PVE::QemuConfig->load_config($vmid); > >> > + PVE::Tools::assert_if_modified($conf->{digest}, $digest); > >> > + > >> > + if ($device && $conf->{$device}) { > >> > + die "Could not import because device $device is already in ". > >> > + "use in VM $vmid. Choose a different device!"; > >> > >> both of these checks might make sense outside of the worker already to > >> give immediate feedback in the API response.. > > 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 changed? > > 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? > > > > 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 > changes in-between? > > then you only need to duplicate the digest check. I like. Done. > > > > >> > + } > >> > >> > + > >> > + my $msg = "There must be exactly as many devices specified as there " . > >> > + " are devices in the diskimage parameter.\n For example for " . > >> > + "--scsi0 local-lvm:0,discard=on --scsi1 local:0,cache=unsafe " . > >> > + "there must be --diskimages scsi0=/source/path,scsi1=/other/path"; > >> > + my $device_count = grep { $use_import->($_) } keys %$param; > >> > >> why though? isn't it sufficient to say there must be a matching device for > >> each diskimages entry and vice-versa? > > 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 > mapping: > > "scsi0 configured for disk import, but no matching import source given" > "virtio0 not configured for disk import, but import source '...' given" That might work together well with the filter-out-disk-parameters idea > >> we could also filter out disk parameters, and call update_vm_api with > >> the rest here (those should be fast, but potentially in the future could > >> run into permission issues that would be nice to return early before > >> doing a massive disk conversion that has to be undone afterwards.. > >> > >> > + > >> > + my @volids_of_imported = (); > >> > + eval { foreach my $opt (keys %$param) { > >> > >> this could then just iterate over the disk parameters