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

[pve-devel] [PATCH storage v4 1/4] 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". Additionally, 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>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v3
* removed unused variables $logfn and $notesfn

 PVE/API2/Storage/Content.pm |  5 ++---
 PVE/Storage.pm              | 20 ++++++++++++++------
 test/archive_info_test.pm   |  6 ++++++
 3 files changed, 22 insertions(+), 9 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..70dd663 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,16 +1586,23 @@ 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";
+    for my $type (qw(log notes)) {
+	my $filename = $archive_info->{"${type}filename"} or next;
+	my $path = "$dirname/$filename";
 
-    if (defined($logfn)) {
-	my $logpath = "$dirname/$logfn";
-	if (-e $logpath) {
-	    unlink $logpath or warn "removing log file $logpath failed: $!\n";
+	if (-e $path) {
+	    unlink $path or warn "Removing $type file failed: $!\n";
 	}
     }
 }
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

[pve-devel] [PATCH storage v4 2/4] Adapted unlink calls for archive files in case of ENOENT

[Daniel Tschlatscher]

This improves handling when two archive remove calls are creating a
race condition where one would formerly encounter an error. Now both
finish successfully.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v3
* No fix #xxxx in commit title

 PVE/Storage.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 70dd663..97ea64e 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1586,7 +1586,7 @@ 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";
+    unlink $archive_path or $! == ENOENT or die "removing archive $archive_path failed: $!\n";
 
     archive_auxiliaries_remove($archive_path);
 }
@@ -1602,7 +1602,7 @@ sub archive_auxiliaries_remove {
 	my $path = "$dirname/$filename";
 
 	if (-e $path) {
-	    unlink $path or warn "Removing $type file failed: $!\n";
+	    unlink $path or $! == ENOENT or warn "Removing $type file failed: $!\n";
 	}
     }
 }
-- 
2.30.2

[pve-devel] [PATCH storage v4 3/4] Switched to using log_warn of PVE::RESTEnvironment

[Daniel Tschlatscher]

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v3
* No fix #xxxx in commit title
* Added import for PVE::RESTEnvironment to call log_warn directly

 PVE/Storage.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 97ea64e..0d53ba2 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -23,6 +23,7 @@ use PVE::JSONSchema;
 use PVE::INotify;
 use PVE::RPCEnvironment;
 use PVE::SSHInfo;
+use PVE::RESTEnvironment qw(log_warn);
 
 use PVE::Storage::Plugin;
 use PVE::Storage::DirPlugin;
@@ -1602,7 +1603,7 @@ sub archive_auxiliaries_remove {
 	my $path = "$dirname/$filename";
 
 	if (-e $path) {
-	    unlink $path or $! == ENOENT or warn "Removing $type file failed: $!\n";
+	    unlink $path or $! == ENOENT or log_warn("Removing $type file failed: $!");
 	}
     }
 }
-- 
2.30.2

