[pve-devel] [PATCH storage 1/3] don't pass along keep-options equal to zero to PBS

[pve-devel] [PATCH storage 1/3] don't pass along keep-options equal to zero to PBS

[Fabian Ebner]

In PBS, zero is not allowed for these options.

Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
---
 PVE/Storage/PBSPlugin.pm | 1 +
 1 file changed, 1 insertion(+)

diff --git a/PVE/Storage/PBSPlugin.pm b/PVE/Storage/PBSPlugin.pm
index f3bf016..7b0c030 100644
--- a/PVE/Storage/PBSPlugin.pm
+++ b/PVE/Storage/PBSPlugin.pm
@@ -314,6 +314,7 @@ sub prune_backups {
 
     my @param;
     foreach my $opt (keys %{$keep}) {
+	next if $keep->{$opt} == 0;
 	push @param, "--$opt";
 	push @param, "$keep->{$opt}";
     }
-- 
2.20.1

[pve-devel] [PATCH storage 2/3] prune mark: keep all if all prune options are zero/missing

[Fabian Ebner]

as an additional safety measure. And add some tests.

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

This also would've prevented the unwanted removals with the maxfiles
handling I had messed up, because there, prune_mark_backup_group was called
with an empty hash.

 PVE/Storage.pm             |  7 +++++++
 test/prune_backups_test.pm | 40 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index 932f0f1..a2f400c 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -1622,6 +1622,13 @@ my $prune_mark = sub {
 sub prune_mark_backup_group {
     my ($backup_group, $keep) = @_;
 
+    if (!scalar(grep {$_ > 0} values %{$keep})) {
+	foreach my $prune_entry (@{$backup_group}) {
+	    $prune_entry->{mark} = 'keep';
+	}
+	return;
+    }
+
     my $prune_list = [ sort { $b->{ctime} <=> $a->{ctime} } @{$backup_group} ];
 
     $prune_mark->($prune_list, $keep->{'keep-last'}, sub {
diff --git a/test/prune_backups_test.pm b/test/prune_backups_test.pm
index 1e6a3d1..8bf564d 100644
--- a/test/prune_backups_test.pm
+++ b/test/prune_backups_test.pm
@@ -239,6 +239,18 @@ my $tests = [
 	},
 	expected => generate_expected(\@vmids, undef, ['keep', 'remove', 'keep', 'remove', 'keep', 'keep']),
     },
+    {
+	description => 'last=daily=weekly=1, others zero, multiple IDs',
+	keep => {
+	    'keep-hourly' => 0,
+	    'keep-last' => 1,
+	    'keep-daily' => 1,
+	    'keep-weekly' => 1,
+	    'keep-monthly' => 0,
+	    'keep-yearly' => 0,
+	},
+	expected => generate_expected(\@vmids, undef, ['keep', 'remove', 'keep', 'remove', 'keep', 'keep']),
+    },
     {
 	description => 'daily=2, one ID',
 	vmid => $vmids[0],
@@ -321,6 +333,34 @@ my $tests = [
 	    },
 	],
     },
+    {
+	description => 'all missing, multiple IDs',
+	keep => {},
+	expected => generate_expected(\@vmids, undef, ['keep', 'keep', 'keep', 'keep', 'keep', 'keep']),
+    },
+    {
+	description => 'all zero, multiple IDs',
+	keep => {
+	    'keep-last' => 0,
+	    'keep-hourly' => 0,
+	    'keep-daily' => 0,
+	    'keep-weekly' => 0,
+	    'keep-monthyl' => 0,
+	    'keep-yearly' => 0,
+	},
+	expected => generate_expected(\@vmids, undef, ['keep', 'keep', 'keep', 'keep', 'keep', 'keep']),
+    },
+    {
+	description => 'some zero, some missing, multiple IDs',
+	keep => {
+	    'keep-last' => 0,
+	    'keep-hourly' => 0,
+	    'keep-daily' => 0,
+	    'keep-monthyl' => 0,
+	    'keep-yearly' => 0,
+	},
+	expected => generate_expected(\@vmids, undef, ['keep', 'keep', 'keep', 'keep', 'keep', 'keep']),
+    },
 ];
 
 plan tests => scalar @$tests;
-- 
2.20.1

[pve-devel] [PATCH storage 3/3] prune: allow having all prune options zero/missing

[Fabian Ebner]

This is basically necessary for the GUI's prune widget, because we want to
pass along all options equal to zero when all the number fields are cleared.
And it's more similar to how it's done in PBS now.

Bumped the APIAGE and APIVER, in case some external plugin needs to adapt to
the now less restrictive schema for 'prune-backups'.

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

One could work around the issue in the GUI by not calling the prune API in
case all number fields are empty, and change the 'keep' value to 'true' for
each backup in the store. But that is rather hacky and being more in line with
PBS seems to be a good thing in any case.

 PVE/Storage.pm        |  4 ++--
 PVE/Storage/Plugin.pm | 10 +---------
 2 files changed, 3 insertions(+), 11 deletions(-)

diff --git a/PVE/Storage.pm b/PVE/Storage.pm
index a2f400c..bd6e15e 100755
--- a/PVE/Storage.pm
+++ b/PVE/Storage.pm
@@ -41,11 +41,11 @@ use PVE::Storage::DRBDPlugin;
 use PVE::Storage::PBSPlugin;
 
 # Storage API version. Increment it on changes in storage API interface.
-use constant APIVER => 7;
+use constant APIVER => 8;
 # Age is the number of versions we're backward compatible with.
 # This is like having 'current=APIVER' and age='APIAGE' in libtool,
 # see https://www.gnu.org/software/libtool/manual/html_node/Libtool-versioning.html
-use constant APIAGE => 6;
+use constant APIAGE => 7;
 
 # load standard plugins
 PVE::Storage::DirPlugin->register();
diff --git a/PVE/Storage/Plugin.pm b/PVE/Storage/Plugin.pm
index b4f3be8..fe56864 100644
--- a/PVE/Storage/Plugin.pm
+++ b/PVE/Storage/Plugin.pm
@@ -82,15 +82,7 @@ our $prune_backups_format = {
 		       'than one backup for a single year, only the latest one is kept.'
     },
 };
-PVE::JSONSchema::register_format('prune-backups', $prune_backups_format, \&validate_prune_backups);
-sub validate_prune_backups {
-    my ($keep) = @_;
-
-    die "at least one keep-option must be set and positive\n"
-	if !grep { $_ } values %{$keep};
-
-    return $keep;
-}
+PVE::JSONSchema::register_format('prune-backups', $prune_backups_format);
 register_standard_option('prune-backups', {
     description => "The retention options with shorter intervals are processed first " .
 		   "with --keep-last being the very first one. Each option covers a " .
-- 
2.20.1

[pve-devel] applied-series: [PATCH storage 1/3] don't pass along keep-options equal to zero to PBS

[Thomas Lamprecht]

On 13.11.20 14:08, Fabian Ebner wrote:
> In PBS, zero is not allowed for these options.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
>  PVE/Storage/PBSPlugin.pm | 1 +
>  1 file changed, 1 insertion(+)
> 
>

applied series, thanks!