radosnamespace: add new CRD #9733

Madhu-1 · 2022-02-14T07:18:57Z

Description of your changes:
This introduces a new CRD to add the ability to create a rados namespace for a given ceph block pool. Typically the name of the pool is the name of the block pool created by the rook.

Signed-off-by: Madhu Rajanna madhupr007@gmail.com

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

Checklist:

Madhu-1 · 2022-02-14T07:19:33Z

@leseb @travisn this is ready for initial review. added DNM as am still testing the changes

subhamkrai

nits

pkg/daemon/ceph/client/radosnamespace.go

pkg/operator/ceph/pool/dependents.go

Madhu-1 · 2022-02-14T08:11:45Z

Removed DNM as my manual testing works fine.

[🎩︎]mrajanna@fedora rook $]kuberc gekuberc get cephblockpoolradosnamespace -oyaml
apiVersion: v1
items:
- apiVersion: ceph.rook.io/v1
  kind: CephBlockPoolRadosNamespace
  metadata:
    creationTimestamp: "2022-02-14T07:55:43Z"
    finalizers:
    - cephblockpoolradosnamespace.ceph.rook.io
    generation: 1
    name: namespace-a
    namespace: rook-ceph
    resourceVersion: "116578"
    uid: f3a5dd75-4fc2-4cc1-bdd9-71eb08673cc8
  spec:
    blockpoolName: replicapool
  status:
    info:
      clusterID: 80fc4f4bacc064be641633e6ed25ba7e
    phase: Ready
kind: List
metadata:
  resourceVersion: ""
  selfLink: ""
[🎩︎]mrajanna@fedora rook $]kuberc get cm
NAME                           DATA   AGE
kube-root-ca.crt               1      4d2h
rook-ceph-csi-config           1      4d2h
rook-ceph-csi-mapping-config   1      4d2h
rook-ceph-mon-endpoints        4      4d2h
rook-ceph-operator-config      18     4d2h
rook-ceph-pdbstatemap          2      4d2h
rook-config-override           1      4d2h
[🎩︎]mrajanna@fedora rook $]kuberc get cm rook-ceph-csi-config -oyaml
apiVersion: v1
data:
  csi-cluster-config-json: '[{"clusterID":"rook-ceph","monitors":["10.104.250.35:6789"]},{"clusterID":"4df0bb6e1b7ddc0c1ea080cbe043dd1a","monitors":["10.104.250.35:6789"],"cephFS":{"subvolumeGroup":"group-a"}},{"clusterID":"80fc4f4bacc064be641633e6ed25ba7e","monitors":["10.104.250.35:6789"],"radosNamespace":"namespace-a"}]'
kind: ConfigMap
metadata:
  creationTimestamp: "2022-02-10T05:13:47Z"
  name: rook-ceph-csi-config
  namespace: rook-ceph
  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: false
    controller: true
    kind: Deployment
    name: rook-ceph-operator
    uid: 795ddb4f-33f9-4617-be83-1a3373df725e
  resourceVersion: "116577"
  uid: 81ce4b71-1383-4320-933e-ad5e9ac036ea
[🎩︎]mrajanna@fedora rook $]kuberc deleterc delete -f deploy/examples/radosnamespace.yaml 
cephblockpoolradosnamespace.ceph.rook.io "namespace-a" deleted
[🎩︎]mrajanna@fedora rook $]kuberc delete -fget cm rook-ceph-csi-config -oyaml
apiVersion: v1
data:
  csi-cluster-config-json: '[{"clusterID":"rook-ceph","monitors":["10.104.250.35:6789"]},{"clusterID":"4df0bb6e1b7ddc0c1ea080cbe043dd1a","monitors":["10.104.250.35:6789"],"cephFS":{"subvolumeGroup":"group-a"}}]'
kind: ConfigMap
metadata:
  creationTimestamp: "2022-02-10T05:13:47Z"
  name: rook-ceph-csi-config
  namespace: rook-ceph
  ownerReferences:
  - apiVersion: apps/v1
    blockOwnerDeletion: false
    controller: true
    kind: Deployment
    name: rook-ceph-operator
    uid: 795ddb4f-33f9-4617-be83-1a3373df725e
  resourceVersion: "116777"
  uid: 81ce4b71-1383-4320-933e-ad5e9ac036ea

Documentation/ceph-pool-radosnamespace.md

deploy/examples/radosnamespace.yaml

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

pkg/daemon/ceph/client/radosnamespace.go

pkg/operator/ceph/pool/radosnamespace/controller.go

pkg/operator/ceph/pool/radosnamespace/controller_test.go

pkg/operator/ceph/pool/radosnamespace/controller.go

