From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 2FB1F1FF0C0 for ; Wed, 21 Oct 2026 11:52:28 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id B06DF21643; Wed, 21 Oct 2026 11:52:25 +0200 (CEST) From: Maximiliano Sandoval To: Kefu Chai Subject: Re: [PATCH manager 2/2] api: ceph: pool: createpool: make 'k' and 'm' optional with a profile In-Reply-To: <20261021054425.3271742-3-k.chai@proxmox.com> (Kefu Chai's message of "Wed, 21 Oct 2026 13:44:25 +0800") References: <20261021054425.3271742-1-k.chai@proxmox.com> <20261021054425.3271742-3-k.chai@proxmox.com> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Wed, 21 Oct 2026 11:52:17 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792576337627 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.369 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_ASCII_DIVIDERS 0.8 Email that uses ascii formatting dividers and possible spam tricks KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: IOGSDPF7NWABZNQU6GMQIDOLQFTAZKGX X-Message-ID-Hash: IOGSDPF7NWABZNQU6GMQIDOLQFTAZKGX X-MailFrom: m.sandoval@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: pve-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Kefu Chai 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 > --- > 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