From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 87BEE69019 for ; Thu, 11 Feb 2021 11:32:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 7DC871A7A8 for ; Thu, 11 Feb 2021 11:32:27 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 8036F1A79E for ; Thu, 11 Feb 2021 11:32:26 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 452724623A for ; Thu, 11 Feb 2021 11:32:26 +0100 (CET) Date: Thu, 11 Feb 2021 11:32:23 +0100 From: Dominic =?iso-8859-1?Q?J=E4ger?= To: Proxmox VE development discussion Message-ID: <20210211103223.GA34013@mala> References: <20210205100442.28163-1-d.jaeger@proxmox.com> <1612945336.hphk4jq6ac.astroid@nora.none> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1612945336.hphk4jq6ac.astroid@nora.none> User-Agent: Mutt/1.10.1 (2018-07-13) X-SPAM-LEVEL: Spam detection results: 0 AWL 1.684 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH v4 qemu-server] Add API for disk & VM import X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Feb 2021 10:32:27 -0000 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.