[pve-devel] [PATCH qemu-server v3 2/3] Move importdisk from qm to API
Dominik Csapak
d.csapak at proxmox.com
Thu Sep 3 15:52:45 CEST 2020
comments inline
On 8/25/20 11:24 AM, 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 <d.jaeger at proxmox.com>
> ---
> v2->v3: unchanged
>
> 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 8da616a..66e630d 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.";
> @@ -4265,4 +4264,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']],
> + ],
this is a big no-no
now everyone that has permissions to create a vm can import any
file they want from any path?
(e.g. another vm image, /dev/sda (!!), etc.)
this is basically circumventing our whole permission system...
for 'qm' this was okay since that was root at pam only, but if
we are in the api, we have to be careful what to import
in general, we have to do either:
* restrict the source path to a very small subset of possible
paths, that are guaranteed to not be dangerous
(/tmp/pve-import-$userid/ ?)
* or leave it as 'root at pam' only
> + },
> + parameters => {
> + additionalProperties => 0,
> + properties => {
> + node => get_standard_option('pve-node'),
> + vmid => get_standard_option('pve-vmid',
> + {completion => \&PVE::QemuServer::complete_vmid}),
> + source => {
> + description => "Path to the disk image to import",
> + 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'),
> + },
> + },
> + 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';
> + 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";
is that not somehow racy?
i start an import and give devices scsi2
then at the right time i update the vm with scsi2 and now i overwrite it
again?
should we not hold a lock ? or can we ignore that ? (at least document it?)
also if i see it right, 'do_import' can do this already?
you even change the comment of the code but you don't use it...
> + };
> + 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..e7cb74c 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_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
> 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
>
More information about the pve-devel
mailing list