[pve-devel] [PATCH v2 manager 0/3] ui: lxc/qemu: add reassign for disks and volumes

[pve-devel] [PATCH v2 manager 0/3] ui: lxc/qemu: add reassign for disks and volumes

[Aaron Lauterer]

This series adds the UI for the recently applied backend series that
allows to reassign a disk / volume from one guest to another.

Patch 2 and 3 are optional. Patch 2 only changes a double negated option
and patch 3 happend in the process of working on an interface for the
reassign functionality. Since the work of modernizing this componend is
done, why not use it

v2: incorporated feedback I got off list, mainly
* using more modern approaches
* more arrow functions
* reducing use of predefined cbind values and using inline functions
  when possible


Aaron Lauterer (3):
  ui: lxc/qemu: add disk reassign
  ui: BusTypeSelector: change noVirtIO to withVirtIO
  ui: hdmove: modernize/refactor

 www/manager6/Makefile                   |   1 +
 www/manager6/form/BusTypeSelector.js    |   4 +-
 www/manager6/form/ControllerSelector.js |   4 +-
 www/manager6/lxc/Resources.js           |  62 ++++-
 www/manager6/qemu/CDEdit.js             |   2 +-
 www/manager6/qemu/CIDriveEdit.js        |   2 +-
 www/manager6/qemu/HDMove.js             | 239 ++++++++++--------
 www/manager6/qemu/HDReassign.js         | 316 ++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js       |  58 ++++-
 9 files changed, 572 insertions(+), 116 deletions(-)
 create mode 100644 www/manager6/qemu/HDReassign.js

-- 
2.30.2

[pve-devel] [PATCH v2 manager 1/3] ui: lxc/qemu: add disk reassign

[Aaron Lauterer]

For the new HDReassign component, we follow the approach of HDMove to
have one componend for qemu and lxc as they are the same for most parts.

The 'Move disk/volume' button is now a SplitButton which has the
'Reassign' button as menu item.  In the lxc resource panel the menu item
is defined extra so we can easily disable it, should the selected mp be
the rootfs.

In the lxc resource and qemu hardware panel we currently add a new
button to handle unused disks/volumes. The button is "switched" with the
'Move' in this case. The width of the buttons is aligned to avoid
movement of other buttons.

Once we are able to also move unused disks/volumes to other storages, we
can remove this.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
Not all cbind values can be omitted AFAICT as we do not have access to
the component context when declaring our items. From what I know, we can
use regular bind, cbind (if we only need to set the value once) or set
the values manually in the initComponent.

Triggering the validation of the mountpoint integerfield when the mount
point type changes (onMpTypeChange) is happening because it is not done
so automatically but necessary as the other MP type (e.g. unused) could
already be in use with that number. There might be a better way that I
am not aware of.


changes since v1: incorporated feedback I got off list

* use more modern approaches
    * arrow functions
    * autoShow
    * template strings
* reduce predefined cbind values and use arrow functions in the cbind
  directly in many cases
* some code style issues and cleanup

 www/manager6/Makefile             |   1 +
 www/manager6/lxc/Resources.js     |  62 +++++-
 www/manager6/qemu/HDReassign.js   | 316 ++++++++++++++++++++++++++++++
 www/manager6/qemu/HardwareView.js |  57 +++++-
 4 files changed, 432 insertions(+), 4 deletions(-)
 create mode 100644 www/manager6/qemu/HDReassign.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index e6e01bd1..94a78d89 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -214,6 +214,7 @@ JSSRC= 							\
 	qemu/HDTPM.js					\
 	qemu/HDMove.js					\
 	qemu/HDResize.js				\
+	qemu/HDReassign.js				\
 	qemu/HardwareView.js				\
 	qemu/IPConfigEdit.js				\
 	qemu/KeyboardEdit.js				\
diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
index 15ee3c67..bec7cf14 100644
--- a/www/manager6/lxc/Resources.js
+++ b/www/manager6/lxc/Resources.js
@@ -156,6 +156,11 @@ Ext.define('PVE.lxc.RessourceView', {
 		return;
 	    }
 
+	    if (rec.data.key.match(/^unused/)) {
+		Ext.Msg.alert('Error', gettext('Not yet supported for unused volumes'));
+		return;
+	    }
+
 	    var win = Ext.create('PVE.window.HDMove', {
 		disk: rec.data.key,
 		nodename: nodename,
@@ -168,6 +173,24 @@ Ext.define('PVE.lxc.RessourceView', {
 	    win.on('destroy', me.reload, me);
 	};
 
+	let run_reassign = function() {
+	    let rec = me.selModel.getSelection()[0];
+	    if (!rec) {
+		return;
+	    }
+
+	    Ext.create('PVE.window.HDReassign', {
+		disk: rec.data.key,
+		nodename: nodename,
+		autoShow: true,
+		vmid: vmid,
+		type: 'lxc',
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	};
+
 	var edit_btn = new Proxmox.button.Button({
 	    text: gettext('Edit'),
 	    selModel: me.selModel,
@@ -227,12 +250,40 @@ Ext.define('PVE.lxc.RessourceView', {
 	    },
 	});
 
-	var move_btn = new Proxmox.button.Button({
+	let reassign_menuitem = new Ext.menu.Item({
+	    text: gettext('Reassign volume'),
+	    tooltip: gettext('Reassign volume to another VM'),
+	    handler: run_reassign,
+	    iconCls: 'fa fa-mail-forward',
+	    reference: 'reassing_item',
+	});
+
+	let move_btn = new PVE.button.Split({
 	    text: gettext('Move Volume'),
 	    selModel: me.selModel,
 	    disabled: true,
 	    dangerous: true,
 	    handler: run_move,
+	    menu: {
+		items: [reassign_menuitem],
+	    },
+	});
+
+	// needed until we can move unused volumes to other storages
+	let reassign_btn = new Proxmox.button.Button({
+	    text: gettext('Reassign volume'),
+	    tooltip: gettext('Reassign volume to another VM'),
+	    handler: run_reassign,
+	    selModel: me.selModel,
+	    hidden: true,
+	    listeners: {
+		render: function(btn) {
+		    // hack: avoid layout-flickering due to size change, use max width for both
+		    let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width);
+		    move_btn.setSize({ width: maxWidth });
+		    btn.setSize({ width: maxWidth });
+		},
+	    },
 	});
 
 	var revert_btn = new PVE.button.PendingRevert();
@@ -253,6 +304,7 @@ Ext.define('PVE.lxc.RessourceView', {
 
 	    var pending = rec.data.delete || me.hasPendingChanges(key);
 	    var isDisk = rowdef.tdCls === 'pve-itype-icon-storage';
+	    let isRootFS = rec.data.key === 'rootfs';
 	    var isUnusedDisk = key.match(/^unused\d+/);
 	    var isUsedDisk = isDisk && !isUnusedDisk;
 
@@ -265,7 +317,12 @@ Ext.define('PVE.lxc.RessourceView', {
 	    }
 	    edit_btn.setDisabled(noedit);
 
-	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
+	    reassign_btn.setHidden(!isUnusedDisk);
+	    reassign_btn.setDisabled(!isUnusedDisk);
+	    move_btn.setHidden(isUnusedDisk);
+	    reassign_menuitem.setDisabled(isRootFS);
+
+	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
 	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
 	    move_btn.setDisabled(!isDisk || !diskCap);
 	    revert_btn.setDisabled(!pending);
@@ -329,6 +386,7 @@ Ext.define('PVE.lxc.RessourceView', {
 		remove_btn,
 		resize_btn,
 		move_btn,
+		reassign_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
new file mode 100644
index 00000000..e6b59cb4
--- /dev/null
+++ b/www/manager6/qemu/HDReassign.js
@@ -0,0 +1,316 @@
+Ext.define('PVE.window.HDReassign', {
+    extend: 'Ext.window.Window',
+    mixins: ['Proxmox.Mixin.CBind'],
+
+    resizable: false,
+    modal: true,
+    width: 350,
+    border: false,
+    layout: 'fit',
+
+    viewModel: {
+	data: {
+	    mpType: '',
+	},
+	formulas: {
+	    mpMaxCount: get => get('mpType') === 'mp'
+		? PVE.Utils.mp_counts.mps - 1
+		: PVE.Utils.mp_counts.unused - 1,
+	},
+    },
+
+    cbindData: function() {
+	let me = this;
+	return {
+	    vmid: me.vmid,
+	    disk: me.disk,
+	    isQemu: me.type === 'qemu',
+	    nodename: me.nodename,
+	};
+    },
+
+    cbind: {
+	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
+	qemu: '{isQemu}',
+    },
+
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	initViewModel: function(model) {
+	    let view = this.getView();
+	    let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp';
+	    model.set('mpType', mpTypeValue);
+	},
+
+	reassign_disk: function(values) {
+	    let me = this;
+	    let view = me.getView();
+	    let qemu = view.qemu;
+	    let params = {
+		vmid: view.vmid,
+		'target-vmid': values.targetVmid,
+	    };
+
+	    params[qemu ? 'disk' : 'volume'] = view.disk;
+
+	    if (view.qemu) {
+		params['target-disk'] = `${values.controller}${values.deviceid}`;
+	    } else {
+		params['target-volume'] = `${values.mpType}${values.mpId}`;
+	    }
+
+	    let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`;
+	    url += qemu ? 'move_disk' : 'move_volume';
+
+	    Proxmox.Utils.API2Request({
+		params: params,
+		url: url,
+		waitMsgTarget: me.getView(),
+		method: 'POST',
+		failure: response => Ext.Msg.alert('Error', response.htmlStatus),
+		success: function(response, options) {
+		    let upid = response.result.data;
+		    view.hide();
+		    Ext.create('Proxmox.window.TaskProgress', {
+			upid,
+			autoShow: true,
+			taskDone: success => success ? view.close() : view.show(),
+		    });
+		},
+	    });
+	},
+
+	validateForm: function(fp, isValid) {
+	    this.getView().lookup('submitButton').setDisabled(!isValid);
+	},
+
+	onReassignClick: function() {
+		let me = this;
+		let view = me.getView();
+		let form = view.lookup('moveFormPanel').getForm();
+		if (form.isValid()) {
+		    me.reassign_disk(form.getValues());
+		}
+	},
+
+	onMpTypeChange: function(value) {
+	    this.getView().getViewModel().set('mpType', value.getValue());
+	    this.getView().lookup('mpIdSelector').validate();
+	},
+
+	onTargetVMChange: function(f, vmid) {
+	    let me = this;
+	    let view = me.getView();
+	    let diskSelector = view.lookup('diskSelector');
+	    if (!vmid) {
+		diskSelector.setVMConfig(null);
+		me.VMConfig = null;
+		return;
+	    }
+
+	    let type = view.type === 'qemu' ? 'qemu' : 'lxc';
+
+	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
+	    Proxmox.Utils.API2Request({
+		url: url,
+		method: 'GET',
+		failure: function(response, opts) {
+		    Ext.Msg.alert('Error', response.htmlStatus);
+		},
+		success: function(response, options) {
+		    if (type === 'qemu') {
+			diskSelector.setVMConfig(response.result.data);
+			diskSelector.setDisabled(false);
+		    } else {
+			let mpIdSelector = view.lookup('mpIdSelector');
+			let mpType = view.lookup('mpType');
+
+			view.VMConfig = response.result.data;
+
+			PVE.Utils.forEachMP((bus, i) => {
+			    let name = view.getViewModel().get('mpType') + i.toString();
+			    if (!Ext.isDefined(view.VMConfig[name])) {
+				mpIdSelector.setValue(i);
+				return false;
+			    }
+			    return undefined;
+			});
+
+			mpType.setDisabled(false);
+			mpIdSelector.setDisabled(false);
+			mpIdSelector.validate();
+		    }
+		},
+	    });
+	},
+    },
+
+    buttons: [
+	{
+	    xtype: 'button',
+	    reference: 'submitButton',
+	    cbind: {
+		text: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
+	    },
+	    handler: 'onReassignClick',
+	    disabled: true,
+	},
+    ],
+
+    items: [
+	{
+	    xtype: 'form',
+	    reference: 'moveFormPanel',
+	    bodyPadding: 10,
+	    border: false,
+	    fieldDefaults: {
+		labelWidth: 100,
+		anchor: '100%',
+	    },
+	    listeners: {
+		validitychange: 'validateForm',
+	    },
+	    items: [
+		{
+		    xtype: 'displayfield',
+		    name: 'sourceDisk',
+		    cbind: {
+			name: get => get('isQemu') ? 'disk' : 'volume',
+			fieldLabel: get => get('isQemu')
+			    ? gettext('Source Disk')
+			    : gettext('Source Mount Point'),
+			value: '{disk}',
+		    },
+		    vtype: 'StorageId',
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'vmComboSelector',
+		    reference: 'targetVMID',
+		    name: 'targetVmid',
+		    allowBlank: false,
+		    bind: {
+			value: '{targetVMID}',
+		    },
+		    cbind: {
+			fieldLabel: get => get('isQemu') ? gettext('Target VM') : gettext('Target CT'),
+		    },
+		    store: {
+			model: 'PVEResources',
+			autoLoad: true,
+			sorters: 'vmid',
+			cbind: {}, // for nested cbinds
+			filters: [
+			    {
+				property: 'type',
+				cbind: {
+				    value: get => get('isQemu') ? /qemu/ : /lxc/,
+				},
+			    },
+			    {
+				property: 'node',
+				cbind: {
+				    value: '{nodename}',
+				},
+			    },
+			    {
+				property: 'vmid',
+				operator: '!=',
+				cbind: {
+				    value: '{vmid}',
+				},
+			    },
+			],
+		    },
+		    listeners: { change: 'onTargetVMChange' },
+		},
+		{
+		    xtype: 'pveControllerSelector',
+		    reference: 'diskSelector',
+		    withUnused: true,
+		    disabled: true,
+		    cbind: {
+			hidden: '{!isQemu}',
+		    },
+		},
+		{
+		    xtype: 'container',
+		    layout: 'hbox',
+		    cbind: {
+			hidden: '{isQemu}',
+			disabled: '{isQemu}',
+		    },
+		    items: [
+			{
+			    xtype: 'pmxDisplayEditField',
+			    cbind: {
+				editable: get => !get('disk').match(/^unused\d+/),
+				value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp',
+			    },
+			    disabled: true,
+			    name: 'mpType',
+			    reference: 'mpType',
+			    fieldLabel: gettext('Add as'),
+			    submitValue: true,
+			    flex: 4,
+			    editConfig: {
+				xtype: 'proxmoxKVComboBox',
+				name: 'mpTypeCombo',
+				reference: 'mpTypeCombo',
+				deleteEmpty: false,
+				cbind: {
+				    hidden: '{isQemu}',
+				},
+				comboItems: [
+				    ['mp', gettext('Mount Point')],
+				    ['unused', gettext('Unused')],
+				],
+				listeners: { change: 'onMpTypeChange' },
+			    },
+			},
+			{
+			    xtype: 'proxmoxintegerfield',
+			    name: 'mpId',
+			    reference: 'mpIdSelector',
+			    minValue: 0,
+			    flex: 1,
+			    allowBlank: false,
+			    validateOnChange: true,
+			    disabled: true,
+			    bind: {
+				maxValue: '{mpMaxCount}',
+			    },
+			    validator: function(value) {
+				let view = this.up('window');
+				let type = view.getViewModel().get('mpType');
+				if (Ext.isDefined(view.VMConfig[type+value])) {
+				    return "Mount point is already in use.";
+				}
+				return true;
+			    },
+			},
+		    ],
+		},
+	    ],
+	},
+    ],
+
+    initComponent: function() {
+	let me = this;
+
+	if (!me.nodename) {
+	    throw "no node name specified";
+	}
+
+	if (!me.vmid) {
+	    throw "no VM ID specified";
+	}
+
+	if (!me.type) {
+	    throw "no type specified";
+	}
+
+	me.callParent();
+    },
+});
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index 6cea4287..b76814e8 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -406,6 +406,11 @@ Ext.define('PVE.qemu.HardwareView', {
 		return;
 	    }
 
+	    if (rec.data.key.match(/^unused/)) {
+		Ext.Msg.alert('Error', gettext('Not yet supported for unused disks'));
+		return;
+	    }
+
 	    var win = Ext.create('PVE.window.HDMove', {
 		disk: rec.data.key,
 		nodename: nodename,
@@ -417,6 +422,24 @@ Ext.define('PVE.qemu.HardwareView', {
 	    win.on('destroy', me.reload, me);
 	};
 
+	let run_reassign = function() {
+	    let rec = sm.getSelection()[0];
+	    if (!rec) {
+		return;
+	    }
+
+	    Ext.create('PVE.window.HDReassign', {
+		disk: rec.data.key,
+		nodename: nodename,
+		autoShow: true,
+		vmid: vmid,
+		type: 'qemu',
+		listeners: {
+		    destroy: () => me.reload(),
+		},
+	    });
+	};
+
 	var edit_btn = new Proxmox.button.Button({
 	    text: gettext('Edit'),
 	    selModel: sm,
@@ -431,11 +454,36 @@ Ext.define('PVE.qemu.HardwareView', {
 	    handler: run_resize,
 	});
 
-	var move_btn = new Proxmox.button.Button({
+	let move_btn = new PVE.button.Split({
 	    text: gettext('Move disk'),
 	    selModel: sm,
 	    disabled: true,
 	    handler: run_move,
+	    menu: {
+		items: [{
+		    text: gettext('Reassign disk'),
+		    tooltip: gettext('Reassign disk to another VM'),
+		    handler: run_reassign,
+		    iconCls: 'fa fa-mail-forward',
+		}],
+	    },
+	});
+
+	// needed until we can move unused disk to other storages
+	let reassign_btn = new Proxmox.button.Button({
+	    text: gettext('Reassign disk'),
+	    tooltip: gettext('Reassign disk to another VM'),
+	    handler: run_reassign,
+	    selModel: sm,
+	    hidden: true,
+	    listeners: {
+		render: function(btn) {
+		    // hack: avoid layout-flickering due to size change, use max width for both
+		    let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width);
+		    move_btn.setSize({ width: maxWidth });
+		    btn.setSize({ width: maxWidth });
+		},
+	    },
 	});
 
 	var remove_btn = new Proxmox.button.Button({
@@ -613,7 +661,11 @@ Ext.define('PVE.qemu.HardwareView', {
 
 	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
 
-	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi || tpmMoveable) || !diskCap);
+	    reassign_btn.setHidden(!isUnusedDisk);
+	    reassign_btn.setDisabled(!isUnusedDisk);
+	    move_btn.setHidden(isUnusedDisk);
+
+	    move_btn.setDisabled(pending || isCloudInit || !(isDisk || isEfi || tpmMoveable) || !diskCap);
 
 	    revert_btn.setDisabled(!pending);
 	};
@@ -777,6 +829,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		edit_btn,
 		resize_btn,
 		move_btn,
+		reassign_btn,
 		revert_btn,
 	    ],
 	    rows: rows,
-- 
2.30.2

[pve-devel] [PATCH v2 manager 2/3] ui: BusTypeSelector: change noVirtIO to withVirtIO

[Aaron Lauterer]

Double negated properties make it harder than necessary to parse
conditions.

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---
 www/manager6/form/BusTypeSelector.js    | 4 ++--
 www/manager6/form/ControllerSelector.js | 4 ++--
 www/manager6/qemu/CDEdit.js             | 2 +-
 www/manager6/qemu/CIDriveEdit.js        | 2 +-
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/www/manager6/form/BusTypeSelector.js b/www/manager6/form/BusTypeSelector.js
index a420e56f..0f040229 100644
--- a/www/manager6/form/BusTypeSelector.js
+++ b/www/manager6/form/BusTypeSelector.js
@@ -2,7 +2,7 @@ Ext.define('PVE.form.BusTypeSelector', {
     extend: 'Proxmox.form.KVComboBox',
     alias: 'widget.pveBusSelector',
 
-    noVirtIO: false,
+    withVirtIO: true,
     withUnused: false,
 
     initComponent: function() {
@@ -10,7 +10,7 @@ Ext.define('PVE.form.BusTypeSelector', {
 
 	me.comboItems = [['ide', 'IDE'], ['sata', 'SATA']];
 
-	if (!me.noVirtIO) {
+	if (me.withVirtIO) {
 	    me.comboItems.push(['virtio', 'VirtIO Block']);
 	}
 
diff --git a/www/manager6/form/ControllerSelector.js b/www/manager6/form/ControllerSelector.js
index 798dc4b2..d84c49d6 100644
--- a/www/manager6/form/ControllerSelector.js
+++ b/www/manager6/form/ControllerSelector.js
@@ -2,7 +2,7 @@ Ext.define('PVE.form.ControllerSelector', {
     extend: 'Ext.form.FieldContainer',
     alias: 'widget.pveControllerSelector',
 
-    noVirtIO: false,
+    withVirtIO: true,
     withUnused: false,
 
     vmconfig: {}, // used to check for existing devices
@@ -73,7 +73,7 @@ Ext.define('PVE.form.ControllerSelector', {
 		    name: 'controller',
 		    itemId: 'controller',
 		    value: PVE.qemu.OSDefaults.generic.busType,
-		    noVirtIO: me.noVirtIO,
+		    withVirtIO: me.withVirtIO,
 		    withUnused: me.withUnused,
 		    allowBlank: false,
 		    flex: 2,
diff --git a/www/manager6/qemu/CDEdit.js b/www/manager6/qemu/CDEdit.js
index 72c01037..fc7a59cc 100644
--- a/www/manager6/qemu/CDEdit.js
+++ b/www/manager6/qemu/CDEdit.js
@@ -71,7 +71,7 @@ Ext.define('PVE.qemu.CDInputPanel', {
 
 	if (!me.confid) {
 	    me.bussel = Ext.create('PVE.form.ControllerSelector', {
-		noVirtIO: true,
+		withVirtIO: false,
 	    });
 	    items.push(me.bussel);
 	}
diff --git a/www/manager6/qemu/CIDriveEdit.js b/www/manager6/qemu/CIDriveEdit.js
index 754b8353..a9ca8bf1 100644
--- a/www/manager6/qemu/CIDriveEdit.js
+++ b/www/manager6/qemu/CIDriveEdit.js
@@ -36,7 +36,7 @@ Ext.define('PVE.qemu.CIDriveInputPanel', {
 	me.items = [
 	    {
 		xtype: 'pveControllerSelector',
-		noVirtIO: true,
+		withVirtIO: false,
 		itemId: 'drive',
 		fieldLabel: gettext('CloudInit Drive'),
 		name: 'drive',
-- 
2.30.2

[pve-devel] [PATCH v2 manager] ui: hdmove: modernize/refactor

[Aaron Lauterer]

Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
---

changes since v1: much of the feedback to the HDReassign.js from the
first patch has been incorporated here as well.

* reducing predefined cbind values for more arrow functions
* using more arrow functions in general
* template strings

 www/manager6/qemu/HDMove.js       | 239 +++++++++++++++++-------------
 www/manager6/qemu/HardwareView.js |   1 +
 2 files changed, 134 insertions(+), 106 deletions(-)

diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
index 181b7bdc..210e6ce8 100644
--- a/www/manager6/qemu/HDMove.js
+++ b/www/manager6/qemu/HDMove.js
@@ -1,48 +1,147 @@
 Ext.define('PVE.window.HDMove', {
     extend: 'Ext.window.Window',
+    mixins: ['Proxmox.Mixin.CBind'],
 
     resizable: false,
+    modal: true,
+    width: 350,
+    border: false,
+    layout: 'fit',
+
+    cbindData: function() {
+	let me = this;
+	return {
+	    isQemu: me.type === 'qemu',
+	    disk: me.disk,
+	    nodename: me.nodename,
+	};
+    },
 
+    cbind: {
+	title: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
+    },
 
-    move_disk: function(disk, storage, format, delete_disk) {
-	var me = this;
-	var qemu = me.type === 'qemu';
-	var params = {};
-	params.storage = storage;
-	params[qemu ? 'disk':'volume'] = disk;
-
-	if (format && qemu) {
-	    params.format = format;
-	}
-
-	if (delete_disk) {
-	    params.delete = 1;
-	}
+    controller: {
+	xclass: 'Ext.app.ViewController',
+
+	move_disk: function(disk, storage, format, delete_disk) {
+	    let me = this;
+	    let view = me.getView();
+	    let qemu = view.type === 'qemu';
+	    let params = {
+		storage: storage,
+	    };
+	    params[qemu ? 'disk':'volume'] = disk;
+
+	    if (format && qemu) {
+		params.format = format;
+	    }
+
+	    if (delete_disk) {
+		params.delete = 1;
+	    }
+
+	    let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`;
+	    url += qemu ? 'move_disk' : 'move_volume';
+
+	    Proxmox.Utils.API2Request({
+		params: params,
+		url: url,
+		waitMsgTarget: me.getView(),
+		method: 'POST',
+		failure: function(response, opts) {
+		    Ext.Msg.alert('Error', response.htmlStatus);
+		},
+		success: function(response, options) {
+		    let upid = response.result.data;
+		    Ext.create('Proxmox.window.TaskViewer', {
+			upid: upid,
+			autoShow: true,
+			listeners: {
+			    destroy: () => view.close(),
+			},
+		    });
+		},
+	    });
+	},
+
+	onMoveClick: function() {
+		let me = this;
+		let view = me.getView();
+		let form = me.lookup('moveFormPanel').getForm();
+		if (form.isValid()) {
+		    let values = form.getValues();
+		    me.move_disk(view.disk, values.hdstorage, values.diskformat,
+				 values.deleteDisk);
+		}
+	},
 
-	var url = '/nodes/' + me.nodename + '/' + me.type + '/' + me.vmid + '/';
-	url += qemu ? 'move_disk' : 'move_volume';
+	validateForm: function(fp, isValid) {
+	    this.getView().lookup('submitButton').setDisabled(!isValid);
+	},
+    },
 
-	Proxmox.Utils.API2Request({
-	    params: params,
-	    url: url,
-	    waitMsgTarget: me,
-	    method: 'POST',
-	    failure: function(response, opts) {
-		Ext.Msg.alert('Error', response.htmlStatus);
+    buttons: [
+	{
+	    xtype: 'button',
+	    reference: 'submitButton',
+	    cbind: {
+		text: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
 	    },
-	    success: function(response, options) {
-		var upid = response.result.data;
-		var win = Ext.create('Proxmox.window.TaskViewer', {
-		    upid: upid,
-		});
-		win.show();
-		win.on('destroy', function() { me.close(); });
+	    handler: 'onMoveClick',
+	    disabled: true,
+	},
+    ],
+
+    items: [
+	{
+	    xtype: 'form',
+	    reference: 'moveFormPanel',
+	    bodyPadding: 10,
+	    border: false,
+	    fieldDefaults: {
+		labelWidth: 100,
+		anchor: '100%',
 	    },
-	});
-    },
+	    listeners: {
+		validitychange: 'validateForm',
+	    },
+	    items: [
+		{
+		    xtype: 'displayfield',
+		    cbind: {
+			name: get => get('isQemu') ? 'disk' : 'volume',
+			fieldLabel: get => get('isQemu')
+			    ? gettext('Disk')
+			    : gettext('Mount Point'),
+			value: '{disk}',
+		    },
+		    vtype: 'StorageId',
+		    allowBlank: false,
+		},
+		{
+		    xtype: 'pveDiskStorageSelector',
+		    storageLabel: gettext('Target Storage'),
+		    cbind: {
+			nodename: '{nodename}',
+			storageContent: get => get('isQemu') ? 'images' : 'rootdir',
+			hideFormat: get => get('disk') === 'tpmstate0',
+		    },
+		    hideSize: true,
+		},
+		{
+		    xtype: 'proxmoxcheckbox',
+		    fieldLabel: gettext('Delete source'),
+		    name: 'deleteDisk',
+		    uncheckedValue: 0,
+		    checked: false,
+		},
+	    ],
+	},
+    ],
 
     initComponent: function() {
-	var me = this;
+	let me = this;
 
 	if (!me.nodename) {
 	    throw "no node name specified";
@@ -53,81 +152,9 @@ Ext.define('PVE.window.HDMove', {
 	}
 
 	if (!me.type) {
-	    me.type = 'qemu';
+	    throw "no type specified";
 	}
 
-	var qemu = me.type === 'qemu';
-
-        var items = [
-            {
-                xtype: 'displayfield',
-                name: qemu ? 'disk' : 'volume',
-                value: me.disk,
-                fieldLabel: qemu ? gettext('Disk') : gettext('Mount Point'),
-                vtype: 'StorageId',
-                allowBlank: false,
-            },
-        ];
-
-	items.push({
-	    xtype: 'pveDiskStorageSelector',
-	    storageLabel: gettext('Target Storage'),
-	    nodename: me.nodename,
-	    storageContent: qemu ? 'images' : 'rootdir',
-	    hideSize: true,
-	    hideFormat: me.disk === 'tpmstate0',
-	});
-
-	items.push({
-	    xtype: 'proxmoxcheckbox',
-	    fieldLabel: gettext('Delete source'),
-	    name: 'deleteDisk',
-	    uncheckedValue: 0,
-	    checked: false,
-	});
-
-	me.formPanel = Ext.create('Ext.form.Panel', {
-	    bodyPadding: 10,
-	    border: false,
-	    fieldDefaults: {
-		labelWidth: 100,
-		anchor: '100%',
-	    },
-	    items: items,
-	});
-
-	var form = me.formPanel.getForm();
-
-	var submitBtn;
-
-	me.title = qemu ? gettext("Move disk") : gettext('Move Volume');
-	submitBtn = Ext.create('Ext.Button', {
-	    text: me.title,
-	    handler: function() {
-		if (form.isValid()) {
-		    var values = form.getValues();
-		    me.move_disk(me.disk, values.hdstorage, values.diskformat,
-				 values.deleteDisk);
-		}
-	    },
-	});
-
-	Ext.apply(me, {
-	    modal: true,
-	    width: 350,
-	    border: false,
-	    layout: 'fit',
-	    buttons: [submitBtn],
-	    items: [me.formPanel],
-	});
-
-
 	me.callParent();
-
-	me.mon(me.formPanel, 'validitychange', function(fp, isValid) {
-	    submitBtn.setDisabled(!isValid);
-	});
-
-	me.formPanel.isValid();
     },
 });
diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
index b76814e8..54946651 100644
--- a/www/manager6/qemu/HardwareView.js
+++ b/www/manager6/qemu/HardwareView.js
@@ -415,6 +415,7 @@ Ext.define('PVE.qemu.HardwareView', {
 		disk: rec.data.key,
 		nodename: nodename,
 		vmid: vmid,
+		type: 'qemu',
 	    });
 
 	    win.show();
-- 
2.30.2

Re: [pve-devel] [PATCH v2 manager 0/3] ui: lxc/qemu: add reassign for disks and volumes

[Aaron Lauterer]

ping? Patches should still apply fine

On 11/15/21 16:02, Aaron Lauterer wrote:
> This series adds the UI for the recently applied backend series that
> allows to reassign a disk / volume from one guest to another.
> 
> Patch 2 and 3 are optional. Patch 2 only changes a double negated option
> and patch 3 happend in the process of working on an interface for the
> reassign functionality. Since the work of modernizing this componend is
> done, why not use it
> 
> v2: incorporated feedback I got off list, mainly
> * using more modern approaches
> * more arrow functions
> * reducing use of predefined cbind values and using inline functions
>    when possible
> 
> 
> Aaron Lauterer (3):
>    ui: lxc/qemu: add disk reassign
>    ui: BusTypeSelector: change noVirtIO to withVirtIO
>    ui: hdmove: modernize/refactor
> 
>   www/manager6/Makefile                   |   1 +
>   www/manager6/form/BusTypeSelector.js    |   4 +-
>   www/manager6/form/ControllerSelector.js |   4 +-
>   www/manager6/lxc/Resources.js           |  62 ++++-
>   www/manager6/qemu/CDEdit.js             |   2 +-
>   www/manager6/qemu/CIDriveEdit.js        |   2 +-
>   www/manager6/qemu/HDMove.js             | 239 ++++++++++--------
>   www/manager6/qemu/HDReassign.js         | 316 ++++++++++++++++++++++++
>   www/manager6/qemu/HardwareView.js       |  58 ++++-
>   9 files changed, 572 insertions(+), 116 deletions(-)
>   create mode 100644 www/manager6/qemu/HDReassign.js
> 

Re: [pve-devel] [PATCH v2 manager 1/3] ui: lxc/qemu: add disk reassign

[Dominik Csapak]

sorry for the late review

some comments inline

On 11/15/21 16:02, Aaron Lauterer wrote:
> For the new HDReassign component, we follow the approach of HDMove to
> have one componend for qemu and lxc as they are the same for most parts.
> 
> The 'Move disk/volume' button is now a SplitButton which has the
> 'Reassign' button as menu item.  In the lxc resource panel the menu item
> is defined extra so we can easily disable it, should the selected mp be
> the rootfs.
> 
> In the lxc resource and qemu hardware panel we currently add a new
> button to handle unused disks/volumes. The button is "switched" with the
> 'Move' in this case. The width of the buttons is aligned to avoid
> movement of other buttons.
> 
> Once we are able to also move unused disks/volumes to other storages, we
> can remove this.
> 
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> Not all cbind values can be omitted AFAICT as we do not have access to
> the component context when declaring our items. From what I know, we can
> use regular bind, cbind (if we only need to set the value once) or set
> the values manually in the initComponent.
> 
> Triggering the validation of the mountpoint integerfield when the mount
> point type changes (onMpTypeChange) is happening because it is not done
> so automatically but necessary as the other MP type (e.g. unused) could
> already be in use with that number. There might be a better way that I
> am not aware of.
> 
> 
> changes since v1: incorporated feedback I got off list
> 
> * use more modern approaches
>      * arrow functions
>      * autoShow
>      * template strings
> * reduce predefined cbind values and use arrow functions in the cbind
>    directly in many cases
> * some code style issues and cleanup
> 
>   www/manager6/Makefile             |   1 +
>   www/manager6/lxc/Resources.js     |  62 +++++-
>   www/manager6/qemu/HDReassign.js   | 316 ++++++++++++++++++++++++++++++
>   www/manager6/qemu/HardwareView.js |  57 +++++-
>   4 files changed, 432 insertions(+), 4 deletions(-)
>   create mode 100644 www/manager6/qemu/HDReassign.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index e6e01bd1..94a78d89 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -214,6 +214,7 @@ JSSRC= 							\
>   	qemu/HDTPM.js					\
>   	qemu/HDMove.js					\
>   	qemu/HDResize.js				\
> +	qemu/HDReassign.js				\
>   	qemu/HardwareView.js				\
>   	qemu/IPConfigEdit.js				\
>   	qemu/KeyboardEdit.js				\
> diff --git a/www/manager6/lxc/Resources.js b/www/manager6/lxc/Resources.js
> index 15ee3c67..bec7cf14 100644
> --- a/www/manager6/lxc/Resources.js
> +++ b/www/manager6/lxc/Resources.js
> @@ -156,6 +156,11 @@ Ext.define('PVE.lxc.RessourceView', {
>   		return;
>   	    }
>   
> +	    if (rec.data.key.match(/^unused/)) {
> +		Ext.Msg.alert('Error', gettext('Not yet supported for unused volumes'));
> +		return;
> +	    }
> +

since we already hide/disable that button accordingly, why have that
error message at all? if the user somehow shows the button via the
browser console, the api will return an error anyway..

that way, we'd save a gettext

>   	    var win = Ext.create('PVE.window.HDMove', {
>   		disk: rec.data.key,
>   		nodename: nodename,
> @@ -168,6 +173,24 @@ Ext.define('PVE.lxc.RessourceView', {
>   	    win.on('destroy', me.reload, me);
>   	};
>   
> +	let run_reassign = function() {
> +	    let rec = me.selModel.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    Ext.create('PVE.window.HDReassign', {
> +		disk: rec.data.key,
> +		nodename: nodename,
> +		autoShow: true,
> +		vmid: vmid,
> +		type: 'lxc',
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    });
> +	};
> +
>   	var edit_btn = new Proxmox.button.Button({
>   	    text: gettext('Edit'),
>   	    selModel: me.selModel,
> @@ -227,12 +250,40 @@ Ext.define('PVE.lxc.RessourceView', {
>   	    },
>   	});
>   
> -	var move_btn = new Proxmox.button.Button({
> +	let reassign_menuitem = new Ext.menu.Item({
> +	    text: gettext('Reassign volume'),
> +	    tooltip: gettext('Reassign volume to another VM'),
> +	    handler: run_reassign,
> +	    iconCls: 'fa fa-mail-forward',
> +	    reference: 'reassing_item',
> +	});
> +
> +	let move_btn = new PVE.button.Split({
>   	    text: gettext('Move Volume'),
>   	    selModel: me.selModel,
>   	    disabled: true,
>   	    dangerous: true,
>   	    handler: run_move,
> +	    menu: {
> +		items: [reassign_menuitem],
> +	    },
> +	});
> +
> +	// needed until we can move unused volumes to other storages
> +	let reassign_btn = new Proxmox.button.Button({
> +	    text: gettext('Reassign volume'),
> +	    tooltip: gettext('Reassign volume to another VM'),
> +	    handler: run_reassign,
> +	    selModel: me.selModel,
> +	    hidden: true,
> +	    listeners: {
> +		render: function(btn) {
> +		    // hack: avoid layout-flickering due to size change, use max width for both
> +		    let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width);
> +		    move_btn.setSize({ width: maxWidth });
> +		    btn.setSize({ width: maxWidth });

mhmm... while for remove/detach disks this seems to work ok,
for this the gettext are so different, the reassign button
look very weird..

maybe putting both in a menu (where the top button itself has
no action) and disable accordingly would make more sense here?

i.e.
Volume/Disk Actions
  - Resize
  - Move
  - relocate

?

> +		},
> +	    },
>   	});
>   
>   	var revert_btn = new PVE.button.PendingRevert();
> @@ -253,6 +304,7 @@ Ext.define('PVE.lxc.RessourceView', {
>   
>   	    var pending = rec.data.delete || me.hasPendingChanges(key);
>   	    var isDisk = rowdef.tdCls === 'pve-itype-icon-storage';
> +	    let isRootFS = rec.data.key === 'rootfs';
>   	    var isUnusedDisk = key.match(/^unused\d+/);
>   	    var isUsedDisk = isDisk && !isUnusedDisk;
>   
> @@ -265,7 +317,12 @@ Ext.define('PVE.lxc.RessourceView', {
>   	    }
>   	    edit_btn.setDisabled(noedit);
>   
> -	    remove_btn.setDisabled(!isDisk || rec.data.key === 'rootfs' || !diskCap || pending);
> +	    reassign_btn.setHidden(!isUnusedDisk);
> +	    reassign_btn.setDisabled(!isUnusedDisk);
> +	    move_btn.setHidden(isUnusedDisk);
> +	    reassign_menuitem.setDisabled(isRootFS);
> +
> +	    remove_btn.setDisabled(!isDisk || isRootFS || !diskCap || pending);
>   	    resize_btn.setDisabled(!isDisk || !diskCap || isUnusedDisk);
>   	    move_btn.setDisabled(!isDisk || !diskCap);
>   	    revert_btn.setDisabled(!pending);
> @@ -329,6 +386,7 @@ Ext.define('PVE.lxc.RessourceView', {
>   		remove_btn,
>   		resize_btn,
>   		move_btn,
> +		reassign_btn,
>   		revert_btn,
>   	    ],
>   	    rows: rows,
> diff --git a/www/manager6/qemu/HDReassign.js b/www/manager6/qemu/HDReassign.js
> new file mode 100644
> index 00000000..e6b59cb4
> --- /dev/null
> +++ b/www/manager6/qemu/HDReassign.js
> @@ -0,0 +1,316 @@
> +Ext.define('PVE.window.HDReassign', {
> +    extend: 'Ext.window.Window',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +
> +    viewModel: {
> +	data: {
> +	    mpType: '',
> +	},
> +	formulas: {
> +	    mpMaxCount: get => get('mpType') === 'mp'
> +		? PVE.Utils.mp_counts.mps - 1
> +		: PVE.Utils.mp_counts.unused - 1,
> +	},
> +    },
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    vmid: me.vmid,
> +	    disk: me.disk,
> +	    isQemu: me.type === 'qemu',
> +	    nodename: me.nodename,
> +	};
> +    },
> +
> +    cbind: {
> +	title: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
> +	qemu: '{isQemu}',
> +    },
> +
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	initViewModel: function(model) {
> +	    let view = this.getView();
> +	    let mpTypeValue = view.disk.match(/^unused\d+/) ? 'unused' : 'mp';
> +	    model.set('mpType', mpTypeValue);
> +	},
> +
> +	reassign_disk: function(values) {

this should probably be 'reassignDisk' according
to our usual naming scheme

> +	    let me = this;
> +	    let view = me.getView();
> +	    let qemu = view.qemu;
> +	    let params = {
> +		vmid: view.vmid,
> +		'target-vmid': values.targetVmid,
> +	    };
> +
> +	    params[qemu ? 'disk' : 'volume'] = view.disk;
> +
> +	    if (view.qemu) {
> +		params['target-disk'] = `${values.controller}${values.deviceid}`;
> +	    } else {
> +		params['target-volume'] = `${values.mpType}${values.mpId}`;
> +	    }
> +
> +	    let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`;
> +	    url += qemu ? 'move_disk' : 'move_volume';
> +
> +	    Proxmox.Utils.API2Request({
> +		params: params,
> +		url: url,
> +		waitMsgTarget: me.getView(),
> +		method: 'POST',
> +		failure: response => Ext.Msg.alert('Error', response.htmlStatus),
> +		success: function(response, options) {
> +		    let upid = response.result.data;
> +		    view.hide();
> +		    Ext.create('Proxmox.window.TaskProgress', {
> +			upid,
> +			autoShow: true,
> +			taskDone: success => success ? view.close() : view.show(),

so if the task is not successful, we show ourselves again
(over the error of the progress window)?
what about the task log window (if opened)?

> +		    });
> +		},
> +	    });
> +	},
> +
> +	validateForm: function(fp, isValid) {
> +	    this.getView().lookup('submitButton').setDisabled(!isValid);
> +	},
> +
> +	onReassignClick: function() {
> +		let me = this;
> +		let view = me.getView();
> +		let form = view.lookup('moveFormPanel').getForm();
> +		if (form.isValid()) {
> +		    me.reassign_disk(form.getValues());
> +		}
> +	},
> +
> +	onMpTypeChange: function(value) {
> +	    this.getView().getViewModel().set('mpType', value.getValue());
> +	    this.getView().lookup('mpIdSelector').validate();
> +	},

these functions make me question if it wouldn't have been easier
to use an 'edit window' instead? any reason for that?

> +
> +	onTargetVMChange: function(f, vmid) {
> +	    let me = this;
> +	    let view = me.getView();
> +	    let diskSelector = view.lookup('diskSelector');
> +	    if (!vmid) {
> +		diskSelector.setVMConfig(null);
> +		me.VMConfig = null;
> +		return;
> +	    }
> +
> +	    let type = view.type === 'qemu' ? 'qemu' : 'lxc';

here you use 'view.type' but above you use 'view.qemu' ?
i'd prefer to have a consistent way of checking the type

> +
> +	    let url = `/nodes/${view.nodename}/${type}/${vmid}/config`;
> +	    Proxmox.Utils.API2Request({
> +		url: url,
> +		method: 'GET',
> +		failure: function(response, opts) {
> +		    Ext.Msg.alert('Error', response.htmlStatus);
> +		},
> +		success: function(response, options) {
> +		    if (type === 'qemu') {

same here

> +			diskSelector.setVMConfig(response.result.data);
> +			diskSelector.setDisabled(false);
> +		    } else {
> +			let mpIdSelector = view.lookup('mpIdSelector');
> +			let mpType = view.lookup('mpType');
> +
> +			view.VMConfig = response.result.data;
> +
> +			PVE.Utils.forEachMP((bus, i) => {
> +			    let name = view.getViewModel().get('mpType') + i.toString();
> +			    if (!Ext.isDefined(view.VMConfig[name])) {
> +				mpIdSelector.setValue(i);
> +				return false;
> +			    }
> +			    return undefined;
> +			});
> +
> +			mpType.setDisabled(false);
> +			mpIdSelector.setDisabled(false);
> +			mpIdSelector.validate();
> +		    }
> +		},
> +	    });
> +	},
> +    },
> +
> +    buttons: [
> +	{
> +	    xtype: 'button',
> +	    reference: 'submitButton',
> +	    cbind: {
> +		text: get => get('isQemu') ? gettext('Reassign disk') : gettext('Reassign volume'),
> +	    },
> +	    handler: 'onReassignClick',
> +	    disabled: true,
> +	},
> +    ],
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
> +	    },
> +	    listeners: {
> +		validitychange: 'validateForm',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'displayfield',
> +		    name: 'sourceDisk',
> +		    cbind: {
> +			name: get => get('isQemu') ? 'disk' : 'volume',
> +			fieldLabel: get => get('isQemu')
> +			    ? gettext('Source Disk')
> +			    : gettext('Source Mount Point'),

wouldn't 'Source' suffice for both? IMHO the context is enough
that the user knows that it's a disk/mp

> +			value: '{disk}',
> +		    },
> +		    vtype: 'StorageId',
> +		    allowBlank: false,
> +		},
> +		{
> +		    xtype: 'vmComboSelector',
> +		    reference: 'targetVMID',
> +		    name: 'targetVmid',
> +		    allowBlank: false,
> +		    bind: {
> +			value: '{targetVMID}',
> +		    },
> +		    cbind: {
> +			fieldLabel: get => get('isQemu') ? gettext('Target VM') : gettext('Target CT'),

same here but with 'Target'

> +		    },
> +		    store: {
> +			model: 'PVEResources',
> +			autoLoad: true,
> +			sorters: 'vmid',
> +			cbind: {}, // for nested cbinds
> +			filters: [
> +			    {
> +				property: 'type',
> +				cbind: {
> +				    value: get => get('isQemu') ? /qemu/ : /lxc/,
> +				},
> +			    },
> +			    {
> +				property: 'node',
> +				cbind: {
> +				    value: '{nodename}',
> +				},
> +			    },
> +			    {
> +				property: 'vmid',
> +				operator: '!=',
> +				cbind: {
> +				    value: '{vmid}',
> +				},
> +			    },
> +			],
> +		    },
> +		    listeners: { change: 'onTargetVMChange' },
> +		},
> +		{
> +		    xtype: 'pveControllerSelector',
> +		    reference: 'diskSelector',
> +		    withUnused: true,
> +		    disabled: true,
> +		    cbind: {
> +			hidden: '{!isQemu}',
> +		    },
> +		},
> +		{
> +		    xtype: 'container',
> +		    layout: 'hbox',
> +		    cbind: {
> +			hidden: '{isQemu}',
> +			disabled: '{isQemu}',
> +		    },
> +		    items: [
> +			{
> +			    xtype: 'pmxDisplayEditField',
> +			    cbind: {
> +				editable: get => !get('disk').match(/^unused\d+/),
> +				value: get => get('disk').match(/^unused\d+/) ? 'unused' : 'mp',
> +			    },
> +			    disabled: true,
> +			    name: 'mpType',
> +			    reference: 'mpType',
> +			    fieldLabel: gettext('Add as'),
> +			    submitValue: true,
> +			    flex: 4,
> +			    editConfig: {
> +				xtype: 'proxmoxKVComboBox',
> +				name: 'mpTypeCombo',
> +				reference: 'mpTypeCombo',
> +				deleteEmpty: false,
> +				cbind: {
> +				    hidden: '{isQemu}',
> +				},
> +				comboItems: [
> +				    ['mp', gettext('Mount Point')],
> +				    ['unused', gettext('Unused')],
> +				],
> +				listeners: { change: 'onMpTypeChange' },
> +			    },
> +			},
> +			{
> +			    xtype: 'proxmoxintegerfield',
> +			    name: 'mpId',
> +			    reference: 'mpIdSelector',
> +			    minValue: 0,
> +			    flex: 1,
> +			    allowBlank: false,
> +			    validateOnChange: true,
> +			    disabled: true,
> +			    bind: {
> +				maxValue: '{mpMaxCount}',
> +			    },
> +			    validator: function(value) {
> +				let view = this.up('window');
> +				let type = view.getViewModel().get('mpType');
> +				if (Ext.isDefined(view.VMConfig[type+value])) {
> +				    return "Mount point is already in use.";
> +				}
> +				return true;
> +			    },
> +			},
> +		    ],
> +		},
> +	    ],
> +	},
> +    ],
> +
> +    initComponent: function() {
> +	let me = this;
> +
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +
> +	if (!me.vmid) {
> +	    throw "no VM ID specified";
> +	}
> +
> +	if (!me.type) {
> +	    throw "no type specified";
> +	}
> +
> +	me.callParent();
> +    },
> +});
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index 6cea4287..b76814e8 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -406,6 +406,11 @@ Ext.define('PVE.qemu.HardwareView', {
>   		return;
>   	    }
>   
> +	    if (rec.data.key.match(/^unused/)) {
> +		Ext.Msg.alert('Error', gettext('Not yet supported for unused disks'));
> +		return;
> +	    }
> +

same question as with containers

>   	    var win = Ext.create('PVE.window.HDMove', {
>   		disk: rec.data.key,
>   		nodename: nodename,
> @@ -417,6 +422,24 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    win.on('destroy', me.reload, me);
>   	};
>   
> +	let run_reassign = function() {
> +	    let rec = sm.getSelection()[0];
> +	    if (!rec) {
> +		return;
> +	    }
> +
> +	    Ext.create('PVE.window.HDReassign', {
> +		disk: rec.data.key,
> +		nodename: nodename,
> +		autoShow: true,
> +		vmid: vmid,
> +		type: 'qemu',
> +		listeners: {
> +		    destroy: () => me.reload(),
> +		},
> +	    });
> +	};
> +
>   	var edit_btn = new Proxmox.button.Button({
>   	    text: gettext('Edit'),
>   	    selModel: sm,
> @@ -431,11 +454,36 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    handler: run_resize,
>   	});
>   
> -	var move_btn = new Proxmox.button.Button({
> +	let move_btn = new PVE.button.Split({
>   	    text: gettext('Move disk'),
>   	    selModel: sm,
>   	    disabled: true,
>   	    handler: run_move,
> +	    menu: {
> +		items: [{
> +		    text: gettext('Reassign disk'),
> +		    tooltip: gettext('Reassign disk to another VM'),
> +		    handler: run_reassign,
> +		    iconCls: 'fa fa-mail-forward',
> +		}],
> +	    },
> +	});
> +
> +	// needed until we can move unused disk to other storages
> +	let reassign_btn = new Proxmox.button.Button({
> +	    text: gettext('Reassign disk'),
> +	    tooltip: gettext('Reassign disk to another VM'),
> +	    handler: run_reassign,
> +	    selModel: sm,
> +	    hidden: true,
> +	    listeners: {
> +		render: function(btn) {
> +		    // hack: avoid layout-flickering due to size change, use max width for both
> +		    let maxWidth = Math.max(btn.getSize().width, move_btn.getSize().width);
> +		    move_btn.setSize({ width: maxWidth });
> +		    btn.setSize({ width: maxWidth });
> +		},
> +	    },
>   	});
>   
>   	var remove_btn = new Proxmox.button.Button({
> @@ -613,7 +661,11 @@ Ext.define('PVE.qemu.HardwareView', {
>   
>   	    resize_btn.setDisabled(pending || !isUsedDisk || !diskCap);
>   
> -	    move_btn.setDisabled(pending || !(isUsedDisk || isEfi || tpmMoveable) || !diskCap);
> +	    reassign_btn.setHidden(!isUnusedDisk);
> +	    reassign_btn.setDisabled(!isUnusedDisk);
> +	    move_btn.setHidden(isUnusedDisk);
> +
> +	    move_btn.setDisabled(pending || isCloudInit || !(isDisk || isEfi || tpmMoveable) || !diskCap);
>   
>   	    revert_btn.setDisabled(!pending);
>   	};
> @@ -777,6 +829,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		edit_btn,
>   		resize_btn,
>   		move_btn,
> +		reassign_btn,
>   		revert_btn,
>   	    ],
>   	    rows: rows,

Re: [pve-devel] [PATCH v2 manager] ui: hdmove: modernize/refactor

[Dominik Csapak]

some comments inline

On 11/15/21 16:02, Aaron Lauterer wrote:
> Signed-off-by: Aaron Lauterer <a.lauterer@proxmox.com>
> ---
> 
> changes since v1: much of the feedback to the HDReassign.js from the
> first patch has been incorporated here as well.
> 
> * reducing predefined cbind values for more arrow functions
> * using more arrow functions in general
> * template strings
> 
>   www/manager6/qemu/HDMove.js       | 239 +++++++++++++++++-------------
>   www/manager6/qemu/HardwareView.js |   1 +
>   2 files changed, 134 insertions(+), 106 deletions(-)
> 
> diff --git a/www/manager6/qemu/HDMove.js b/www/manager6/qemu/HDMove.js
> index 181b7bdc..210e6ce8 100644
> --- a/www/manager6/qemu/HDMove.js
> +++ b/www/manager6/qemu/HDMove.js
> @@ -1,48 +1,147 @@
>   Ext.define('PVE.window.HDMove', {
>       extend: 'Ext.window.Window',
> +    mixins: ['Proxmox.Mixin.CBind'],
>   
>       resizable: false,
> +    modal: true,
> +    width: 350,
> +    border: false,
> +    layout: 'fit',
> +
> +    cbindData: function() {
> +	let me = this;
> +	return {
> +	    isQemu: me.type === 'qemu',
> +	    disk: me.disk,
> +	    nodename: me.nodename,
> +	};
> +    },
>   
> +    cbind: {
> +	title: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
> +    },
>   
> -    move_disk: function(disk, storage, format, delete_disk) {
> -	var me = this;
> -	var qemu = me.type === 'qemu';
> -	var params = {};
> -	params.storage = storage;
> -	params[qemu ? 'disk':'volume'] = disk;
> -
> -	if (format && qemu) {
> -	    params.format = format;
> -	}
> -
> -	if (delete_disk) {
> -	    params.delete = 1;
> -	}
> +    controller: {
> +	xclass: 'Ext.app.ViewController',
> +
> +	move_disk: function(disk, storage, format, delete_disk) {

as with the other patch, this should probably be 'moveDisk'

> +	    let me = this;
> +	    let view = me.getView();
> +	    let qemu = view.type === 'qemu';
> +	    let params = {
> +		storage: storage,
> +	    };
> +	    params[qemu ? 'disk':'volume'] = disk;
> +
> +	    if (format && qemu) {
> +		params.format = format;
> +	    }
> +
> +	    if (delete_disk) {
> +		params.delete = 1;
> +	    }
> +
> +	    let url = `/nodes/${view.nodename}/${view.type}/${view.vmid}/`;
> +	    url += qemu ? 'move_disk' : 'move_volume';
> +
> +	    Proxmox.Utils.API2Request({
> +		params: params,
> +		url: url,
> +		waitMsgTarget: me.getView(),
> +		method: 'POST',
> +		failure: function(response, opts) {
> +		    Ext.Msg.alert('Error', response.htmlStatus);
> +		},
> +		success: function(response, options) {
> +		    let upid = response.result.data;
> +		    Ext.create('Proxmox.window.TaskViewer', {
> +			upid: upid,
> +			autoShow: true,
> +			listeners: {
> +			    destroy: () => view.close(),
> +			},
> +		    });
> +		},
> +	    });
> +	},
> +
> +	onMoveClick: function() {
> +		let me = this;
> +		let view = me.getView();
> +		let form = me.lookup('moveFormPanel').getForm();
> +		if (form.isValid()) {
> +		    let values = form.getValues();
> +		    me.move_disk(view.disk, values.hdstorage, values.diskformat,
> +				 values.deleteDisk);
> +		}
> +	},
>   
> -	var url = '/nodes/' + me.nodename + '/' + me.type + '/' + me.vmid + '/';
> -	url += qemu ? 'move_disk' : 'move_volume';
> +	validateForm: function(fp, isValid) {

imho the name is confusing, since it does not actually
validate the form, but is called after the form was
(succesfully or not) validated

> +	    this.getView().lookup('submitButton').setDisabled(!isValid);
> +	},
> +    },
>   
> -	Proxmox.Utils.API2Request({
> -	    params: params,
> -	    url: url,
> -	    waitMsgTarget: me,
> -	    method: 'POST',
> -	    failure: function(response, opts) {
> -		Ext.Msg.alert('Error', response.htmlStatus);
> +    buttons: [
> +	{
> +	    xtype: 'button',
> +	    reference: 'submitButton',
> +	    cbind: {
> +		text: get => get('isQemu') ? gettext("Move disk") : gettext('Move Volume'),
>   	    },
> -	    success: function(response, options) {
> -		var upid = response.result.data;
> -		var win = Ext.create('Proxmox.window.TaskViewer', {
> -		    upid: upid,
> -		});
> -		win.show();
> -		win.on('destroy', function() { me.close(); });
> +	    handler: 'onMoveClick',
> +	    disabled: true,
> +	},
> +    ],
> +
> +    items: [
> +	{
> +	    xtype: 'form',
> +	    reference: 'moveFormPanel',
> +	    bodyPadding: 10,
> +	    border: false,
> +	    fieldDefaults: {
> +		labelWidth: 100,
> +		anchor: '100%',
>   	    },
> -	});
> -    },
> +	    listeners: {
> +		validitychange: 'validateForm',
> +	    },
> +	    items: [
> +		{
> +		    xtype: 'displayfield',
> +		    cbind: {
> +			name: get => get('isQemu') ? 'disk' : 'volume',
> +			fieldLabel: get => get('isQemu')
> +			    ? gettext('Disk')
> +			    : gettext('Mount Point'),
> +			value: '{disk}',
> +		    },
> +		    vtype: 'StorageId',
> +		    allowBlank: false,
> +		},
> +		{
> +		    xtype: 'pveDiskStorageSelector',
> +		    storageLabel: gettext('Target Storage'),
> +		    cbind: {
> +			nodename: '{nodename}',
> +			storageContent: get => get('isQemu') ? 'images' : 'rootdir',
> +			hideFormat: get => get('disk') === 'tpmstate0',
> +		    },
> +		    hideSize: true,
> +		},
> +		{
> +		    xtype: 'proxmoxcheckbox',
> +		    fieldLabel: gettext('Delete source'),
> +		    name: 'deleteDisk',
> +		    uncheckedValue: 0,
> +		    checked: false,
> +		},
> +	    ],
> +	},
> +    ],
>   
>       initComponent: function() {
> -	var me = this;
> +	let me = this;
>   
>   	if (!me.nodename) {
>   	    throw "no node name specified";
> @@ -53,81 +152,9 @@ Ext.define('PVE.window.HDMove', {
>   	}
>   
>   	if (!me.type) {
> -	    me.type = 'qemu';
> +	    throw "no type specified";
>   	}
>   
> -	var qemu = me.type === 'qemu';
> -
> -        var items = [
> -            {
> -                xtype: 'displayfield',
> -                name: qemu ? 'disk' : 'volume',
> -                value: me.disk,
> -                fieldLabel: qemu ? gettext('Disk') : gettext('Mount Point'),
> -                vtype: 'StorageId',
> -                allowBlank: false,
> -            },
> -        ];
> -
> -	items.push({
> -	    xtype: 'pveDiskStorageSelector',
> -	    storageLabel: gettext('Target Storage'),
> -	    nodename: me.nodename,
> -	    storageContent: qemu ? 'images' : 'rootdir',
> -	    hideSize: true,
> -	    hideFormat: me.disk === 'tpmstate0',
> -	});
> -
> -	items.push({
> -	    xtype: 'proxmoxcheckbox',
> -	    fieldLabel: gettext('Delete source'),
> -	    name: 'deleteDisk',
> -	    uncheckedValue: 0,
> -	    checked: false,
> -	});
> -
> -	me.formPanel = Ext.create('Ext.form.Panel', {
> -	    bodyPadding: 10,
> -	    border: false,
> -	    fieldDefaults: {
> -		labelWidth: 100,
> -		anchor: '100%',
> -	    },
> -	    items: items,
> -	});
> -
> -	var form = me.formPanel.getForm();
> -
> -	var submitBtn;
> -
> -	me.title = qemu ? gettext("Move disk") : gettext('Move Volume');
> -	submitBtn = Ext.create('Ext.Button', {
> -	    text: me.title,
> -	    handler: function() {
> -		if (form.isValid()) {
> -		    var values = form.getValues();
> -		    me.move_disk(me.disk, values.hdstorage, values.diskformat,
> -				 values.deleteDisk);
> -		}
> -	    },
> -	});
> -
> -	Ext.apply(me, {
> -	    modal: true,
> -	    width: 350,
> -	    border: false,
> -	    layout: 'fit',
> -	    buttons: [submitBtn],
> -	    items: [me.formPanel],
> -	});
> -
> -
>   	me.callParent();
> -
> -	me.mon(me.formPanel, 'validitychange', function(fp, isValid) {
> -	    submitBtn.setDisabled(!isValid);
> -	});
> -
> -	me.formPanel.isValid();
>       },
>   });
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index b76814e8..54946651 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -415,6 +415,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   		disk: rec.data.key,
>   		nodename: nodename,
>   		vmid: vmid,
> +		type: 'qemu',
>   	    });
>   
>   	    win.show();

Re: [pve-devel] [PATCH v2 manager 1/3] ui: lxc/qemu: add disk reassign

[Aaron Lauterer]

On 2/21/22 16:44, Dominik Csapak wrote:
> sorry for the late review
> 
> some comments inline
> 
> On 11/15/21 16:02, Aaron Lauterer wrote:
[...]
>> diff --git a/www/manager6/lxc/Resources.js
>> b/www/manager6/lxc/Resources.js index 15ee3c67..bec7cf14 100644 ---
>> a/www/manager6/lxc/Resources.js +++
>> b/www/manager6/lxc/Resources.js @@ -156,6 +156,11 @@
>> Ext.define('PVE.lxc.RessourceView', { return; } +        if
>> (rec.data.key.match(/^unused/)) { +        Ext.Msg.alert('Error',
>> gettext('Not yet supported for unused volumes')); +        return; 
>> +        } +
> 
> since we already hide/disable that button accordingly, why have that 
> error message at all? if the user somehow shows the button via the 
> browser console, the api will return an error anyway..
> 
> that way, we'd save a gettext

good point

> 

[..]

>> @@ -227,12 +250,40 @@ Ext.define('PVE.lxc.RessourceView', { }, }); 
>> -    var move_btn = new Proxmox.button.Button({ +    let
>> reassign_menuitem = new Ext.menu.Item({ +        text:
>> gettext('Reassign volume'), +        tooltip: gettext('Reassign
>> volume to another VM'), +        handler: run_reassign, +
>> iconCls: 'fa fa-mail-forward', +        reference:
>> 'reassing_item', +    }); + +    let move_btn = new
>> PVE.button.Split({ text: gettext('Move Volume'), selModel:
>> me.selModel, disabled: true, dangerous: true, handler: run_move, +
>> menu: { +        items: [reassign_menuitem], +        }, +    }); 
>> + +    // needed until we can move unused volumes to other
>> storages +    let reassign_btn = new Proxmox.button.Button({ +
>> text: gettext('Reassign volume'), +        tooltip:
>> gettext('Reassign volume to another VM'), +        handler:
>> run_reassign, +        selModel: me.selModel, +        hidden:
>> true, +        listeners: { +        render: function(btn) { +
>> // hack: avoid layout-flickering due to size change, use max width
>> for both +            let maxWidth = Math.max(btn.getSize().width,
>> move_btn.getSize().width); +            move_btn.setSize({ width:
>> maxWidth }); +            btn.setSize({ width: maxWidth });
> 
> mhmm... while for remove/detach disks this seems to work ok, for this
> the gettext are so different, the reassign button look very weird..
> 
> maybe putting both in a menu (where the top button itself has no
> action) and disable accordingly would make more sense here?
> 
> i.e. Volume/Disk Actions - Resize - Move - relocate
> 
> ?

Sounds like a good plan and as quickly discussed off list, will also make it easier to look consistent in different transla
tions.

> [...]
>> diff --git a/www/manager6/qemu/HDReassign.js
>> b/www/manager6/qemu/HDReassign.js new file mode 100644 index
>> 00000000..e6b59cb4
[..]

>> + +    reassign_disk: function(values) {
> 
> this should probably be 'reassignDisk' according to our usual naming
> scheme

thx for catching that

> 
>> +        let me = this; +        let view = me.getView(); +
>> let qemu = view.qemu; +        let params = { +        vmid:
>> view.vmid, +        'target-vmid': values.targetVmid, +        }; 
>> + +        params[qemu ? 'disk' : 'volume'] = view.disk; + +
>> if (view.qemu) { +        params['target-disk'] =
>> `${values.controller}${values.deviceid}`; +        } else { +
>> params['target-volume'] = `${values.mpType}${values.mpId}`; +
>> } + +        let url =
>> `/nodes/${view.nodename}/${view.type}/${view.vmid}/`; +        url
>> += qemu ? 'move_disk' : 'move_volume'; + +
>> Proxmox.Utils.API2Request({ +        params: params, +        url:
>> url, +        waitMsgTarget: me.getView(), +        method:
>> 'POST', +        failure: response => Ext.Msg.alert('Error',
>> response.htmlStatus), +        success: function(response, options)
>> { +            let upid = response.result.data; +
>> view.hide(); +            Ext.create('Proxmox.window.TaskProgress',
>> { +            upid, +            autoShow: true, +
>> taskDone: success => success ? view.close() : view.show(),
> 
> so if the task is not successful, we show ourselves again (over the
> error of the progress window)? what about the task log window (if
> opened)?

As discussed off list, closing the window always is what we want. In case of errors, we do see the error window or have the task log opened.

> 
>> +            }); +        }, +        }); +    }, + +
>> validateForm: function(fp, isValid) { +
>> this.getView().lookup('submitButton').setDisabled(!isValid); +
>> }, + +    onReassignClick: function() { +        let me = this; +
>> let view = me.getView(); +        let form =
>> view.lookup('moveFormPanel').getForm(); +        if
>> (form.isValid()) { +
>> me.reassign_disk(form.getValues()); +        } +    }, + +
>> onMpTypeChange: function(value) { +
>> this.getView().getViewModel().set('mpType', value.getValue()); +
>> this.getView().lookup('mpIdSelector').validate(); +    },
> 
> these functions make me question if it wouldn't have been easier to
> use an 'edit window' instead? any reason for that?

It has been a while since I sent these patches, but I think there were a few reasons to go with a regular window. One of them IIRC was that, similar to the resize and move windows, we also want to show a custom labeled button "Reassign {Volume|Disk}"  instead of "OK" or one of the options we have with the edit window.

> 
>> + +    onTargetVMChange: function(f, vmid) { +        let me =
>> this; +        let view = me.getView(); +        let diskSelector =
>> view.lookup('diskSelector'); +        if (!vmid) { +
>> diskSelector.setVMConfig(null); +        me.VMConfig = null; +
>> return; +        } + +        let type = view.type === 'qemu' ?
>> 'qemu' : 'lxc';
> 
> here you use 'view.type' but above you use 'view.qemu' ? i'd prefer
> to have a consistent way of checking the type

True, I also found another instance and aligned them.

[...]
>> listeners: { +        validitychange: 'validateForm', +        }, +
>> items: [ +        { +            xtype: 'displayfield', +
>> name: 'sourceDisk', +            cbind: { +            name: get =>
>> get('isQemu') ? 'disk' : 'volume', +            fieldLabel: get =>
>> get('isQemu') +                ? gettext('Source Disk') +
>> : gettext('Source Mount Point'),
> 
> wouldn't 'Source' suffice for both? IMHO the context is enough that
> the user knows that it's a disk/mp

Good point

Re: [pve-devel] [PATCH v2 manager 1/3] ui: lxc/qemu: add disk reassign

[Dominik Csapak]

On 3/4/22 14:29, Aaron Lauterer wrote:
>>> +            }); +        }, +        }); +    }, + +
>>> validateForm: function(fp, isValid) { +
>>> this.getView().lookup('submitButton').setDisabled(!isValid); +
>>> }, + +    onReassignClick: function() { +        let me = this; +
>>> let view = me.getView(); +        let form =
>>> view.lookup('moveFormPanel').getForm(); +        if
>>> (form.isValid()) { +
>>> me.reassign_disk(form.getValues()); +        } +    }, + +
>>> onMpTypeChange: function(value) { +
>>> this.getView().getViewModel().set('mpType', value.getValue()); +
>>> this.getView().lookup('mpIdSelector').validate(); +    },
>>
>> these functions make me question if it wouldn't have been easier to
>> use an 'edit window' instead? any reason for that?
> 
> It has been a while since I sent these patches, but I think there were a few reasons to go with a 
> regular window. One of them IIRC was that, similar to the resize and move windows, we also want to 
> show a custom labeled button "Reassign {Volume|Disk}"  instead of "OK" or one of the options we have 
> with the edit window.
> 

we have the 'submitText' property for the edit window which always takes precedence
so that shouldn't be the problem