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

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On February 11, 2021 11:32 am, Dominic Jäger wrote:
> Thank you for looking so carefully!
> 
> On Wed, Feb 10, 2021 at 10:40:56AM +0100, Fabian Grünbichler wrote:
>> 
>> On February 5, 2021 11:04 am, Dominic Jäger wrote:
>> > Extend qm importdisk/importovf functionality to the API.
>> >  
>> > @@ -4325,4 +4324,378 @@ __PACKAGE__->register_method({
>> >  	return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type});
>> >      }});
>> >  
>> > +# Raise exception if $format is not supported by $storageid
>> > +my $check_format_is_supported = sub {
>> > +    my ($format, $storageid) = @_;
>> > +
>> > +    return if !$format;
>> > +
>> > +    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.

> 
>> 
>> > +    }
>> > +};
>> > +
>> > +# 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..

>> 
>> > +
>> > +# vmid ... target VM ID
>> > +# source ... absolute path of the source image (volid must be converted before)
>> 
>> that restriction is not actually needed, see below
>> 
>> > +# 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-100-disk-2)
>> > +my $import_disk_image = sub {
>> > +    my ($param) = @_;
>> > +    my $vmid = $param->{vmid};
>> > +    my $requested_format = $param->{format};
>> > +    my $storage = $param->{storage};
>> > +    my $source = $param->{source};
>> > +
>> > +    my $vm_conf = PVE::QemuConfig->load_config($vmid);
>> 
>> could be passed in, the call sites already have it.. and it might allow 
>> to pull in some of the surrounding codes at the call sites that is 
>> similar (config updating, property string building) into this helper
> 
> It's actually unused. I'll remove it.
>> 
>> > +    my $store_conf = PVE::Storage::config();
>> 
>> could also be passed in here
> * see below
>> 
>> > +    if (!$source) {
>> > +	die "It is necessary to pass the source parameter";
>> > +    }
>> 
>> just a check for definedness
> Hmm but if someone does
> $import_disk_image->({vmid => 102, source => "", storage => "local-lvm"});
> 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

> 
>> 
>> > +    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.

> 
>> 
>> > +
>> > +__PACKAGE__->register_method ({
>> > +    name => 'importdisk',
>> > +    path => '{vmid}/importdisk',
>> > +    method => 'POST',
>> > +    protected => 1, # for worker upid file
>> 
>> huh? no - because we need elevated privileges to allocate disks on 
>> storages..
> Might be left over, I'll check.
>> 
>> > +    proxyto => 'node',
>> > +    description => "Import an external disk image into a VM. The image format ".
>> > +	"has to be supported by qemu-img.",
>> > +    parameters => {
>> > +	additionalProperties => 0,
>> > +	properties => {
>> > +	    node => get_standard_option('pve-node'),
>> > +	    vmid => get_standard_option('pve-vmid',
>> > +		{completion => \&PVE::QemuServer::complete_vmid}),
>> > +	    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.

> 
>> 
>> > +	    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 ;)

>> 
>> > +	    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?

>> > +	    if ($format_explicit && $format_device_option) {
>> > +		raise_param_exc({format => "Disk format may be specified only once!"});
>> 
>> format already specified in device_options?
> Also good for me.
> 
>> 
>> > +	    }
>> > +	}
>> > +	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
}

>> 
>> > +	$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.

> 
>> > +	    }
>> 
>> > +
>> > +	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"

> 
>> 
>> > +	}
>> > +
>> > +	my $worker = sub {
>> > +	    eval { PVE::QemuConfig->create_and_lock_config($vmid, 0, 'import') };
>> > +	    die "Unable to create config for VM import: $@" if $@;
>> 
>> should happen outside the worker (so that a VMID clash is detected 
>> early). inside the worker you just lock and load, then check that the 
>> 'import' lock is still there..
>> 
>> 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
> I thought about that, too. But I didn't take future permission issues into account. And then having a single loop and no filters seemed like the easier-to-read option to me.
> 
> I'll change it.
>> 
>> > +		next if ($opt eq 'start');
>> > +
>> > +		my $updated_value;
>> > +		if ($use_import->($opt)) {
>> > +		    # $opt is bus/device like ide0, scsi5
>> > +
>> > +		    my $device = PVE::QemuServer::parse_drive($opt, $param->{$opt});
>> 
>> $drive? $device is used in this very patch for something else ;)
> Right.
> 
> 
> "OK, will fix" to everything that I haven't mentioned. I certainly read it.