public inbox for pve-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted
@ 2022-05-20 13:28 Daniel Tschlatscher
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT Daniel Tschlatscher
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Daniel Tschlatscher @ 2022-05-20 13:28 UTC (permalink / raw)
  To: pve-devel

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>
---
 PVE/API2/Storage/Content.pm |  5 ++---
 PVE/Storage.pm              | 20 +++++++++++++++-----
 test/archive_info_test.pm   |  6 ++++++
 3 files changed, 23 insertions(+), 8 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..93c3b4e 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,25 @@ 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};
+    my $notesfn = $archive_info->{notesfilename};
 
-    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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT
  2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
@ 2022-05-20 13:28 ` Daniel Tschlatscher
  2022-06-13  8:34   ` Fabian Ebner
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment Daniel Tschlatscher
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Tschlatscher @ 2022-05-20 13:28 UTC (permalink / raw)
  To: pve-devel

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>
---
 PVE/Storage.pm | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 93c3b4e..6f2558f 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);
 }
@@ -1604,7 +1604,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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment
  2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT Daniel Tschlatscher
@ 2022-05-20 13:28 ` Daniel Tschlatscher
  2022-06-13  8:34   ` Fabian Ebner
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT Daniel Tschlatscher
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Tschlatscher @ 2022-05-20 13:28 UTC (permalink / raw)
  To: pve-devel

Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>
---
 PVE/Storage.pm | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 6f2558f..98ecd60 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1604,7 +1604,8 @@ 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 PVE::RESTEnvironment::log_warn("Removing $type file failed: $!");
 	}
     }
 }
-- 
2.30.2





^ permalink raw reply	[flat|nested] 10+ messages in thread

