Re: [pve-devel] [PATCH manager] close #4513: ui: backup job: add performance tab

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

stumbled upon this again when checking out old(er) patches without comments,
some higher level comments inline.

Am 15/03/2023 um 14:24 schrieb Fiona Ebner:
> pigz is not exposed, because it only works after manually installing
> the pigz package.
> 
> ionice is not exposed, because it only works in combination with the
> BFQ scheduler and even then not in all cases (only affects the
> compressor when doing snapshot/suspend mode backup of a VM).
> 
> These can still be added with appropriate notes if there is enough
> user demand.
> 
> Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
> ---
>  www/manager6/Makefile                   |   1 +
>  www/manager6/dc/Backup.js               |  12 +++
>  www/manager6/panel/BackupPerformance.js | 110 ++++++++++++++++++++++++
>  3 files changed, 123 insertions(+)
>  create mode 100644 www/manager6/panel/BackupPerformance.js
> 
> diff --git a/www/manager6/Makefile b/www/manager6/Makefile
> index b73b729a..0c92b984 100644
> --- a/www/manager6/Makefile
> +++ b/www/manager6/Makefile
> @@ -88,6 +88,7 @@ JSSRC= 							\
>  	grid/ResourceGrid.js				\
>  	panel/ConfigPanel.js				\
>  	panel/BackupJobPrune.js				\
> +	panel/BackupPerformance.js			\
>  	panel/HealthWidget.js				\
>  	panel/IPSet.js					\
>  	panel/RunningChart.js				\
> diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
> index d0046177..8da6e7d7 100644
> --- a/www/manager6/dc/Backup.js
> +++ b/www/manager6/dc/Backup.js
> @@ -172,6 +172,11 @@ Ext.define('PVE.dc.BackupEdit', {
>  				PVE.Utils.unEscapeNotesTemplate(data['notes-template']);
>  			}
>  
> +			if (data.performance) {
> +			    Object.assign(data, data.performance);
> +			    delete data.performance;
> +			}
> +
>  			view.setValues(data);
>  		    },
>  		});
> @@ -411,6 +416,13 @@ Ext.define('PVE.dc.BackupEdit', {
>  			},
>  		    ],
>  		},
> +		{
> +		    xtype: 'pveBackupPerformancePanel',
> +		    title: gettext('Performance'),
> +		    cbind: {
> +			isCreate: '{isCreate}',
> +		    },
> +		},
>  	    ],
>  	},
>      ],
> diff --git a/www/manager6/panel/BackupPerformance.js b/www/manager6/panel/BackupPerformance.js
> new file mode 100644
> index 00000000..17e0f34c
> --- /dev/null
> +++ b/www/manager6/panel/BackupPerformance.js
> @@ -0,0 +1,110 @@
> +/*
> + * Input panel for backup performance settings intended to be used as part of an edit/create window.
> + */
> +Ext.define('PVE.panel.BackupPerformance', {
> +    extend: 'Proxmox.panel.InputPanel',
> +    xtype: 'pveBackupPerformancePanel',
> +    mixins: ['Proxmox.Mixin.CBind'],
> +
> +    cbindData: function() {
> +	let me = this;
> +	me.isCreate = !!me.isCreate;
> +	return {};
> +    },
> +
> +    onGetValues: function(formValues) {
> +	if (this.needMask) { // isMasked() may not yet be true if not rendered once
> +	    return {};
> +	}
> +
> +	let options = { 'delete': [] };
> +
> +	let performance = {};
> +	let performanceOptions = ['max-workers'];
> +
> +	for (const [key, value] of Object.entries(formValues)) {
> +	    if (performanceOptions.includes(key)) {
> +		performance[key] = value;
> +	    // deleteEmpty is not currently implemented for pveBandwidthField
> +	    } else if (key === 'bwlimit' && value === '') {
> +		options.delete.push('bwlimit');
> +	    } else if (key === 'delete') {
> +		if (Array.isArray(value)) {
> +		    value.filter(opt => !performanceOptions.includes(opt)).forEach(
> +			opt => options.delete.push(opt),
> +		    );
> +		} else if (!performanceOptions.includes(formValues.delete)) {
> +		    options.delete.push(value);
> +		}
> +	    } else {
> +		options[key] = value;
> +	    }
> +	}
> +
> +	if (Object.keys(performance).length > 0) {
> +	    options.performance = PVE.Parser.printPropertyString(performance);
> +	} else {
> +	    options.delete.push('performance');
> +	}
> +
> +	if (this.isCreate) {
> +	    delete options.delete;
> +	}
> +
> +	return options;
> +    },
> +

hmm, maybe we should go for a layout with one field per row, but besides that a
short description about what it controls, like:

Label [ Field ] Description

Where the descriptions are sorta in the style of the CPU flag ones for VM CPU
edit, i.e., provide some context and hint of what this does.

Because max-workers "VM Worker Count" is not easily understandable IMO, i.e.,
one could think that's parallel backups done by separate workers (like e.g.,
max worker is used for migrate-all or HA LRM parallel workers)

> +    column1: [
> +	{
> +	    xtype: 'proxmoxintegerfield',
> +	    name: 'zstd',
> +	    fieldLabel: Ext.String.format('{0} ({1})', gettext('Thread count'), 'zstd'),
> +	    emptyText: gettext('use fallback'),

disabling that when ZSTD is not selected might slightly improve UX too.

> +	    minValue: 0,
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': gettext('With 0, half of the available cores are used'),
> +	    },
> +	},
> +	{
> +	    xtype: 'proxmoxintegerfield',
> +	    name: 'max-workers',
> +	    minValue: 1,
> +	    maxValue: 256,
> +	    fieldLabel: Ext.String.format('{0} ({1})', gettext('Worker count'), 'VM'),
> +	    emptyText: gettext('use fallback'),
> +	    cbind: {
> +		deleteEmpty: '{!isCreate}',
> +	    },
> +	},
> +    ],
> +
> +    column2: [
> +	{
> +	    xtype: 'pveBandwidthField',
> +	    name: 'bwlimit',
> +	    fieldLabel: gettext('Bandwidth Limit'),
> +	    emptyText: gettext('use fallback'),
> +	    backendUnit: 'KiB',
> +	    allowZero: true,
> +	    emptyValue: '',
> +	    autoEl: {
> +		tag: 'div',
> +		'data-qtip': Ext.String.format(gettext('Use {0} for unlimited'), 0),
> +	    },
> +	},
> +    ],
> +
> +    columnB: [
> +	{
> +	    xtype: 'component',
> +	    userCls: 'pmx-hint',
> +	    name: 'pbs-hint',
> +	    padding: '5 1',
> +	    html: gettext("Note that vzdump.conf is used as a fallback"),
> +	},
> +    ],
> +});