[pve-devel] [PATCH v3 manager/container/guest-common/docs 0/5] fix #3069: add pbs-entries-max to vzdump.conf

[pve-devel] [PATCH v3 manager/container/guest-common/docs 0/5] fix #3069: add pbs-entries-max to vzdump.conf

[Alexander Zeidler]

Users reported[1] about failing container backups to PBS when having a large amount of files in a directory.

proxmox-backup-client has already 'entries-max' implemented. The default value prevents out of memory (OOM) situations. Overriding this parameter needs at least 'Sys.Modify' permissions.

This patch series makes the new optional performance parameter 'pbs-entries-max' available in vzdump and vzdump.conf.

[1] https://bugzilla.proxmox.com/show_bug.cgi?id=3069

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  * apply Fiona's comments. For details see patch emails.
  * add new commit (pve-docs)
  * update coverletter text

Changes since v1:
  * apply Fiona's comments. For details see patch emails.


pve-manager:
Alexander Zeidler (2):
  api: backup: refactor backup permission check
  fix #3069: vzdump: add property 'performance: pbs-entries-max=N'

 PVE/API2/Backup.pm  | 2 +-
 configs/vzdump.conf | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)


pve-container:
Alexander Zeidler (1):
  fix #3069: vzdump: add 'pbs-entries-max' parameter

 src/PVE/VZDump/LXC.pm | 6 ++++++
 1 file changed, 6 insertions(+)


pve-guest-common:
Alexander Zeidler (1):
  fix #3069: vzdump: schema: add 'pbs-entries-max' property

 src/PVE/VZDump/Common.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)


pve-docs:
Alexander Zeidler (1):
  vzdump: mention new parameter 'pbs-entries-max'

 vzdump.adoc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

-- 
2.39.2

[pve-devel] [PATCH v3 manager/container/guest-common/docs 1/5] api: backup: refactor backup permission check

[Alexander Zeidler]

Alter style to make the parameter check more concise

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  * none

Changes since v1:
  Apply Fiona's comments:
  * shorten permission check


 PVE/API2/Backup.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 45eb47e2..70753c2e 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -49,7 +49,7 @@ sub assert_param_permission_common {
 	raise_param_exc({ $key => "Only root may set this option."}) if exists $param->{$key};
     }
 
-    if (defined($param->{bwlimit}) || defined($param->{ionice}) || defined($param->{performance})) {
+    if (grep { defined($param->{$_}) } qw(bwlimit ionice performance)) {
 	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
     }
 }
-- 
2.39.2

[pve-devel] [PATCH v3 manager 2/5] fix #3069: vzdump: add property 'performance: pbs-entries-max=N'

[Alexander Zeidler]

configuring pbs-entries-max can avoid failing backups due to a high
amount of files in folders where a folder exclusion is not possible

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  Apply Fiona's comments:
  * remove property from permission check since it's included in performance
  * vzdump: make property a sub property of performance

Changes since v1:
  * add 'pbs-entries-max' according to the new shortened permission check


 configs/vzdump.conf | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configs/vzdump.conf b/configs/vzdump.conf
index 2ea09ae0..954a4784 100644
--- a/configs/vzdump.conf
+++ b/configs/vzdump.conf
@@ -16,3 +16,4 @@
 #exclude-path: PATHLIST
 #pigz: N
 #notes-template: {{guestname}}
+#performance: pbs-entries-max=N
-- 
2.39.2

Re: [pve-devel] [PATCH v3 manager 2/5] fix #3069: vzdump: add property 'performance: pbs-entries-max=N'

[Fiona Ebner]

Am 27.07.23 um 10:37 schrieb Alexander Zeidler:
> configuring pbs-entries-max can avoid failing backups due to a high
> amount of files in folders where a folder exclusion is not possible
> 
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
> Changes since v2:
>   Apply Fiona's comments:
>   * remove property from permission check since it's included in performance
>   * vzdump: make property a sub property of performance
> 
> Changes since v1:
>   * add 'pbs-entries-max' according to the new shortened permission check
> 
> 
>  configs/vzdump.conf | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/configs/vzdump.conf b/configs/vzdump.conf
> index 2ea09ae0..954a4784 100644
> --- a/configs/vzdump.conf
> +++ b/configs/vzdump.conf
> @@ -16,3 +16,4 @@
>  #exclude-path: PATHLIST
>  #pigz: N
>  #notes-template: {{guestname}}
> +#performance: pbs-entries-max=N

There already is a #performance: max-workers=N further up. You should
add it to that, because duplicate options won't be handled correctly AFAIK.

[pve-devel] [PATCH v3 container 3/5]: fix #3069: vzdump: add 'pbs-entries-max' parameter

[Alexander Zeidler]

configuring pbs-entries-max can avoid failing backups due to a high
amount of files in folders where a folder exclusion is not possible

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  Apply Fiona's comments:
  * fix parameter check
  * fix both style nits

Changes since v1:
  * reword loginfo message and include new set value
  * fix indentation


 src/PVE/VZDump/LXC.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
