From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 8DF0B1FF0C0 for ; Wed, 21 Oct 2026 11:31:32 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 932FB215A7; Wed, 21 Oct 2026 11:31:28 +0200 (CEST) From: Maximiliano Sandoval To: Kefu Chai Subject: Re: [PATCH manager 1/2] api: ceph: pool: destroypool: keep EC profiles pveceph did not create In-Reply-To: <20261021054425.3271742-2-k.chai@proxmox.com> (Kefu Chai's message of "Wed, 21 Oct 2026 13:44:24 +0800") References: <20261021054425.3271742-1-k.chai@proxmox.com> <20261021054425.3271742-2-k.chai@proxmox.com> User-Agent: mu4e 1.12.9; emacs 30.1 Date: Wed, 21 Oct 2026 11:31:22 +0200 Message-ID: MIME-Version: 1.0 Content-Type: text/plain X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792575083018 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.220 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) POISEN_SPAM_PILL 0.1 Meta: its spam POISEN_SPAM_PILL_1 0.1 random spam to be learned in bayes POISEN_SPAM_PILL_3 0.1 random spam to be learned in bayes 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: NI2V22WNXPYDDFNAEC25VIQ4CI7EKDEF X-Message-ID-Hash: NI2V22WNXPYDDFNAEC25VIQ4CI7EKDEF 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: 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_ for the pool -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 > --- > 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 -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