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 590F1670D9 for ; Wed, 29 Jul 2020 15:00:44 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 4E252EC1D for ; Wed, 29 Jul 2020 15:00:44 +0200 (CEST) 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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id F0AA0EC12 for ; Wed, 29 Jul 2020 15:00:42 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id B95DC433D1 for ; Wed, 29 Jul 2020 15:00:42 +0200 (CEST) To: Proxmox VE development discussion , =?UTF-8?Q?Dominic_J=c3=a4ger?= References: <20200707100412.81731-1-d.jaeger@proxmox.com> <20200707100412.81731-4-d.jaeger@proxmox.com> From: Aaron Lauterer Message-ID: <863926e1-a312-93fb-9183-1c753667af34@proxmox.com> Date: Wed, 29 Jul 2020 15:00:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.0.1 MIME-Version: 1.0 In-Reply-To: <20200707100412.81731-4-d.jaeger@proxmox.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.485 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.951 Looks like a legit reply (A) 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [importdisk.pm, qemu.pm, qm.pm, drive.pm] Subject: Re: [pve-devel] [PATCH qemu-server 3/3] Move importdisk from qm to API 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: Wed, 29 Jul 2020 13:00:44 -0000 Some small nits inline, mainly regarding the wording of descriptions and messages. One thing I noticed is with multi line strings. The concat `.` should probably be at the end of the previous line if you split it up but I would prefer to have long lines instead of it being split. Long lines make it easier to search for a certain (log) message in the code with grep. On 7/7/20 12:04 PM, Dominic Jäger wrote: > Additionally, add parameters that enable directly (avoiding the intermediate > step as unused disk) attaching the disk to a bus/device with all known disk > options. > > Required to create a GUI for importdisk. > > Signed-off-by: Dominic Jäger > --- > PVE/API2/Qemu.pm | 113 ++++++++++++++++++++++++++++++++++- > PVE/CLI/qm.pm | 57 +----------------- > PVE/QemuServer/Drive.pm | 14 +++++ > PVE/QemuServer/ImportDisk.pm | 2 +- > 4 files changed, 127 insertions(+), 59 deletions(-) > > diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm > index b33359d..6efbbe3 100644 > --- a/PVE/API2/Qemu.pm > +++ b/PVE/API2/Qemu.pm > @@ -24,6 +24,7 @@ use PVE::QemuServer; > use PVE::QemuServer::Drive; > use PVE::QemuServer::CPUConfig; > use PVE::QemuServer::Monitor qw(mon_cmd); > +use PVE::QemuServer::ImportDisk qw(do_import); > use PVE::QemuMigrate; > use PVE::RPCEnvironment; > use PVE::AccessControl; > @@ -45,8 +46,6 @@ BEGIN { > } > } > > -use Data::Dumper; # fixme: remove > - > use base qw(PVE::RESTHandler); > > my $opt_force_description = "Force physical removal. Without this, we simple remove the disk from the config file and create an additional configuration entry called 'unused[n]', which contains the volume ID. Unlink of unused[n] always cause physical removal."; > @@ -4252,4 +4251,114 @@ __PACKAGE__->register_method({ > return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type}); > }}); > > +__PACKAGE__->register_method ({ > + name => 'importdisk', > + path => '{vmid}/importdisk', > + method => 'POST', > + protected => 1, # for worker upid file > + proxyto => 'node', > + description => "Import an external disk image into a VM. " > + ."The image format has to be supported by qemu-img.", > + permissions => { > + check => [ 'and', > + [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']], > + [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']], > + [ 'perm', '/vms/{vmid}', ['VM.Allocate']], > + ], > + }, > + parameters => { > + additionalProperties => 0, > + properties => { > + node => get_standard_option('pve-node'), > + vmid => get_standard_option('pve-vmid', > + {completion => \&PVE::QemuServer::complete_vmid}), > + source => { > + description => 'Path of the disk image that should be imported', What about: "Path to the disk image to import". > + type => 'string', > + }, > + device => { > + type => 'string', > + description => "What device the imported image should be " "Bus/Device type of the new disk" > + ."(e.g. 'ide0', 'scsi2'). Will add the image as unused disk " > + ."if omitted.", > + enum => [PVE::QemuServer::Drive::valid_drive_names()], > + optional => 1, > + }, > + device_options => { > + type => 'string', > + format => 'drive_options', > + description => "What options to set for the device " "Options to set for the new disk" > + ."(e.g. 'discard=on,backup=0')", > + optional => 1, > + }, > + storage => get_standard_option('pve-storage-id', { > + description => 'Which storage to use for the imported image.', "The storage to which the image will be imported to." > + completion => \&PVE::QemuServer::complete_storage, > + }), > + format => { > + type => 'string', > + description => 'Target format.', > + enum => [ 'raw', 'qcow2', 'vmdk' ], > + optional => 1, > + }, > + digest => get_standard_option('pve-config-digest'), > + }, > + }, > + returns => { type => 'string'}, > + code => sub { > + my ($params) = @_; > + my $rpcenv = PVE::RPCEnvironment::get(); > + my $authuser = $rpcenv->get_user(); > + > + my $vmid = extract_param($params, 'vmid'); > + my $source = extract_param($params, 'source'); > + my $digest = extract_param($params, 'digest'); > + my $device_options = extract_param($params, 'device_options'); > + my $device = extract_param($params, 'device'); > + my $storage = extract_param($params, 'storage'); > + my $vm_conf = PVE::QemuConfig->load_config($vmid); > + > + die "Could not import because source parameter is an empty string\n" > + if ($source eq ""); > + die "Could not import because source '$source' is not an absolute path\n" > + if $source !~ m!/!; > + die "Could not import because source '$source' does not exist" > + if !-e $source; > + die "VM $vmid config checksum missmatch (file change by other user?)\n" > + if $digest && $digest ne $vm_conf->{digest}; > + die "Could not import because device $device is already in use in " > + ."VM $vmid. Choose a different device!\n" > + if $device && $vm_conf->{$device}; > + > + my $worker = sub { > + my $message = $device ? "to $device on" : 'as unused disk to'; wouldn't it make more sense to phrase it "to $device for" and 'as unused disk for'? It will result in something like "importing disk 'myfile.qcow' as unused disk for VM XXX" > + print "Importing disk '$source' $message VM $vmid ...\n"; > + my ($unused_disk, $volid) = eval { > + PVE::QemuServer::ImportDisk::do_import( > + $source, $vmid, $storage, > + {format => extract_param($params, 'format')}, > + ) > + }; > + die "Importing disk failed: $@\n" if $@; > + > + if ($device) { > + eval { > + $update_vm_api->({ > + vmid => $vmid, > + $device => "$volid,$device_options", > + }, 1); > + }; > + # Importing large disks takes a lot of time > + # => don't remove disk automatically on option update error > + die "Copying disk to $unused_disk succeeded but attaching it " > + ."as $device and setting its options to $device_options " > + ."failed: $@.\n Adjust them manually.\n" if $@; > + } > + print "Successfully imported disk '$source' as " > + .($device ? $device : $unused_disk) . ": $volid'\n"; > + }; > + my $upid = $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker); > + return $upid; > + }}); > + > 1; > diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm > index 282fa86..4a304ce 100755 > --- a/PVE/CLI/qm.pm > +++ b/PVE/CLI/qm.pm > @@ -445,61 +445,6 @@ __PACKAGE__->register_method ({ > return undef; > }}); > > -__PACKAGE__->register_method ({ > - name => 'importdisk', > - path => 'importdisk', > - method => 'POST', > - description => "Import an external disk image as an unused disk in a VM. The > - image format has to be supported by qemu-img(1).", > - parameters => { > - additionalProperties => 0, > - properties => { > - vmid => get_standard_option('pve-vmid', {completion => \&PVE::QemuServer::complete_vmid}), > - source => { > - description => 'Path to the disk image to import', > - type => 'string', > - optional => 0, > - }, > - storage => get_standard_option('pve-storage-id', { > - description => 'Target storage ID', > - completion => \&PVE::QemuServer::complete_storage, > - optional => 0, > - }), > - format => { > - type => 'string', > - description => 'Target format', > - enum => [ 'raw', 'qcow2', 'vmdk' ], > - optional => 1, > - }, > - }, > - }, > - returns => { type => 'null'}, > - code => sub { > - my ($param) = @_; > - > - my $vmid = extract_param($param, 'vmid'); > - my $source = extract_param($param, 'source'); > - my $storeid = extract_param($param, 'storage'); > - my $format = extract_param($param, 'format'); > - > - my $vm_conf = PVE::QemuConfig->load_config($vmid); > - PVE::QemuConfig->check_lock($vm_conf); > - die "$source: non-existent or non-regular file\n" if (! -f $source); > - > - my $storecfg = PVE::Storage::config(); > - PVE::Storage::storage_check_enabled($storecfg, $storeid); > - > - my $target_storage_config = PVE::Storage::storage_config($storecfg, $storeid); > - die "storage $storeid does not support vm images\n" > - if !$target_storage_config->{content}->{images}; > - > - print "importing disk '$source' to VM $vmid ...\n"; > - my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import($source, $vmid, $storeid, { format => $format }); > - print "Successfully imported disk as '$drive_id:$volid'\n"; > - > - return undef; > - }}); > - > __PACKAGE__->register_method ({ > name => 'terminal', > path => 'terminal', > @@ -984,7 +929,7 @@ our $cmddef = { > > terminal => [ __PACKAGE__, 'terminal', ['vmid']], > > - importdisk => [ __PACKAGE__, 'importdisk', ['vmid', 'source', 'storage']], > + importdisk => [ "PVE::API2::Qemu", 'importdisk', ['vmid', 'source', 'storage'], { node => $nodename }], > > importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']], > > diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm > index 91c33f8..87727ea 100644 > --- a/PVE/QemuServer/Drive.pm > +++ b/PVE/QemuServer/Drive.pm > @@ -201,6 +201,7 @@ my %wwn_fmt = ( > }, > ); > > + > my $add_throttle_desc = sub { > my ($key, $type, $what, $unit, $longunit, $minimum) = @_; > my $d = { > @@ -308,6 +309,19 @@ my $alldrive_fmt = { > %wwn_fmt, > }; > > +my %optional_file_drivdesc_base = %drivedesc_base; s/drivdesc/drivedesc/ or maybe even to drive_desc > +$optional_file_drivdesc_base{file}{optional} = 1; # TODO verify > +my $drive_options_fmt = { > + %optional_file_drivdesc_base, # first bad thing here is the missing file stuff > + %iothread_fmt, > + %model_fmt, > + %queues_fmt, > + %scsiblock_fmt, > + %ssd_fmt, > + %wwn_fmt, > +}; > +PVE::JSONSchema::register_format('drive_options', $drive_options_fmt); > + > my $efidisk_fmt = { > volume => { alias => 'file' }, > file => { > diff --git a/PVE/QemuServer/ImportDisk.pm b/PVE/QemuServer/ImportDisk.pm > index 51ad52e..d89cd4d 100755 > --- a/PVE/QemuServer/ImportDisk.pm > +++ b/PVE/QemuServer/ImportDisk.pm > @@ -38,7 +38,7 @@ sub do_import { > } > > if ($drive_name) { > - # should never happen as setting $drive_name is not exposed to public interface > + # Exposed in importdisk API only > die "cowardly refusing to overwrite existing entry: $drive_name\n" if $vm_conf->{$drive_name}; > > my $modified = {}; # record what $option we modify >