[pve-devel] [PATCH-SERIES v3 manager/docs] close #4513: add performance tab for backup jobs

[pve-devel] [PATCH-SERIES v3 manager/docs] close #4513: add performance tab for backup jobs

[Fiona Ebner]

Improve fallback for the 'performance' sub-properties by using a
per-property fallback and honor schema defaults.

Expose commonly used performance-related properties in the backup job
UI under a new tab.


Changes in v3:
    * new patch to actually honor default values for performance
      format/schema
    * new patch to switch to per-property fallback for performance
      properties
    * also handle new (came in after v2) pbs-entries-max in UI (make
      sure to preserve value, but don't expose it)
    * drop already applied patch


pve-manager:

Fiona Ebner (4):
  vzdump: actually honor schema defaults for performance
  vzdump: use per-property fallback for performance settings
  close #4513: ui: backup job: add performance tab
  ui: backup job: disable zstd thread count field when zstd isn't used

 PVE/VZDump.pm                           |  33 ++++-
 www/manager6/Makefile                   |   1 +
 www/manager6/dc/Backup.js               |  30 +++++
 www/manager6/panel/BackupPerformance.js | 155 ++++++++++++++++++++++++
 4 files changed, 214 insertions(+), 5 deletions(-)
 create mode 100644 www/manager6/panel/BackupPerformance.js


pve-docs:

Fiona Ebner (1):
  backup: update information about performance settings

 vzdump.adoc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

-- 
2.39.2

[pve-devel] [PATCH v3 manager 1/4] vzdump: actually honor schema defaults for performance

[Fiona Ebner]

The 'performance' option itself defines no 'default' in the schema, so
what happened is that the defaults used by the backends (i.e. QEMU and
proxmox-backup-client) would be used. Luckily, they correspond to the
default values defined in the schema, i.e. in the 'backup-performance'
format. Make the code future-proof and use the actual defaults defined
in the schema instead of relying on that correspondence.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v3.

 PVE/VZDump.pm | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 454ab494..182fb80b 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -277,8 +277,14 @@ sub read_vzdump_defaults {
 	     defined($default) ? ($_ => $default) : ()
 	} keys %$confdesc_for_defaults
     };
+    my $performance_fmt = PVE::JSONSchema::get_format('backup-performance');
+    $defaults->{performance} = {
+	map {
+	    my $default = $performance_fmt->{$_}->{default};
+	    defined($default) ? ($_ => $default) : ()
+	} keys $performance_fmt->%*
+    };
     $parse_prune_backups_maxfiles->($defaults, "defaults in VZDump schema");
-    parse_performance($defaults);
 
     my $raw;
     eval { $raw = PVE::Tools::file_get_contents($fn); };
-- 
2.39.2

[pve-devel] [PATCH v3 manager 2/4] vzdump: use per-property fallback for performance settings

[Fiona Ebner]

Currently, fallback for the 'performance' option is done as a whole,
taking away flexibility from the user. It also means that when only
one of the two sub-properties is specified, the other one will default
to the backend (i.e. QEMU or proxmox-backup-client) default rather
than the schema default. For the latter point in particular, it can be
argued to be incorrect. These limitations will only get worse in the
future with more sub-properties.

Switch to a per-property fallback mechanism to improve the situation,
having each go through the usual preference order (CLI/job > node-wide
default > schema default).

Technically, this is a breaking change, but pbs-entries-max is rather
new and potential for breakage seems rather low. Requirements for
breakage:
* job (or CLI) that defines only one of the performance options
* job also covers a guest where the other performance option applies
* the other performance option is defined in the node-wide configuration
* the node-wide setting is worse for the job than the implicit backend
  default (because this change will have the node-wide default win over
  the implicit backend default).

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

New in v3.

 PVE/VZDump.pm | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 182fb80b..9a82260f 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -139,6 +139,17 @@ my sub parse_performance {
     }
 }
 
