[pve-devel] [RFC qemu-server 1/2] Move importdisk from qm to API

[Thomas Lamprecht]

On 5/22/20 12:08 PM, Dominic Jäger wrote:
> I'd appreciate a few hints for the importdisk GUI feature.
> 
> Yes or no to automatically adding the drive as active (not unused)?

Well, IMO makes sense but I'd add an parameter for either a config ID (scsi0, sata2,
...) which it is added if free, and/or the bus (controller) where we then select the
next free slot if any.

> 
> Signed-off-by: Dominic Jäger <d.jaeger at proxmox.com>
> ---
>  PVE/API2/Qemu.pm             | 87 +++++++++++++++++++++++++++++++++++-
>  PVE/CLI/qm.pm                | 57 +----------------------
>  PVE/QemuServer/ImportDisk.pm |  2 +-
>  3 files changed, 87 insertions(+), 59 deletions(-)
> 
> diff --git a/PVE/API2/Qemu.pm b/PVE/API2/Qemu.pm
> index fd51bf3..5e75605 100644
> --- a/PVE/API2/Qemu.pm
> +++ b/PVE/API2/Qemu.pm
> @@ -23,6 +23,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;
> @@ -44,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.";
> @@ -4203,4 +4202,88 @@ __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,
> +    proxyto => 'node',
> +    description => "Import an external disk image into a VM.
> +	The image format has to be supported by qemu-img(1).",

please use concatenated multi line strings for description, i.e.:

description => "Import an external disk image into a VM. "
    ."The image format has to be supported by qemu-img(1).",

> +    permissions => {
> +	description => "You need 'VM.Config.Disk' permissions on /vms/{vmid},
> +	    and 'Datastore.AllocateSpace' permissions on the storage.",
> +	check => [ 'and',
> +		   ['perm', '/vms/{vmid}', [ 'VM.Config.Disk' ]],
> +		   ['perm', '/storage/{storage}', [ 'Datastore.AllocateSpace' ]],
> +	    ],
> +    },
> +    parameters => {
> +	additionalProperties => 0,
> +	properties => PVE::QemuServer::json_config_properties ({

hmm, doesn't this puts _all_ of our VM config properties in here?? Most probably
do not make sense, only some disk/bus related ones, I'd say.

> +	    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',
> +		optional => 0,
> +	    },
> +	    format => {
> +		type => 'string',
> +		description => 'Target format',
> +		enum => [ 'raw', 'qcow2', 'vmdk' ],
> +		optional => 1,
> +	    },
> +	}),
> +    },
> +    returns => { type => 'string'},
> +    code => sub {
> +	my ($param) = @_;
> +	print("start\n");
> +
> +	my $rpcenv = PVE::RPCEnvironment::get();
> +	my $authuser = $rpcenv->get_user();
> +
> +	my $node = extract_param($param, 'node');
> +	my $vmid = extract_param($param, 'vmid');
> +	my $source = extract_param($param, 'source');
> +	my $drive = extract_param($param, 'drive');
> +	my $format = extract_param($param, 'format');
> +	my $storecfg = PVE::Storage::config();
> +
> +	# TODO nice error message if drive is not reachable
> +	my $check_replication = sub { }; # TODO implement me, maybe?
> +	my $driveX;
> +	foreach my $opt (keys %$param) {
> +	    die ("Die because multiple drives\n") if defined($driveX);

nit: no parenthesis in die

> +	    if (PVE::QemuServer::is_valid_drivename($opt)) {
> +		# cleanup drive path
> +		my $drive = PVE::QemuServer::parse_drive($opt, $param->{$opt});
> +		raise_param_exc({ $opt => "unable to parse drive options" }) if !$drive;
> +		PVE::QemuServer::cleanup_drive_path($opt, $storecfg, $drive);
> +		$check_replication->($drive); # TODO do I need this?
> +		$param->{$opt} = PVE::QemuServer::print_drive($drive);
> +		die "$source: non-existent or non-regular file\n" if (! -f $source);
> +		$driveX = $drive;
> +	    }
> +	}
> +	$drive = $driveX;
> +
> +	my $message = $drive ? "to drive $drive->{'file'} on" : 'as unused drive to';
> +	print "Importing disk '$source' " . $message . " VM $vmid ...\n";

FYI: double quoted strings allow $variable interpolation, so you do not need to
concat this extra.

> +	my $worker = sub {
> +	    eval {
> +		my $dn = "$drive->{'interface'}$drive->{'index'}";
> +		print("before import dn is $dn\n");
> +		my ($storeid) = $drive->{'file'} =~ /^(.*):/;
> +		print("storeid before import is $storeid\n");
> +		my ($drive_id, $volid) = PVE::QemuServer::ImportDisk::do_import(
> +		    $source, $vmid, $storeid, { format => $format, drive_name => $dn });
> +		print "Successfully imported disk '$source' as '$drive_id: $volid'\n";
> +	    };
> +	    die "Importing disk failed: $@\n" if $@;
> +	};
> +	return $rpcenv->fork_worker('importdisk', $vmid, $authuser, $worker);
> +    }});
> +
>  1;
> diff --git a/PVE/CLI/qm.pm b/PVE/CLI/qm.pm
> index 282fa86..75e07fa 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']],
>  
>      importovf => [ __PACKAGE__, 'importovf', ['vmid', 'manifest', 'storage']],
>  
> 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
>