From e3ca5bfbccc5c903910f455d55d966dab874549d Mon Sep 17 00:00:00 2001 From: Travis Nielsen Date: Tue, 31 May 2022 15:47:29 -0600 Subject: [PATCH] pool: delete ceph pool when blockpool cr is deleted During deletion of a CephBlockPool CR, the underlying ceph pool was not being deleted. During the deletion sequence this is due to the spec being refreshed upon a call to check for dependents in ReportDeletionNotBlockedDueToDependents(). The pool name was then coming up blank during the deletion request and rook of course then wasn't finding the pool to delete. Now Rook properly sets the pool name in the named spec every time it is converted internally from a CephBlockPool type to a NamedPoolSpec. Signed-off-by: Travis Nielsen --- pkg/apis/ceph.rook.io/v1/pool.go | 26 +++++++++++++--------- pkg/apis/ceph.rook.io/v1/pool_test.go | 4 ++-- pkg/operator/ceph/pool/controller.go | 27 +++++------------------ pkg/operator/ceph/pool/controller_test.go | 13 +++++++---- pkg/operator/ceph/pool/validate.go | 2 +- 5 files changed, 33 insertions(+), 39 deletions(-) diff --git a/pkg/apis/ceph.rook.io/v1/pool.go b/pkg/apis/ceph.rook.io/v1/pool.go index bd00b7dc6..c14d74003 100644 --- a/pkg/apis/ceph.rook.io/v1/pool.go +++ b/pkg/apis/ceph.rook.io/v1/pool.go @@ -53,7 +53,7 @@ func (p *ReplicatedSpec) IsTargetRatioEnabled() bool { func (p *CephBlockPool) ValidateCreate() error { logger.Infof("validate create cephblockpool %v", p) - err := ValidateCephBlockPool(&p.Spec) + err := ValidateCephBlockPool(p) if err != nil { return err } @@ -61,14 +61,14 @@ func (p *CephBlockPool) ValidateCreate() error { } // ValidateCephBlockPool validates specifically a CephBlockPool's spec (not just any NamedPoolSpec) -func ValidateCephBlockPool(s *NamedBlockPoolSpec) error { - if s.Name == "device_health_metrics" || s.Name == ".mgr" || s.Name == ".nfs" { - if s.IsErasureCoded() { - return errors.Errorf("invalid CephBlockPool spec: ceph built-in pool %q cannot be erasure coded", s.Name) +func ValidateCephBlockPool(p *CephBlockPool) error { + if p.Spec.Name == "device_health_metrics" || p.Spec.Name == ".mgr" || p.Spec.Name == ".nfs" { + if p.Spec.IsErasureCoded() { + return errors.Errorf("invalid CephBlockPool spec: ceph built-in pool %q cannot be erasure coded", p.Name) } } - return validatePoolSpec(s.ToNamedPoolSpec()) + return validatePoolSpec(p.ToNamedPoolSpec()) } // validate any NamedPoolSpec @@ -98,17 +98,23 @@ func validatePoolSpec(ps NamedPoolSpec) error { return nil } -func (p *NamedBlockPoolSpec) ToNamedPoolSpec() NamedPoolSpec { +func (p *CephBlockPool) ToNamedPoolSpec() NamedPoolSpec { + // If the name is not overridden in the pool spec.name, set it to the name of the pool CR + name := p.Spec.Name + if name == "" { + // Set the name of the pool CR since a name override wasn't specified in the spec + name = p.Name + } return NamedPoolSpec{ - Name: p.Name, - PoolSpec: p.PoolSpec, + Name: name, + PoolSpec: p.Spec.PoolSpec, } } func (p *CephBlockPool) ValidateUpdate(old runtime.Object) error { logger.Info("validate update cephblockpool") ocbp := old.(*CephBlockPool) - err := ValidateCephBlockPool(&p.Spec) + err := ValidateCephBlockPool(p) if err != nil { return err } diff --git a/pkg/apis/ceph.rook.io/v1/pool_test.go b/pkg/apis/ceph.rook.io/v1/pool_test.go index 95de4d917..1b37403c1 100644 --- a/pkg/apis/ceph.rook.io/v1/pool_test.go +++ b/pkg/apis/ceph.rook.io/v1/pool_test.go @@ -37,11 +37,11 @@ func TestValidatePoolSpec(t *testing.T) { }, }, } - err := validatePoolSpec(p.Spec.ToNamedPoolSpec()) + err := validatePoolSpec(p.ToNamedPoolSpec()) assert.NoError(t, err) p.Spec.ErasureCoded.DataChunks = 1 - err = validatePoolSpec(p.Spec.ToNamedPoolSpec()) + err = validatePoolSpec(p.ToNamedPoolSpec()) assert.Error(t, err) } diff --git a/pkg/operator/ceph/pool/controller.go b/pkg/operator/ceph/pool/controller.go index 9c168d436..432f52143 100644 --- a/pkg/operator/ceph/pool/controller.go +++ b/pkg/operator/ceph/pool/controller.go @@ -221,10 +221,6 @@ func (r *ReconcileCephBlockPool) reconcile(request reconcile.Request) (reconcile } } - // Set the name of the Ceph pool to be reconciled internally, which - // may be different from the pool CR name. - cephBlockPool.Spec.Name = getCephName(*cephBlockPool) - // DELETE: the CR was deleted if !cephBlockPool.GetDeletionTimestamp().IsZero() { deps, err := cephBlockPoolDependents(r.context, r.clusterInfo, cephBlockPool) @@ -242,8 +238,8 @@ func (r *ReconcileCephBlockPool) reconcile(request reconcile.Request) (reconcile r.recorder.Event(cephBlockPool, corev1.EventTypeNormal, string(cephv1.ReconcileStarted), "starting blockpool deletion") - logger.Infof("deleting pool %q", cephBlockPool.Name) - poolSpec := cephBlockPool.Spec.ToNamedPoolSpec() + poolSpec := cephBlockPool.ToNamedPoolSpec() + logger.Infof("deleting pool %q", poolSpec.Name) err = deletePool(r.context, clusterInfo, &poolSpec) if err != nil { return opcontroller.ImmediateRetryResult, *cephBlockPool, errors.Wrapf(err, "failed to delete pool %q. ", cephBlockPool.Name) @@ -299,7 +295,7 @@ func (r *ReconcileCephBlockPool) reconcile(request reconcile.Request) (reconcile return reconcile.Result{}, *cephBlockPool, errors.Wrap(err, "failed to enable/disable stats collection for pool(s)") } - poolSpec := cephBlockPool.Spec.ToNamedPoolSpec() + poolSpec := cephBlockPool.ToNamedPoolSpec() checker := newMirrorChecker(r.context, r.client, r.clusterInfo, request.NamespacedName, &poolSpec) // ADD PEERS logger.Debug("reconciling create rbd mirror peer configuration") @@ -361,7 +357,7 @@ func (r *ReconcileCephBlockPool) reconcile(request reconcile.Request) (reconcile } func (r *ReconcileCephBlockPool) reconcileCreatePool(clusterInfo *cephclient.ClusterInfo, cephCluster *cephv1.ClusterSpec, cephBlockPool *cephv1.CephBlockPool) (reconcile.Result, error) { - poolSpec := cephBlockPool.Spec.ToNamedPoolSpec() + poolSpec := cephBlockPool.ToNamedPoolSpec() err := createPool(r.context, clusterInfo, cephCluster, &poolSpec) if err != nil { return opcontroller.ImmediateRetryResult, errors.Wrapf(err, "failed to create pool %q.", cephBlockPool.GetName()) @@ -371,19 +367,6 @@ func (r *ReconcileCephBlockPool) reconcileCreatePool(clusterInfo *cephclient.Clu return reconcile.Result{}, nil } -func getCephName(pool cephv1.CephBlockPool) string { - // If the name is not overridden in the pool spec.name, set it to the name of the pool CR - if pool.Spec.Name == "" { - return pool.Name - } - if pool.Spec.Name == pool.Name { - // The same pool name is set, nothing is being overridden - return pool.Name - } - // The pool names that can be allowed to be overridden are enforced in the crd schema - return pool.Spec.Name -} - // Create the pool func createPool(context *clusterd.Context, clusterInfo *cephclient.ClusterInfo, clusterSpec *cephv1.ClusterSpec, p *cephv1.NamedPoolSpec) error { // Set the application name to rbd by default, but override for special pools @@ -445,7 +428,7 @@ func configureRBDStats(clusterContext *clusterd.Context, clusterInfo *cephclient for _, cephBlockPool := range cephBlockPoolList.Items { if cephBlockPool.GetDeletionTimestamp() == nil && cephBlockPool.Spec.EnableRBDStats { // add to list of CephBlockPool with enableRBDStats set to true and not marked for deletion - enableStatsForPools = append(enableStatsForPools, getCephName(cephBlockPool)) + enableStatsForPools = append(enableStatsForPools, cephBlockPool.ToNamedPoolSpec().Name) } } logger.Debugf("RBD per-image IO statistics will be collected for pools: %v", enableStatsForPools) diff --git a/pkg/operator/ceph/pool/controller_test.go b/pkg/operator/ceph/pool/controller_test.go index 4035c3a1c..5e7930aa2 100644 --- a/pkg/operator/ceph/pool/controller_test.go +++ b/pkg/operator/ceph/pool/controller_test.go @@ -123,22 +123,27 @@ func TestCreatePool(t *testing.T) { func TestCephPoolName(t *testing.T) { t.Run("spec not set", func(t *testing.T) { p := cephv1.CephBlockPool{ObjectMeta: metav1.ObjectMeta{Name: "metapool"}} - name := getCephName(p) + name := p.ToNamedPoolSpec().Name assert.Equal(t, "metapool", name) }) t.Run("same name already set", func(t *testing.T) { p := cephv1.CephBlockPool{ObjectMeta: metav1.ObjectMeta{Name: "metapool"}, Spec: cephv1.NamedBlockPoolSpec{Name: "metapool"}} - name := getCephName(p) + name := p.ToNamedPoolSpec().Name assert.Equal(t, "metapool", name) }) t.Run("override device metrics", func(t *testing.T) { p := cephv1.CephBlockPool{ObjectMeta: metav1.ObjectMeta{Name: "device-metrics"}, Spec: cephv1.NamedBlockPoolSpec{Name: "device_health_metrics"}} - name := getCephName(p) + name := p.ToNamedPoolSpec().Name assert.Equal(t, "device_health_metrics", name) }) + t.Run("override mgr", func(t *testing.T) { + p := cephv1.CephBlockPool{ObjectMeta: metav1.ObjectMeta{Name: "default-mgr"}, Spec: cephv1.NamedBlockPoolSpec{Name: ".mgr"}} + name := p.ToNamedPoolSpec().Name + assert.Equal(t, ".mgr", name) + }) t.Run("override nfs", func(t *testing.T) { p := cephv1.CephBlockPool{ObjectMeta: metav1.ObjectMeta{Name: "default-nfs"}, Spec: cephv1.NamedBlockPoolSpec{Name: ".nfs"}} - name := getCephName(p) + name := p.ToNamedPoolSpec().Name assert.Equal(t, ".nfs", name) }) } diff --git a/pkg/operator/ceph/pool/validate.go b/pkg/operator/ceph/pool/validate.go index 75963a886..c607b917c 100644 --- a/pkg/operator/ceph/pool/validate.go +++ b/pkg/operator/ceph/pool/validate.go @@ -33,7 +33,7 @@ func validatePool(context *clusterd.Context, clusterInfo *cephclient.ClusterInfo return errors.New("missing namespace") } - if err := cephv1.ValidateCephBlockPool(&p.Spec); err != nil { + if err := cephv1.ValidateCephBlockPool(p); err != nil { return err }