[pve-devel] [PATCH storage v2 1/2] fix #3972: Remove the .notes file when a backup is deleted

[pve-devel] [PATCH storage v2 1/2] fix #3972: Remove the .notes file when a backup is deleted

[Daniel Tschlatscher]

When a VM or Container backup was deleted, the .notes file was not
removed, therefore, over time the dump folder would get polluted with
notes for backups that no longer existed.
As backup names contain a timestamp and as the notes cannot be reused
because of this, I think it is safe to just delete them just like we
do with the .log file.

Furthermore, I sourced the deletion of the log and notes file into a
new function called "archive_auxiliaries_remove". This change makes it
easier to remove these files through archive_remove(), but also other
parts in the code while keeping it DRY.
I saw this change necessary because archive_remove() does not invoke
external plugins, and doing so might have unintended side effects.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 PVE/API2/Storage/Content.pm |  5 ++---
 PVE/Storage.pm              | 19 +++++++++++++++++--
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
index 8ff858d..41b577c 100644
--- a/PVE/API2/Storage/Content.pm
+++ b/PVE/API2/Storage/Content.pm
@@ -456,9 +456,8 @@ __PACKAGE__->register_method ({
 	    print "Removed volume '$volid'\n";
 	    if ($vtype eq 'backup'
 		&& $path =~ /(.*\/vzdump-\w+-\d+-\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2})[^\/]+$/) {
-		my $logpath = "$1.log";
-		# try to cleanup our backup log file too, if still existing, #318
-		unlink($logpath) if -e $logpath;
+		# Remove log file #318 and notes file #3972 if they still exist
+		PVE::Storage::archive_auxiliaries_remove($path);
 	    }
 	};
 
diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 72458cf..5317043 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1566,6 +1566,7 @@ sub archive_info {
 
 	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${extension}$/) {
 	    $info->{logfilename} = "$1.log";
+	    $info->{notesfilename} = "$filename.notes";
 	    $info->{vmid} = int($2);
 	    $info->{ctime} = timelocal($8, $7, $6, $5, $4 - 1, $3);
 	    $info->{is_std_name} = 1;
@@ -1585,11 +1586,18 @@ sub archive_remove {
     die "cannot remove protected archive '$archive_path'\n"
 	if -e protection_file_path($archive_path);
 
+    unlink $archive_path or die "removing archive $archive_path failed: $!\n";
+
+    archive_auxiliaries_remove($archive_path);
+}
+
+sub archive_auxiliaries_remove {
+    my ($archive_path) = @_;
+
     my $dirname = dirname($archive_path);
     my $archive_info = eval { archive_info($archive_path) } // {};
     my $logfn = $archive_info->{logfilename};
-
-    unlink $archive_path or die "removing archive $archive_path failed: $!\n";
+    my $notesfn = $archive_info->{notesfilename};
 
     if (defined($logfn)) {
 	my $logpath = "$dirname/$logfn";
@@ -1597,6 +1605,13 @@ sub archive_remove {
 	    unlink $logpath or warn "removing log file $logpath failed: $!\n";
 	}
     }
+
+    if (defined($notesfn)) {
+	my $notespath = "$dirname/$notesfn";
+	if (-e $notespath) {
+	    unlink $notespath or warn "removing notes file $notespath failed: $!\n";
+	}
+    }
 }
 
 sub extract_vzdump_config_tar {
-- 
2.30.2

[pve-devel] [PATCH storage v2 2/2] fix #3972: test: Adapt test cases to changes in archive_info

[Daniel Tschlatscher]

The archive_info object now returns one more field containing the
name of the notes file. The test cases have to be adapted to expect
this new value as the package will not compile otherwise.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 test/archive_info_test.pm | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/test/archive_info_test.pm b/test/archive_info_test.pm
index 7e84b6a..bbc0e6f 100644
--- a/test/archive_info_test.pm
+++ b/test/archive_info_test.pm
@@ -24,6 +24,7 @@ my $tests = [
 	expected    => {
 	    'filename'     => "vzdump-lxc-$vmid-3070_01_01-00_00_00.tgz",
 	    'logfilename'  => "vzdump-lxc-$vmid-3070_01_01-00_00_00.log",
+	    'notesfilename'=> "vzdump-lxc-$vmid-3070_01_01-00_00_00.tgz.notes",
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -39,6 +40,7 @@ my $tests = [
 	expected    => {
 	    'filename'     => "vzdump-lxc-$vmid-1970_01_01-02_00_30.tgz",
 	    'logfilename'  => "vzdump-lxc-$vmid-1970_01_01-02_00_30.log",
+	    'notesfilename'=> "vzdump-lxc-$vmid-1970_01_01-02_00_30.tgz.notes",
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -54,6 +56,7 @@ my $tests = [
 	expected    => {
 	    'filename'     => "vzdump-lxc-$vmid-2020_03_30-21_39_30.tgz",
 	    'logfilename'  => "vzdump-lxc-$vmid-2020_03_30-21_39_30.log",
+	    'notesfilename'=> "vzdump-lxc-$vmid-2020_03_30-21_39_30.tgz.notes",
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -69,6 +72,7 @@ my $tests = [
 	expected    => {
 	    'filename'     => "vzdump-openvz-$vmid-2020_03_30-21_39_30.tgz",
 	    'logfilename'  => "vzdump-openvz-$vmid-2020_03_30-21_39_30.log",
+	    'notesfilename'=> "vzdump-openvz-$vmid-2020_03_30-21_39_30.tgz.notes",
 	    'type'         => 'openvz',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -84,6 +88,7 @@ my $tests = [
 	expected    => {
 	    'filename'     => "vzdump-qemu-$vmid-2020_03_30-21_39_30.tgz",
 	    'logfilename'  => "vzdump-qemu-$vmid-2020_03_30-21_39_30.log",
+	    'notesfilename'=> "vzdump-qemu-$vmid-2020_03_30-21_39_30.tgz.notes",
 	    'type'         => 'qemu',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -138,6 +143,7 @@ for my $virt (sort keys %$bkp_suffix) {
 	    expected    => {
 		'filename'     => "vzdump-$virt-$vmid-2020_03_30-21_12_40.$format.$suffix",
 		'logfilename'  => "vzdump-$virt-$vmid-2020_03_30-21_12_40.log",
+		'notesfilename'=> "vzdump-$virt-$vmid-2020_03_30-21_12_40.$format.$suffix.notes",
 		'type'         => "$virt",
 		'format'       => "$format",
 		'decompressor' => $decomp->{$suffix},
-- 
2.30.2

Re: [pve-devel] [PATCH storage v2 2/2] fix #3972: test: Adapt test cases to changes in archive_info

[Fabian Ebner]

Am 12.05.22 um 15:17 schrieb Daniel Tschlatscher:
> The archive_info object now returns one more field containing the
> name of the notes file. The test cases have to be adapted to expect
> this new value as the package will not compile otherwise.

Ideally, the package will compile after each commit (good when
bisecting, cherry-picking etc.), so this should be part of the previous
patch.

Re: [pve-devel] [PATCH storage v2 1/2] fix #3972: Remove the .notes file when a backup is deleted

[Fabian Ebner]

Looks good to me, just a few suggestions for further improvements.

Am 12.05.22 um 15:17 schrieb Daniel Tschlatscher:
> When a VM or Container backup was deleted, the .notes file was not
> removed, therefore, over time the dump folder would get polluted with
> notes for backups that no longer existed.
> As backup names contain a timestamp and as the notes cannot be reused
> because of this, I think it is safe to just delete them just like we
> do with the .log file.
> 
> Furthermore, I sourced the deletion of the log and notes file into a
> new function called "archive_auxiliaries_remove". This change makes it
> easier to remove these files through archive_remove(), but also other
> parts in the code while keeping it DRY.

I like the helper. I think the second sentence above is not needed
though, as that's true for most helper functions ;)

> I saw this change necessary because archive_remove() does not invoke
> external plugins, and doing so might have unintended side effects.
> 

IMHO the point is that we have to keep calling vdisk_free() for PBS and
external plugins in the API call. If archive_remove() were designed to
call plugins, I'd argue that the side effects would be intended.
Usually, the plugin knows best what to do.

> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
> ---
>  PVE/API2/Storage/Content.pm |  5 ++---
>  PVE/Storage.pm              | 19 +++++++++++++++++--
>  2 files changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/PVE/API2/Storage/Content.pm b/PVE/API2/Storage/Content.pm
> index 8ff858d..41b577c 100644
> --- a/PVE/API2/Storage/Content.pm
> +++ b/PVE/API2/Storage/Content.pm
> @@ -456,9 +456,8 @@ __PACKAGE__->register_method ({
>  	    print "Removed volume '$volid'\n";
>  	    if ($vtype eq 'backup'
>  		&& $path =~ /(.*\/vzdump-\w+-\d+-\d{4}_\d{2}_\d{2}-\d{2}_\d{2}_\d{2})[^\/]+$/) {
> -		my $logpath = "$1.log";
> -		# try to cleanup our backup log file too, if still existing, #318
> -		unlink($logpath) if -e $logpath;
> +		# Remove log file #318 and notes file #3972 if they still exist
> +		PVE::Storage::archive_auxiliaries_remove($path);
>  	    }
>  	};
>  
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 72458cf..5317043 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1566,6 +1566,7 @@ sub archive_info {
>  
>  	if ($volid =~ /^(vzdump-${type}-([1-9][0-9]{2,8})-(\d{4})_(\d{2})_(\d{2})-(\d{2})_(\d{2})_(\d{2}))\.${extension}$/) {
>  	    $info->{logfilename} = "$1.log";
> +	    $info->{notesfilename} = "$filename.notes";

Nit: Could use PVE::Storage::Plugin::NOTES_EXT

>  	    $info->{vmid} = int($2);
>  	    $info->{ctime} = timelocal($8, $7, $6, $5, $4 - 1, $3);
>  	    $info->{is_std_name} = 1;
> @@ -1585,11 +1586,18 @@ sub archive_remove {
>      die "cannot remove protected archive '$archive_path'\n"
>  	if -e protection_file_path($archive_path);
>  
> +    unlink $archive_path or die "removing archive $archive_path failed: $!\n";

Not new, but this could be improved by using
  unlink $archive_path or $! == ENOENT or die "..."
like we do in other places. That would gracefully handle the case where
two operations raced, e.g. pruning and manual removal. Previously, one
of them would fail, because the file was already gone. Please make that
its own patch if you go for it, thanks!

> +
> +    archive_auxiliaries_remove($archive_path);
> +}
> +
> +sub archive_auxiliaries_remove {
> +    my ($archive_path) = @_;
> +
>      my $dirname = dirname($archive_path);
>      my $archive_info = eval { archive_info($archive_path) } // {};
>      my $logfn = $archive_info->{logfilename};
> -
> -    unlink $archive_path or die "removing archive $archive_path failed: $!\n";
> +    my $notesfn = $archive_info->{notesfilename};
>  

Style nit: The rest could be turned into a loop to avoid code repetition
like:

    for my $type (qw(log notes)) {
        my $fn = $archive_info->{"${type}filename"} or next;
        my $path = ...

>      if (defined($logfn)) {
>  	my $logpath = "$dirname/$logfn";
> @@ -1597,6 +1605,13 @@ sub archive_remove {
>  	    unlink $logpath or warn "removing log file $logpath failed: $!\n";

Same here with ENOENT

>  	}
>      }
> +
> +    if (defined($notesfn)) {
> +	my $notespath = "$dirname/$notesfn";
> +	if (-e $notespath) {
> +	    unlink $notespath or warn "removing notes file $notespath failed: $!\n";

and here.

> +	}
> +    }
>  }
>  
>  sub extract_vzdump_config_tar {

Re: [pve-devel] [PATCH storage v2 1/2] fix #3972: Remove the .notes file when a backup is deleted

[Fabian Ebner]

Am 12.05.22 um 15:17 schrieb Daniel Tschlatscher:
>  	    unlink $logpath or warn "removing log file $logpath failed: $!\n";

Forgot to say that we could switch to using PVE::RESTEnvironment's
log_warn here

>  	}
>      }
> +
> +    if (defined($notesfn)) {
> +	my $notespath = "$dirname/$notesfn";
> +	if (-e $notespath) {
> +	    unlink $notespath or warn "removing notes file $notespath failed: $!\n";

and here

> +	}
> +    }
>  }