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 6268D1FF0AB for ; Wed, 21 Oct 2026 07:44:47 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id D4DAA21635; Wed, 21 Oct 2026 07:44:37 +0200 (CEST) From: Kefu Chai To: pve-devel@lists.proxmox.com Subject: [PATCH manager 1/2] api: ceph: pool: destroypool: keep EC profiles pveceph did not create Date: Wed, 21 Oct 2026 13:44:24 +0800 Message-ID: <20261021054425.3271742-2-k.chai@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20261021054425.3271742-1-k.chai@proxmox.com> References: <20261021054425.3271742-1-k.chai@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1792561471188 X-SPAM-LEVEL: Spam detection results: 0 AWL -0.256 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: RR3MPI5TAYSWL65X7UNQOSUGXK3UUTLH X-Message-ID-Hash: RR3MPI5TAYSWL65X7UNQOSUGXK3UUTLH X-MailFrom: k.chai@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 X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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.", 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 -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