[PATCH manager 1/2] api: ceph: pool: destroypool: keep EC profiles pveceph did not create

[Kefu Chai <k.chai@proxmox.com>]

destroypool removes the erasure code profile of the pool it destroys
unless remove_ecprofile=0 is passed. When createpool made that profile
itself, as pve_ec_<name> for the pool <name>-data, this cleans up after
it. But a pool can also use an existing profile through the 'profile'
option, such as 'default' or one built with the Ceph tools, and then
destroying the pool removes that profile too once no other pool uses
it.

So by default only remove the profile createpool made for the pool
being destroyed. An explicit remove_ecprofile=1 still removes any
profile.

While at it, test for a profile with length() rather than truthiness,
so a profile named '0' still gets its CRUSH rule and profile cleaned
up.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 PVE/API2/Ceph/Pool.pm | 16 ++++---
 PVE/Ceph/Tools.pm     |  6 +++
 test/CephPool_test.pl | 97 +++++++++++++++++++++++++++++++++++++++++++
 test/Makefile         |  6 ++-
 4 files changed, 119 insertions(+), 6 deletions(-)
 create mode 100755 test/CephPool_test.pl

diff --git a/PVE/API2/Ceph/Pool.pm b/PVE/API2/Ceph/Pool.pm
index 8fd11b82..1d6474e0 100644
--- a/PVE/API2/Ceph/Pool.pm
+++ b/PVE/API2/Ceph/Pool.pm
@@ -574,11 +574,10 @@ __PACKAGE__->register_method({
                 default => 0,
             },
             remove_ecprofile => {
-                description =>
-                    "Remove the erasure code profile. Defaults to true, if applicable.",
+                description => "Remove the erasure code profile. Defaults to true for a"
+                    . " profile pveceph created for the pool, and to false for any other.",
                 type => 'boolean',
                 optional => 1,
-                default => 1,
             },
         },
     },
