docs: document how to have StorageClass consume Rados Namespace #14173

obnoxxx · 2024-05-07T18:12:37Z

description

This change expands the documentation to describe how to create a storageclass that consumes a CephBlockPoolRadodNamespace.

Issue resolved by this Pull Request:
Resolves #13214

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn · 2024-05-07T19:42:06Z

deploy/examples/cephblockpoolradosnamespace-test.yaml

@@ -0,0 +1,8 @@
+apiVersion: ceph.rook.io/v1


The only "test.yaml" files needed are basically for pools to use replica 1, or when some other component needs to define a single daemon. Since there is nothing related to replica 1 in this file, let's revert this file.

travisn · 2024-05-07T19:43:34Z

deploy/examples/csi/rbd/storageclass-radosnamespace-test.yaml

@@ -0,0 +1,66 @@
+apiVersion: ceph.rook.io/v1


Let's rename this file without the "test" suffix. I think no need to commit a test version of this file. It won't be common to use during development. Normally the "test" file are only used during development.

deploy/examples/csi/rbd/storageclass-radosnamespace-test.yaml

travisn · 2024-05-07T19:45:06Z

deploy/examples/csi/rbd/storageclass-radosnamespace-test.yaml

+spec:
+  # The name of the CephBlockPool CR where the namespace is created.
+  blockPoolName: replicapool
+


nit: remove blank line

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

travisn

Just a few more suggestions on whitespace and comments

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

travisn · 2024-05-07T22:36:58Z

deploy/examples/csi/rbd/storageclass-radosnamespace.yaml

+  failureDomain: osd
+  replicated:
+    size: 3
+    # Disallow setting pool with replica 1, this could lead to data loss without recovery.


This comment is not accurate since we changed back to a replica 3 pool. For simplicity of the example, how about just remove the comments from the CephBlockPool? They can see comments in other pool examples if needed.

parth-gr · 2024-05-08T08:04:31Z

deploy/examples/csi/rbd/storageclass-radosnamespace.yaml

+  # normally, clusterID is the kubernetes namespace where the rook cluster is running
+  # But putting the ClusterID of  a CephBlockPoolRadosNamespace CR here instead
+  # will let the storageClass consume that Rados namespace.
+  clusterID: 80fc4f4bacc064be641633e6ed25ba7e


will this be always same for this example setup, if not we should not hardcode this

best I know yes, it will be the same (being a hash of the blockpool name etc). this is why I put it into the example file.

Okay that's great, seems constant

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

Line 362 in 517b1bf

clusterID := fmt.Sprintf("%s-%s-block-%s", cephBlockPoolRadosNamespace.Namespace, cephBlockPoolRadosNamespace.Spec.BlockPoolName, getRadosNamespaceName(cephBlockPoolRadosNamespace))

I'm not sure why we decided to have a hash, instead we can always use the full name. Or request a new field from csi for enrolling radosnamesapce

@parth-gr the line you are copying is missing the surrounding context.

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

Lines 361 to 364 in 517b1bf

func buildClusterID(cephBlockPoolRadosNamespace *cephv1.CephBlockPoolRadosNamespace) string {

clusterID := fmt.Sprintf("%s-%s-block-%s", cephBlockPoolRadosNamespace.Namespace, cephBlockPoolRadosNamespace.Spec.BlockPoolName, getRadosNamespaceName(cephBlockPoolRadosNamespace))

return k8sutil.Hash(clusterID)

}

Notice that the function here (buildClusterID()) returns Hash(clusterID). Thus, the ID is the hashed form of the name.

I believe Hash() was used because it ensures that the ID doesn't exceed the allowed number of characters for naming.

