Re: [pve-devel] [PATCH v4 qemu-server] Add API for disk & VM import

["Dominic Jäger" <d.jaeger@proxmox.com>]

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