Re: [pve-devel] [PATCH manager v14 12/12] ui: add options to add virtio-fs to qemu config

["Laurențiu Leahu-Vlăducu" <l.leahu-vladucu@proxmox.com>]

Some comments inline


Otherwise, please consider:

Reviewed-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>
Tested-by: Laurențiu Leahu-Vlăducu <l.leahu-vladucu@proxmox.com>


On 04.03.25 12:58, Markus Frank wrote:
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> v14:
> * disable expose-xattr when expose-acl is set
> * added missing writeback cache option
> 
>   www/manager6/Makefile             |   1 +
>   www/manager6/Utils.js             |   1 +
>   www/manager6/qemu/HardwareView.js |  19 +++++
>   www/manager6/qemu/VirtiofsEdit.js | 137 ++++++++++++++++++++++++++++++
>   4 files changed, 158 insertions(+)
>   create mode 100644 www/manager6/qemu/VirtiofsEdit.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index fabbdd24..fdf0e816 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -271,6 +271,7 @@ JSSRC= 							\
>   	qemu/Smbios1Edit.js				\
>   	qemu/SystemEdit.js				\
>   	qemu/USBEdit.js					\
> +	qemu/VirtiofsEdit.js				\
>   	sdn/Browser.js					\
>   	sdn/ControllerView.js				\
>   	sdn/Status.js					\
> diff --git a/www/manager6/Utils.js b/www/manager6/Utils.js
> index 90011a8f..0f242ae1 100644
> --- a/www/manager6/Utils.js
> +++ b/www/manager6/Utils.js
> @@ -1645,6 +1645,7 @@ Ext.define('PVE.Utils', {
>   	serial: 4,
>   	rng: 1,
>   	tpmstate: 1,
> +	virtiofs: 10,
>       },
>   
>       // we can have usb6 and up only for specific machine/ostypes
> diff --git a/www/manager6/qemu/HardwareView.js b/www/manager6/qemu/HardwareView.js
> index c6d193fc..34aeb51e 100644
> --- a/www/manager6/qemu/HardwareView.js
> +++ b/www/manager6/qemu/HardwareView.js
> @@ -319,6 +319,16 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    never_delete: !caps.nodes['Sys.Console'],
>   	    header: gettext("VirtIO RNG"),
>   	};
> +	for (let i = 0; i < PVE.Utils.hardware_counts.virtiofs; i++) {
> +	    let confid = "virtiofs" + i.toString();
> +	    rows[confid] = {
> +		group: 50,
> +		order: i,
> +		iconCls: 'folder',
> +		editor: 'PVE.qemu.VirtiofsEdit',
> +		header: gettext('Virtiofs') + ' (' + confid +')',
> +	    };
> +	}
>   
>   	var sorterFn = function(rec1, rec2) {
>   	    var v1 = rec1.data.key;
> @@ -595,6 +605,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    const noVMConfigDiskPerm = !caps.vms['VM.Config.Disk'];
>   	    const noVMConfigCDROMPerm = !caps.vms['VM.Config.CDROM'];
>   	    const noVMConfigCloudinitPerm = !caps.vms['VM.Config.Cloudinit'];
> +	    const noVMConfigOptionsPerm = !caps.vms['VM.Config.Options'];
>   
>   	    me.down('#addUsb').setDisabled(noHWPerm || isAtUsbLimit());
>   	    me.down('#addPci').setDisabled(noHWPerm || isAtLimit('hostpci'));
> @@ -604,6 +615,7 @@ Ext.define('PVE.qemu.HardwareView', {
>   	    me.down('#addRng').setDisabled(noSysConsolePerm || isAtLimit('rng'));
>   	    efidisk_menuitem.setDisabled(noVMConfigDiskPerm || isAtLimit('efidisk'));
>   	    me.down('#addTpmState').setDisabled(noVMConfigDiskPerm || isAtLimit('tpmstate'));
> +	    me.down('#addVirtiofs').setDisabled(noVMConfigOptionsPerm || isAtLimit('virtiofs'));
>   	    me.down('#addCloudinitDrive').setDisabled(noVMConfigCDROMPerm || noVMConfigCloudinitPerm || hasCloudInit);
>   
>   	    if (!rec) {
> @@ -748,6 +760,13 @@ Ext.define('PVE.qemu.HardwareView', {
>   				disabled: !caps.nodes['Sys.Console'],
>   				handler: editorFactory('RNGEdit'),
>   			    },
> +			    {
> +				text: gettext("Virtiofs"),
> +				itemId: 'addVirtiofs',
> +				iconCls: 'fa fa-folder',
> +				disabled: !caps.nodes['Sys.Console'],
> +				handler: editorFactory('VirtiofsEdit'),
> +			    },
>   			],
>   		    }),
>   		},
> diff --git a/www/manager6/qemu/VirtiofsEdit.js b/www/manager6/qemu/VirtiofsEdit.js
> new file mode 100644
> index 00000000..0bbb5213
> --- /dev/null
> +++ b/www/manager6/qemu/VirtiofsEdit.js
> @@ -0,0 +1,137 @@
> +Ext.define('PVE.qemu.VirtiofsInputPanel', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pveVirtiofsInputPanel',
> +    onlineHelp: 'qm_virtiofs',
> +
> +    insideWizard: false,
> +
> +    onGetValues: function(values) {
> +	var me = this;
> +	var confid = me.confid;
> +	var params = {};
> +	delete values.delete;
> +	params[confid] = PVE.Parser.printPropertyString(values, 'dirid');
> +	return params;
> +    },
> +
> +    setSharedfiles: function(confid, data) {
> +	var me = this;
> +	me.confid = confid;
> +	me.virtiofs = data;
> +	me.setValues(me.virtiofs);
> +    },
> +    initComponent: function() {
> +	let me = this;
> +
> +	me.nodename = me.pveSelNode.data.node;
> +	if (!me.nodename) {
> +	    throw "no node name specified";
> +	}
> +	me.items = [
> +	    {
> +		xtype: 'pveDirMapSelector',
> +		emptyText: 'dirid',
> +		nodename: me.nodename,
> +		fieldLabel: gettext('Directory ID'),
> +		name: 'dirid',
> +		allowBlank: false,
> +	    },
> +	    {
> +		xtype: 'proxmoxKVComboBox',
> +		fieldLabel: gettext('Cache'),
> +		name: 'cache',
> +		value: '__default__',
> +		deleteDefaultValue: false,
> +		comboItems: [
> +		    ['__default__', Proxmox.Utils.defaultText + ' (auto)'],
> +		    ['auto', 'auto'],
> +		    ['always', 'always'],
> +		    ['never', 'never'],
> +		],

Is it a good idea to use cache=auto by default? I know that "cache=auto" 
is the upstream default, but I was wondering whether it's not safer to 
use "never" by default in PVE to minimize the amount of potential 
issues. Feel free to contradict me, though ;)

By searching the internet I found some reports regarding cache=auto, 
e.g. 
https://discuss.linuxcontainers.org/t/current-state-of-virtiofs-under-virtual-machines/19166

> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Writeback cache'),
> +		name: 'writeback',
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('expose-xattr'),
> +		name: 'expose-xattr',
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('expose-acl (implies expose-xattr)'),
> +		name: 'expose-acl',
> +		listeners: {
> +		    change: function(f, value) {
> +			let xattr = me.down('field[name=expose-xattr]');
> +			xattr.setDisabled(value);
> +			if (value) {
> +			    xattr.setValue(0);
> +			}
> +		    },
> +		},
> +	    },
> +	    {
> +		xtype: 'proxmoxcheckbox',
> +		fieldLabel: gettext('Direct IO'),
> +		name: 'direct-io',
> +	    },
> +	];

I think some of these options should be marked as advanced, as they 
won't be needed in most cases (feel free to contradict me, though).

While it's not always immediately clear what some of these options do, I 
think it's hard to summarize the functionality in a short tooltip, so 
linking to the docs (as you already do) is fine to me.

What is still unclear to me is what some caching combinations do, e.g. 
the user can set Cache to Never, but can also enable "Writeback cache". 
I know this has to do with QEMU and you simply exposed the GUI for 
setting the advanced options, but I was just wondering whether it 
wouldn't be possible to expose these options to our users in a way that 
the difference between the modes is clearer, e.g. "Read cache" and 
"Writeback cache". Feel free to use some entirely different naming, 
though ;)

> +
> +	me.virtiofs = {};
> +	me.confid = 'virtiofs0';
> +	me.callParent();
> +    },
> +});
> +
> +Ext.define('PVE.qemu.VirtiofsEdit', {
> +    extend: 'Proxmox.window.Edit',
> +
> +    subject: gettext('Filesystem Passthrough'),
> +
> +    initComponent: function() {
> +	var me = this;
> +
> +	me.isCreate = !me.confid;
> +
> +	var ipanel = Ext.create('PVE.qemu.VirtiofsInputPanel', {
> +	    confid: me.confid,
> +	    pveSelNode: me.pveSelNode,
> +	    isCreate: me.isCreate,
> +	});
> +
> +	Ext.applyIf(me, {
> +	    items: ipanel,
> +	});
> +
> +	me.callParent();
> +
> +	me.load({
> +	    success: function(response) {
> +		me.conf = response.result.data;
> +		var i, confid;
> +		if (!me.isCreate) {
> +		    var value = me.conf[me.confid];
> +		    var virtiofs = PVE.Parser.parsePropertyString(value, "dirid");
> +		    if (!virtiofs) {
> +			Ext.Msg.alert(gettext('Error'), 'Unable to parse virtiofs options');
> +			me.close();
> +			return;
> +		    }
> +		    ipanel.setSharedfiles(me.confid, virtiofs);
> +		} else {
> +		    for (i = 0; i < PVE.Utils.hardware_counts.virtiofs; i++) {
> +			confid = 'virtiofs' + i.toString();
> +			if (!Ext.isDefined(me.conf[confid])) {
> +			    me.confid = confid;
> +			    break;
> +			}
> +		    }
> +		    ipanel.setSharedfiles(me.confid, {});
> +		}
> +	    },
> +	});
> +    },
> +});



_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel