Re: [PATCH manager 2/2] api: ceph: pool: createpool: make 'k' and 'm' optional with a profile

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

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

> The documentation tells the user to build a custom EC profile with the
> Ceph tools and then pass it as 'profile'. That call is rejected:
>
>   # pveceph pool create test-ec --erasure-coding profile=default
>   400 Parameter verification failed.
>   erasure-coding: invalid format - format error
>   erasure-coding.m: property is missing and it is not optional
>   erasure-coding.k: property is missing and it is not optional
>
> 'k' and 'm' only feed create_ecprofile(), which createpool skips for a
> pool that gets an existing profile. So mark them optional and require
> them only when no profile is given. The schema cannot say that, so the
> descriptions do.
>
> Until now, 'k', 'm', 'failure-domain' and 'device-class' given next to
> a profile were dropped without a word, and the pool got whatever the
> profile defines. Reject values that differ from the profile instead,
> so a mistake shows up before any pool is created. Values that match
> are still accepted. A single 'osd erasure-code-profile get' reads the
> profile and also tells whether it exists, so it replaces
> ecprofile_exists(), which nothing else uses.

In my opinion, failing earlier rather than ignoring user input is a sane
approach. +1

>
> This changes the API for one kind of caller: since the old schema
> required 'k' and 'm', a script may pass placeholder values next to a
> profile. If they differ from the profile, the call now fails, and the
> script should drop them. The GUI does not set 'profile'.
>
> Also check whether a profile was given with defined(), here and in
> create_pool(), so a profile named '0' is not taken as missing.
>
> Signed-off-by: Kefu Chai <k.chai@proxmox.com>
> ---
>  PVE/API2/Ceph/Pool.pm |  56 ++++++++++---
>  PVE/Ceph/Tools.pm     |  19 +++--
>  test/CephPool_test.pl | 178 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 233 insertions(+), 20 deletions(-)
>
> diff --git a/PVE/API2/Ceph/Pool.pm b/PVE/API2/Ceph/Pool.pm
> index 1d6474e0..aac94996 100644
> --- a/PVE/API2/Ceph/Pool.pm
> +++ b/PVE/API2/Ceph/Pool.pm
> @@ -379,33 +379,37 @@ my $ec_format = {
>      k => {
>          type => 'integer',
>          description => "Number of data chunks. Will create an erasure coded pool plus a"
> -            . " replicated pool for metadata.",
> +            . " replicated pool for metadata. Required if no 'profile' is given.",
>          minimum => 2,
> +        optional => 1,
>      },
>      m => {
>          type => 'integer',
>          description => "Number of coding chunks. Will create an erasure coded pool plus a"
> -            . " replicated pool for metadata.",
> +            . " replicated pool for metadata. Required if no 'profile' is given.",
>          minimum => 1,
> +        optional => 1,
>      },
>      'failure-domain' => {
>          type => 'string',
> -        description => "CRUSH failure domain. Default is 'host'. Will create an erasure"
> -            . " coded pool plus a replicated pool for metadata.",
> +        description => "CRUSH failure domain. Default is 'host' if no 'profile' is given."
> +            . " Will create an erasure coded pool plus a replicated pool for metadata. With a"
> +            . " 'profile', it must match the profile's.",

nit: If 'profile' is set, it must match the failure-domain of the
profile. (Similar for other's).

>          format_description => 'domain',
>          optional => 1,
> -        default => 'host',
>      },
>      'device-class' => {
>          type => 'string',
>          description => "CRUSH device class. Will create an erasure coded pool plus a"
> -            . " replicated pool for metadata.",
> +            . " replicated pool for metadata. With a 'profile', it must match the profile's.",
>          format_description => 'class',
>          optional => 1,
>      },
>      profile => {
> -        description => "Override the erasure code (EC) profile to use. Will create an"
> -            . " erasure coded pool plus a replicated pool for metadata.",
> +        description => "Use this existing erasure code (EC) profile instead of creating one"
> +            . " from 'k', 'm', 'failure-domain' and 'device-class'. Any of those given too"
> +            . " must match the profile. Will create an erasure coded pool plus a replicated"
> +            . " pool for metadata.",
>          type => 'string',
>          format_description => 'profile',
>          optional => 1,
> @@ -418,8 +422,33 @@ sub ec_parse_and_check {
>  
>      my $ec = parse_property_string($ec_format, $property);
>  
> -    die "Erasure code profile '$ec->{profile}' does not exist.\n"
> -        if $ec->{profile} && !PVE::Ceph::Tools::ecprofile_exists($ec->{profile}, $rados);
> +    my $name = $ec->{profile};
> +    if (!defined($name)) {
> +        die "'k' and 'm' are required if no 'profile' is given.\n"
> +            if !defined($ec->{k}) || !defined($ec->{m});
> +        return $ec;
> +    }
> +
> +    my $profile = PVE::Ceph::Tools::get_ecprofile($name, $rados)
> +        // die "Erasure code profile '$name' does not exist.\n";
> +
> +    # the profile decides these, so refuse values it does not have
> +    # Ceph keeps integers as typed, e.g. '02' or '+2'
> +    my $int = sub { my ($v) = @_; return PVE::JSONSchema::is_integer($v) ? $v + 0 : $v };
> +    my $actual = {

nit: perhaps $actual_profile?

> +        k => $int->($profile->{k} // ''),
> +        m => $int->($profile->{m} // ''),
> +        'failure-domain' => $profile->{'crush-failure-domain'} || 'host',
> +        'device-class' => $profile->{'crush-device-class'} // '',
> +    };
> +    for my $opt (sort keys %$actual) {

nit: Ceph uses key/value [1], perhaps the parameters could be called
$key, $value (for ones obtained as parameters) and $profile_key for the
actual value.

[1] https://docs.ceph.com/en/latest/rados/operations/erasure-code-profile/#osd-erasure-code-profile-set

> +        my $given = $ec->{$opt};
> +        next if !defined($given);
> +        $given += 0 if $opt eq 'k' || $opt eq 'm'; # the schema checked it is an integer
> +        next if $given eq $actual->{$opt};
> +        my $has = $actual->{$opt} ne '' ? "'$actual->{$opt}'" : 'none';
> +        die "'$opt' is '$ec->{$opt}', but erasure code profile '$name' has $has.\n";
> +    }
>  
>      return $ec;
>  }
> @@ -448,8 +477,9 @@ __PACKAGE__->register_method({
>              },
>              'erasure-coding' => {
>                  description => "Create an erasure coded pool for RBD with an accompaning"
> -                    . " replicated pool for metadata storage. With EC, the common ceph options 'size',"
> -                    . " 'min_size' and 'crush_rule' parameters will be applied to the metadata pool.",
> +                    . " replicated pool for metadata storage. Needs either 'profile' or both 'k'"
> +                    . " and 'm'. With EC, the common ceph options 'size', 'min_size' and"
> +                    . " 'crush_rule' parameters will be applied to the metadata pool.",
>                  type => 'string',
>                  format => $ec_format,
>                  optional => 1,
> @@ -499,7 +529,7 @@ __PACKAGE__->register_method({
>                  PVE::RADOS->new(timeout => PVE::Ceph::Tools::get_config('long_rados_timeout'));
>  
>              if ($ec) {
> -                if (!$ec->{profile}) {
> +                if (!defined($ec->{profile})) {
>                      $ec->{profile} = PVE::Ceph::Tools::get_ecprofile_name($pool, $rados);
>                      eval {
>                          PVE::Ceph::Tools::create_ecprofile(
> diff --git a/PVE/Ceph/Tools.pm b/PVE/Ceph/Tools.pm
> index cfd0c296..804ae262 100644
> --- a/PVE/Ceph/Tools.pm
> +++ b/PVE/Ceph/Tools.pm
> @@ -8,6 +8,7 @@ use File::Basename;
>  use IO::File;
>  use JSON;
>  use List::Util qw(any);
> +use POSIX qw(ENOENT);
>  
>  use PVE::Tools qw(run_command dir_glob_foreach extract_param);
>  use PVE::Cluster qw(cfs_read_file);
> @@ -329,7 +330,8 @@ sub get_pool_properties {
>  sub get_pool_type {
>      my ($pool, $rados) = @_;
>      $rados = PVE::RADOS->new() if !defined($rados);
> -    return 'erasure' if get_pool_properties($pool, $rados)->{erasure_code_profile};
> +    # a profile may be named '0'
> +    return 'erasure' if length(get_pool_properties($pool, $rados)->{erasure_code_profile});
>      return 'replicated';
>  }
>  
> @@ -346,8 +348,9 @@ sub create_pool {
>          format => 'plain',
>      };
>      $mon_params->{pool_type} = extract_param($param, 'pool_type') if $param->{pool_type};
> +    # a profile may be named '0'
>      $mon_params->{erasure_code_profile} = extract_param($param, 'erasure_code_profile')
> -        if $param->{erasure_code_profile};
> +        if defined($param->{erasure_code_profile});
>  
>      # Ceph checks mon_max_pg_per_osd against the rule given on create, not on a later rule change
>      $mon_params->{rule} = extract_param($param, 'crush_rule')
> @@ -645,14 +648,16 @@ sub ceph_cluster_status {
>      return $status;
>  }
>  
> -sub ecprofile_exists {
> +# returns undef if there is no such profile
> +sub get_ecprofile {
>      my ($name, $rados) = @_;
>      $rados = PVE::RADOS->new() if !$rados;
>  
> -    my $res = $rados->mon_command({ prefix => 'osd erasure-code-profile ls' });
> -
> -    my $profiles = { map { $_ => 1 } @$res };
> -    return $profiles->{$name};
> +    my $cmd = { prefix => 'osd erasure-code-profile get', name => $name };
> +    my $res = $rados->mon_cmd($cmd, 1);
> +    return if $res->{return_code} == -ENOENT;
> +    die "error with '$cmd->{prefix}': $res->{status_message}\n" if $res->{return_code} < 0;
> +    return $res->{data};
>  }
>  
>  sub create_ecprofile {
> diff --git a/test/CephPool_test.pl b/test/CephPool_test.pl
> index 730b712f..c4385fba 100755
> --- a/test/CephPool_test.pl
> +++ b/test/CephPool_test.pl
> @@ -24,6 +24,40 @@ use PVE::Ceph::Tools;
>      }
>  }
>  
> +{
> +
> +    package FakeRados;
> +
> +    use POSIX qw(ENOENT);
> +
> +    # a profile given as a number makes 'get' fail with that error code
> +    sub new {
> +        my ($class, %profiles) = @_;
> +        return bless { profiles => \%profiles, cmds => [] }, $class;
> +    }
> +
> +    sub mon_cmd {
> +        my ($self, $cmd, $no_result_catch) = @_;
> +        die "unexpected command '$cmd->{prefix}'\n"
> +            if $cmd->{prefix} ne 'osd erasure-code-profile get';
> +        my $reply = $self->{profiles}->{ $cmd->{name} } // -ENOENT;
> +        my $res =
> +            ref($reply)
> +            ? { return_code => 0, data => $reply }
> +            : { return_code => $reply, status_message => "error $reply" };
> +        # like PVE::RADOS::mon_cmd
> +        die "error with '$cmd->{prefix}': mon_cmd failed - $res->{status_message}\n"
> +            if !$no_result_catch && $res->{return_code} < 0;
> +        return $res;
> +    }
> +
> +    sub mon_command {
> +        my ($self, $cmd) = @_;
> +        push $self->{cmds}->@*, $cmd;
> +        return;
> +    }
> +}
> +
>  # call an API method, run its worker inline, and return what it printed
>  my $run = sub {
>      my ($method, $param) = @_;
> @@ -94,4 +128,148 @@ for my $case (@own_cases) {
>      }
>  }
>  
> +# --- what an erasure coding property string accepts ---------------------------------------
> +# as Ceph reports them: numbers as strings, unset keys left out
> +my $rados = FakeRados->new(
> +    default => { k => '2', m => '2', plugin => 'isa', technique => 'reed_sol_van' },
> +    ssd => {
> +        k => '4',
> +        m => '2',
> +        'crush-failure-domain' => 'osd',
> +        'crush-device-class' => 'ssd',
> +    },
> +    0 => { k => '3', m => '1' },
> +    typed => { k => '02', m => '+1' },
> +    broken => -5,
> +);
> +
> +my $parse = sub { PVE::API2::Ceph::Pool::ec_parse_and_check($_[0], $rados) };
> +
> +is_deeply($parse->('profile=default'), { profile => 'default' }, "a profile needs no 'k' and 'm'");
> +is_deeply($parse->('k=4,m=2'), { k => 4, m => 2 }, "'k' and 'm' without a profile");
> +is_deeply($parse->('profile=0'), { profile => '0' }, "a profile named '0' counts as given");
> +is($parse->(undef), undef, 'no erasure coding at all is fine');
> +
> +my @parse_cases = (
> +    # property, expected error or undef if accepted, name
> +    ['profile=ssd', undef, "a profile with another failure domain is accepted on its own"],
> +    ['profile=default,k=2,m=2,failure-domain=host', undef, 'values matching the profile'],
> +    [
> +        'profile=ssd,k=4,m=2,failure-domain=osd,device-class=ssd',
> +        undef,
> +        'values matching a profile with all keys set',
> +    ],
> +    ['profile=default,k=02,m=+2', undef, "'k' and 'm' are compared as numbers"],
> +    [
> +        'profile=typed,k=2,m=1',
> +        undef,
> +        "'k' and 'm' Ceph keeps as typed are compared as numbers too",
> +    ],
> +    [
> +        'profile=default,k=4,m=2',
> +        qr/'k' is '4', but .* 'default' has '2'/,
> +        "a 'k' the profile does not have is rejected",
> +    ],
> +    [
> +        'profile=ssd,failure-domain=host',
> +        qr/'failure-domain' is 'host', but .* has 'osd'/,
> +        'a different failure domain is rejected',
> +    ],
> +    [
> +        'profile=default,device-class=ssd',
> +        qr/'device-class' is 'ssd', but .* has none/,
> +        'a device class the profile lacks is rejected',
> +    ],
> +    ['k=4', qr/'k' and 'm' are required/, "'k' without 'm' is rejected"],
> +    ['m=2', qr/'k' and 'm' are required/, "'m' without 'k' is rejected"],
> +    ['failure-domain=osd', qr/'k' and 'm' are required/, 'a bare failure domain is not enough'],
> +    [
> +        'profile=does-not-exist',
> +        qr/^Erasure code profile .* does not exist/,
> +        'an unknown profile',
> +    ],
> +    [
> +        'profile=broken',
> +        qr/error with .*: error -5/,
> +        'other errors reading the profile are passed on',
> +    ],
> +);
> +for my $case (@parse_cases) {
> +    my ($property, $error, $name) = @$case;
> +    eval { $parse->($property) };
> +    if (defined($error)) {
> +        like($@, $error, $name);
> +    } else {
> +        is($@, '', $name);
> +    }
> +}
> +
> +# --- what reaches the monitor -------------------------------------------------------------
> +{
> +    no warnings 'redefine';
> +    local *PVE::Ceph::Tools::set_pool = sub { };
> +
> +    my $rados = FakeRados->new();
> +    my $param = { pool_type => 'erasure', erasure_code_profile => '0' };
> +    PVE::Ceph::Tools::create_pool('foo-data', $param, $rados);
> +    is($rados->{cmds}->[0]->{erasure_code_profile}, '0', "create_pool passes the profile '0' on");
> +    ok(!exists($param->{erasure_code_profile}), 'and leaves it out of what set_pool gets');
> +}
> +
> +{
> +    no warnings 'redefine';
> +    my $props;
> +    local *PVE::Ceph::Tools::get_pool_properties = sub { $props };
> +
> +    my @cases = (
> +        # pool properties, type, name
> +        [{ erasure_code_profile => '0' }, 'erasure', "a pool on profile '0' is erasure"],
> +        [{}, 'replicated', 'a pool without a profile is not'],
> +        [{ erasure_code_profile => '' }, 'replicated', 'nor is one with an empty profile'],
> +    );
> +    for my $case (@cases) {
> +        ($props, my $type, my $name) = @$case;
> +        is(PVE::Ceph::Tools::get_pool_type('foo', 'rados'), $type, $name);
> +    }
> +}
> +
> +# --- what createpool creates --------------------------------------------------------------
> +{
> +    no warnings qw(redefine once);
> +    my (@profiles, @pools);
> +    my $rados = FakeRados->new(0 => { k => '2', m => '1' });
> +
> +    local *PVE::RPCEnvironment::get = sub { bless {}, 'PoolRPCEnvironment' };
> +    local *PVE::Cluster::check_cfs_quorum = sub { };
> +    local *PVE::Ceph::Tools::check_ceph_configured = sub { };
> +    local *PVE::Ceph::Tools::get_config = sub { 30 };
> +    local *PVE::RADOS::new = sub { $rados };
> +    local *PVE::Ceph::Tools::create_ecprofile = sub { push @profiles, join(' ', @_[0 .. 2]) };
> +    local *PVE::Ceph::Tools::create_pool = sub {
> +        my ($pool, $param) = @_;
> +        push @pools, "$pool:" . ($param->{erasure_code_profile} // '');
> +    };
> +
> +    my @cases = (
> +        # erasure coding, profiles created, pools created with their profile, name
> +        ['profile=0', '', 'foo-data:0, foo-metadata:', "the profile '0' is used as given"],
> +        [
> +            'k=2,m=1',
> +            'pve_ec_foo 2 1',
> +            'foo-data:pve_ec_foo, foo-metadata:',
> +            "without a profile, one is made from 'k' and 'm'",
> +        ],
> +    );
> +    for my $case (@cases) {
> +        my ($property, $profiles, $pools, $name) = @$case;
> +        (@profiles, @pools) = ();
> +        $run->(
> +            'createpool',
> +            { name => 'foo', node => 'node', 'erasure-coding' => $property, add_storages => 0 },
> +        );
> +        is(join(', ', @profiles), $profiles, "$name (profiles)");
> +        is(join(', ', @pools), $pools, "$name (pools)");
> +    }
> +}
> +
>  done_testing();

-- 
Maximiliano