@@ -618,16 +617,23 @@ __PACKAGE__->register_method({
 
             PVE::Ceph::Tools::destroy_pool($pool, $rados);
 
-            if (my $ecprofile = $pool_properties->{erasure_code_profile}) {
+            my $ecprofile = $pool_properties->{erasure_code_profile};
+            if (length($ecprofile)) { # a replicated pool has none, or an empty one
                 print "found erasure coded profile '$ecprofile', destroying its CRUSH rule\n";
                 my $crush_rule = $pool_properties->{crush_rule};
                 eval { PVE::Ceph::Tools::destroy_crush_rule($crush_rule, $rados); };
                 warn "removing crush rule '${crush_rule}' failed: $@\n" if $@;
 
-                if ($param->{remove_ecprofile} // 1) {
+                my $remove = $param->{remove_ecprofile}
+                    // PVE::Ceph::Tools::is_own_ecprofile($ecprofile, $pool);
+                if ($remove) {
                     print "destroying erasure coded profile '$ecprofile'\n";
                     eval { PVE::Ceph::Tools::destroy_ecprofile($ecprofile, $rados) };
                     warn "removing EC profile '${ecprofile}' failed: $@\n" if $@;
+                } elsif (!defined($param->{remove_ecprofile})) {
+                    print
+                        "keeping erasure coded profile '$ecprofile', pveceph did not create it"
+                        . " for this pool. Pass remove_ecprofile=1 to remove it.\n";
                 }
             }
 
diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
index 1ca0722d..cfd0c296 100644
--- a/PVE/Ceph/Tools.pm
+++ b/PVE/Ceph/Tools.pm
@@ -691,6 +691,12 @@ sub get_ecprofile_name {
     return "pve_ec_${name}";
 }
 
+# whether createpool made $profile for the data pool $pool, which it names <name>-data
+sub is_own_ecprofile {
+    my ($profile, $pool) = @_;
+    return $pool =~ m/^(.+)-data$/ && $profile eq get_ecprofile_name($1);
+}
+
 sub destroy_crush_rule {
     my ($rule, $rados) = @_;
     $rados = PVE::RADOS->new() if !$rados;
diff --git a/test/CephPool_test.pl b/test/CephPool_test.pl
new file mode 100755
index 00000000..730b712f
--- /dev/null
+++ b/test/CephPool_test.pl
@@ -0,0 +1,97 @@
+#!/usr/bin/perl
+
+use strict;
+use warnings;
+
+use lib ('.', '..');
+
+use Test::More;
+use PVE::API2::Ceph::Pool;
+use PVE::Ceph::Tools;
+
+{
+
+    package PoolRPCEnvironment;
+
+    sub get_user { return 'root@pam'; }
+
+    sub check { }
+
+    sub fork_worker {
+        my ($self, $type, $id, $user, $worker) = @_;
+        $worker->('test-upid');
+        return 'test-upid';
+    }
+}
+
+# call an API method, run its worker inline, and return what it printed
+my $run = sub {
+    my ($method, $param) = @_;
+    my $out = '';
+    open(my $stdout, '>', \$out) or die $!;
+    local *STDOUT = $stdout;
+    PVE::API2::Ceph::Pool->map_method_by_name($method)->{code}->($param);
+    return $out;
+};
+
+# --- the profile a pool owns --------------------------------------------------------------
+my @own_cases = (
+    # profile, pool, owned, name
+    ['pve_ec_foo', 'foo-data', 1, 'the profile made for the pool is its own'],
+    ['default', 'foo-data', 0, "'default' is not"],
+    ['pve_ec_bar', 'foo-data', 0, "another pool's profile is not"],
+    ['pve_ec_foo', 'foo', 0, "a pool without '-data' has no own profile"],
+    ['pve_ec_x-data', 'x-data-data', 1, "only the last '-data' is stripped"],
+);
+for my $case (@own_cases) {
+    my ($profile, $pool, $owned, $name) = @$case;
+    is(PVE::Ceph::Tools::is_own_ecprofile($profile, $pool) ? 1 : 0, $owned, $name);
+}
+
+# --- what destroypool removes -------------------------------------------------------------
+{
+    no warnings qw(redefine once);
+    my ($props, @removed);
+
+    local *PVE::RPCEnvironment::get = sub { bless {}, 'PoolRPCEnvironment' };
+    local *PVE::Ceph::Tools::check_ceph_inited = sub { };
+    local *PVE::Storage::config = sub { { ids => {} } };
+    local *PVE::RADOS::new = sub { 'rados' };
+    local *PVE::Ceph::Tools::get_pool_properties = sub { $props };
+    local *PVE::Ceph::Tools::destroy_pool = sub { };
+    local *PVE::Ceph::Tools::destroy_crush_rule = sub { push @removed, "rule $_[0]" };
+    local *PVE::Ceph::Tools::destroy_ecprofile = sub { push @removed, "profile $_[0]" };
+
+    my @cases = (
+        # profile, extra parameters, what gets removed, whether it says it keeps one, name
+        ['pve_ec_foo', {}, 'rule r, profile pve_ec_foo', 0, 'its own profile is removed'],
+        ['default', {}, 'rule r', 1, 'a profile the user made is kept, and the log says so'],
+        [
+            'default',
+            { remove_ecprofile => 1 },
+            'rule r, profile default',
+            0,
+            'remove_ecprofile=1 removes any profile',
+        ],
+        [
+            'pve_ec_foo',
+            { remove_ecprofile => 0 },
+            'rule r',
+            0,
+            'remove_ecprofile=0 keeps even its own profile',
+        ],
+        ['0', { remove_ecprofile => 1 }, 'rule r, profile 0', 0, "a profile named '0' counts"],
+        [undef, {}, '', 0, 'a replicated pool has no profile to remove'],
+        ['', {}, '', 0, 'nor has one that reports an empty profile'],
+    );
+    for my $case (@cases) {
+        my ($profile, $extra, $expected, $keeps, $name) = @$case;
+        $props = { crush_rule => 'r', erasure_code_profile => $profile };
+        @removed = ();
+        my $out = $run->('destroypool', { name => 'foo-data', %$extra });
+        is(join(', ', @removed), $expected, $name);
+        is($out =~ /keeping erasure coded profile/ ? 1 : 0, $keeps, "$name (log)");
+    }
+}
+
+done_testing();
diff --git a/test/Makefile b/test/Makefile
index 2dfd20a4..b8a3edde 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -5,7 +5,7 @@ all:
 export PERLLIB=..
 
 .PHONY: check
-check: test-replication test-balloon test-vzdump test-osd test-ceph-health test-ceph-services test-ceph-auth test-ceph-key-migration test-ceph-lockbox-migration test-custom-cpu-models test-pvesh
+check: test-replication test-balloon test-vzdump test-osd test-ceph-health test-ceph-pool test-ceph-services test-ceph-auth test-ceph-key-migration test-ceph-lockbox-migration test-custom-cpu-models test-pvesh
 
 .PHONY: test-balloon
 test-balloon:
@@ -36,6 +36,10 @@ test-osd:
 test-ceph-health:
 	./CephHealth_test.pl
 
+.PHONY: test-ceph-pool
+test-ceph-pool:
+	./CephPool_test.pl
+
 .PHONY: test-ceph-services
 test-ceph-services:
 	./CephServices_test.pl
-- 
2.47.3