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

[Maximiliano Sandoval <m.sandoval@proxmox.com>]

Kefu Chai <k.chai@proxmox.com> writes:

Comments below.

> 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.",

Minor nit: Perhaps:

"Remove the erasure code profile. Enabled by default for pools created
 during 'pveceph create', disabled otherwise."

>                  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

nit: Can this be written as follows?

# Replicated pool profiles have zero length
if (my $ecprofile = $pool_properties->{erasure_code_profile} && length($ecprofile)) {

>                  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";

nit: Looks good to me, but perhaps this is a bit too late to mention this.

If the first incantation of pveceph pool destroy is called without
remove_ecprofile then the profile is not destroyed and repeating the
call with the remove_ecprofile=1 wont remove the profile as it exits
early, e.g.

root@c3-pve-201:~# ceph osd erasure-code-profile set test k=3 m=2
root@c3-pve-201:~# pveceph pool create test-ec --erasure-coding profile=test
[..]
root@c3-pve-201:~# pveceph pool destroy test-ec-data ; pveceph pool destroy test-ec-metadata
root@c3-pve-201:~# pveceph pool destroy test-ec-data  --remove_ecprofile 1
checking storage 'test-ec' for RBD images..
rbd error: rbd: listing images failed: (2) No such file or directory
root@c3-pve-201:~# ceph osd erasure-code-profile ls
test
default

>                  }
>              }
>  
> 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;

nit: New files could preferably use

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

-- 
Maximiliano