* [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT
  2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT Daniel Tschlatscher
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment Daniel Tschlatscher
@ 2022-05-20 13:28 ` Daniel Tschlatscher
  2022-06-13  8:34   ` Fabian Ebner
  2022-05-25 15:06 ` [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Thomas Lamprecht
  2022-06-13  8:33 ` Fabian Ebner
  4 siblings, 1 reply; 10+ messages in thread
From: Daniel Tschlatscher @ 2022-05-20 13:28 UTC (permalink / raw)
  To: pve-devel

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>
---
 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 98ecd60..d2e1a5b 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1565,8 +1565,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..7e53189 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -21,6 +21,7 @@ use base qw(PVE::SectionConfig);
 use constant COMPRESSOR_RE => 'gz|lzo|zst';
 
 use constant NOTES_EXT => ".notes";
+use constant LOG_EXT => ".log";
 
 our @COMMON_TAR_FLAGS = qw(
     --one-file-system
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





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted
  2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
                   ` (2 preceding siblings ...)
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT Daniel Tschlatscher
@ 2022-05-25 15:06 ` Thomas Lamprecht
  2022-05-25 16:25   ` Daniel Tschlatscher
  2022-06-13  8:33 ` Fabian Ebner
  4 siblings, 1 reply; 10+ messages in thread
From: Thomas Lamprecht @ 2022-05-25 15:06 UTC (permalink / raw)
  To: Proxmox VE development discussion, Daniel Tschlatscher

On 20/05/2022 15:28, 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>
> ---

the "changes since v2" sections are missing from all patches, making it
harder to check if all comments got addressed or what other changes there
were...

>  PVE/API2/Storage/Content.pm |  5 ++---
>  PVE/Storage.pm              | 20 +++++++++++++++-----
>  test/archive_info_test.pm   |  6 ++++++
>  3 files changed, 23 insertions(+), 8 deletions(-)
> 





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted
  2022-05-25 15:06 ` [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Thomas Lamprecht
@ 2022-05-25 16:25   ` Daniel Tschlatscher
  0 siblings, 0 replies; 10+ messages in thread
From: Daniel Tschlatscher @ 2022-05-25 16:25 UTC (permalink / raw)
  To: Thomas Lamprecht, Proxmox VE development discussion


On 5/25/22 17:06, Thomas Lamprecht wrote:
> On 20/05/2022 15:28, 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>
>> ---
> the "changes since v2" sections are missing from all patches, making it
> harder to check if all comments got addressed or what other changes there
> were...
>
>>   PVE/API2/Storage/Content.pm |  5 ++---
>>   PVE/Storage.pm              | 20 +++++++++++++++-----
>>   test/archive_info_test.pm   |  6 ++++++
>>   3 files changed, 23 insertions(+), 8 deletions(-)
>>
Sorry for that, the changes from v2 are:

- Test cases are now part of the initial commit to make it compilable by 
itself.

- File 'unlink' now checks whether the file is already gone to avoid 
errors when removals are racing

- The code to 'unlink' the files was made a bit more concise and DRY by 
utilizing a loop

- Instead of plain 'warn' now use 'log_warn()'

- The file extensions for '.notes' and '.log' files now use constants 
declared in 'PVE::Storage::Plugin'





^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted
  2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
                   ` (3 preceding siblings ...)
  2022-05-25 15:06 ` [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Thomas Lamprecht
@ 2022-06-13  8:33 ` Fabian Ebner
  4 siblings, 0 replies; 10+ messages in thread
From: Fabian Ebner @ 2022-06-13  8:33 UTC (permalink / raw)
  To: pve-devel, d.tschlatscher

Am 20.05.22 um 15:28 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". 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>

Only found small nits, so consider the series:
Reviewed-by: Fabian Ebner <f.ebner@proxmox.com>

> @@ -1585,16 +1586,25 @@ 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};
> +    my $notesfn = $archive_info->{notesfilename};

Both $logfn and $notesfn are unused.

>  
> -    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";
>  	}
>      }
>  }




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT Daniel Tschlatscher
@ 2022-06-13  8:34   ` Fabian Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Fabian Ebner @ 2022-06-13  8:34 UTC (permalink / raw)
  To: pve-devel, d.tschlatscher

Am 20.05.22 um 15:28 schrieb 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>

Commit title shouldn't include "fix #3972". The first patch does that ;)




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment Daniel Tschlatscher
@ 2022-06-13  8:34   ` Fabian Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Fabian Ebner @ 2022-06-13  8:34 UTC (permalink / raw)
  To: pve-devel, d.tschlatscher

Am 20.05.22 um 15:28 schrieb Daniel Tschlatscher:
> Signed-off-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>

Commit title shouldn't include "fix #3972"

> ---
>  PVE/Storage.pm | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/PVE/Storage.pm b/PVE/Storage.pm
> index 6f2558f..98ecd60 100755
> --- a/PVE/Storage.pm
> +++ b/PVE/Storage.pm
> @@ -1604,7 +1604,8 @@ 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 PVE::RESTEnvironment::log_warn("Removing $type file failed: $!");

Missing use/import for the module. And you could import log_warn
directly so that it can be used without the package prefix to make
everything fit on one line here.

>  	}
>      }
>  }




^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT
  2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT Daniel Tschlatscher
@ 2022-06-13  8:34   ` Fabian Ebner
  0 siblings, 0 replies; 10+ messages in thread
From: Fabian Ebner @ 2022-06-13  8:34 UTC (permalink / raw)
  To: pve-devel, d.tschlatscher

Am 20.05.22 um 15:28 schrieb Daniel Tschlatscher:
> diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
> index 0a100b7..7e53189 100644
> --- a/PVE/Storage/Plugin.pm
> +++ b/PVE/Storage/Plugin.pm
> @@ -21,6 +21,7 @@ use base qw(PVE::SectionConfig);
>  use constant COMPRESSOR_RE => 'gz|lzo|zst';
>  
>  use constant NOTES_EXT => ".notes";
> +use constant LOG_EXT => ".log";

Nit: not ordered alphabetically




^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2022-06-13  8:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-20 13:28 [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Daniel Tschlatscher
2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 2/4] fix #3972: Adapted unlink calls for archive files in case of ENOENT Daniel Tschlatscher
2022-06-13  8:34   ` Fabian Ebner
2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 3/4] fix #3972: Switched to using log_warn of PVE::RESTEnvironment Daniel Tschlatscher
2022-06-13  8:34   ` Fabian Ebner
2022-05-20 13:28 ` [pve-devel] [PATCH storage v3 4/4] Added a LOG_EXT constant as a counterpart to NOTES_EXT Daniel Tschlatscher
2022-06-13  8:34   ` Fabian Ebner
2022-05-25 15:06 ` [pve-devel] [PATCH storage v3 1/4] fix #3972: Remove the .notes file when a backup is deleted Thomas Lamprecht
2022-05-25 16:25   ` Daniel Tschlatscher
2022-06-13  8:33 ` Fabian Ebner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal