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
