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

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

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.

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.",
         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 = {
+        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) {
+        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();
-- 
2.47.3