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

[pve-devel] [PATCH 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


pve-manager:

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

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


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 | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.39.2

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

[Alexander Zeidler]

Unify style before adding another parameter check

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
 PVE/API2/Backup.pm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
index 45eb47e2..cae889f4 100644
--- a/PVE/API2/Backup.pm
+++ b/PVE/API2/Backup.pm
@@ -49,8 +49,11 @@ 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})) {
-	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
+    for my $key (qw(bwlimit ionice performance)) {
+	if (defined($param->{$key})) {
+	    $rpcenv->check($user, "/", [ 'Sys.Modify' ]);
+	    last;
+	}
     }
 }
 
-- 
2.39.2

[pve-devel] [PATCH manager 2/4] 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>
---
 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 cae889f4..4fd495e2 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};
     }
 
-    for my $key (qw(bwlimit ionice performance)) {
+    for my $key (qw(bwlimit ionice performance pbs-entries-max)) {
 	if (defined($param->{$key})) {
 	    $rpcenv->check($user, "/", [ 'Sys.Modify' ]);
 	    last;
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

[pve-devel] [PATCH 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>
---
 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..a1d2ec8 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("override max number of entries to hold in memory");
+	}
+
 	my @storage = ($opts->{scfg}, $opts->{storage});
 
 	my $logfunc = sub { my $line = shift; $self->loginfo($line) };
-- 
2.39.2

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

[Alexander Zeidler]

Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
---
 src/PVE/VZDump/Common.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
index a6fe483..28ab0d3 100644
--- a/src/PVE/VZDump/Common.pm
+++ b/src/PVE/VZDump/Common.pm
@@ -282,6 +282,12 @@ my $confdesc = {
 	requires => 'storage',
 	optional => 1,
     },
+    "pbs-entries-max" => {
+	type        => 'integer',
+	description => "Override max number of entries to hold in memory (only applicable for PBS).",
+	optional    => 1,
+	minimum     => 1,
+	},
 };
 
 sub get_confdesc {
-- 
2.39.2

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

[Fiona Ebner]

Am 13.06.23 um 15:42 schrieb Alexander Zeidler:
> Unify style before adding another parameter check
> 
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  PVE/API2/Backup.pm | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/API2/Backup.pm b/PVE/API2/Backup.pm
> index 45eb47e2..cae889f4 100644
> --- a/PVE/API2/Backup.pm
> +++ b/PVE/API2/Backup.pm
> @@ -49,8 +49,11 @@ 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})) {
> -	$rpcenv->check($user, "/", [ 'Sys.Modify' ]);
> +    for my $key (qw(bwlimit ionice performance)) {
> +	if (defined($param->{$key})) {
> +	    $rpcenv->check($user, "/", [ 'Sys.Modify' ]);
> +	    last;
> +	}

Style nit: using something like
if ( grep { defined($_); } qw(...) ) {
would keep the line count low ;)

>      }
>  }
>  

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

[Fiona Ebner]

Am 13.06.23 um 15:42 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>
> ---
>  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..a1d2ec8 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("override max number of entries to hold in memory");
Style nit: wrong indentation.

I'd also include the new value in the log line. And maybe replacing "to
hold in memory" with "for file-based backup" makes it easier to grasp
from a user perspective?

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

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

[Fiona Ebner]

Am 13.06.23 um 15:42 schrieb Alexander Zeidler:
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  src/PVE/VZDump/Common.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
> index a6fe483..28ab0d3 100644
> --- a/src/PVE/VZDump/Common.pm
> +++ b/src/PVE/VZDump/Common.pm
> @@ -282,6 +282,12 @@ my $confdesc = {
>  	requires => 'storage',
>  	optional => 1,
>      },
> +    "pbs-entries-max" => {
> +	type        => 'integer',
> +	description => "Override max number of entries to hold in memory (only applicable for PBS).",

This is not really telling from a user perspective. Shouldn't we mention
how it relates to folders here? Also: only applicable for file-based PBS
backups.

> +	optional    => 1,
> +	minimum     => 1,

Style nit: we don't align the arrows for any other parameter here

> +	},
>  };
>  
>  sub get_confdesc {

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

[Fiona Ebner]

Am 13.06.23 um 15:42 schrieb Alexander Zeidler:
> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
> ---
>  src/PVE/VZDump/Common.pm | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
> index a6fe483..28ab0d3 100644
> --- a/src/PVE/VZDump/Common.pm
> +++ b/src/PVE/VZDump/Common.pm
> @@ -282,6 +282,12 @@ my $confdesc = {
>  	requires => 'storage',
>  	optional => 1,
>      },
> +    "pbs-entries-max" => {
> +	type        => 'integer',
> +	description => "Override max number of entries to hold in memory (only applicable for PBS).",
> +	optional    => 1,
> +	minimum     => 1,
> +	},
>  };

Now I'm wondering if this would make sense as part of the 'performance'
setting? Depending on how you interpret it, it could fit or not. Any
other opinions?

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

[Thomas Lamprecht]

Am 14/06/2023 um 14:51 schrieb Fiona Ebner:
> Am 13.06.23 um 15:42 schrieb Alexander Zeidler:
>> Signed-off-by: Alexander Zeidler <a.zeidler@proxmox.com>
>> ---
>>  src/PVE/VZDump/Common.pm | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/PVE/VZDump/Common.pm b/src/PVE/VZDump/Common.pm
>> index a6fe483..28ab0d3 100644
>> --- a/src/PVE/VZDump/Common.pm
>> +++ b/src/PVE/VZDump/Common.pm
>> @@ -282,6 +282,12 @@ my $confdesc = {
>>  	requires => 'storage',
>>  	optional => 1,
>>      },
>> +    "pbs-entries-max" => {
>> +	type        => 'integer',
>> +	description => "Override max number of entries to hold in memory (only applicable for PBS).",
>> +	optional    => 1,
>> +	minimum     => 1,
>> +	},
>>  };
> 
> Now I'm wondering if this would make sense as part of the 'performance'
> setting? Depending on how you interpret it, it could fit or not. Any
> other opinions?

+1, rather re-use existing infra for this and the performance settings are
a good enough fit, FWICT