parth-gr · 2024-05-08T08:05:36Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+Extract the clusterID from the CephBlockPoolRadosNamespace CR:
+
+ ```shell
+ $ kubectl -n rook-ceph get cephblockpoolradosnamespace -o yaml


can we provide the direct cmd

$KUBECTL -n "$NAMESPACE" get cephblockpoolradosnamespace.ceph.rook.io/"$RADOS_NAMESPACE" -o jsonpath='{.status.info.clusterID}'

@parth-gr wrote:

can we provide the direct cmd

$KUBECTL -n "$NAMESPACE" get cephblockpoolradosnamespace.ceph.rook.io/"$RADOS_NAMESPACE" -o jsonpath='{.status.info.clusterID}'

good idea! I will play with it a bit but it is currently unclear to me how/where the variables $KUBECTL, $NAMESPACE, etc are expected to be set.

you can directly use kubectl and namespace as rook-ceph

you can directly use kubectl and namespace as rook-ceph

Makes sense, thanks! I will try an possibly update it this way.

@parth-gr , I tried this:

kubectl -n rook-ceph get cephblockpoolradosnamespaces.ceph.rook.io/namespace-a -o jsonpath='{status.info.ClusterID}'

but only got garbage as output.

I also tried to find a command to use jsonpath to directly get the namespace name (in this case namespace-a, but no success yet.

lets catch up and check, if it doesn't works

@parth-gr more details about the error I am facing:

$ kubectl -n rook-ceph get cephblockpoolradosnamespaces.ceph.rook.io/namespace-a -o jsonpath='{status.info.ClusterID}' error: error executing jsonpath "{status.info.ClusterID}": Error executing template: unrecognized identifier status. Printing more information for debugging the template: template was: {status.info.ClusterID} object given to jsonpath engine was: map[string]interface {}{"apiVersion":"ceph.rook.io/v1", "kind":"CephBlockPoolRadosNamespace", "metadata":map[string]interface {}{"creationTimestamp":"2024-05-07T14:27:11Z", "finalizers":[]interface {}{"cephblockpoolradosnamespace.ceph.rook.io"}, "generation":1, "managedFields":[]interface {}{map[string]interface {}{"apiVersion":"ceph.rook.io/v1", "fieldsType":"FieldsV1", "fieldsV1":map[string]interface {}{"f:spec":map[string]interface {}{".":map[string]interface {}{}, "f:blockPoolName":map[string]interface {}{}}}, "manager":"kubectl-create", "operation":"Update", "time":"2024-05-07T14:27:11Z"}, map[string]interface {}{"apiVersion":"ceph.rook.io/v1", "fieldsType":"FieldsV1", "fieldsV1":map[string]interface {}{"f:metadata":map[string]interface {}{"f:finalizers":map[string]interface {}{".":map[string]interface {}{}, "v:\"cephblockpoolradosnamespace.ceph.rook.io\"":map[string]interface {}{}}}}, "manager":"rook", "operation":"Update", "time":"2024-05-07T14:27:11Z"}, map[string]interface {}{"apiVersion":"ceph.rook.io/v1", "fieldsType":"FieldsV1", "fieldsV1":map[string]interface {}{"f:status":map[string]interface {}{".":map[string]interface {}{}, "f:info":map[string]interface {}{".":map[string]interface {}{}, "f:clusterID":map[string]interface {}{}}, "f:phase":map[string]interface {}{}}}, "manager":"rook", "operation":"Update", "subresource":"status", "time":"2024-05-08T12:34:52Z"}}, "name":"namespace-a", "namespace":"rook-ceph", "resourceVersion":"28365", "uid":"fcae1125-b349-4260-b9da-d46f795128ce"}, "spec":map[string]interface {}{"blockPoolName":"replicapool"}, "status":map[string]interface {}{"info":map[string]interface {}{"clusterID":"80fc4f4bacc064be641633e6ed25ba7e"}, "phase":"Failure"}} $

it should be a small c not the capital one for clusterID

@parth-gr , I tried this:

kubectl -n rook-ceph get cephblockpoolradosnamespaces.ceph.rook.io/namespace-a -o jsonpath='{status.info.ClusterID}'

but only got garbage as output.

I also tried to find a command to use jsonpath to directly get the namespace name (in this case namespace-a, but no success yet.

@parth-gr now I managed to produce a working command and updated the doc accordingly :)

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

Madhu-1 · 2024-05-08T10:30:18Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+apiVersion: storage.k8s.io/v1
+kind: StorageClass
+metadata:
+  name: rook-ceph-block


can we name this something else as we already have this name for RBD storageclass created for blockpool

@Madhu-1 wrote:

can we name this something else as we already have this name for RBD storageclass created for blockpool

sure. what do you suggest? How about rook-ceph-block-rados-ns?

can we name this something else as we already have this name for RBD storageclass created for blockpool

sure. what do you suggest? How about rook-ceph-block-rados-ns?

@Madhu-1 fwiw, I used this name in the doc now.

Madhu-1 · 2024-05-08T16:32:17Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+Extract the clusterID from the CephBlockPoolRadosNamespace CR:
+
+ ```shell
+ $ kubectl -n rook-ceph get cephblockpoolradosnamespace -o yaml


