[PATCH manager 0/2] ceph: pool: create EC pools from an existing profile

[PATCH manager 0/2] ceph: pool: create EC pools from an existing profile

[Kefu Chai]

Hi,

the docs suggest building a custom erasure code profile with the Ceph
tools and then passing it to 'pveceph pool create' as 'profile'. That
doesn't actually work right now, since the schema still wants 'k' and
'm':

  # pveceph pool create test-ec --erasure-coding profile=default
  400 Parameter verification failed.
  erasure-coding.m: property is missing and it is not optional
  erasure-coding.k: property is missing and it is not optional

Patch 2 makes 'k' and 'm' optional when a profile is given. If they are
passed anyway, they now have to match the profile, where they used to be
ignored. So a script that passed placeholder values next to a profile
would now get an error, which I think is the safer choice, but I'm not
sure everyone agrees.

Patch 1 needs to go in first. destroypool removes the pool's profile by
default, so once patch 2 makes 'profile=default' easy, destroying such a
pool would take 'default' with it. With patch 1 it only removes the
profile createpool made for that pool, unless asked to.

Both patches come with tests in a new test/CephPool_test.pl, and I tried
creating and destroying such pools on a three node test cluster.
Feedback welcome, especially on the stricter check in patch 2.

Kefu Chai (2):
  api: ceph: pool: destroypool: keep EC profiles pveceph did not create
  api: ceph: pool: createpool: make 'k' and 'm' optional with a profile

 PVE/API2/Ceph/Pool.pm |  72 ++++++++---
 PVE/Ceph/Tools.pm     |  25 ++--
 test/CephPool_test.pl | 275 ++++++++++++++++++++++++++++++++++++++++++
 test/Makefile         |   6 +-
 4 files changed, 352 insertions(+), 26 deletions(-)
 create mode 100755 test/CephPool_test.pl

-- 
2.47.3

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

[Kefu Chai]

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

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

[Maximiliano Sandoval]

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

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

[Kefu Chai]

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

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

[Maximiliano Sandoval]

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

Re: [PATCH manager 0/2] ceph: pool: create EC pools from an existing profile

[Maximiliano Sandoval]

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

Comments on each patch.

> Hi,
>
> the docs suggest building a custom erasure code profile with the Ceph
> tools and then passing it to 'pveceph pool create' as 'profile'. That
> doesn't actually work right now, since the schema still wants 'k' and
> 'm':
>
>   # pveceph pool create test-ec --erasure-coding profile=default
>   400 Parameter verification failed.
>   erasure-coding.m: property is missing and it is not optional
>   erasure-coding.k: property is missing and it is not optional
>
> Patch 2 makes 'k' and 'm' optional when a profile is given. If they are
> passed anyway, they now have to match the profile, where they used to be
> ignored. So a script that passed placeholder values next to a profile
> would now get an error, which I think is the safer choice, but I'm not
> sure everyone agrees.
>
> Patch 1 needs to go in first. destroypool removes the pool's profile by
> default, so once patch 2 makes 'profile=default' easy, destroying such a
> pool would take 'default' with it. With patch 1 it only removes the
> profile createpool made for that pool, unless asked to.
>
> Both patches come with tests in a new test/CephPool_test.pl, and I tried
> creating and destroying such pools on a three node test cluster.
> Feedback welcome, especially on the stricter check in patch 2.

The patches look good to me modulo small nits, none of which is a
blocker on its own.

PS: I did not review the tests.

Reviewed-by: Maximiliano Sandoval <m.sandoval@proxmox.com>
Tested-by: Maximiliano Sandoval <m.sandoval@proxmox.com>

-- 
Maximiliano