[pve-devel] [PATCH-SERIES] small vzdump improvements

[pve-devel] [PATCH-SERIES] small vzdump improvements

[Fabian Ebner]

The first three patches are rather minor things, improving warnings/mails.
The fourth patch makes parse errors for section configs visible to the caller,
which can then decide if/how to handle them. And the fifth patch uses this new
information for aborting a backup when there are parse errors in the backup
storage configuration.

The last patch needs a dependency bump: pve-manager -> pve-common


pve-manager:

Fabian Ebner (4):
  vzdump: defaults: convert to prune-backups early enough
  vzdump: sendmail: fix html by closing the tags
  vzdump: fix error format for errors coming from storage_info
  vzdump: make parse error for storage critical

 PVE/VZDump.pm                     | 12 ++++++++----
 test/vzdump_new_retention_test.pl |  8 +-------
 2 files changed, 9 insertions(+), 11 deletions(-)


pve-common:

Fabian Ebner (1):
  SectionConfig: add errors to result

 src/PVE/SectionConfig.pm | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

-- 
2.20.1

[pve-devel] [PATCH manager 1/5] vzdump: defaults: convert to prune-backups early enough

[Fabian Ebner]

Fixes the case where reading from /etc/vzdump.conf fails.

Also convert the options read from /etc/vzdump.conf before the loop. That
avoids showing a wrong warning when 'prune-backups' is configured in
/etc/vzdump.conf, and maxfiles isn't. Previously, because 'maxfiles' from the
schema defaults was automatically set, the call to parse_prune_backups_maxfiles
after the loop threw the warning that both options are defined.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 6a4e641b..f75e4b16 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -211,6 +211,7 @@ sub read_vzdump_defaults {
 	     defined($default) ? ($_ => $default) : ()
 	} keys %$confdesc
     };
+    $parse_prune_backups_maxfiles->($defaults, "defaults in VZDump schema");
 
     my $raw;
     eval { $raw = PVE::Tools::file_get_contents($fn); };
@@ -225,6 +226,7 @@ sub read_vzdump_defaults {
 	my @mailto = split_list($res->{mailto});
 	$res->{mailto} = [ @mailto ];
     }
+    $parse_prune_backups_maxfiles->($res, "options in '$fn'");
 
     foreach my $key (keys %$defaults) {
 	$res->{$key} = $defaults->{$key} if !defined($res->{$key});
@@ -235,8 +237,6 @@ sub read_vzdump_defaults {
 	delete $res->{dumpdir};
     }
 
-    $parse_prune_backups_maxfiles->($res, "options in '$fn'");
-
     return $res;
 }
 
-- 
2.20.1

[pve-devel] applied: Re: [PATCH manager 1/5] vzdump: defaults: convert to prune-backups early enough

[Thomas Lamprecht]

On 21/12/2020 14:48, Fabian Ebner wrote:
> Fixes the case where reading from /etc/vzdump.conf fails.
> 
> Also convert the options read from /etc/vzdump.conf before the loop. That
> avoids showing a wrong warning when 'prune-backups' is configured in
> /etc/vzdump.conf, and maxfiles isn't. Previously, because 'maxfiles' from the
> schema defaults was automatically set, the call to parse_prune_backups_maxfiles
> after the loop threw the warning that both options are defined.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 2/5] vzdump: sendmail: fix html by closing the tags

[Fabian Ebner]

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index f75e4b16..3b864514 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -394,13 +394,15 @@ sub sendmail {
     }
     $html_log_part .= escape_html($detail_post) if defined($detail_post);
     $html_log_part .= "</pre>";
