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

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

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?

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

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

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

> 
> > +	    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?
> 
> > +	    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?
> > +	    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'));
> 
> > +	$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...

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

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