[pve-devel] [PATCH storage] prune mark: correctly keep track of already included backups

[Fabian Ebner <f.ebner@proxmox.com>]

This needs to happen in a separate loop, because some time intervals are not
subsets of others, i.e. weeks and months. Previously, with a daily backup
schedule, having:
* a backup on Sun, 06 Dec 2020 kept by keep-daily
* a backup on Sun, 29 Nov 2020 kept by keep-weekly
would lead to the backup on Mon, 30 Nov 2020 to be selected for keep-monthly,
because the iteration did not yet reach the backup on Sun, 29 Nov 2020 that
would mark November as being covered.

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

The PBS implementation is not affected by this as it uses two loops.
I don't see how this would cause bug #3199 though...

 PVE/Storage.pm             | 11 +++++----
 test/prune_backups_test.pm | 49 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 5 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index aded60e..c1a21b4 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1646,13 +1646,14 @@ my $prune_mark = sub {
     foreach my $prune_entry (@{$prune_entries}) {
 	my $mark = $prune_entry->{mark};
 	my $id = $id_func->($prune_entry->{ctime});
+	$already_included->{$id} = 1 if defined($mark) && $mark eq 'keep';
+    }
 
-	next if $already_included->{$id};
+    foreach my $prune_entry (@{$prune_entries}) {
+	my $mark = $prune_entry->{mark};
+	my $id = $id_func->($prune_entry->{ctime});
 
-	if (defined($mark)) {
-	    $already_included->{$id} = 1 if $mark eq 'keep';
-	    next;
-	}
+	next if defined($mark) || $already_included->{$id};
 
 	if (!$newly_included->{$id}) {
 	    last if scalar(keys %{$newly_included}) >= $keep_count;
diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
index 8bf564d..c69c467 100644
--- a/test/prune_backups_test.pm
+++ b/test/prune_backups_test.pm
@@ -74,6 +74,23 @@ push @{$mocked_backups_lists->{novmid}}, (
 	'ctime' => 1234,
     },
 );
+push @{$mocked_backups_lists->{threeway}}, (
+    {
+	'volid' => "$storeid:backup/vzdump-qemu-7654-2019_12_25-12_18_21.tar.zst",
+	'ctime' => $basetime - 7*24*60*60,
+	'vmid'  => 7654,
+    },
+    {
+	'volid' => "$storeid:backup/vzdump-qemu-7654-2019_12_31-12_18_21.tar.zst",
+	'ctime' => $basetime - 24*60*60,
+	'vmid'  => 7654,
+    },
+    {
+	'volid' => "$storeid:backup/vzdump-qemu-7654-2020_01_01-12_18_21.tar.zst",
+	'ctime' => $basetime,
+	'vmid'  => 7654,
+    },
+);
 my $current_list;
 my $mock_plugin = Test::MockModule->new('PVE::Storage::Plugin');
 $mock_plugin->redefine(list_volumes => sub {
@@ -361,6 +378,38 @@ my $tests = [
 	},
 	expected => generate_expected(\@vmids, undef, ['keep', 'keep', 'keep', 'keep', 'keep', 'keep']),
     },
+    {
+	description => 'daily=weekly=monthly=1',
+	keep => {
+	    'keep-daily' => 1,
+	    'keep-weekly' => 1,
+	    'keep-monthly' => 1,
+	},
+	list => 'threeway',
+	expected => [
+	    {
+		'volid' => "$storeid:backup/vzdump-qemu-7654-2019_12_25-12_18_21.tar.zst",
+		'ctime' => $basetime - 7*24*60*60,
+		'type'  => 'qemu',
+		'vmid'  => 7654,
+		'mark'  => 'keep',
+	    },
+	    {
+		'volid' => "$storeid:backup/vzdump-qemu-7654-2019_12_31-12_18_21.tar.zst",
+		'ctime' => $basetime - 24*60*60,
+		'type'  => 'qemu',
+		'vmid'  => 7654,
+		'mark'  => 'remove', # month is already covered by the backup kept by keep-weekly!
+	    },
+	    {
+		'volid' => "$storeid:backup/vzdump-qemu-7654-2020_01_01-12_18_21.tar.zst",
+		'ctime' => $basetime,
+		'type'  => 'qemu',
+		'vmid'  => 7654,
+		'mark'  => 'keep',
+	    },
+	],
+    },
 ];
 
 plan tests => scalar @$tests;
-- 
2.20.1