needs a rebase ;)
On September 15, 2020 1:33 pm, Dominic Jäger wrote:
> 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/<vmid>
> 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 <d.jae...@proxmox.com>
> ---
> 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
maybe we could have a description of $param here? doing that often helps
to show whether this is a good choice ;) e.g., in this case
$param->{original_source} is never used..
I would suggest moving the config loading outside and passing the
configs to this. importovf will probably hold a lock and load the
configs once, so no need to put this inside this shared method.
> +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";
> + }
use assert_if_modified, but also move this to the API call where the
config is locked+loaded..
> +
> + my $msg = $param->{device} ? "to $param->{device} on" : 'as unused disk
> to';
> + print "Importing disk '$param->{source_as_path}' $msg VM
> $param->{vmid}...\n";
this sounds strange, maybe Dylan can contribute a proper English phrasing
for this ;)
> +
> + my $src_size = PVE::Storage::file_size_info($param->{source_as_path});
needs check for undef
> + my $dst_format = PVE::QemuServer::resolve_dst_disk_format(
> + $store_conf, $param->{storage}, undef, $param->{format});
isn't this a bit strange? if I request to import as qcow2, but my
storage does not support it, I probably want to get an error and not
silently be converted to the default format of the storage?
> + 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']],
I doubt this is actually what you wanted here (all those permissions
combined don't really make sense).
Let's see whether we can disentangle it a bit:
- we are modifying a VM config (adding a new disk entry or a new unused
entry) - this is should require the same permissions as modifying that
part of the config, which is probably VM.Config.Disk and
Datastore.AllocateSpace
- we are allocating a new vdisk volume on a storage (this should require
Datastore.Allocatespace)
we also potentially read from a storage (which requires a separate
check) or from an arbitrary path (which requires yet another check).
> + ],
> + },
> + 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',
we have a format for something similar (volid or path) already, maybe we
can extend it?
> + },
> + 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";
> + }
should be handled by schema, not manually.
> + if ($device && !PVE::QemuServer::is_valid_drivename($device)) {
> + die "Invalid device name: $device!";
> + }
this should not be possible, since it's an enum and already checked by
the API
> + if ($device_options && !$device) {
> + die "Cannot use --device_options without specifying --device!"
> + }
this can be encoded in the schema (with 'requires')
> + eval {
> + PVE::Storage::check_volume_access($rpcenv, $authuser, $storecfg,
> + $vmid, $original_source)
> + };
so the STORAGE:PATH example from the description is also limited to
root@pam
> + 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";
$device is potentially undef here
> + };
> +
> + 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
this is not really related to this patch, right?
> $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
>
>
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