leseb · 2022-02-14T13:47:23Z

pkg/operator/ceph/pool/radosnamespace/controller_test.go

+	})
+}
+
+func Test_buildClusterID(t *testing.T) {


probably not needed if we move the function to opcontroller

travisn · 2022-02-14T20:43:31Z

.commitlintrc.json

@@ -30,6 +30,7 @@
        "rgw",
        "security",
        "subvolumegroup",
+	"radosnamespace",


what if we keep it simpler for the prefix?

Suggested change

"radosnamespace",

"rados",

as the concept is related to radosnamespace in a pool can this be kept as radosnamespace only?

We might even just call it namespace, this will better match the latest proposed crd name. This is just a commit prefix so shorter is fine.

deploy/examples/radosnamespace.yaml

pkg/daemon/ceph/client/radosnamespace.go

pkg/operator/ceph/pool/dependents.go

pkg/operator/ceph/pool/radosnamespace/controller.go

travisn · 2022-02-14T21:03:06Z

Documentation/ceph-pool-radosnamespace.md

+Rook allows creation of Ceph BlockPool
+[RadosNamespaces](https://docs.ceph.com/en/latest/man/8/rbd/) through the
+custom resource definitions (CRDs). BlockPool Rados Namespace is an abstraction
+for a block pool. For more information about BlockPool and namespace refer to


Suggested change

for a block pool. For more information about BlockPool and namespace refer to

for a Ceph RADOS namespace. For more information about BlockPool and namespace refer to

It's an abstraction for a block pool. we can have N number of rados namespaces in a pool.

How about this?

Suggested change

for a block pool. For more information about BlockPool and namespace refer to

inside a block pool. For more information about BlockPool and namespace refer to

sounds good 👍

Documentation/ceph-pool-radosnamespace.md

travisn · 2022-02-14T21:09:27Z

Documentation/ceph-pool-radosnamespace.md

@@ -0,0 +1,44 @@
+---
+title: RadosNamespace CRD


Instead of its own topic, what if we just added a new section to ceph-pool-crd.md? This really seems like a detail related to the block pools so it doesn't need its own topic. This would be similar to the object multisite page that lists the three related CRDs for multisite. See ceph-object-multisite-crd.md.

I followed the pattern followed when we added subvolumegroup CRD, we have a new document for subvolumegroups https://rook.io/docs/rook/v1.8/ceph-fs-subvolumegroup.html, if we expand the CRD and add more details to radosnamespace CRD or the pool CRD the file content might grow. if required I can change this and add it to pool CRD section,

Seems like the subvolumegroup doc could likewise be merged with the filesystem CRD doc. @leseb wdyt?

will wait for @leseb response or I can do a follow-up PR for both filesystem and namespace doc.

Documentation/ceph-pool-radosnamespace.md

deploy/olm/assemble/metadata-common.yaml

pkg/operator/ceph/pool/dependents.go

pkg/operator/ceph/pool/radosnamespace/controller.go

umangachapagain · 2022-02-15T06:01:11Z

pkg/operator/ceph/pool/radosnamespace/controller.go

+	cephBlockPoolRadosNamespace := &cephv1.CephBlockPoolRadosNamespace{}
+	if err := client.Get(r.opManagerContext, name, cephBlockPoolRadosNamespace); err != nil {
+		if kerrors.IsNotFound(err) {
+			logger.Debug("CephBlockPoolRadosNamespace resource not found. Ignoring since object must be deleted.")
+			return
+		}
+		logger.Warningf("failed to retrieve ceph blockpool rados namespace %q to update status to %q. %v", name, status, err)
+		return
+	}


We already do this before calling updateStatus. Can't we just pass that same object here and issue an update?

should be possible. @leseb any thoughts on this one?

The get is there to make sure we fetch the latest version of the object before updating it. I have a PR I'll rebase on top of this, to use a better pattern: #9686

Isn't it problematic to update status on latest version even before operator reconciles it? retryOnConflict makes it easier to try updating the same object version that the operator reconciled.

Documentation/ceph-pool-radosnamespace.md

travisn · 2022-02-24T14:34:49Z

Documentation/ceph-pool-radosnamespace.md

@@ -0,0 +1,44 @@
+---
+title: RadosNamespace CRD


Documentation/ceph-pool-radosnamespace.md

travisn · 2022-02-24T14:42:18Z

Documentation/ceph-pool-radosnamespace.md

+[RadosNamespaces](https://docs.ceph.com/en/latest/man/8/rbd/) through the
+custom resource definitions (CRDs). BlockPool Rados Namespace is an abstraction
+inside a block pool. For more information about BlockPool and namespace refer to
+the [Ceph docs](https://docs.ceph.com/en/latest/man/8/rbd/).


Could you add a sentence that gives an idea why a namespace would be useful or when it should be used?

sure will do it.

tests/scripts/github-action-helper.sh

travisn · 2022-03-31T18:15:56Z

.github/workflows/canary-integration-test.yml

-      if: failure() && github.event_name == 'pull_request'
-      uses: mxschmitt/action-tmate@v3
-      timeout-minutes: 60
+      - name: checkout


Can you adjust your editor's auto-indentation so it doesn't change all these blocks? It's difficult to see the diff, and we don't want it changing all the time between commits.

My appologies, I didn't check the latest push, Editor messed it up. fixing it.

travisn · 2022-03-31T18:22:19Z

pkg/operator/ceph/pool/radosnamespace/controller.go

+	}
+	// Success! Let's update the status
+	if cephCluster.Spec.External.Enable {
+		r.updateStatus(r.client, request.NamespacedName, cephv1.ConditionConnected)


Why do we have a "connected" status for the rados namespace? The cluster CR tracks if there is a connection, but this CR only creates/deletes rados namespaces, right? Seems like it's either created or not, rather than connected.

And according to the check above, creating a rados namespace from an external cluster isn't supported. Shouldn't that check just return an error and fail the reconcile? Or what is the purpose in reconciling the rados namespace for an external cluster?

Why do we have a "connected" status for the rados namespace? The cluster CR tracks if there is a connection, but this CR only creates/deletes rados namespaces, right? Seems like it's either created or not, rather than connected.

This is the same as the https://github.com/rook/rook/blob/master/pkg/operator/ceph/file/subvolumegroup/controller.go#L265-L272 nothing new in this one. i have seen in other places also cephv1.ConditionType used for status.Phase, just reusing the same here also.

Or what is the purpose in reconciling the rados namespace for an external cluster?

This is requested to generate the clusterID which is required for the storageclass creation, the whole idea of supporting external cluster got added in #9694

@leseb What is the need to support subvolume groups or rados namespaces as "connected" status in external mode? It seems like nothing is reconciled in external mode, not sure what external mode is doing here. "connected" just seems like a status for the cluster CR, not other CRs like this.

The external condition is a bit of a hack/noop to not perform some commands and expect the producer cluster (external) to perform the operation instead.
I tend to agree that we could have avoided the "Connected" state for subvolumegrp CRD and perhaps this one but we need to decide if we keep it for this new controller or change both :)

Just to be clear, the controllers don't need to do anything for external clusters for the rados namespaces and subvolume groups, right? In that case I'd vote that we don't support them in external mode and just error out if they are created in an external cluster.

No they do need to do something on external mode, which is updating the ceph-csi configmap to input the subvolumegroup/namespace crd name cpeh-csi should use.

Ok I see now where the configmap is updated. I think what would help is just a little code refactor like suggested above so it's more obvious.

travisn · 2022-03-31T18:25:24Z

pkg/operator/ceph/pool/radosnamespace/controller.go

+		return reconcile.Result{}, nil
+	}
+
+	if !cephCluster.Spec.External.Enable {


The rados namespace behaves so differently for external clusters, how about refactoring this into two methods to handle external or local? The multiple checks for external clusters is confusing to me. Or just error for external clusters as suggested below?

Followed the same code pattern used for https://github.com/rook/rook/blob/master/pkg/operator/ceph/file/subvolumegroup/controller.go#L211-L271 if required to separate it out i can do it. please let me know what do you think

I think the refactor would really help understand the code flow, something like:

if external cluster { // call a helper method to update the configmap return } // for local clusters create the namespace // call helper method to update the configmap

Done. added help function and did refactoring.

mergify · 2022-04-04T13:42:46Z

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

travisn · 2022-04-04T13:46:57Z

@Madhu-1 just a simple merge conflict to resolve...

subhamkrai

looks good after the merge conflict resolves.

BlaineEXE

LGTM after merge conflict resolved.

This introduces a new CRD to add the ability to create rados namespace for a given ceph block pool. Typically the name of the pool is the name of the blockpool created by rook. Closes: rook#7035 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Madhu-1 · 2022-04-05T04:41:14Z

@travisn Resolved merge conflicts. PTAL

.github/workflows/canary-integration-test.yml

radosnamespace: add new CRD (backport #9733)

THe radosnamespace CRD is already added by rook#9733, Checking the external script with the namespace created by the CRD Signed-off-by: parth-gr <paarora@redhat.com>

Madhu-1 added the do-not-merge DO NOT MERGE :) label Feb 14, 2022

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from 5c628cc to 44860b9 Compare February 14, 2022 07:28

subhamkrai requested changes Feb 14, 2022

View reviewed changes

pkg/daemon/ceph/client/radosnamespace.go Outdated Show resolved Hide resolved

pkg/operator/ceph/pool/dependents.go Outdated Show resolved Hide resolved

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from f04b3f2 to d921888 Compare February 14, 2022 07:45

Madhu-1 removed the do-not-merge DO NOT MERGE :) label Feb 14, 2022

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from 964bf40 to 6fe3d73 Compare February 14, 2022 08:11

Madhu-1 requested a review from subhamkrai February 14, 2022 08:12

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from c2a01c0 to e2fc574 Compare February 14, 2022 09:33

Madhu-1 added the ceph main ceph tag label Feb 14, 2022

Madhu-1 requested review from leseb and travisn February 14, 2022 11:39

leseb requested changes Feb 14, 2022

View reviewed changes

Madhu-1 force-pushed the fix-7035 branch from e2fc574 to 098016c Compare February 14, 2022 15:09

Madhu-1 requested a review from leseb February 14, 2022 15:09

Madhu-1 force-pushed the fix-7035 branch from 098016c to 39d5af8 Compare February 14, 2022 15:11

travisn requested changes Feb 14, 2022

View reviewed changes

Madhu-1 force-pushed the fix-7035 branch from 39d5af8 to 6a824e0 Compare February 15, 2022 04:55

Madhu-1 requested a review from travisn February 15, 2022 04:55

umangachapagain reviewed Feb 15, 2022

View reviewed changes

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from d119e82 to 90471ed Compare February 16, 2022 15:29

travisn requested changes Feb 24, 2022

View reviewed changes

Madhu-1 requested a review from travisn March 2, 2022 05:53

Madhu-1 force-pushed the fix-7035 branch from 90471ed to 80ec18a Compare March 2, 2022 05:53

Madhu-1 force-pushed the fix-7035 branch from f5efbe5 to f6021c3 Compare March 31, 2022 04:44

Madhu-1 requested review from travisn and BlaineEXE March 31, 2022 04:46

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from 19918a4 to e286058 Compare March 31, 2022 09:40

travisn requested changes Mar 31, 2022

View reviewed changes

Madhu-1 force-pushed the fix-7035 branch from e286058 to e9444b5 Compare April 1, 2022 05:01

Madhu-1 requested a review from travisn April 1, 2022 05:01

Madhu-1 force-pushed the fix-7035 branch 2 times, most recently from ea69c49 to af789ed Compare April 4, 2022 05:41

travisn approved these changes Apr 4, 2022

View reviewed changes

travisn added the backport-release-1.9 label Apr 4, 2022

subhamkrai reviewed Apr 4, 2022

View reviewed changes

BlaineEXE approved these changes Apr 4, 2022

View reviewed changes

namespace: add new CRD

b0fc7c9

This introduces a new CRD to add the ability to create rados namespace for a given ceph block pool. Typically the name of the pool is the name of the blockpool created by rook. Closes: rook#7035 Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

Madhu-1 force-pushed the fix-7035 branch from af789ed to b0fc7c9 Compare April 5, 2022 04:40

subhamkrai approved these changes Apr 5, 2022

View reviewed changes

parth-gr approved these changes Apr 5, 2022

View reviewed changes

.github/workflows/canary-integration-test.yml Show resolved Hide resolved

travisn merged commit 9b6c0c9 into rook:master Apr 5, 2022

mergify bot mentioned this pull request Apr 5, 2022

radosnamespace: add new CRD (backport #9733) #9989

Merged

travisn added a commit that referenced this pull request Apr 6, 2022

Merge pull request #9989 from rook/mergify/bp/release-1.9/pr-9733

19f8b25

radosnamespace: add new CRD (backport #9733)

parth-gr mentioned this pull request May 9, 2022

namespace: adding radosnamespace in canary CI #10233

Merged

7 tasks

	for a block pool. For more information about BlockPool and namespace refer to
	for a Ceph RADOS namespace. For more information about BlockPool and namespace refer to

	for a block pool. For more information about BlockPool and namespace refer to
	inside a block pool. For more information about BlockPool and namespace refer to

radosnamespace: add new CRD #9733

radosnamespace: add new CRD #9733

Conversation

Madhu-1 commented Feb 14, 2022

Madhu-1 commented Feb 14, 2022

subhamkrai left a comment

Choose a reason for hiding this comment

Madhu-1 commented Feb 14, 2022

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

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

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

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

mergify bot commented Apr 4, 2022

travisn commented Apr 4, 2022

subhamkrai left a comment

Choose a reason for hiding this comment

BlaineEXE left a comment

Choose a reason for hiding this comment

Madhu-1 commented Apr 5, 2022