[pve-devel] [PATCH storage v4 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT

[Daniel Tschlatscher]

and refactored usages for .log and .notes with them.
At some parts in the test case code I had to source new variables to
shorten the line length to not exceed the 100 column line limit.

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>
---
Changes from v3
* File extension constants are now ordered alphabetically

 PVE/Storage.pm            |  4 ++--
 PVE/Storage/Plugin.pm     |  1 +
 test/archive_info_test.pm | 32 ++++++++++++++++++--------------
 3 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 0d53ba2..b9c53a1 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1566,8 +1566,8 @@ sub archive_info {
 	$info->{type} = $type;
 
 	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->{logfilename} = "$1".PVE::Storage::Plugin::LOG_EXT;
+	    $info->{notesfilename} = "$filename".PVE::Storage::Plugin::NOTES_EXT;
 	    $info->{vmid} = int($2);
 	    $info->{ctime} = timelocal($8, $7, $6, $5, $4 - 1, $3);
 	    $info->{is_std_name} = 1;
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index 0a100b7..8a41df1 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -20,6 +20,7 @@ use base qw(PVE::SectionConfig);
 
 use constant COMPRESSOR_RE => 'gz|lzo|zst';
 
+use constant LOG_EXT => ".log";
 use constant NOTES_EXT => ".notes";
 
 our @COMMON_TAR_FLAGS = qw(
diff --git a/test/archive_info_test.pm b/test/archive_info_test.pm
index bbc0e6f..cf03c3d 100644
--- a/test/archive_info_test.pm
+++ b/test/archive_info_test.pm
@@ -10,6 +10,9 @@ use Test::More;
 
 my $vmid = 16110;
 
+my $LOG_EXT = PVE::Storage::Plugin::LOG_EXT;
+my $NOTES_EXT = PVE::Storage::Plugin::NOTES_EXT;
+
 # an array of test cases, each test is comprised of the following keys:
 # description => to identify a single test
 # archive     => the input filename for archive_info
@@ -23,8 +26,8 @@ my $tests = [
 	archive     => "backup/vzdump-lxc-$vmid-3070_01_01-00_00_00.tgz",
 	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",
+	    'logfilename'  => "vzdump-lxc-$vmid-3070_01_01-00_00_00".$LOG_EXT,
+	    'notesfilename'=> "vzdump-lxc-$vmid-3070_01_01-00_00_00.tgz".$NOTES_EXT,
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -39,8 +42,8 @@ my $tests = [
 	archive     => "backup/vzdump-lxc-$vmid-1970_01_01-02_00_30.tgz",
 	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",
+	    'logfilename'  => "vzdump-lxc-$vmid-1970_01_01-02_00_30".$LOG_EXT,
+	    'notesfilename'=> "vzdump-lxc-$vmid-1970_01_01-02_00_30.tgz".$NOTES_EXT,
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -55,8 +58,8 @@ my $tests = [
 	archive     => "backup/vzdump-lxc-$vmid-2020_03_30-21_39_30.tgz",
 	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",
+	    'logfilename'  => "vzdump-lxc-$vmid-2020_03_30-21_39_30".$LOG_EXT,
+	    'notesfilename'=> "vzdump-lxc-$vmid-2020_03_30-21_39_30.tgz".$NOTES_EXT,
 	    'type'         => 'lxc',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -71,8 +74,8 @@ my $tests = [
 	archive     => "backup/vzdump-openvz-$vmid-2020_03_30-21_39_30.tgz",
 	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",
+	    'logfilename'  => "vzdump-openvz-$vmid-2020_03_30-21_39_30".$LOG_EXT,
+	    'notesfilename'=> "vzdump-openvz-$vmid-2020_03_30-21_39_30.tgz".$NOTES_EXT,
 	    'type'         => 'openvz',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -87,8 +90,8 @@ my $tests = [
 	archive     => "/here/be/Back-ups/vzdump-qemu-$vmid-2020_03_30-21_39_30.tgz",
 	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",
+	    'logfilename'  => "vzdump-qemu-$vmid-2020_03_30-21_39_30".$LOG_EXT,
+	    'notesfilename'=> "vzdump-qemu-$vmid-2020_03_30-21_39_30.tgz".$NOTES_EXT,
 	    'type'         => 'qemu',
 	    'format'       => 'tar',
 	    'decompressor' => ['tar', '-z'],
@@ -135,15 +138,16 @@ my $bkp_suffix = {
 # create more test cases for backup files matches
 for my $virt (sort keys %$bkp_suffix) {
     my ($format, $decomp) = $bkp_suffix->{$virt}->@*;
+    my $archive_name = "vzdump-$virt-$vmid-2020_03_30-21_12_40";
 
     for my $suffix (sort keys %$decomp) {
 	push @$tests, {
 	    description => "Backup archive, $virt, $format.$suffix",
-	    archive     => "backup/vzdump-$virt-$vmid-2020_03_30-21_12_40.$format.$suffix",
+	    archive     => "backup/$archive_name.$format.$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",
+		'filename'     => "$archive_name.$format.$suffix",
+		'logfilename'  => $archive_name.$LOG_EXT,
+		'notesfilename'=> "$archive_name.$format.$suffix".$NOTES_EXT,
 		'type'         => "$virt",
 		'format'       => "$format",
 		'decompressor' => $decomp->{$suffix},
-- 
2.30.2

[pve-devel] applied series: [PATCH storage v4 1/4] fix #3972: Remove the .notes file when a backup is deleted

[Wolfgang Bumiller]

applied series and removed the empty lines between the S-o-b and R-b
lines.

On Tue, Jun 14, 2022 at 11:00:10AM +0200, Daniel Tschlatscher wrote:
> 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". Additionally, 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>
> 
> Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>