+my sub merge_performance {
+    my ($prefer, $fallback) = @_;
+
+    my $res = {};
+    for my $opt (keys PVE::JSONSchema::get_format('backup-performance')->%*) {
+	$res->{$opt} = $prefer->{$opt} // $fallback->{$opt}
+	    if defined($prefer->{$opt}) || defined($fallback->{$opt});
+    }
+    return $res;
+}
+
 my $parse_prune_backups_maxfiles = sub {
     my ($param, $kind) = @_;
 
@@ -312,8 +323,12 @@ sub read_vzdump_defaults {
     $parse_prune_backups_maxfiles->($res, "options in '$fn'");
     parse_performance($res);
 
-    foreach my $key (keys %$defaults) {
-	$res->{$key} = $defaults->{$key} if !defined($res->{$key});
+    for my $key (keys $defaults->%*) {
+	if (!defined($res->{$key})) {
+	    $res->{$key} = $defaults->{$key};
+	} elsif ($key eq 'performance') {
+	    $res->{$key} = merge_performance($res->{$key}, $defaults->{$key});
+	}
     }
 
     if (defined($res->{storage}) && defined($res->{dumpdir})) {
@@ -575,8 +590,10 @@ sub new {
 	if ($k eq 'dumpdir' || $k eq 'storage') {
 	    $opts->{$k} = $defaults->{$k} if !defined ($opts->{dumpdir}) &&
 		!defined ($opts->{storage});
-	} else {
-	    $opts->{$k} = $defaults->{$k} if !defined ($opts->{$k});
+	} elsif (!defined($opts->{$k})) {
+	    $opts->{$k} = $defaults->{$k};
+	} elsif ($k eq 'performance') {
+	    $opts->{$k} = merge_performance($opts->{$k}, $defaults->{$k});
 	}
     }
 
-- 
2.39.2

[pve-devel] [PATCH v3 manager 3/4] 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).

The pbs-entries-max performance option is not exposed. It is rather
niche and hard to understand. It serves as an escape hatch for
rare/extreme cases.

These can still be added with appropriate notes if there is enough
user demand.

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v3:
  * also handle pbs-entries-max

Is there a better way to preserve the value than using a hidden field?

 www/manager6/Makefile                   |   1 +
 www/manager6/dc/Backup.js               |  12 ++
 www/manager6/panel/BackupPerformance.js | 142 ++++++++++++++++++++++++
 3 files changed, 155 insertions(+)
 create mode 100644 www/manager6/panel/BackupPerformance.js

diff --git a/www/manager6/Makefile b/www/manager6/Makefile
index dccd2ba1..ed454a71 100644
--- a/www/manager6/Makefile
+++ b/www/manager6/Makefile
@@ -97,6 +97,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 0c8d2d4f..1b2f95d2 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -218,6 +218,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);
 		    },
 		});
