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 42FC061F0E for ; Tue, 15 Sep 2020 13:34:12 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 3B5A31A024 for ; Tue, 15 Sep 2020 13:33:42 +0200 (CEST) Received: from dev.dominic.proxmox.com (212-186-127-178.static.upcbusiness.at [212.186.127.178]) by firstgate.proxmox.com (Proxmox) with ESMTP id E1D2C1A004 for ; Tue, 15 Sep 2020 13:33:39 +0200 (CEST) Received: by dev.dominic.proxmox.com (Postfix, from userid 0) id AC6B222082; Tue, 15 Sep 2020 13:33:39 +0200 (CEST) From: =?UTF-8?q?Dominic=20J=C3=A4ger?= To: pve-devel@lists.proxmox.com Date: Tue, 15 Sep 2020 13:33:23 +0200 Message-Id: <20200915113324.313395-2-d.jaeger@proxmox.com> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200915113324.313395-1-d.jaeger@proxmox.com> References: <20200915113324.313395-1-d.jaeger@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 AWL -0.540 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods KHOP_HELO_FCRDNS 0.399 Relay HELO differs from its IP's reverse DNS NO_DNS_FOR_FROM 0.379 Envelope sender has no MX or A DNS records SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an 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. [qemu.pm, drive.pm, usb.pm, cloudinit.pm, importdisk.pm, agent.pm, pci.pm, memory.pm, qm.pm, ovf.pm, qemuserver.pm] Subject: [pve-devel] [PATCH qemu-server v4 1/2] 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: Tue, 15 Sep 2020 11:34:12 -0000 Required to create a GUI for importdisk. Add parameters that enable directly attaching the disk to a bus/device with all known disk options. This avoids intermediate steps as unused disk. We allow different places as source * Regular VM images on PVE storages (Normal users + root) * Other disk images on PVE storages (Normal users + root) (they have already been displayed in the FileSelector before) * Any path (root only) Using any path for normal users would be security risk. But if you have to move a disk image to a PVE storage you only are not too many steps * rename image according to PVE schema * qm rescan * double click in GUI to attach away from making the whole importdisk obsolete. Enabling arbitrary paths for root additionally makes it unnecessary to move the disk image or create an appropriate storage. That means no knowledge about PVE storage content naming schemes ("why do I have to move it into a images/ subfolder of a directory based storage?") is required. Importing could then be comprised of only two steps: 1. mount external drive (hopefully most PVE admins can figure this out) 2. Click through GUI window and insert /mount/externalDrive/exportedFromEsxi.vmdk Uploading disk images to avoid the PVE storage naming knowledge can still be added in the future. However, such a function might not be ideal for big images because * Upload via browser might fail easily? * Potentially copying huge images from server to local to server? So having the absolute path as an option between renaming everything manually and just uploading it in GUI without CLI knowledge looks like a useful addition to me. This patch combines the main part of the previous qm importdisk and do_import into a helper $convert_and_attach in PVE/API2/Qemu.pm to avoid race conditions and potentially duplicating code from update_vm_api into do_import. Furthermore, the only other place where it was invoked was importovf, which now also uses the helper. importovf will be moved to PVE/API2/Qemu.pm, too, so placing the helper somewhere else does not look useful to me. Signed-off-by: Dominic Jäger --- v3->v4: * More detailed permissions * Solve race conditions and code reuse questions by completely moving do_import to PVE/API2/Qemu.pm; lock the whole procedure * As I found both discussed approaches - Image selector (Thomas) - Paths (Dominik) useful I included in both. Hope I didn't misunderstand any of you. PVE/API2/Qemu.pm | 184 ++++++++++++++++++++++++++++++++++- PVE/CLI/qm.pm | 70 ++----------- PVE/QemuServer.pm | 2 +- PVE/QemuServer/Drive.pm | 13 +++ PVE/QemuServer/ImportDisk.pm | 85 ---------------- PVE/QemuServer/Makefile | 1 - 6 files changed, 205 insertions(+), 150 deletions(-) delete mode 100755 PVE/QemuServer/ImportDisk.pm diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm index 8da616a..9aa85b5 100644 --- a/PVE/API2/Qemu.pm +++ b/PVE/API2/Qemu.pm @@ -45,8 +45,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."; @@ -4265,4 +4263,186 @@ __PACKAGE__->register_method({ return PVE::QemuServer::Cloudinit::dump_cloudinit_config($conf, $param->{vmid}, $param->{type}); }}); +# TODO Make locally scoped when importovf is moved from qm to API / this package +our $convert_and_attach_disk = sub { + my ($param) = @_; + my $vm_conf = PVE::QemuConfig->load_config($param->{vmid}); + my $store_conf = PVE::Storage::config(); + PVE::QemuConfig->check_lock($vm_conf) if !$param->{skiplock}; + if ($param->{device} && $vm_conf->{$param->{device}}) { + die "Could not import because device $param->{device} is already in ". + "use in VM $param->{vmid}. Choose a different device!\n"; + } + if ($param->{digest} && $param->{digest} ne $vm_conf->{digest}) { + die "VM $param->{vmid} config checksum missmatch (file change by other user?)\n"; + } + + my $msg = $param->{device} ? "to $param->{device} on" : 'as unused disk to'; + print "Importing disk '$param->{source_as_path}' $msg VM $param->{vmid}...\n"; + + my $src_size = PVE::Storage::file_size_info($param->{source_as_path}); + my $dst_format = PVE::QemuServer::resolve_dst_disk_format( + $store_conf, $param->{storage}, undef, $param->{format}); + my $dst_volid = PVE::Storage::vdisk_alloc($store_conf, $param->{storage}, + $param->{vmid}, $dst_format, undef, $src_size / 1024); + + eval { + local $SIG{INT} = + local $SIG{TERM} = + local $SIG{QUIT} = + local $SIG{HUP} = + local $SIG{PIPE} = sub { die "Interrupted by signal $!\n"; }; + + my $zeroinit = PVE::Storage::volume_has_feature($store_conf, + 'sparseinit', $dst_volid); + + PVE::Storage::activate_volumes($store_conf, [$dst_volid]); + PVE::QemuServer::qemu_img_convert($param->{source_as_path}, $dst_volid, + $src_size, undef, $zeroinit); + PVE::Storage::deactivate_volumes($store_conf, [$dst_volid]); + + if ($param->{device}) { + my $device_param = $dst_volid; + $device_param .= ",$param->{device_options}" if $param->{device_options}; + $update_vm_api->({ + vmid => $param->{vmid}, + $param->{device} => $device_param, + skiplock => $param->{skiplock} || 0, # Avoid uninitialized values + }, 1); + } else { + $param->{device} = PVE::QemuConfig->add_unused_volume($vm_conf, $dst_volid); + PVE::QemuConfig->write_config($param->{vmid}, $vm_conf); + } + }; + if (my $err = $@) { + eval { PVE::Storage::vdisk_free($store_conf, $dst_volid) }; + warn "Cleanup of $dst_volid failed: $@ \n" if $@; + + die "Importing disk '$param->{source_as_path}' failed: $err\n" if $err; + } + + return $dst_volid; +}; + +__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.Audit']], + [ 'perm', '/storage/{storage}', ['Datastore.Allocate']], + [ 'perm', '/storage/{storage}', ['Datastore.AllocateTemplate']], + [ 'perm', '/storage/{storage}', ['Datastore.AllocateSpace']], + [ 'perm', '/vms/{vmid}', ['VM.Allocate']], + [ 'perm', '/vms/{vmid}', ['VM.Config.Disk']], + ], + }, + 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 ". + "path on the server.", + type => 'string', + }, + device => { + type => 'string', + description => "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 => "Options to set for the new disk ". + "(e.g. 'discard=on,backup=0')", + optional => 1, + }, + storage => get_standard_option('pve-storage-id', { + description => "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'), + skiplock => get_standard_option('skiplock'), + }, + }, + returns => { type => 'string'}, + code => sub { + my ($param) = @_; + my $rpcenv = PVE::RPCEnvironment::get(); + my $authuser = $rpcenv->get_user(); + + my $vmid = extract_param($param, 'vmid'); + my $original_source = extract_param($param, 'source'); + my $digest = extract_param($param, 'digest'); + my $device_options = extract_param($param, 'device_options'); + my $device = extract_param($param, 'device'); + # importovf holds a lock itself which would make automatically updating + # VM configs fail + my $skiplock = extract_param($param, 'skiplock'); + my $storecfg = PVE::Storage::config(); + + if ($skiplock && $authuser ne 'root@pam') { + raise_perm_exc("Only root may use skiplock."); + } + if ($original_source eq "") { + die "Could not import because source parameter is an empty string!\n"; + } + if ($device && !PVE::QemuServer::is_valid_drivename($device)) { + die "Invalid device name: $device!"; + } + if ($device_options && !$device) { + die "Cannot use --device_options without specifying --device!" + } + eval { + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg, + $vmid, $original_source) + }; + raise_perm_exc($@) if $@; + + # A path is required for $convert_and_attach_disk + my $volid_as_path = eval { # Nonempty iff $original_source is a volid + PVE::Storage::path($storecfg, $original_source); + }; + my $source_as_path = $volid_as_path || $original_source ; + if (!-e $source_as_path) { + die "Could not import because source '$original_source' does not exist!\n"; + } + + my $worker = sub { + my $dst_volid = PVE::QemuConfig->lock_config($vmid, $convert_and_attach_disk, + { + vmid => $vmid, + original_source => $original_source, + device => $device, + device_options => $device_options, + storage => extract_param($param, 'storage'), + source_as_path => $source_as_path, + format => extract_param($param, 'format'), + skiplock => $skiplock, + }); + print "Successfully imported disk '$original_source ' as ". + "$device: $dst_volid\n"; + }; + + return $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker); + }}); + 1; diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm index 282fa86..f71b3d6 100755 --- a/PVE/CLI/qm.pm +++ b/PVE/CLI/qm.pm @@ -31,7 +31,6 @@ use PVE::QemuConfig; use PVE::QemuServer::Drive; use PVE::QemuServer::Helpers; use PVE::QemuServer::Agent qw(agent_available); -use PVE::QemuServer::ImportDisk; use PVE::QemuServer::Monitor qw(mon_cmd); use PVE::QemuServer::OVF; use PVE::QemuServer; @@ -445,61 +444,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', @@ -640,17 +584,21 @@ __PACKAGE__->register_method ({ $conf->{cores} = $parsed->{qm}->{cores} if defined($parsed->{qm}->{cores}); eval { - # order matters, as do_import() will load_config() internally + # order matters, as $convert_and_attach_disk will load_config() internally $conf->{vmgenid} = PVE::QemuServer::generate_uuid(); $conf->{smbios1} = PVE::QemuServer::generate_smbios1_uuid(); PVE::QemuConfig->write_config($vmid, $conf); foreach my $disk (@{ $parsed->{disks} }) { my ($file, $drive) = ($disk->{backing_file}, $disk->{disk_address}); - PVE::QemuServer::ImportDisk::do_import($file, $vmid, $storeid, { - drive_name => $drive, + $PVE::API2::Qemu::convert_and_attach_disk->({ + node => $nodename, + vmid => $vmid, + source_as_path => $file, + storage => $storeid, + device => $drive, format => $format, - skiplock => 1, + skiplock => 1, # Required to update VM configs }); } @@ -984,7 +932,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.pm b/PVE/QemuServer.pm index 2747c66..715c507 100644 --- a/PVE/QemuServer.pm +++ b/PVE/QemuServer.pm @@ -6613,7 +6613,7 @@ sub qemu_img_convert { $src_path = PVE::Storage::path($storecfg, $src_volid, $snapname); $src_is_iscsi = ($src_path =~ m|^iscsi://|); $cachemode = 'none' if $src_scfg->{type} eq 'zfspool'; - } elsif (-f $src_volid) { + } elsif (-f $src_volid || -b _) { # -b for LVM images for example $src_path = $src_volid; if ($src_path =~ m/\.($PVE::QemuServer::Drive::QEMU_FORMAT_RE)$/) { $src_format = $1; diff --git a/PVE/QemuServer/Drive.pm b/PVE/QemuServer/Drive.pm index 91c33f8..af52035 100644 --- a/PVE/QemuServer/Drive.pm +++ b/PVE/QemuServer/Drive.pm @@ -308,6 +308,19 @@ my $alldrive_fmt = { %wwn_fmt, }; +my %optional_file_drivedesc_base = %drivedesc_base; +$optional_file_drivedesc_base{file}{optional} = 1; +my $drive_options_fmt = { + %optional_file_drivedesc_base, + %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 deleted file mode 100755 index 51ad52e..0000000 --- a/PVE/QemuServer/ImportDisk.pm +++ /dev/null @@ -1,85 +0,0 @@ -package PVE::QemuServer::ImportDisk; - -use strict; -use warnings; - -use PVE::Storage; -use PVE::QemuServer; -use PVE::Tools qw(run_command extract_param); - -# imports an external disk image to an existing VM -# and creates by default a drive entry unused[n] pointing to the created volume -# $params->{drive_name} may be used to specify ide0, scsi1, etc ... -# $params->{format} may be used to specify qcow2, raw, etc ... -sub do_import { - my ($src_path, $vmid, $storage_id, $params) = @_; - - my $drive_name = extract_param($params, 'drive_name'); - my $format = extract_param($params, 'format'); - if ($drive_name && !(PVE::QemuServer::is_valid_drivename($drive_name))) { - die "invalid drive name: $drive_name\n"; - } - - # get the needed size from source disk - my $src_size = PVE::Storage::file_size_info($src_path); - - # get target format, target image's path, and whether it's possible to sparseinit - my $storecfg = PVE::Storage::config(); - my $dst_format = PVE::QemuServer::resolve_dst_disk_format($storecfg, $storage_id, undef, $format); - - my $dst_volid = PVE::Storage::vdisk_alloc($storecfg, $storage_id, $vmid, $dst_format, undef, $src_size / 1024); - - my $zeroinit = PVE::Storage::volume_has_feature($storecfg, 'sparseinit', $dst_volid); - - my $create_drive = sub { - my $vm_conf = PVE::QemuConfig->load_config($vmid); - if (!$params->{skiplock}) { - PVE::QemuConfig->check_lock($vm_conf); - } - - if ($drive_name) { - # should never happen as setting $drive_name is not exposed to public interface - die "cowardly refusing to overwrite existing entry: $drive_name\n" if $vm_conf->{$drive_name}; - - my $modified = {}; # record what $option we modify - $modified->{$drive_name} = 1; - $vm_conf->{pending}->{$drive_name} = $dst_volid; - PVE::QemuConfig->write_config($vmid, $vm_conf); - - my $running = PVE::QemuServer::check_running($vmid); - if ($running) { - my $errors = {}; - PVE::QemuServer::vmconfig_hotplug_pending($vmid, $vm_conf, $storecfg, $modified, $errors); - warn "hotplugging imported disk '$_' failed: $errors->{$_}\n" for keys %$errors; - } else { - PVE::QemuServer::vmconfig_apply_pending($vmid, $vm_conf, $storecfg); - } - } else { - $drive_name = PVE::QemuConfig->add_unused_volume($vm_conf, $dst_volid); - PVE::QemuConfig->write_config($vmid, $vm_conf); - } - }; - - eval { - # trap interrupts so we have a chance to clean up - local $SIG{INT} = - local $SIG{TERM} = - local $SIG{QUIT} = - local $SIG{HUP} = - local $SIG{PIPE} = sub { die "interrupted by signal $!\n"; }; - - PVE::Storage::activate_volumes($storecfg, [$dst_volid]); - PVE::QemuServer::qemu_img_convert($src_path, $dst_volid, $src_size, undef, $zeroinit); - PVE::Storage::deactivate_volumes($storecfg, [$dst_volid]); - PVE::QemuConfig->lock_config($vmid, $create_drive); - }; - if (my $err = $@) { - eval { PVE::Storage::vdisk_free($storecfg, $dst_volid) }; - warn "cleanup of $dst_volid failed: $@\n" if $@; - die $err; - } - - return ($drive_name, $dst_volid); -} - -1; diff --git a/PVE/QemuServer/Makefile b/PVE/QemuServer/Makefile index fd8cfbb..ecdab56 100644 --- a/PVE/QemuServer/Makefile +++ b/PVE/QemuServer/Makefile @@ -1,7 +1,6 @@ SOURCES=PCI.pm \ USB.pm \ Memory.pm \ - ImportDisk.pm \ OVF.pm \ Cloudinit.pm \ Agent.pm \ -- 2.20.1