pool: Allow configuration of built-in pools with non-k8s names #9363

travisn · 2021-12-08T23:34:53Z

Description of your changes:
The built-in pools device_health_metrics and .nfs created by ceph need to be configured for replicas, failure domain, etc. To support this, we allow the pool to be created as a CR. Since K8s does not support underscores in the resource names the operator must translate this special pool name into the name expected by ceph.

This also sets the basis for allowing filesystem data pools to specify the desired pool name instead of requiring a generated name as in progress with #9296.

Which issue is resolved by this Pull Request:
Resolves #9209

Checklist:

pkg/operator/ceph/nfs/nfs.go

BlaineEXE · 2021-12-09T00:03:36Z

pkg/apis/ceph.rook.io/v1/types.go

+	// The desired name of the pool if different from the default name.
+	// +optional
+	Name string `json:"name,omitempty"`


Since only two names are supported, should we do an enum for those?

I believe it must be an enum so we can have validation from the API server for "", .nfs and device_health_metrics.

BlaineEXE · 2021-12-09T00:05:17Z

pkg/operator/ceph/pool/controller.go

+		return pool.Spec.Name, nil
+	}
+
+	return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")


Suggested change

return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

return "", errors.Errorf("not allowed to override the pool name except for the \"device_health_metrics\" and \".nfs\" pools")

Suggested change

return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