-    my $html_end .= "\n</body></html>\n";
+    my $html_end = "\n</body></html>\n";
     # end html part
 
     if (length($text) + length($text_log_part) +
-	length($html) + length($html_log_part) < MAX_MAIL_SIZE)
+	length($html) + length($html_log_part) +
+	length($html_end) < MAX_MAIL_SIZE)
     {
 	$html .= $html_log_part;
+	$html .= $html_end;
 	$text .= $text_log_part;
     } else {
 	my $msg = "Log output was too long to be sent by mail. ".
-- 
2.20.1

[pve-devel] applied: Re: [PATCH manager 2/5] vzdump: sendmail: fix html by closing the tags

[Thomas Lamprecht]

On 21/12/2020 14:48, Fabian Ebner wrote:
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
>

applied, thanks!

[pve-devel] [PATCH manager 3/5] vzdump: fix error format for errors coming from storage_info

[Fabian Ebner]

Errors from storage_info() are newline-terminated, because perl would append
the line number otherwise. Chomp those errors, because sendmail() relies
on the presence of a newline to decide if it's multiple problems or only one.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/VZDump.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index 3b864514..d6f9709b 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -498,6 +498,7 @@ sub new {
     if ($opts->{storage}) {
 	my $info = eval { storage_info ($opts->{storage}) };
 	if (my $err = $@) {
+	    chomp($err);
 	    $errors .= "could not get storage information for '$opts->{storage}': $err";
 	} else {
 	    $opts->{dumpdir} = $info->{dumpdir};
-- 
2.20.1

[pve-devel] applied: Re: [PATCH manager 3/5] vzdump: fix error format for errors coming from storage_info

[Thomas Lamprecht]

On 21/12/2020 14:48, Fabian Ebner wrote:
> Errors from storage_info() are newline-terminated, because perl would append
> the line number otherwise. Chomp those errors, because sendmail() relies
> on the presence of a newline to decide if it's multiple problems or only one.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/VZDump.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>

applied, thanks!

[pve-devel] [PATCH/RFC common 4/5] SectionConfig: parse_config: add errors to result

[Fabian Ebner]

so that callers can know about them. This is useful in places where we'd rather
abort then continue with a faulty configuration. For example, when reading the
storage configuration before executing a backup job.

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

I skimmed over the cascade of usages and could not find a problem with
introducing the new key. Potentially problematic code would be something
that's iterating over the keys for $cfg and do something bad with the
new 'errors', but AFAICS, there is no such code.

 src/PVE/SectionConfig.pm | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/src/PVE/SectionConfig.pm b/src/PVE/SectionConfig.pm
index b46b59e..af0af03 100644
--- a/src/PVE/SectionConfig.pm
+++ b/src/PVE/SectionConfig.pm
@@ -311,6 +311,7 @@ sub parse_config {
 	}
     };
 
+    my $errors = [];
     while (@lines) {
 	my $line = $nextline->();
 	next if !$line;
@@ -349,7 +350,15 @@ sub parse_config {
 			die "duplicate attribute\n" if defined($config->{$k});
 			$config->{$k} = $plugin->check_value($type, $k, $v, $sectionId);
 		    };
-		    warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $@" if $@;
+		    if (my $err = $@) {
+			warn "$errprefix (section '$sectionId') - unable to parse value of '$k': $err";
+			push @$errors, {
+			    context => $errprefix,
+			    section => $sectionId,
+			    key => $k,
+			    err => $err,
+			};
+		    }
 
 		} else {
 		    warn "$errprefix (section '$sectionId') - ignore config line: $line\n";
@@ -368,8 +377,12 @@ sub parse_config {
 	}
     }
 
-
-    my $cfg = { ids => $ids, order => $order, digest => $digest};
+    my $cfg = {
+	ids => $ids,
+	order => $order,
+	digest => $digest
+    };
+    $cfg->{errors} = $errors if scalar(@$errors) > 0;
 
     return $cfg;
 }
-- 
2.20.1

[pve-devel] applied: Re: [PATCH/RFC common 4/5] SectionConfig: parse_config: add errors to result

[Thomas Lamprecht]

On 21/12/2020 14:48, Fabian Ebner wrote:
> so that callers can know about them. This is useful in places where we'd rather
> abort then continue with a faulty configuration. For example, when reading the
> storage configuration before executing a backup job.
> 
> Originally-by: Thomas Lamprecht <t.lamprecht@proxmox.com>
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> I skimmed over the cascade of usages and could not find a problem with
> introducing the new key. Potentially problematic code would be something
> that's iterating over the keys for $cfg and do something bad with the
> new 'errors', but AFAICS, there is no such code.
> 
>  src/PVE/SectionConfig.pm | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
>

I may be biased, as it's my patch, but even after a few weeks it just seems like
propagating the error information is a hard requirement for use-sites to be able
to do the right thing, whatever that then is.

applied, thanks!

[pve-devel] [PATCH/RFC manager 5/5] vzdump: make parse error for storage critical

[Fabian Ebner]

The actual error is already printed on the CLI and in the task log, so
there's no real need to make the error message in storage_info() more than
"parse error\n". It also can/will end up in the mail subject, which is another
reason to keep it simple.

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

Needs a dependency bump for the previous patch

 PVE/VZDump.pm                     | 1 +
 test/vzdump_new_retention_test.pl | 8 +-------
 2 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
index d6f9709b..9f486295 100644
--- a/PVE/VZDump.pm
+++ b/PVE/VZDump.pm
@@ -96,6 +96,7 @@ sub storage_info {
     my $storage = shift;
 
     my $cfg = PVE::Storage::config();
+    die "parse error\n" if grep { $_->{section} eq $storage } @{$cfg->{errors}};
     my $scfg = PVE::Storage::storage_config($cfg, $storage);
     my $type = $scfg->{type};
 
diff --git a/test/vzdump_new_retention_test.pl b/test/vzdump_new_retention_test.pl
index 569419fb..6a2b9170 100755
--- a/test/vzdump_new_retention_test.pl
+++ b/test/vzdump_new_retention_test.pl
@@ -340,7 +340,6 @@ my @tests = (
 	    remove => 1,
 	},
     },
-    # TODO make parse error critical?
     {
 	description => 'mixed 2',
 	vzdump_param => {
@@ -349,12 +348,7 @@ my @tests = (
 	storage_param => {
 	    'prune-backups' => 'keephourly=24',
 	},
-	expected => {
-	    'prune-backups' => {
-		'keep-last' => 7,
-	    },
-	    remove => 1,
-	},
+	expected => "could not get storage information for 'local': parse error\n",
     },
     {
 	description => 'mixed 3',
-- 
2.20.1

Re: [pve-devel] [PATCH/RFC manager 5/5] vzdump: make parse error for storage critical

[Thomas Lamprecht]

On 21/12/2020 14:48, Fabian Ebner wrote:
> The actual error is already printed on the CLI and in the task log, so
> there's no real need to make the error message in storage_info() more than
> "parse error\n". It also can/will end up in the mail subject, which is another
> reason to keep it simple.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Needs a dependency bump for the previous patch

that's the sole real reason I'm not applying this just yet, please ping me if
common got bumped and I forget this one.

> 
>  PVE/VZDump.pm                     | 1 +
>  test/vzdump_new_retention_test.pl | 8 +-------
>  2 files changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/PVE/VZDump.pm b/PVE/VZDump.pm
> index d6f9709b..9f486295 100644
> --- a/PVE/VZDump.pm
> +++ b/PVE/VZDump.pm
> @@ -96,6 +96,7 @@ sub storage_info {
>      my $storage = shift;
>  
>      my $cfg = PVE::Storage::config();
> +    die "parse error\n" if grep { $_->{section} eq $storage } @{$cfg->{errors}};

I know the errors got out with warn already, but maybe we want to re-print
them here, could be done with a extract-errors helper, which optionally takes
a section and then prints out a "could not parse '$key'" or the like for all
or the errors of that section, respectively - just as an idea...

>      my $scfg = PVE::Storage::storage_config($cfg, $storage);
>      my $type = $scfg->{type};
>  
> diff --git a/test/vzdump_new_retention_test.pl b/test/vzdump_new_retention_test.pl
> index 569419fb..6a2b9170 100755
> --- a/test/vzdump_new_retention_test.pl
> +++ b/test/vzdump_new_retention_test.pl
> @@ -340,7 +340,6 @@ my @tests = (
>  	    remove => 1,
>  	},
>      },
> -    # TODO make parse error critical?
>      {
>  	description => 'mixed 2',
>  	vzdump_param => {
> @@ -349,12 +348,7 @@ my @tests = (
>  	storage_param => {
>  	    'prune-backups' => 'keephourly=24',
>  	},
> -	expected => {
> -	    'prune-backups' => {
> -		'keep-last' => 7,
> -	    },
> -	    remove => 1,
> -	},
> +	expected => "could not get storage information for 'local': parse error\n",
>      },
>      {
>  	description => 'mixed 3',
>