index 5783ffa..a2bcdd7 100644
--- a/src/PVE/VZDump/LXC.pm
+++ b/src/PVE/VZDump/LXC.pm
@@ -395,6 +395,12 @@ sub archive {
 	push @$param, '--backup-id', $vmid;
 	push @$param, '--backup-time', $task->{backup_time};
 
+	if (my $entries_max = $opts->{"performance"}->{"pbs-entries-max"}) {
+	    push $param->@*, '--entries-max', $entries_max;
+	    $self->loginfo("set max number of entries in memory for file-based backups to "
+		."$entries_max");
+	}
+
 	my @storage = ($opts->{scfg}, $opts->{storage});
 
 	my $logfunc = sub { my $line = shift; $self->loginfo($line) };
-- 
2.39.2

Re: [pve-devel] [PATCH v3 container 3/5]: fix #3069: vzdump: add 'pbs-entries-max' parameter

[Fiona Ebner]

Am 27.07.23 um 10:37 schrieb Alexander Zeidler:
> configuring pbs-entries-max can avoid failing backups due to a high
> amount of files in folders where a folder exclusion is not possible
> 
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>

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

> diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> index 5783ffa..a2bcdd7 100644
> --- a/src/PVE/VZDump/LXC.pm
> +++ b/src/PVE/VZDump/LXC.pm
> @@ -395,6 +395,12 @@ sub archive {
>  	push @$param, '--backup-id', $vmid;
>  	push @$param, '--backup-time', $task->{backup_time};
>  
> +	if (my $entries_max = $opts->{"performance"}->{"pbs-entries-max"}) {
> +	    push $param->@*, '--entries-max', $entries_max;
> +	    $self->loginfo("set max number of entries in memory for file-based backups to "
> +		."$entries_max");

Style nit: not entirely sure if this is allowed by our style guide
(because of the argument going over two lines but not separated from the
parentheses) or if it should be one of

>             $self->loginfo(
>                 "set max number of entries in memory for file-based backups to $entries_max");

>             $self->loginfo(
>                 "set max number of entries in memory for file-based backups to $entries_max"
>             );

[pve-devel] [PATCH v3 guest-common 4/5] fix #3069: vzdump: schema: add 'pbs-entries-max' property

[Alexander Zeidler]

configuring pbs-entries-max can avoid failing backups due to a high
amount of files in folders where a folder exclusion is not possible

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  Apply Fiona's comments:
  * rephrase description
  * fix dot style nit

Changes since v1:
  * improve description
  * move description to 'performance' section
  * remove arrow alignment


 src/PVE/VZDump/Common.pm | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index 31e9fdb..ba4a61a 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -88,6 +88,16 @@ PVE::JSONSchema::register_format('backup-performance', {
 	default => 16,
 	optional => 1,
     },
+    'pbs-entries-max' => {
+	description => "Applies to container backups sent to PBS. Limits the number of entries "
+	    ."allowed in memory at a given time to avoid unintended OOM situations. Increase it to "
+	    ."enable backups of folders with a large amount of files. It must be minimum the sum "
+	    ."of all file/folder siblings at each level in its path.",
+	type => 'integer',
+	minimum => 1,
+	default => 1048576,
+	optional => 1,
+    },
 });
 
 my $confdesc = {
-- 
2.39.2

Re: [pve-devel] [PATCH v3 guest-common 4/5] fix #3069: vzdump: schema: add 'pbs-entries-max' property

[Fiona Ebner]

Am 27.07.23 um 10:37 schrieb Alexander Zeidler:
> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
> index 31e9fdb..ba4a61a 100644
> --- a/src/PVE/VZDump/Common.pm
> +++ b/src/PVE/VZDump/Common.pm
> @@ -88,6 +88,16 @@ PVE::JSONSchema::register_format('backup-performance', {
>  	default => 16,
>  	optional => 1,
>      },
> +    'pbs-entries-max' => {
> +	description => "Applies to container backups sent to PBS. Limits the number of entries "
> +	    ."allowed in memory at a given time to avoid unintended OOM situations. Increase it to "
> +	    ."enable backups of folders with a large amount of files. It must be minimum the sum "

Nit: I thought the traversing algorithm we have in the backup client
requires at most that number of elements or did I misunderstand again?
So lower values could work depending on the actual structure, thus
saying "must" is not technically correct. So maybe say "should be at
least the sum" ("at least" sounds more natural than "minimum" IMHO)?

> +	    ."of all file/folder siblings at each level in its path.",

[pve-devel] [PATCH v3 docs 5/5] vzdump: mention new parameter 'pbs-entries-max'

[Alexander Zeidler]

configuring pbs-entries-max can avoid failing backups due to a high
amount of files in folders where a folder exclusion is not possible

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes since v2:
  * add this commit


 vzdump.adoc | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/vzdump.adoc b/vzdump.adoc
index f3eadcd..f86c91c 100644
--- a/vzdump.adoc
+++ b/vzdump.adoc
@@ -218,7 +218,8 @@ 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
+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]]
-- 
2.39.2