[pve-devel] [PATCH widget-toolkit 0/1] Add GUI for importdisk

[pve-devel] [PATCH widget-toolkit 0/1] Add GUI for importdisk

[Dominic Jäger]

This series makes importing disks possible via GUI which is one step to make
migrating from other hypervisors easier.

Dominic Jäger (1):
  EditWindow: Change url to 'importdisk' for import

 src/window/Edit.js | 4 ++++
 1 file changed, 4 insertions(+)

-- 
2.20.1

[pve-devel] [PATCH widget-toolkit 1/3] EditWindow: Change url to 'importdisk' for import

[Dominic Jäger]

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 src/window/Edit.js | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/src/window/Edit.js b/src/window/Edit.js
index c165141..816d68f 100644
--- a/src/window/Edit.js
+++ b/src/window/Edit.js
@@ -134,6 +134,10 @@ Ext.define('Proxmox.window.Edit', {
 	    values = undefined;
 	}
 
+	if (me.isImport) {
+	    url = url.replace('config', 'importdisk');
+	}
+
 	Proxmox.Utils.API2Request({
 	    url: url,
 	    waitMsgTarget: me,
-- 
2.20.1

[pve-devel] [PATCH manager 2/3] Hardware View: Add GUI for importdisk

[Dominic Jäger]

Make importing single disks easier.
Required to import a whole VM via GUI.

Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
---
 www/manager6/qemu/HDEdit.js       | 80 ++++++++++++++++++++++---------
 www/manager6/qemu/HardwareView.js | 18 +++++++
 2 files changed, 76 insertions(+), 22 deletions(-)

diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
index e2a5b914..7f6a7b2b 100644
--- a/www/manager6/qemu/HDEdit.js
+++ b/www/manager6/qemu/HDEdit.js
@@ -76,23 +76,43 @@ Ext.define('PVE.qemu.HDInputPanel', {
 	    me.drive.format = values.diskformat;
 	}
 
-	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
-	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
-	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
-	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
-
-        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
-        Ext.Array.each(names, function(name) {
-            var burst_name = name + '_max';
-	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
-	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
-        });
-
-
-	params[confid] = PVE.Parser.printQemuDrive(me.drive);
+	if (me.isImport) {
+	    // These keys & values are accepted by the API as they are
+	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
+	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
+	    burst = burst.concat(burst.map(x => `${x}_max`));
+	    let available = simple.concat(burst);
+	    let addValues = key => `${key}=${values[key]}`;
+	    let selectedKeys = x => values[x];
+	    let options = available.filter(selectedKeys).map(addValues).join();
+	    // These need modification for the API
+	    options += values.discard ? ',discard=on' : '';
+	    options += values.noreplicate ? ',replicate=0' : '';
+	    params.device_options = options;
+	} else {
+	    PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
+	    PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
+	    PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
+	    PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
+
+	    var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
+		Ext.Array.each(names, function(name) {
+		    var burst_name = name + '_max';
+		    PVE.Utils.propertyStringSet(me.drive, values[name], name);
+		    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
+	    });
+	}
 
+	if (me.isImport) {
+	    params.source = values.inputImage;
+	    params.device = values.controller + values.deviceid;
+	    params.storage = values.hdstorage;
+	    if (values.diskformat) params.format = values.diskformat;
+	} else {
+	    params[confid] = PVE.Parser.printQemuDrive(me.drive);
+	}
 	return params;
     },
 
@@ -199,14 +219,17 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		allowBlank: false
 	    });
 	    me.column1.push(me.unusedDisks);
-	} else if (me.isCreate) {
-	    me.column1.push({
+	} else if (me.isCreate || me.isImport) {
+	    let selector = {
 		xtype: 'pveDiskStorageSelector',
 		storageContent: 'images',
 		name: 'disk',
 		nodename: me.nodename,
-		autoSelect: me.insideWizard
-	    });
+		hideSize: me.isImport,
+		autoSelect: me.insideWizard || me.isImport,
+	    };
+	    if (me.isImport) selector.storageLabel = 'Target storage';
+	    me.column1.push(selector);
 	} else {
 	    me.column1.push({
 		xtype: 'textfield',
@@ -231,6 +254,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
 		name: 'discard'
 	    }
 	);
+	if (me.isImport) {
+	    me.column2.push({
+		xtype: 'textfield',
+		fieldLabel: gettext('Source image'),
+		name: 'inputImage',
+		emptyText: '/home/user/disk.qcow2',
+	    });
+	}
 
 	me.advancedColumn1.push(
 	    {
@@ -372,14 +403,19 @@ Ext.define('PVE.qemu.HDEdit', {
 	    confid: me.confid,
 	    nodename: nodename,
 	    unused: unused,
-	    isCreate: me.isCreate
+	    isCreate: me.isCreate,
+	    isImport: me.isImport,
 	});
 
 	var subject;
 	if (unused) {
 	    me.subject = gettext('Unused Disk');
+	} else if (me.isImport) {
+	    me.title = 'Import Hard Disk';
+	    me.submitText = 'Import';
+	    me.backgroundDelay = undefined;
 	} else if (me.isCreate) {
-            me.subject = gettext('Hard Disk');
+	    me.subject = gettext('Hard Disk');
 	} else {
            me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
 	}
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 40b3fe86..f9735998 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -436,6 +436,23 @@ Ext.define('PVE.qemu.HardwareView', {
 	    handler: run_move
 	});
 
+	var import_btn = new Proxmox.button.Button({
+	    text: gettext('Import disk'),
+	    hidden: !(caps.vms['VM.Allocate'] &&
+		caps.storage['Datastore.AllocateTemplate'] &&
+		caps.storage['Datastore.AllocateSpace']),
+	    handler: function() {
+		var win = Ext.create('PVE.qemu.HDEdit', {
+		    method: 'POST',
+		    url: `/api2/extjs/${baseurl}`,
+		    pveSelNode: me.pveSelNode,
+		    isImport: true,
+		});
+		win.on('destroy', me.reload, me);
+		win.show();
+	    },
+	});
+
 	var remove_btn = new Proxmox.button.Button({
 	    text: gettext('Remove'),
 	    defaultText: gettext('Remove'),
@@ -752,6 +769,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn,
 		resize_btn,
 		move_btn,
+		import_btn,
 		revert_btn
 	    ],
 	    rows: rows,
-- 
2.20.1

[pve-devel] [PATCH qemu-server 3/3] Move importdisk from qm to API

[Dominic Jäger]

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@proxmox.com>
---
 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',
+		type => 'string',
+	    },
+	    device => {
+		type => 'string',
+		description => "What device the imported image should be "
+		    ."(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 "
+		    ."(e.g. 'discard=on,backup=0')",
+		optional => 1,
+	    },
+	    storage => get_standard_option('pve-storage-id', {
+		description => 'Which storage to use for the imported image.',
+		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";
+	};
+	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;
+$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
-- 
2.20.1

Re: [pve-devel] [PATCH qemu-server 3/3] Move importdisk from qm to API

[Aaron Lauterer]

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 <d.jaeger@proxmox.com>
> ---
>   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
> 

Re: [pve-devel] [PATCH manager 2/3] Hardware View: Add GUI for importdisk

[Aaron Lauterer]

Some thing inline.

On 7/7/20 12:04 PM, Dominic Jäger wrote:
> Make importing single disks easier.
> Required to import a whole VM via GUI.
> 
> Signed-off-by: Dominic Jäger <d.jaeger@proxmox.com>
> ---
>   www/manager6/qemu/HDEdit.js       | 80 ++++++++++++++++++++++---------
>   www/manager6/qemu/HardwareView.js | 18 +++++++
>   2 files changed, 76 insertions(+), 22 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index e2a5b914..7f6a7b2b 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -76,23 +76,43 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   	    me.drive.format = values.diskformat;
>   	}
>   
> -	PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> -	PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> -	PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> -	PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> -
> -        var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> -        Ext.Array.each(names, function(name) {
> -            var burst_name = name + '_max';
> -	    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> -	    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> -        });
> -
> -
> -	params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	if (me.isImport) {
> +	    // These keys & values are accepted by the API as they are
> +	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
> +	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +	    burst = burst.concat(burst.map(x => `${x}_max`));
> +	    let available = simple.concat(burst);
> +	    let addValues = key => `${key}=${values[key]}`;
> +	    let selectedKeys = x => values[x];
> +	    let options = available.filter(selectedKeys).map(addValues).join();
> +	    // These need modification for the API
> +	    options += values.discard ? ',discard=on' : '';
> +	    options += values.noreplicate ? ',replicate=0' : '';
> +	    params.device_options = options;

One or two newlines in the above block would be nice to make the distinction between the logical blocks more obvious.

> +	} else {
> +	    PVE.Utils.propertyStringSet(me.drive, !values.backup, 'backup', '0');
> +	    PVE.Utils.propertyStringSet(me.drive, values.noreplicate, 'replicate', 'no');
> +	    PVE.Utils.propertyStringSet(me.drive, values.discard, 'discard', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.ssd, 'ssd', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.iothread, 'iothread', 'on');
> +	    PVE.Utils.propertyStringSet(me.drive, values.cache, 'cache');
> +
> +	    var names = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> +		Ext.Array.each(names, function(name) {
> +		    var burst_name = name + '_max';
> +		    PVE.Utils.propertyStringSet(me.drive, values[name], name);
> +		    PVE.Utils.propertyStringSet(me.drive, values[burst_name], burst_name);
> +	    });
> +	}
>   
> +	if (me.isImport) {
> +	    params.source = values.inputImage;
> +	    params.device = values.controller + values.deviceid;
> +	    params.storage = values.hdstorage;
> +	    if (values.diskformat) params.format = values.diskformat;
> +	} else {
> +	    params[confid] = PVE.Parser.printQemuDrive(me.drive);
> +	}
>   	return params;
>       },
>   
> @@ -199,14 +219,17 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		allowBlank: false
>   	    });
>   	    me.column1.push(me.unusedDisks);
> -	} else if (me.isCreate) {
> -	    me.column1.push({
> +	} else if (me.isCreate || me.isImport) {
> +	    let selector = {
>   		xtype: 'pveDiskStorageSelector',
>   		storageContent: 'images',
>   		name: 'disk',
>   		nodename: me.nodename,
> -		autoSelect: me.insideWizard
> -	    });
> +		hideSize: me.isImport,
> +		autoSelect: me.insideWizard || me.isImport,
> +	    };
> +	    if (me.isImport) selector.storageLabel = 'Target storage';

this should be a `gettext('Target storage')` so it can be translated.

> +	    me.column1.push(selector);
>   	} else {
>   	    me.column1.push({
>   		xtype: 'textfield',
> @@ -231,6 +254,14 @@ Ext.define('PVE.qemu.HDInputPanel', {
>   		name: 'discard'
>   	    }
>   	);
> +	if (me.isImport) {
> +	    me.column2.push({
> +		xtype: 'textfield',
> +		fieldLabel: gettext('Source image'),
> +		name: 'inputImage',
> +		emptyText: '/home/user/disk.qcow2',
> +	    });
> +	}
>   
>   	me.advancedColumn1.push(
>   	    {
> @@ -372,14 +403,19 @@ Ext.define('PVE.qemu.HDEdit', {
>   	    confid: me.confid,
>   	    nodename: nodename,
>   	    unused: unused,
> -	    isCreate: me.isCreate
> +	    isCreate: me.isCreate,
> +	    isImport: me.isImport,
>   	});
>   
>   	var subject;
>   	if (unused) {
>   	    me.subject = gettext('Unused Disk');
> +	} else if (me.isImport) {
> +	    me.title = 'Import Hard Disk';

for consistency I would not change the title and instead set the following:
me.subject = gettext('Import Disk'),

> +	    me.submitText = 'Import';
> +	    me.backgroundDelay = undefined;
>   	} else if (me.isCreate) {
> -            me.subject = gettext('Hard Disk');
> +	    me.subject = gettext('Hard Disk');
>   	} else {
>              me.subject = gettext('Hard Disk') + ' (' + me.confid + ')';
>   	}
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 40b3fe86..f9735998 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -436,6 +436,23 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    handler: run_move
>   	});
>   
> +	var import_btn = new Proxmox.button.Button({
> +	    text: gettext('Import disk'),
> +	    hidden: !(caps.vms['VM.Allocate'] &&
> +		caps.storage['Datastore.AllocateTemplate'] &&
> +		caps.storage['Datastore.AllocateSpace']),
> +	    handler: function() {
> +		var win = Ext.create('PVE.qemu.HDEdit', {
> +		    method: 'POST',
> +		    url: `/api2/extjs/${baseurl}`,
> +		    pveSelNode: me.pveSelNode,
> +		    isImport: true,
> +		});
> +		win.on('destroy', me.reload, me);
> +		win.show();
> +	    },
> +	});
> +
>   	var remove_btn = new Proxmox.button.Button({
>   	    text: gettext('Remove'),
>   	    defaultText: gettext('Remove'),
> @@ -752,6 +769,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		edit_btn,
>   		resize_btn,
>   		move_btn,
> +		import_btn,
>   		revert_btn
>   	    ],
>   	    rows: rows,
> 

Re: [pve-devel] [PATCH manager 2/3] Hardware View: Add GUI for importdisk

[Dominic Jäger]

Thank you for taking a look!

> > +	if (me.isImport) {
> > +	    // These keys & values are accepted by the API as they are
> > +	    let simple = ['backup', 'ssd', 'iothread', 'cache'];
> > +	    let burst = ['mbps_rd', 'mbps_wr', 'iops_rd', 'iops_wr'];
> > +	    burst = burst.concat(burst.map(x => `${x}_max`));
> > +	    let available = simple.concat(burst);
> > +	    let addValues = key => `${key}=${values[key]}`;
> > +	    let selectedKeys = x => values[x];
> > +	    let options = available.filter(selectedKeys).map(addValues).join();
> > +	    // These need modification for the API
> > +	    options += values.discard ? ',discard=on' : '';
> > +	    options += values.noreplicate ? ',replicate=0' : '';
> > +	    params.device_options = options;
> 
> One or two newlines in the above block would be nice to make the distinction between the logical blocks more obvious.
Done.
> 
> > +	    if (me.isImport) selector.storageLabel = 'Target storage';
> 
> this should be a `gettext('Target storage')` so it can be translated.
Done.

> for consistency I would not change the title and instead set the following:
> me.subject = gettext('Import Disk'),
Done.

Re: [pve-devel] [PATCH qemu-server 3/3] Move importdisk from qm to API

[Dominic Jäger]

On Wed, Jul 29, 2020 at 03:00:41PM +0200, Aaron Lauterer wrote:
> 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

I intially placed it in front because the Perl style guide has && in front in a
multi-line example. However, most of this file has dots in the end and the
guide forbids mixing only, so I changed it.

> 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.

Fabian G. just told me that 80 character line length limit also holds for comments.

> > +	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."
Fits the rest of the descriptions better => Changed all of them. Thank you :)

> 
> > +	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'?

Didn't include this (yet) because I actually don't think so.
The idea was
1) Import sth to sth => "Importing disk 'myfile.qcow2' to VM100."
   How are we importing it? as unused disk
   "Importing disk 'myfile.qcow2' as unused disk to VM100."
2) Import sth to sth => "Importing disk 'myfile.qcow2' to scsi0."
   Where is scsi0? on VM100
   "Importing disk 'myfile.qcow2' to scsi0 on VM100."

> It will result in something like "importing disk 'myfile.qcow' as unused disk for VM XXX"
I think there should be a "to" somewhere.

Would for 2) "Importing disk 'myfile.qcow2' to VM100 as scsi0" make sense, maybe?
> 
> > +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);
> > +
Thank you, changed it to drivedesc :) It's drivedesc in the rest of the file, without underscore.