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