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

[Dominik Csapak <d.csapak@proxmox.com>]

On 8/25/20 11:24 AM, 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>
> ---
> v2->v3: Use the new submitUrl parameter from widget-tookit
> 
> Depends on both other patches
> 
>   www/manager6/qemu/HDEdit.js       | 83 +++++++++++++++++++++++--------
>   www/manager6/qemu/HardwareView.js | 20 ++++++++
>   2 files changed, 81 insertions(+), 22 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDEdit.js b/www/manager6/qemu/HDEdit.js
> index e2a5b914..edebbbc1 100644
> --- a/www/manager6/qemu/HDEdit.js
> +++ b/www/manager6/qemu/HDEdit.js
> @@ -76,23 +76,46 @@ 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;

why do you do this?

you can simply reuse the propertyStringSet function again, just
use a different object

...drive code...

let drive_obj = import ? {} : me.drive;

PVE.Utils.propertyStringSet(drive_obj....);

and then use drive_obj instead of either your values or me.drive
(you can use PVE.Parser.printPropertyString to get the 'propertyString' 
format)

on another note, here in onGetValues you can replace the url with the
correct submiturl if you want, no need to change widget toolkit

> +	} 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 +222,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 = gettext('Target storage');
> +	    me.column1.push(selector);
>   	} else {
>   	    me.column1.push({
>   		xtype: 'textfield',
> @@ -231,6 +257,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 +406,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.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..5598214b 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -436,6 +436,25 @@ 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() {
> +		let url = `/api2/extjs/${baseurl}`;
> +		var win = Ext.create('PVE.qemu.HDEdit', {
> +		    method: 'POST',
> +		    url: url,
> +		    submitUrl: url.replace('config', 'importdisk'),

this is wrong, replace only replaces the first instance

if my node is named 'confignode1'

the resulting url is now

/api2/extjs/nodes/importdisknode1/qemu/$ID/config

(which probably does no harm but will not work)

if you do this here, i would construct 2 baseurls
and construct both urls independendtly

if you decide to replace config with importdisk
in HDEdit, i would use a regex replace
  /\/config$/\/importdisk/
or use

url.lastIndexOf("/config")

to not run into such issues
> +		    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 +771,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		edit_btn,
>   		resize_btn,
>   		move_btn,
> +		import_btn,
>   		revert_btn
>   	    ],
>   	    rows: rows,
>