it should be a small c not the capital one for clusterID

Madhu-1 · 2024-05-08T16:34:00Z

deploy/examples/csi/rbd/storageclass-radosnamespace.yaml

@@ -0,0 +1,64 @@
+apiVersion: ceph.rook.io/v1


The only concern of this new storageclass is that we need to take care of updating this as well if we add/remove any new fields for rbd specific option for rbd storageclass , this is kind of a maintenance burden we will get :(

I share this concern. Though I'm on the fence about whether to keep the example. At the end of the day, though, I think the only truly unique thing here is rados namespace and storageclass's clusterID. I think the documentation is sufficiently clear enough that users shouldn't require an example manifest to understand, and maybe it would be best to remove this so it doesn't grow stale over time.

@BlaineEXE wrote:

I share this concern. Though I'm on the fence about whether to keep the example. At the end of the day, though, I think the only truly unique thing here is rados namespace and storageclass's clusterID. I think the documentation is sufficiently clear enough that users shouldn't require an example manifest to understand, and maybe it would be best to remove this so it doesn't grow stale over time.

Agreed. I can remove the example manifest from the PR.

@BlaineEXE example manifest is now removed.

I do think the full example manifests are worth the maintenance burden for common scenarios. But I'm fine skipping this example for now since it's less common.

travisn · 2024-05-17T15:53:33Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+```
+
+In this example, replace `namespace-a` by the actual name of the radosnamespace
+created before


missing part of the sentence?

@travisn wrote:

missing part of the sentence?

sorry, what is missing or unclear here?

There is no period at the end of what I presume to be a full sentence. I think that is the source of the confusion.

BlaineEXE · 2024-05-17T15:59:13Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+
+Extract the clusterID from the CephBlockPoolRadosNamespace CR:
+
+ ```shell


shell is used for shell scripts. console is for console/terminal commands and output.

Also, there is no need to indent this one space from the left.

Suggested change

```shell

```console

@BlaineEXE wrote :

shell is used for shell scripts. console is for console/terminal commands and output.

Also, there is no need to indent this one space from the left.

done (manually.

BlaineEXE · 2024-05-17T17:22:11Z

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

+Now set the `clusterID` retrieved from the previous step into the `clusterID` of the storage class.
+Example:


Is it your intent to have "Example:" on a new line? If so, by markdown syntax rules, there needs to be an empty line between "Now set..." and "Example:". Otherwise, "Example:" will appear at the end of the previous sentence when rendered.

I usually use VSCodes "markdown preview" feature when writing docs so I can be sure the basic rendering shows what I intend.

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md

travisn

LGTM after commits are squashed!

closes: rook#13214 Co-authored-by: Travis Nielsen <tnielsen@redhat.com> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com> Signed-off-by: Michael Adam <obnox@samba.org>

obnoxxx · 2024-05-18T10:30:01Z

@travisn wrote:

LGTM after commits are squashed!

done. squashed now.

docs: document how to have StorageClass consume Rados Namespace (backport #14173)

obnoxxx requested review from travisn, Madhu-1 and BlaineEXE May 7, 2024 18:12

obnoxxx changed the title ~~docs: document how to hace StorageClass consume Rados Namespace~~ docs: document how to have StorageClass consume Rados Namespace May 7, 2024

obnoxxx force-pushed the sc-radosnamespace branch from bb2f694 to 3908430 Compare May 7, 2024 18:41

travisn requested changes May 7, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch 5 times, most recently from 919c271 to 10890c7 Compare May 7, 2024 20:42

travisn requested changes May 7, 2024

View reviewed changes

parth-gr added the skip-ci label May 8, 2024

parth-gr requested changes May 8, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch from 1fd19be to e48fb02 Compare May 8, 2024 09:22

BlaineEXE reviewed May 8, 2024

View reviewed changes

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md Outdated Show resolved Hide resolved

Madhu-1 reviewed May 8, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch 3 times, most recently from 31d26d8 to 08bc131 Compare May 17, 2024 15:40

travisn reviewed May 17, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch 2 times, most recently from 117cc48 to 614c7ea Compare May 17, 2024 15:58

BlaineEXE requested changes May 17, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch 5 times, most recently from f04620a to d8accad Compare May 17, 2024 16:45

BlaineEXE reviewed May 17, 2024

View reviewed changes

obnoxxx force-pushed the sc-radosnamespace branch from d8accad to e984345 Compare May 17, 2024 17:45

BlaineEXE approved these changes May 17, 2024

View reviewed changes

travisn reviewed May 17, 2024

View reviewed changes

Documentation/CRDs/Block-Storage/ceph-block-pool-rados-namespace-crd.md Show resolved Hide resolved

travisn reviewed May 17, 2024

View reviewed changes

docs: document how to have StorageClass consume Rados Namespace

8e7ac95

closes: rook#13214 Co-authored-by: Travis Nielsen <tnielsen@redhat.com> Co-authored-by: Blaine Gardner <b.blaine.gardner@gmail.com> Signed-off-by: Michael Adam <obnox@samba.org>

obnoxxx force-pushed the sc-radosnamespace branch from cd9cce5 to 8e7ac95 Compare May 18, 2024 10:22

obnoxxx added the backport-release-1.14 label May 18, 2024

travisn approved these changes May 18, 2024

View reviewed changes

parth-gr approved these changes May 20, 2024

View reviewed changes

travisn merged commit 3d62020 into rook:master May 20, 2024
19 checks passed

mergify bot mentioned this pull request May 20, 2024

docs: document how to have StorageClass consume Rados Namespace (backport #14173) #14242

Merged

6 tasks

travisn added a commit that referenced this pull request May 20, 2024

Merge pull request #14242 from rook/mergify/bp/release-1.14/pr-14173

4b97f98

docs: document how to have StorageClass consume Rados Namespace (backport #14173)

	func buildClusterID(cephBlockPoolRadosNamespace *cephv1.CephBlockPoolRadosNamespace) string {
	clusterID := fmt.Sprintf("%s-%s-block-%s", cephBlockPoolRadosNamespace.Namespace, cephBlockPoolRadosNamespace.Spec.BlockPoolName, getRadosNamespaceName(cephBlockPoolRadosNamespace))
	return k8sutil.Hash(clusterID)
	}


		Extract the clusterID from the CephBlockPoolRadosNamespace CR:

		```shell

		Now set the `clusterID` retrieved from the previous step into the `clusterID` of the storage class.
		Example:

docs: document how to have StorageClass consume Rados Namespace #14173

docs: document how to have StorageClass consume Rados Namespace #14173

Conversation

obnoxxx commented May 7, 2024 • edited

description

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

travisn 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BlaineEXE May 17, 2024 • 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

obnoxxx May 17, 2024 • 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

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 May 17, 2024 • edited

Choose a reason for hiding this comment

travisn left a comment

Choose a reason for hiding this comment

obnoxxx commented May 18, 2024

obnoxxx commented May 7, 2024 •

edited

BlaineEXE May 17, 2024 •

edited

obnoxxx May 17, 2024 •

edited

BlaineEXE May 17, 2024 •

edited