@@ -487,6 +492,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..6680754a
--- /dev/null
+++ b/www/manager6/panel/BackupPerformance.js
@@ -0,0 +1,142 @@
+/*
+ * 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', 'pbs-entries-max'];
+
+	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: '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),
+	    },
+	},
+	{
+	    xtype: 'proxmoxintegerfield',
+	    name: 'zstd',
+	    fieldLabel: Ext.String.format(gettext('{0} Threads'), 'Zstd'),
+	    fieldStyle: 'text-align: right',
+	    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: gettext('VM Workers'),
+	    fieldStyle: 'text-align: right',
+	    emptyText: gettext('use fallback'),
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+	{
+	    // It's part of the 'performance' property string, so have a field to preserve the
+	    // value, but don't expose it. It's a rather niche setting and difficult to
+	    // convey/understand what it does.
+	    xtype: 'proxmoxintegerfield',
+	    name: 'pbs-entries-max',
+	    hidden: true,
+	    fieldLabel: 'TODO',
+	    fieldStyle: 'text-align: right',
+	    emptyText: gettext('use fallback'),
+	    cbind: {
+		deleteEmpty: '{!isCreate}',
+	    },
+	},
+    ],
+
+    column2: [
+	{
+	    xtype: 'displayfield',
+	    value: gettext('Limit I/O bandwidth'),
+	},
+	{
+	    xtype: 'displayfield',
+	    value: `${gettext('Threads used for zstd compression')} (${gettext('non-PBS')})`,
+	},
+	{
+	    xtype: 'displayfield',
+	    value: `${gettext('I/O workers in the QEMU process')} (${gettext('VM only')})`,
+	},
+	{
+	    xtype: 'displayfield',
+	    value: 'TODO',
+	    hidden: true, // see definition of pbs-entries-max field
+	},
+    ],
+
+    columnB: [
+	{
+	    xtype: 'component',
+	    userCls: 'pmx-hint',
+	    padding: '5 1',
+	    html: gettext("Note that vzdump.conf is used as a fallback"),
+	},
+    ],
+});
-- 
2.39.2

[pve-devel] [PATCH v3 manager 4/4] ui: backup job: disable zstd thread count field when zstd isn't used

[Fiona Ebner]

Also need to check for enable/disable of the compression selector,
because with PBS the value zstd is set, but the thread count setting
doesn't apply.

Suggested-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

No changes in v3.

 www/manager6/dc/Backup.js               | 18 ++++++++++++++++++
 www/manager6/panel/BackupPerformance.js | 13 +++++++++++++
 2 files changed, 31 insertions(+)

diff --git a/www/manager6/dc/Backup.js b/www/manager6/dc/Backup.js
index 1b2f95d2..17dbe345 100644
--- a/www/manager6/dc/Backup.js
+++ b/www/manager6/dc/Backup.js
@@ -161,6 +161,18 @@ Ext.define('PVE.dc.BackupEdit', {
 	    }
 	},
 
+	compressionChange: function(f, value, oldValue) {
+	    this.getView().lookup('backupPerformance').updateCompression(value, f.isDisabled());
+	},
+
+	compressionDisable: function(f) {
+	    this.getView().lookup('backupPerformance').updateCompression(f.getValue(), true);
+	},
+
+	compressionEnable: function(f) {
+	    this.getView().lookup('backupPerformance').updateCompression(f.getValue(), false);
+	},
+
 	init: function(view) {
 	    let me = this;
 	    if (view.isCreate) {
@@ -380,6 +392,11 @@ Ext.define('PVE.dc.BackupEdit', {
 					deleteEmpty: '{!isCreate}',
 				    },
 				    value: 'zstd',
+				    listeners: {
+					change: 'compressionChange',
+					disable: 'compressionDisable',
+					enable: 'compressionEnable',
+				    },
 				},
 				{
 				    xtype: 'pveBackupModeSelector',
@@ -494,6 +511,7 @@ Ext.define('PVE.dc.BackupEdit', {
 		},
 		{
 		    xtype: 'pveBackupPerformancePanel',
+		    reference: 'backupPerformance',
 		    title: gettext('Performance'),
 		    cbind: {
 			isCreate: '{isCreate}',
diff --git a/www/manager6/panel/BackupPerformance.js b/www/manager6/panel/BackupPerformance.js
index 6680754a..a9d6a1fb 100644
--- a/www/manager6/panel/BackupPerformance.js
+++ b/www/manager6/panel/BackupPerformance.js
@@ -12,6 +12,10 @@ Ext.define('PVE.panel.BackupPerformance', {
 	return {};
     },
 
+    controller: {
+	xclass: 'Ext.app.ViewController',
+    },
+
     onGetValues: function(formValues) {
 	if (this.needMask) { // isMasked() may not yet be true if not rendered once
 	    return {};
@@ -54,6 +58,14 @@ Ext.define('PVE.panel.BackupPerformance', {
 	return options;
     },
 
+    updateCompression: function(value, disabled) {
+	if (!disabled && value === 'zstd') {
+	    this.lookup('zstdThreadCount').setDisabled(false);
+	} else {
+	    this.lookup('zstdThreadCount').setDisabled(true);
+	}
+    },
+
     column1: [
 	{
 	    xtype: 'pveBandwidthField',
@@ -71,6 +83,7 @@ Ext.define('PVE.panel.BackupPerformance', {
 	{
 	    xtype: 'proxmoxintegerfield',
 	    name: 'zstd',
+	    reference: 'zstdThreadCount',
 	    fieldLabel: Ext.String.format(gettext('{0} Threads'), 'Zstd'),
 	    fieldStyle: 'text-align: right',
 	    emptyText: gettext('use fallback'),
-- 
2.39.2

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

[Fiona Ebner]

Signed-off-by: Fiona Ebner <f.ebner@proxmox.com>
---

Changes in v3:
    * rebase on current master

 vzdump.adoc | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/vzdump.adoc b/vzdump.adoc
index 8dc49f5..682c9d1 100644
--- a/vzdump.adoc
+++ b/vzdump.adoc
@@ -214,13 +214,13 @@ 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) and
-`pbs-entries-max` (affects container backups only). See the
-xref:vzdump_configuration[configuration options] for details.
+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) and `pbs-entries-max` (affects container backups
+only). See the xref:vzdump_configuration[configuration options] for details.
 
 [[vzdump_retention]]
 Backup Retention
-- 
2.39.2

Re: [pve-devel] [PATCH-SERIES v3 manager/docs] close #4513: add performance tab for backup jobs

[Fiona Ebner]

Am 07.11.23 um 14:49 schrieb Fiona Ebner:
> Improve fallback for the 'performance' sub-properties by using a
> per-property fallback and honor schema defaults.
> 
> Expose commonly used performance-related properties in the backup job
> UI under a new tab.
> 

Ping. Still applies. I need to tell forum users how to do this via CLI
rather frequently, so would be nice to have in the UI too.

Re: [pve-devel] [PATCH-SERIES v3 manager/docs] close #4513: add performance tab for backup jobs

[Thomas Lamprecht]

Am 25/03/2024 um 15:09 schrieb Fiona Ebner:
> Am 07.11.23 um 14:49 schrieb Fiona Ebner:
>> Improve fallback for the 'performance' sub-properties by using a
>> per-property fallback and honor schema defaults.
>>
>> Expose commonly used performance-related properties in the backup job
>> UI under a new tab.
>>
> 
> Ping. Still applies. I need to tell forum users how to do this via CLI
> rather frequently, so would be nice to have in the UI too.
> 

I do not like using "Performance" here, that conveys that this includes
important options that one must set to improve performance, and it's
settings won't help with overall performance (buy a "faster" HW for that).

In short, while the name is not wrong in a technical sense, all knobs will
change performance on some axis, that's not really matching with user
expectations IMO.

Even just calling this "Options" would be better IMO, even better might
be to call it "Advanced" and move also the single advanced option we
currently have ("Repeat missed" in General tab) over there.

That way newer users are made aware off that these options are
definitively not a must to adapt to get good performance, but rather for
some advanced options for tuning/limiting/etc. ...

Re: [pve-devel] [PATCH-SERIES v3 manager/docs] close #4513: add performance tab for backup jobs

[Fiona Ebner]

Am 16.04.24 um 12:16 schrieb Thomas Lamprecht:
> Am 25/03/2024 um 15:09 schrieb Fiona Ebner:
>> Am 07.11.23 um 14:49 schrieb Fiona Ebner:
>>> Improve fallback for the 'performance' sub-properties by using a
>>> per-property fallback and honor schema defaults.
>>>
>>> Expose commonly used performance-related properties in the backup job
>>> UI under a new tab.
>>>
>>
>> Ping. Still applies. I need to tell forum users how to do this via CLI
>> rather frequently, so would be nice to have in the UI too.
>>
> 
> I do not like using "Performance" here, that conveys that this includes
> important options that one must set to improve performance, and it's
> settings won't help with overall performance (buy a "faster" HW for that).
> 
> In short, while the name is not wrong in a technical sense, all knobs will
> change performance on some axis, that's not really matching with user
> expectations IMO.
> 
> Even just calling this "Options" would be better IMO, even better might
> be to call it "Advanced" and move also the single advanced option we
> currently have ("Repeat missed" in General tab) over there.
> 
> That way newer users are made aware off that these options are
> definitively not a must to adapt to get good performance, but rather for
> some advanced options for tuning/limiting/etc. ...

Sounds good to me. Sent a v4 incorporating these suggestions:
https://lists.proxmox.com/pipermail/pve-devel/2024-April/063004.html