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

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

[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;
+    },
+
+    column1: [
+	{
+	    xtype: 'proxmoxintegerfield',
+	    name: 'zstd',
+	    fieldLabel: Ext.String.format('{0} ({1})', gettext('Thread count'), 'zstd'),
+	    emptyText: gettext('use fallback'),
+	    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"),
+	},
+    ],
+});
-- 
2.30.2

[pve-devel] [PATCH docs] backup: update information about performance settings

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 vzdump.adoc | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/vzdump.adoc b/vzdump.adoc
index ce46529..26bbe2b 100644
--- a/vzdump.adoc
+++ b/vzdump.adoc
@@ -214,11 +214,12 @@ the behaviour for catching up. By enabling the `Repeat missed` option
 (`repeat-missed` in the config), you can tell the scheduler that it should run
 missed jobs as soon as possible.
 
-There are a few settings for tuning backup performance not exposed in the UI.
-The most notable is `bwlimit` for limiting IO bandwidth. The amount of threads
-used for the compressor can be controlled with the `pigz` (replacing `gzip`),
-respectively, `zstd` setting. Furthermore, there are `ionice` and, as part of
-the `performance` setting, `max-workers` (affects VM backups only). See the
+There are a few settings for tuning backup performance (some not exposed in the
+UI). The most notable is `bwlimit` for limiting IO bandwidth. The amount of
+threads used for the compressor can be controlled with the `pigz` (replacing
+`gzip`), respectively, `zstd` setting. Furthermore, there are `ionice` (when the
+BFQ scheduler is used) and, as part of the `performance` setting, `max-workers`
+(affects VM backups only). See the
 xref:vzdump_configuration[configuration options] for details.
 
 [[vzdump_retention]]
-- 
2.30.2

[pve-devel] [PATCH guest-common] vzdump: config: improve description of ionice setting

[Fiona Ebner]

The CFQ scheduler was removed with Linux 5.0 and ionice is now used
by the newer BFQ scheduler. Mention what the special value 8 does.
Also mention that for snapshot and suspend mode backups of VMs, the
setting only affects the compressor, because the kvm process is not a
child process of vzdump then and does not inherit the ionice priority.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---
 src/PVE/VZDump/Common.pm | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 4b0e8e0..830c3e8 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -209,7 +209,10 @@ my $confdesc = {
     },
     ionice => {
 	type => 'integer',
-	description => "Set CFQ ionice priority.",
+	description => "Set IO priority when using the BFQ scheduler. For snapshot and suspend "
+	    ."mode backups of VMs, this only affects the compressor. A value of 8 means the idle "
+	    ."priority is used, otherwise the best-effort priority is used with the specified "
+	    ."value.",
 	optional => 1,
 	minimum => 0,
 	maximum => 8,
-- 
2.30.2

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

[Thomas Lamprecht]

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"),
> +	},
> +    ],
> +});