From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D767760439 for ; Mon, 14 Dec 2020 16:03:23 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CD5769ED3 for ; Mon, 14 Dec 2020 16:03:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 777039EC8 for ; Mon, 14 Dec 2020 16:03:22 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 44FD2450C9 for ; Mon, 14 Dec 2020 16:03:22 +0100 (CET) From: Fabian Ebner To: pve-devel@lists.proxmox.com Date: Mon, 14 Dec 2020 15:03:17 +0000 Message-Id: <20201214150317.28734-1-f.ebner@proxmox.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.009 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: [pve-devel] [PATCH storage] prune mark: correctly keep track of already included backups X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 14 Dec 2020 15:03:23 -0000 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 --- 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