return "", errors.New("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

BlaineEXE · 2021-12-09T00:07:31Z

pkg/operator/ceph/nfs/controller.go

-	// Always create the default pool
-	err = r.createDefaultNFSRADOSPool(cephNFS)
-	if err != nil {
-		return reconcile.Result{}, errors.Wrapf(err, "failed to create default pool %q", cephNFS.Spec.RADOS.Pool)
-	}
-


We do I think, at minimum, want to verify that the pool exists. Though I still think that creating it might be good. That is, unless the lower level ceph osd pool create .nfs resets a customized pool back to default values. In that case, maybe it'll be easier to chat in huddle.

I'll add a check for the existence of the pool

pkg/operator/ceph/object/objectstore.go

pkg/operator/ceph/pool/controller.go

BlaineEXE · 2021-12-09T00:12:00Z

pkg/operator/ceph/pool/controller.go

@@ -341,11 +347,31 @@ func (r *ReconcileCephBlockPool) reconcileCreatePool(clusterInfo *cephclient.Clu
 	return reconcile.Result{}, nil
 }

+func getCephName(pool cephv1.CephBlockPool) (string, error) {


Should this be part of the API so it can be used for validation by the ceph/client or other places as well?

ceph/client doesn't have a concept of the difference between the k8s and ceph pool names, so it seems correct to keep the conversion of k8s name to ceph pool name here in the controller.

Right. But we could add a method to the API so that CephBlockPool has a .Pool() method that the client could use, and we could do sanitization of the pool name there to ensure any internal usage isn't breaking our own rules.

... Ah, but I just realized we have to break our own rules for some of the CephObjectStore pools like .rgw.

leseb · 2021-12-09T11:16:26Z

pkg/apis/ceph.rook.io/v1/types.go

+	// The desired name of the pool if different from the default name.
+	// +optional
+	Name string `json:"name,omitempty"`


I believe it must be an enum so we can have validation from the API server for "", .nfs and device_health_metrics.

leseb · 2021-12-09T11:20:44Z

pkg/daemon/ceph/client/pool.go

-func CreatePoolWithProfile(context *clusterd.Context, clusterInfo *ClusterInfo, clusterSpec *cephv1.ClusterSpec, poolName string, pool cephv1.PoolSpec, appName string) error {
+func CreatePoolWithProfile(context *clusterd.Context, clusterInfo *ClusterInfo, clusterSpec *cephv1.ClusterSpec, pool cephv1.NamedPoolSpec, appName string) error {
+	if pool.Name == "" {
+		return errors.Errorf("pool name must be specified")


Suggested change

return errors.Errorf("pool name must be specified")

return errors.New("pool name must be specified")

leseb · 2021-12-09T11:20:58Z

pkg/daemon/ceph/client/pool.go

 	}

 	if !pool.IsErasureCoded() {
 		// neither a replicated or EC pool
-		return fmt.Errorf("pool %q type is not defined as replicated or erasure coded", poolName)
+		return fmt.Errorf("pool %q type is not defined as replicated or erasure coded", pool.Name)


Suggested change

return fmt.Errorf("pool %q type is not defined as replicated or erasure coded", pool.Name)

return errors.Errorf("pool %q type is not defined as replicated or erasure coded", pool.Name)

leseb · 2021-12-09T13:23:01Z

pkg/operator/ceph/pool/controller.go

@@ -341,11 +347,31 @@ func (r *ReconcileCephBlockPool) reconcileCreatePool(clusterInfo *cephclient.Clu
 	return reconcile.Result{}, nil
 }

+func getCephName(pool cephv1.CephBlockPool) (string, error) {
+	if pool.Name == "" {
+		return "", errors.Errorf("metadata name must be set")


Suggested change

return "", errors.Errorf("metadata name must be set")

return "", errors.New("metadata name must be set")

Isn't that checked by the API server already?

yes, I didn't have it at first, and added it for unit test completeness. But I think i'll remove it again since it's overkill.

leseb · 2021-12-09T13:24:23Z

pkg/operator/ceph/pool/controller.go

+		return pool.Name, nil
+	}
+	// Only allow the name to be overridden for certain built-in pool names
+	if pool.Spec.Name == "device_health_metrics" || pool.Spec.Name == ".nfs" {


If we have an enum in the API definition we don't need this.

leseb · 2021-12-09T13:24:37Z

pkg/operator/ceph/pool/controller.go

+		return pool.Spec.Name, nil
+	}
+
+	return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")


Suggested change

return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

return "", errors.New("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

BlaineEXE

Implementation seems good. I think it's just cleaning up docs/examples a bit more? I think NFS docs could get a mention, and the nfs-test.yaml needs a new .nfs CephBlockPool also.

BlaineEXE · 2021-12-10T23:21:41Z

deploy/examples/nfs.yaml

+#################################################################################################################
+# Create a Ceph pool with settings for replication in production environments. A minimum of 3 OSDs on
+# different hosts are required in this example.
+#  kubectl create -f pool.yaml


Suggested change

# kubectl create -f pool.yaml

# kubectl create -f nfs.yaml

pkg/operator/ceph/nfs/nfs.go

BlaineEXE · 2021-12-10T23:25:09Z

pkg/operator/ceph/nfs/nfs.go

-		return errors.Wrapf(err, "failed to create default NFS pool %q. %s", poolName, string(output))
+		return errors.Wrapf(err, "pool %q could not be retrieved", poolName)


Maybe we could improve this error message to instruct users to create the .nfs pool via CephBlockPool CRD? Maybe pointing them to a section in the NFS doc about that? (which I think we'll also have to add?)

deploy/examples/pool-device-health-metrics.yaml

pkg/daemon/ceph/client/pool.go

leseb · 2021-12-14T09:42:54Z

pkg/operator/ceph/nfs/nfs.go

 	if err != nil {
-		return errors.Wrapf(err, "failed to create default NFS pool %q. %s", poolName, string(output))
+		return errors.Wrapf(err, "pool %q could not be retrieved, ensure the .nfs pool is defined", poolName)


We have a const for .nfs so let's use it?

mergify · 2021-12-14T18:07:30Z

This pull request has merge conflicts that must be resolved before it can be merged. @travisn please rebase it. https://rook.io/docs/rook/latest/development-flow.html#updating-your-fork

The built-in pools device_health_metrics and .nfs created by ceph need to be configured for replicas, failure domain, etc. To support this, we allow the pool to be created as a CR. Since K8s does not support underscores in the resource names the operator must translate this special pool name into the name expected by ceph. This also sets the basis for allowing filesystem data pools to specify the desired pool name instead of requiring a generated name. Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

Creating an EC pool is causing the CI to hang when the EC pool is initialized since there aren't enough OSDs to satisfy the EC parameters. Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

pool: Allow configuration of built-in pools with non-k8s names (backport #9363)

travisn added the backport-release-1.8 label Dec 8, 2021

travisn requested review from leseb, BlaineEXE and satoru-takeuchi December 8, 2021 23:34

travisn commented Dec 8, 2021

View reviewed changes

pkg/operator/ceph/nfs/nfs.go Show resolved Hide resolved

travisn force-pushed the configure-built-in-pool branch from 6fdb6de to c43c014 Compare December 8, 2021 23:45

travisn mentioned this pull request Dec 8, 2021

file: allow to create CephFS dataPools with predefined names #9296

Merged

10 tasks

BlaineEXE reviewed Dec 9, 2021

View reviewed changes

leseb reviewed Dec 9, 2021

View reviewed changes

travisn force-pushed the configure-built-in-pool branch 3 times, most recently from c21c9ba to a062b89 Compare December 10, 2021 21:50

BlaineEXE requested changes Dec 10, 2021

View reviewed changes

travisn force-pushed the configure-built-in-pool branch 7 times, most recently from 859eb14 to 565bfb4 Compare December 14, 2021 06:36

leseb reviewed Dec 14, 2021

View reviewed changes

travisn force-pushed the configure-built-in-pool branch 2 times, most recently from 90425bd to 0ff55e3 Compare December 14, 2021 14:52

leseb approved these changes Dec 14, 2021

View reviewed changes

travisn force-pushed the configure-built-in-pool branch from 0ff55e3 to 84a4b71 Compare December 14, 2021 19:37

BlaineEXE approved these changes Dec 14, 2021

View reviewed changes

travisn force-pushed the configure-built-in-pool branch 3 times, most recently from 29e06f3 to c9bc968 Compare December 15, 2021 20:59

travisn force-pushed the configure-built-in-pool branch 2 times, most recently from 61802ad to bea646a Compare December 15, 2021 22:13

travisn mentioned this pull request Dec 15, 2021

pool: add rbd ec pool support in external cluster #9276

Merged

10 tasks

travisn added 2 commits December 15, 2021 15:32

test: only create replicated pools in ci

60cc67f

Creating an EC pool is causing the CI to hang when the EC pool is initialized since there aren't enough OSDs to satisfy the EC parameters. Signed-off-by: Travis Nielsen <tnielsen@redhat.com>

travisn force-pushed the configure-built-in-pool branch from bea646a to 60cc67f Compare December 15, 2021 22:32

travisn merged commit 07f101e into rook:master Dec 15, 2021

travisn deleted the configure-built-in-pool branch December 15, 2021 23:03

mergify bot mentioned this pull request Dec 15, 2021

pool: Allow configuration of built-in pools with non-k8s names (backport #9363) #9437

Merged

mergify bot added a commit that referenced this pull request Dec 15, 2021

Merge pull request #9437 from rook/mergify/bp/release-1.8/pr-9363

37a20ba

pool: Allow configuration of built-in pools with non-k8s names (backport #9363)

travisn mentioned this pull request Feb 14, 2022

radosnamespace: add new CRD #9733

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pool: Allow configuration of built-in pools with non-k8s names #9363

pool: Allow configuration of built-in pools with non-k8s names #9363

travisn commented Dec 8, 2021

BlaineEXE Dec 9, 2021

leseb Dec 9, 2021

travisn Dec 9, 2021

BlaineEXE Dec 9, 2021

leseb Dec 9, 2021

BlaineEXE Dec 9, 2021

travisn Dec 10, 2021

BlaineEXE Dec 9, 2021

travisn Dec 10, 2021

BlaineEXE Dec 10, 2021 •

edited

leseb Dec 9, 2021

leseb Dec 9, 2021

leseb Dec 9, 2021

leseb Dec 9, 2021

leseb Dec 9, 2021

travisn Dec 10, 2021

leseb Dec 9, 2021

leseb Dec 9, 2021

BlaineEXE left a comment

BlaineEXE Dec 10, 2021

BlaineEXE Dec 10, 2021

leseb Dec 14, 2021

mergify bot commented Dec 14, 2021

	return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")
	return "", errors.Errorf("not allowed to override the pool name except for the \"device_health_metrics\" and \".nfs\" pools")

	return "", errors.Errorf("not allowed to override the pool name except for the device_health_metrics and .nfs pools")
	return "", errors.New("not allowed to override the pool name except for the device_health_metrics and .nfs pools")

	return errors.Errorf("pool name must be specified")
	return errors.New("pool name must be specified")

	return fmt.Errorf("pool %q type is not defined as replicated or erasure coded", pool.Name)
	return errors.Errorf("pool %q type is not defined as replicated or erasure coded", pool.Name)

	return "", errors.Errorf("metadata name must be set")
	return "", errors.New("metadata name must be set")

		return errors.Wrapf(err, "failed to create default NFS pool %q. %s", poolName, string(output))
		return errors.Wrapf(err, "pool %q could not be retrieved", poolName)

pool: Allow configuration of built-in pools with non-k8s names #9363

pool: Allow configuration of built-in pools with non-k8s names #9363

Conversation

travisn commented Dec 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE Dec 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented Dec 14, 2021

BlaineEXE Dec 10, 2021 •

edited