[PATCH manager v2 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. When the profile is kept, say so and how to remove it later
with the Ceph tools, since running destroypool again with
remove_ecprofile=1 fails once the pool is gone.

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.

Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Signed-off-by: Kefu Chai <k.chai@proxmox.com>
---
 PVE/API2/Ceph/Pool.pm | 17 +++++---
 PVE/Ceph/Tools.pm     |  6 +++
 test/CephPool_test.pl | 96 +++++++++++++++++++++++++++++++++++++++++++
 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..dd505872 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. Enabled by default if"
+                    . " 'pveceph pool create' made the profile for this pool, disabled otherwise.",
                 type => 'boolean',
                 optional => 1,
-                default => 1,
             },
         },
     },
@@ -618,16 +617,24 @@ __PACKAGE__->register_method({
 
             PVE::Ceph::Tools::destroy_pool($pool, $rados);
 
-            if (my $ecprofile = $pool_properties->{erasure_code_profile}) {
+            # a replicated pool has no profile, or an empty one
+            if (length(my $ecprofile = $pool_properties->{erasure_code_profile})) {
                 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. To remove it once no pool uses it, run"
+                        . " 'ceph osd erasure-code-profile rm $ecprofile'.\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..9077aacf
--- /dev/null
+++ b/test/CephPool_test.pl
@@ -0,0 +1,96 @@
+#!/usr/bin/perl
+
+use v5.36;
+
+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