csi: option to customize csi driver name prefix #13622

Madhu-1 · 2024-01-25T11:57:35Z

For now, we are using the operator namespace name as the prefix for the csi driver, This PR provides an option for the users if someone wants to have their prefix for the csi driver, if someone tries to change the prefix for existing csi driver rook operator will fail to reconcile the csi driver. If the option is not set the default behavior will continue as it is where the operator namespace will be used as a prefix for the csi driver name(s).

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.

obnoxxx

typo diver instead of driver in the commit message and PR description.

failing CI test could be a flake.

deploy/examples/operator.yaml

travisn · 2024-01-25T17:24:04Z

pkg/operator/ceph/csi/csi.go

@@ -320,5 +320,8 @@ func (r *ReconcileCSI) setParams(ver *version.Info) error {
 	if strings.EqualFold(k8sutil.GetValue(r.opConfig.Parameters, "CSI_NFS_ATTACH_REQUIRED", "true"), "false") {
 		CSIParam.NFSAttachRequired = false
 	}
+
+	CSIParam.DriverNamePrefix = k8sutil.GetValue(r.opConfig.Parameters, "CSI_DRIVER_NAME_PREFIX", "")


Can we just default to the operator namespace here?

Suggested change

CSIParam.DriverNamePrefix = k8sutil.GetValue(r.opConfig.Parameters, "CSI_DRIVER_NAME_PREFIX", "")

CSIParam.DriverNamePrefix = k8sutil.GetValue(r.opConfig.Parameters, "CSI_DRIVER_NAME_PREFIX", r.opConfig.OperatorNamespace)

travisn · 2024-01-25T17:25:03Z

pkg/operator/ceph/csi/spec.go

+	}
+
+	// If the driver name prefix is not set, set it to the operator namespace.
+	if tp.DriverNamePrefix == "" {


This is not needed if we set the default as mentioned previously

travisn · 2024-01-25T17:28:56Z

pkg/operator/ceph/csi/spec.go

+	}
+
+	if EnableRBD {
+		rbdDriverNamePrefix, err := getCSIDriverNamePrefixFromDeployment(r.opManagerContext, r.context.Clientset, r.opConfig.OperatorNamespace, csiRBDProvisioner, "csi-rbdplugin")


Instead of checking all three deployments, can we just check one of them? Either they will all succeed, or they will all fail, so we shouldn't need to check all three.

@travisn i added all 3 to ensure that we are good for all the 3 drivers because there are cases where either 1 is a disabled case. please let me know if you still prefer to check for only one, if its it should be rbd or cephfs?

I was thinking let's check the first one that is enabled, then we can skip checking the others. If these checks are moved out to a helper method it will be simple to implement by returning after the first enabled one is checked.

deploy/examples/operator.yaml

BlaineEXE · 2024-01-25T18:20:16Z

Is this a commonly-requested feature? TBH, I think the overall simplicity of using the operator namespace as the prefix is good. It means that we don't need to document any special "if you did this, then this example changes", etc. etc. etc.

Being able to provide a simple "this is the way to do this" is a big win for users overall, IMO.

Also consider that if any users decide to do this and they use the cluster helm chart, they will probably complain about their storageclasses not being generated right.

If users are having a problem with the generated name being too long, would it be sufficient to provide an example for how to change the namespace that Rook is installed in instead?

Madhu-1 · 2024-01-26T10:13:43Z

Is this a commonly-requested feature? TBH, I think the overall simplicity of using the operator namespace as the prefix is good. It means that we don't need to document any special "if you did this, then this example changes", etc. etc. etc.

This is not a commonly-requested feature but it is kind of really required if someone doesn't want a dynamic CSI driver name changes based on the namespace where Rook is deployed.

Being able to provide a simple "this is the way to do this" is a big win for users overall, IMO.

Also consider that if any users decide to do this and they use the cluster helm chart, they will probably complain about their storageclasses not being generated right.

I didn't consider this case because i did search only in the .go file.

If users are having a problem with the generated name being too long, would it be sufficient to provide an example for how to change the namespace that Rook is installed in instead?

The problem is not with the namespace name length (but it could also be one reason) but the main problem is with dynamic csi driver names as i mentioned above.

travisn · 2024-01-26T14:37:31Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+`my-prefix.nfs.csi.ceph.com`.
+
+!!! note
+ Please be careful when setting the `CSI_DRIVER_NAME_PREFIX`


May need indentation of 4 spaces for the paragraph to get the rendered docs correctly

travisn · 2024-01-26T14:38:58Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+...
+```
+
+Please note that for the RBD driver, the prefix will be


How about a bullet list?

Suggested change

Please note that for the RBD driver, the prefix will be

When the prefix is set, the driver names will be:

- RBD: `my-prefix.rbd.csi.ceph.com`

- CephFS: ...

- NFS: ...

travisn · 2024-01-26T14:39:09Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+...
+```
+
+Same prefix must be set in the volumesnapshotclass as well:


Suggested change

Same prefix must be set in the volumesnapshotclass as well:

The same prefix must be set in the volumesnapshotclass as well:

travisn · 2024-01-26T14:40:08Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

@@ -35,6 +35,52 @@ example, if the Rook operator is running in the namespace `my-namespace` the
 provisioner value should be `my-namespace.rbd.csi.ceph.com`. The same provisioner
 name must be set in both the storageclass and snapshotclass.

+## Configure custom Driver name prefix for CSI Drivers
+
+To use a custom prefix for the CSI drivers, you need to set the


Let's mention that the namespace is the default prefix

Suggested change

To use a custom prefix for the CSI drivers, you need to set the

To use a custom prefix for the CSI drivers instead of the namespace prefix, set the

travisn · 2024-01-26T14:40:49Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+
+To use a custom prefix for the CSI drivers, you need to set the
+`CSI_DRIVER_NAME_PREFIX` environment variable in the operator configmap. For
+instance, to use the prefix `my-prefix` for the CSI drivers, you can set the


Don't say "you" in the documentation, just use the command form to be more direct.

Suggested change

instance, to use the prefix `my-prefix` for the CSI drivers, you can set the

instance, to use the prefix `my-prefix` for the CSI drivers, set the

travisn · 2024-01-26T14:41:24Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+
+Once the configmap is updated, the CSI drivers will be deployed with the
+`my-prefix` prefix. You must set the same prefix in both the storageclass and
+snapshotclass. For example, if you want to use the prefix `my-prefix` for the


There are a few other places to change, I won't mention them all

Suggested change

snapshotclass. For example, if you want to use the prefix `my-prefix` for the

snapshotclass. For example, to use the prefix `my-prefix` for the

deploy/charts/rook-ceph-cluster/values.yaml

pkg/operator/ceph/csi/spec.go

travisn · 2024-01-26T14:49:17Z

pkg/operator/ceph/csi/spec.go

+	}
+
+	if EnableRBD {
+		rbdDriverNamePrefix, err := getCSIDriverNamePrefixFromDeployment(r.opManagerContext, r.context.Clientset, r.opConfig.OperatorNamespace, csiRBDProvisioner, "csi-rbdplugin")


I was thinking let's check the first one that is enabled, then we can skip checking the others. If these checks are moved out to a helper method it will be simple to implement by returning after the first enabled one is checked.

deploy/examples/operator.yaml

BlaineEXE · 2024-01-26T16:10:12Z

Is this a commonly-requested feature? TBH, I think the overall simplicity of using the operator namespace as the prefix is good. It means that we don't need to document any special "if you did this, then this example changes", etc. etc. etc.

This is not a commonly-requested feature but it is kind of really required if someone doesn't want a dynamic CSI driver name changes based on the namespace where Rook is deployed.

If users are having a problem with the generated name being too long, would it be sufficient to provide an example for how to change the namespace that Rook is installed in instead?

The problem is not with the namespace name length (but it could also be one reason) but the main problem is with dynamic csi driver names as i mentioned above.

I still don't understand why dynamic CSI driver names are an issue. If users aren't complaining of this, it seems like an extra amount of complexity to continue maintaining when we don't need to.

Madhu-1 · 2024-01-26T16:36:10Z

Is this a commonly-requested feature? TBH, I think the overall simplicity of using the operator namespace as the prefix is good. It means that we don't need to document any special "if you did this, then this example changes", etc. etc. etc.

This is not a commonly-requested feature but it is kind of really required if someone doesn't want a dynamic CSI driver name changes based on the namespace where Rook is deployed.

If users are having a problem with the generated name being too long, would it be sufficient to provide an example for how to change the namespace that Rook is installed in instead?

The problem is not with the namespace name length (but it could also be one reason) but the main problem is with dynamic csi driver names as i mentioned above.

I still don't understand why dynamic CSI driver names are an issue. If users aren't complaining of this, it seems like an extra amount of complexity to continue maintaining when we don't need to.

dynamic csi driver name is an issue because it's not expected to change the csi driver names based on the deployment namespaces just for example kubervirt always expects the csi driver name to be consistent because it provides the best option to the users based on the hardcoded driver names. This option is getting added to ensure that we can be backward compatible and also open an option to allow user(s) always to set the static driver names in their cluster.

--- update ---

https://github.com/kubevirt/containerized-data-importer/blob/0b4ebe81af353d2611344ac8cfef64cc1c4d58ad/pkg/storagecapabilities/storagecapabilities.go#L43 One example where it's having hardcoded values. after discussing more with the kubevirt team (internally) it's suggested that it's not expected to change the driver names based on the namespace name because the names are the representation of the csi driver it should not get changed dynamically because no one can predict the names in dynamic cases.

Documentation/Storage-Configuration/Advanced/ceph-configuration.md

travisn · 2024-01-26T16:43:47Z

Documentation/Storage-Configuration/Advanced/ceph-configuration.md

    -e "s/\(.*\): [-_A-Za-z0-9]*\.\(.*\) # driver:namespace:cluster/\1: $ROOK_CLUSTER_NAMESPACE.\2 # driver:namespace:cluster/g" \
  common.yaml operator.yaml cluster.yaml # add other files or change these as desired for your config

 # You need to use `apply` for all Ceph clusters after the first if you have only one Operator
 kubectl apply -f common.yaml -f operator.yaml -f cluster.yaml # add other files as desired for yourconfig
 ```

+Refer to the CSI driver


Suggested change

Refer to the CSI driver

Also see the CSI driver

travisn · 2024-01-26T16:44:15Z

Documentation/Storage-Configuration/Advanced/ceph-configuration.md

    -e "s/\(.*\): [-_A-Za-z0-9]*\.\(.*\) # driver:namespace:cluster/\1: $ROOK_CLUSTER_NAMESPACE.\2 # driver:namespace:cluster/g" \
  common.yaml operator.yaml cluster.yaml # add other files or change these as desired for your config

 # You need to use `apply` for all Ceph clusters after the first if you have only one Operator
 kubectl apply -f common.yaml -f operator.yaml -f cluster.yaml # add other files as desired for yourconfig
 ```

+Refer to the CSI driver
+[documentation](../Ceph-CSI/ceph-csi-drivers.md#Configure-CSI-Drivers-in-non-default-namespace)
+to update examples to use a non-default namespace.


How about more specific?

Suggested change

to update examples to use a non-default namespace.

to update the csi provisioner names in the storage class.

travisn · 2024-01-26T16:46:08Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

@@ -35,6 +35,87 @@ example, if the Rook operator is running in the namespace `my-namespace` the
 provisioner value should be `my-namespace.rbd.csi.ceph.com`. The same provisioner
 name must be set in both the storageclass and snapshotclass.

+search for `# csi-provisioner-name` in the [csi


The docs actually cannot use a relative path to the code because the docs are generated to a different site. I'd suggest no need for a link, we can have a simple comment like this:

Suggested change

search for `# csi-provisioner-name` in the [csi

To find the provisioner name in the example storage classes, search for: `# csi-provisioner-name`

travisn · 2024-01-26T16:47:47Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+example](../deploy/examples/csi/) folder and update the name of the
+`provisioner`/`driver` as required.
+
+The following `sed` command can be used to update the provisioner name in the


In the interest of simple docs, I'd suggest not even including this example. Maintaining this sed will be difficult too.

travisn · 2024-01-26T16:49:44Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+Once the configmap is updated, the CSI drivers will be deployed with the
+`my-prefix` prefix. The same prefix must be set in both the storageclass and
+snapshotclass. For example, to use the prefix `my-prefix` for the
+CSI drivers, you can set the following in the storageclass:


Suggested change

CSI drivers, you can set the following in the storageclass:

CSI drivers, update the provisioner in the storageclass:

travisn · 2024-01-26T16:50:39Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+    environment variable. It should be done only in fresh deployments because
+    changing the prefix in an existing cluster will result in unexpected behavior.
+
+search for `# csi-provisioner-name` in the [csi


Same suggestions as above for this section

travisn · 2024-01-26T16:56:25Z

deploy/examples/csi/cephfs/snapshotclass.yaml

+# The prefix of the driver name can be configured by updating
+# CSI_DRIVER_NAME_PREFIX in the rook-ceph-operator-config configmap by default
+# it is namespace name where rook operator is running i.e rook-ceph.
+driver: .cephfs.csi.ceph.com # csi-provisioner-name


We still need to keep the default rook-ceph prefix there. And actually I'm thinking we don't need lines 6-8 documented in every storage class. It's not going to be a common scenario to update the storage class, so they can just use the documentation topic, and no need to document here. If we just have the # csi-provisioner-name tag, then they can easily search where to update it.

i think while doing testing the rook-ceph got removed, brought it back and removed extra text as well

deploy/examples/operator.yaml

pkg/operator/ceph/csi/spec.go

Documentation/Storage-Configuration/Advanced/ceph-configuration.md

travisn · 2024-01-26T18:08:43Z

Documentation/Storage-Configuration/Ceph-CSI/ceph-csi-drivers.md

+To find the provisioner name in the example storageclasses and
+volumesnapshotclass, search for: `# csi-provisioner-name`
+
+## Configure custom Driver name prefix for CSI Drivers


This section seems like it needs to be combined with the previous section. How about making it a subsection?

Suggested change

## Configure custom Driver name prefix for CSI Drivers

### Configure custom Driver name prefix for CSI Drivers

pkg/operator/ceph/csi/spec.go

travisn · 2024-01-26T18:14:15Z

pkg/operator/ceph/csi/spec.go

+		if rbdDriverNamePrefix != "" && rbdDriverNamePrefix != driverNamePrefix {
+			return errors.Errorf("rbd driver already exists with prefix %q, cannot use prefix %q", rbdDriverNamePrefix, driverNamePrefix)
+		}
+		return nil


If the driver was not found, looks like this will return here instead of checking the next driver. If no prefix was found, shall we check the next driver?

i missed that case, move the check as required, this should cover both cases now. Thanks

For now we are using the operator namespace name as the prefix for the csi driver, This PR provides an option for the users if someone wants to have their own prefix for the csi driver, if someone tries to change the prefix for existing csi driver rook operator will fail to reconcile the csi driver. Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>

travisn

CI issues look unrelated

obnoxxx

The change looks good and clean now. pylint test is failing but this has to be unrelated since no python code is changed.

csi: option to customize csi driver name prefix (backport #13622)

Madhu-1 force-pushed the csi-name-prefix branch from a660653 to 75be669 Compare January 25, 2024 12:09

obnoxxx requested changes Jan 25, 2024

View reviewed changes

Madhu-1 force-pushed the csi-name-prefix branch from 75be669 to 56a843f Compare January 25, 2024 12:46

Madhu-1 requested a review from obnoxxx January 25, 2024 13:10

parth-gr reviewed Jan 25, 2024

View reviewed changes

deploy/examples/operator.yaml Show resolved Hide resolved

travisn requested changes Jan 25, 2024

View reviewed changes

Madhu-1 force-pushed the csi-name-prefix branch from 56a843f to 766d7df Compare January 26, 2024 10:09

Madhu-1 force-pushed the csi-name-prefix branch 2 times, most recently from e7a8833 to 66b9525 Compare January 26, 2024 10:52

Madhu-1 requested a review from travisn January 26, 2024 10:52

Madhu-1 force-pushed the csi-name-prefix branch 2 times, most recently from bfaaf3c to 3bdd341 Compare January 26, 2024 10:58

travisn requested changes Jan 26, 2024

View reviewed changes

Madhu-1 force-pushed the csi-name-prefix branch 3 times, most recently from 7e3ca99 to b54c690 Compare January 26, 2024 16:29

Madhu-1 requested a review from travisn January 26, 2024 16:30

Madhu-1 force-pushed the csi-name-prefix branch from b54c690 to 5deda74 Compare January 26, 2024 16:32

Madhu-1 force-pushed the csi-name-prefix branch from 5deda74 to 5bb3ea5 Compare January 26, 2024 16:36

travisn requested changes Jan 26, 2024

View reviewed changes

Madhu-1 force-pushed the csi-name-prefix branch 2 times, most recently from 92ca762 to b1d0192 Compare January 26, 2024 17:15

Madhu-1 requested a review from travisn January 26, 2024 17:15

Madhu-1 force-pushed the csi-name-prefix branch from b1d0192 to 1ee7f82 Compare January 26, 2024 17:22

travisn requested changes Jan 26, 2024

View reviewed changes

Madhu-1 force-pushed the csi-name-prefix branch from 1ee7f82 to c35a853 Compare January 29, 2024 09:54

Madhu-1 requested a review from travisn January 29, 2024 09:57

travisn added the backport-release-1.13 label Jan 29, 2024

travisn approved these changes Jan 29, 2024

View reviewed changes

travisn mentioned this pull request Jan 29, 2024

helm: allow defining multiple cephEcStorageClass #13405

Merged

6 tasks

obnoxxx approved these changes Jan 29, 2024

View reviewed changes

travisn merged commit 3b1a428 into rook:master Jan 29, 2024
49 of 50 checks passed

mergify bot mentioned this pull request Jan 29, 2024

csi: option to customize csi driver name prefix (backport #13622) #13640

Merged

travisn added a commit that referenced this pull request Jan 31, 2024

Merge pull request #13640 from rook/mergify/bp/release-1.13/pr-13622

dd7e6e4

csi: option to customize csi driver name prefix (backport #13622)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

csi: option to customize csi driver name prefix #13622

csi: option to customize csi driver name prefix #13622

Madhu-1 commented Jan 25, 2024 •

edited

obnoxxx left a comment

travisn Jan 25, 2024

travisn Jan 25, 2024

travisn Jan 25, 2024

Madhu-1 Jan 26, 2024

travisn Jan 26, 2024

BlaineEXE commented Jan 25, 2024 •

edited

Madhu-1 commented Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

BlaineEXE commented Jan 26, 2024

Madhu-1 commented Jan 26, 2024 •

edited

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

Madhu-1 Jan 26, 2024

travisn Jan 26, 2024

travisn Jan 26, 2024

Madhu-1 Jan 29, 2024

travisn left a comment

obnoxxx left a comment

	CSIParam.DriverNamePrefix = k8sutil.GetValue(r.opConfig.Parameters, "CSI_DRIVER_NAME_PREFIX", "")
	CSIParam.DriverNamePrefix = k8sutil.GetValue(r.opConfig.Parameters, "CSI_DRIVER_NAME_PREFIX", r.opConfig.OperatorNamespace)

-Please note that for the RBD driver, the prefix will be
+When the prefix is set, the driver names will be:
+- RBD: `my-prefix.rbd.csi.ceph.com`
+- CephFS: ...
+- NFS: ...

	Same prefix must be set in the volumesnapshotclass as well:
	The same prefix must be set in the volumesnapshotclass as well:

	To use a custom prefix for the CSI drivers, you need to set the
	To use a custom prefix for the CSI drivers instead of the namespace prefix, set the

	instance, to use the prefix `my-prefix` for the CSI drivers, you can set the
	instance, to use the prefix `my-prefix` for the CSI drivers, set the

	snapshotclass. For example, if you want to use the prefix `my-prefix` for the
	snapshotclass. For example, to use the prefix `my-prefix` for the

	to update examples to use a non-default namespace.
	to update the csi provisioner names in the storage class.

	search for `# csi-provisioner-name` in the [csi
	To find the provisioner name in the example storage classes, search for: `# csi-provisioner-name`

	CSI drivers, you can set the following in the storageclass:
	CSI drivers, update the provisioner in the storageclass:

	## Configure custom Driver name prefix for CSI Drivers
	### Configure custom Driver name prefix for CSI Drivers

csi: option to customize csi driver name prefix #13622

csi: option to customize csi driver name prefix #13622

Conversation

Madhu-1 commented Jan 25, 2024 • edited

obnoxxx 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 commented Jan 25, 2024 • edited

Madhu-1 commented Jan 26, 2024

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 commented Jan 26, 2024

Madhu-1 commented Jan 26, 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

travisn left a comment

Choose a reason for hiding this comment

obnoxxx left a comment

Choose a reason for hiding this comment

Madhu-1 commented Jan 25, 2024 •

edited

BlaineEXE commented Jan 25, 2024 •

edited

Madhu-1 commented Jan 26, 2024 •

edited