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

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

[Alexander Zeidler]

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

proxmox-backup-client has already 'entries-max' implemented. The default value is also a kind of DOS prevention feature. Overriding this parameter needs at least 'Sys.Modify' permissions.

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

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


Changes from v1:
Incorporated Fiona's comments. For details see patch emails.


pve-manager:

Alexander Zeidler (2):
  api: backup: refactor backup permission check
  api: backup: add 'pbs-entries-max' to permission check & config

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


pve-container:

Alexander Zeidler (1):
  add 'pbs-entries-max' parameter

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


pve-guest-common:

Alexander Zeidler (1):
  vzdump: schema: add 'pbs-entries-max' property

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

-- 
2.39.2

[pve-devel] [PATCH v2 manager 1/4] 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 from v1:
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

Re: [pve-devel] [PATCH v2 manager 1/4] api: backup: refactor backup permission check

[Fiona Ebner]

Am 15.06.23 um 16:14 schrieb Alexander Zeidler:
> Alter style to make the parameter check more concise
> 
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>

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

> ---
> Changes from v1:
> 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' ]);
>      }
>  }

[pve-devel] applied: [PATCH v2 manager 1/4] api: backup: refactor backup permission check

[Fabian Grünbichler]

applied this one with R-B, thanks!

On July 7, 2023 2:53 pm, Fiona Ebner wrote:
> Am 15.06.23 um 16:14 schrieb Alexander Zeidler:
>> Alter style to make the parameter check more concise
>> 
>> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> 
> Reviewed-by: Fiona Ebner <f.ebner@proxmox.com>
> 
>> ---
>> Changes from v1:
>> 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' ]);
>>      }
>>  }
> 
> 
> _______________________________________________
> pve-devel mailing list
> pve-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
> 
> 
> 

[pve-devel] [PATCH v2 manager 2/4] api: backup: add 'pbs-entries-max' to permission check & config

[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 from v1:
Add 'pbs-entries-max' according to the new shortened permission check

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

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 70753c2e..206ef1d9 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 (grep { defined($param->{$_}) } qw(bwlimit ionice performance)) {
+    if (grep { defined($param->{$_}) } qw(bwlimit ionice performance pbs-entries-max)) {
 	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
     }
 }
diff --git a/configs/vzdump.conf b/configs/vzdump.conf
index 2ea09ae0..2fbf3999 100644
--- a/configs/vzdump.conf
+++ b/configs/vzdump.conf
@@ -16,3 +16,4 @@
 #exclude-path: PATHLIST
 #pigz: N
 #notes-template: {{guestname}}
+#pbs-entries-max: N
-- 
2.39.2

Re: [pve-devel] [PATCH v2 manager 2/4] api: backup: add 'pbs-entries-max' to permission check & config

[Fiona Ebner]

Am 15.06.23 um 16:14 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 from v1:
> Add 'pbs-entries-max' according to the new shortened permission check
> 
>  PVE/API2/Backup.pm  | 2 +-
>  configs/vzdump.conf | 1 +
>  2 files changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 70753c2e..206ef1d9 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 (grep { defined($param->{$_}) } qw(bwlimit ionice performance)) {
> +    if (grep { defined($param->{$_}) } qw(bwlimit ionice performance pbs-entries-max)) {

This is not needed, as it's a sub-option of performance now.

>  	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
>      }
>  }
> diff --git a/configs/vzdump.conf b/configs/vzdump.conf
> index 2ea09ae0..2fbf3999 100644
> --- a/configs/vzdump.conf
> +++ b/configs/vzdump.conf
> @@ -16,3 +16,4 @@
>  #exclude-path: PATHLIST
>  #pigz: N
>  #notes-template: {{guestname}}
> +#pbs-entries-max: N

It's not a lone-standing option, but part of performance.

[pve-devel] [PATCH v2 container 3/4]: 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 from v1:
Reword loginfo message and include new set value
Fix indentation


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

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

Re: [pve-devel] [PATCH v2 container 3/4]: add 'pbs-entries-max' parameter

[Fiona Ebner]

Am 15.06.23 um 16:14 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 from v1:
> Reword loginfo message and include new set value
> Fix indentation
> 
> 
>  src/PVE/VZDump/LXC.pm | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/src/PVE/VZDump/LXC.pm b/src/PVE/VZDump/LXC.pm
> index 5783ffa..a9e0ffb 100644
> --- a/src/PVE/VZDump/LXC.pm
> +++ b/src/PVE/VZDump/LXC.pm
> @@ -395,6 +395,11 @@ sub archive {
>  	push @$param, '--backup-id', $vmid;
>  	push @$param, '--backup-time', $task->{backup_time};
>  
> +	if ($opts->{"pbs-entries-max"}) {
It's part of performance now so this doesn't work.

Style nit: you can write
if (my $foo = $bar->{"too-long-to-read-and-type-each-time"}) {
and then just use $foo instead :)

> +	    push @$param, '--entries-max', $opts->{"pbs-entries-max"};
> +	    $self->loginfo("set max number of entries in memory for file-based backups to $opts->{'pbs-entries-max'}");

Style nit: line longer than 100 characters

> +	}
> +
>  	my @storage = ($opts->{scfg}, $opts->{storage});
>  
>  	my $logfunc = sub { my $line = shift; $self->loginfo($line) };

[pve-devel] [PATCH v2 guest-common 4/4] vzdump: schema: add 'pbs-entries-max' property

[Alexander Zeidler]

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
Changes from 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 a6fe483..83b715d 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 file-based PBS backups. Increase it to enable backups of ".
+	    "folders with a large amount of files. The number adds up from the maximum path ".
+	    "depth of a stored file/folder plus all antecedent file/folder siblings until ".
+	    "there (but not their recursive files).",
+	type => 'integer',
+	minimum => 1,
+	default => 1048576,
+	optional => 1,
+    },
 });
 
 my $confdesc = {
-- 
2.39.2

Re: [pve-devel] [PATCH v2 guest-common 4/4] vzdump: schema: add 'pbs-entries-max' property

[Fiona Ebner]

Am 15.06.23 um 16:14 schrieb Alexander Zeidler:
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
> Changes from 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 a6fe483..83b715d 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 file-based PBS backups. Increase it to enable backups of ".

Maybe add "Limits the number of entries allowed in memory at a given
time." as a second sentence?

Style nit: I think Thomas said he prefers the continuation dot on the
next line in a recent mail.

> +	    "folders with a large amount of files. The number adds up from the maximum path ".
> +	    "depth of a stored file/folder plus all antecedent file/folder siblings until ".
> +	    "there (but not their recursive files).",

IMHO, the last sentence is a bit hard to read. Maybe something like "For
a given file/folder, the number of entries required to hold in memory is
the sum of all file/folder siblings at each level in its path." is a bit
more straight-forward. Or did I misinterpret it :P?

> +	type => 'integer',
> +	minimum => 1,
> +	default => 1048576,
> +	optional => 1,
> +    },
>  });
>  
>  my $confdesc = {