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.
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
