update driver name as per csi spec #223

Madhu-1 · 2019-02-22T07:06:28Z

Closes #222

rootfs · 2019-02-26T00:44:53Z

deploy/rbd/helm/templates/nodeplugin-daemonset.yaml

@@ -39,7 +39,11 @@ spec:
          lifecycle:
            preStop:
              exec:
-                  command: ["/bin/sh", "-c", "rm -rf /registration/csi-rbdplugin /registration/csi-rbdplugin-reg.sock"]          
+                  command: [


cc @kfox1111

looks ok to me.

j-griffith

LGTM

humblec · 2019-03-08T08:04:51Z

@Madhu-1 whats pending here ?

ShyamsundarR

I do understand this is WIP/DNM, and so following items maybe already on your list. Calling it out explicitly for your attention in case it is not.

driverName defaults in main.go for cephfs and rbd needs to be updated
Similarly files under docs directory need to be updated
- As we take the driver name as an argument, should we enforce in the code that the trailer should be csi.ceph.com and report bail out otherwise
- We should also enforce in the code, as this is an argument, that the total length is not more than 63 chars, as that is what the CSI spec calls out for the name length

ShyamsundarR · 2019-03-08T15:41:29Z

deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml

@@ -34,11 +34,11 @@ spec:
            - "--v=5"
          env:
            - name: ADDRESS


For all ADDRESS environment variables and its counterpart volume mounts named "socket-dir" we should drop the explicit naming scheme using the drivername/pluginname. These are local to the container sockets and directories for use, and do not need a naming scheme that includes the drivername.

Comment applies to all files under deploy.

I say this to reduce amount of changes required to such deployment YAMLs when there is a need to deploy multiple instances of Ceph-CSI drivers, with different leading driver names. (e.g rook-instance-1-cephfsplugin.csi.ceph.com and rook-instance-1-cephfsplugin.csi.ceph.com, for CSI naming when dealing with multiple CSI instances see https://github.com/rook/rook/pull/2677/files#r261420176 )

@ShyamsundarR can you open a separate issue for this one?

I thought the change can be done along with this one, as it is related and touches exactly the same set of lines that a patch addressing this would.

IMO these also require discussion, what was the idea behind doing the things in the way as there are now, and also what advantage we are going to get by changing things, last but not least requires some amount of testing and validation. I would like to take it up as a separate task

Fine, we can deal with this in a separate PR.

dungdm93 · 2019-03-11T07:14:55Z

Hello guys,

My suggestion the better names are:

csi.ceph.com/rbd
csi.ceph.com/cephfs

Because it follow the same pattern as in-tree Kubernetes plugins as well as Rook:

kubernetes.io/rbd
kubernetes.io/gce-pd
kubernetes.io/aws-ebs
ceph.rook.io/block
...

kfox1111 · 2019-03-11T15:36:07Z

Does this have any effect on existing instances?

ShyamsundarR · 2019-03-11T17:33:32Z

pkg/util/util.go

@@ -68,3 +71,23 @@ func CreatePersistanceStorage(sPath, metaDataStore, driverName string) (CachePer
 func createPersistentStorage(persistentStoragePath string) error {
 	return os.MkdirAll(persistentStoragePath, os.FileMode(0755))
 }
+
+// ValidateDriverName validates the driver name
+func ValidateDriverName(driverName string) error {


Validation comments:

We should use the validation API IsDNS1035Label (which includes the length check as well)

We should validate trailer as ".csi.ceph.com" so that we retain the domain for any instance

current code is taken from the kubernetes core validation

https://github.com/kubernetes/kubernetes/blob/c094f873a9d1ab444c0834fa3d3adf5ed2f44105/pkg/apis/core/validation/validation.go#L1442-L1458

func ValidateCSIDriverName(driverName string, fldPath *field.Path) field.ErrorList { allErrs := field.ErrorList{} if len(driverName) == 0 { allErrs = append(allErrs, field.Required(fldPath, "")) } if len(driverName) > 63 { allErrs = append(allErrs, field.TooLong(fldPath, driverName, 63)) } for _, msg := range validation.IsDNS1123Subdomain(strings.ToLower(driverName)) { allErrs = append(allErrs, field.Invalid(fldPath, driverName, msg)) } return allErrs }

drivername could be anything, example org.test.csi-hostpath etc i dont think we should validate the trainer as .csi.ceph.com. if its required i can do that one

It looks like the kubernetes implementation and spec differ? I would stick to the spec for now.

ShyamsundarR · 2019-03-11T17:34:28Z

cmd/cephfs/main.go

@@ -43,6 +43,13 @@ func init() {

 func main() {

+	err := util.ValidateDriverName(*driverName)


Validation can cause breakage for implementations that have been using a name other than the default in the driverName parameter, that is not conformant to the current scheme.

Should we call this out in the docs/commit or somewhere? @gman0 @rootfs

Example: assume, some usage passed in the option "--drivername=my-ceph-rbd", on updating to the latest image, these drivers would fail to start as the name would fail validation

(applies to rbd/main.go as well)

with current change AFAIK nothing breaks if we add a validation to checks ceph.csi.com then the things will start breaking

tested sample code for validation

import ( "fmt" "strings" "github.com/pkg/errors" "k8s.io/apimachinery/pkg/util/validation" ) func ValidateDriverName(driverName string) error { if len(driverName) == 0 { return errors.New("driver name is empty") } if len(driverName) > 63 { return errors.New("driver name length should be less than 63 chars") } var err error for _, msg := range validation.IsDNS1123Subdomain(strings.ToLower(driverName)) { if err == nil { err = errors.New(msg) continue } err = errors.Wrap(err, msg) } return err } func main() { driverNames := []string{"csi-rbdplugin", "csi-cephfsplugin", "cephfsplugin.csi.ceph.com", "rbdplugin.csi.ceph.com", "TEST-DRIVER", "TEST_DRIVER123"} for _, name := range driverNames { err := ValidateDriverName(name) if err != nil { fmt.Printf("driver name %s failed with err %s\n", name, err) } } } ------------------------------------------------------------- go run main.go driver name TEST_DRIVER123 failed with err a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is '[a-z0-9]([-a-z0-9]*[a-z0-9])?(\.[a-z0-9]([-a-z0-9]*[a-z0-9])?)*')

ShyamsundarR · 2019-03-11T17:37:17Z

cmd/cephfs/main.go

+	}
+	//update plugin name
+	cephfs.PluginFolder = cephfs.PluginFolder + *driverName
+
 	cp, err := util.CreatePersistanceStorage(cephfs.PluginFolder, *metadataStorage, *driverName)


There is a breaking change when using metaDataStoage as the "node" and the drivername is changed from the default. The node level storage will now be queried under the new name, and hence if it was present under the older name it would not be found and older volumeID maps would not be found.

Reasoning is due to changing driverName input to NewCachePersister, when metaDataStore is "node"

As this is just a node level storage, we should retain the older name as the drivername that is passed to this function. It would still break in case some usage passed in the option "--drivername" to be different than the default, which needs separate discussion/consolidation anyway (as in validation itself may break and hence driver would never come up, and we possibly need to note any migration steps hence).

(Applies to rbd as well)

this also won't do any breaking change as the driver name validation is not failing,
current code changes should work with old cluster, i will validate these changes and update here

ShyamsundarR · 2019-03-11T17:39:19Z

cmd/cephfs/main.go

+		klog.Fatalln(err)
+	}
+	//update plugin name
+	cephfs.PluginFolder = cephfs.PluginFolder + *driverName


cephfs.PluginFolder and rbd.PluginFolder need not be derived from the drivername, as in the case of rbd this is not used at all (afaik), and in the case of cephfs it is used to provide a location where root of CephFS can be mounted and create/purge operations on specific volume IDs can be performed. We hence need not make these names variables and derive their values from the drivername value.
Instead retaining these as constants would be better, and on an upgrade to the new image we should start using the new (or pre-existing older) paths, as we do not depend on information stored in these paths.

the main intention of this PR to fix the issue related to driver name. to keep the current implementation as it is i have done these changes

ShyamsundarR · 2019-03-11T17:43:30Z

Does this have any effect on existing instances?

Useful question, thanks.

I see some impact if using metaDataStorage as "node", as the current changes would fail to find the node level storage directory and hence loose the existing volumeID maps. If using default driverName from the past, then this can be handled in the code.

The other impact on existing instances would be failure to start the driver, in case the name does not conform to the standard enforced. This would need a change to the drivername parameter to the existing pods deployed.

Madhu-1 · 2019-03-12T04:50:45Z

Does this have any effect on existing instances?

Useful question, thanks.

I see some impact if using metaDataStorage as "node", as the current changes would fail to find the node level storage directory and hence loose the existing volumeID maps. If using default driverName from the past, then this can be handled in the code.

The other impact on existing instances would be failure to start the driver, in case the name does not conform to the standard enforced. This would need a change to the drivername parameter to the existing pods deployed.

this will break only if the driver name is not specified during the deployment, which makes to use default driver name.

Madhu-1 · 2019-03-12T06:04:26Z

this PR doesn't do any breaking changes if ceph-csi is deployed with driver name.

ShyamsundarR · 2019-03-12T12:05:43Z

this PR doesn't do any breaking changes if ceph-csi is deployed with driver name.

My comments pertain to iff we enforce the validation rules. Also, as you (@Madhu-1 ) educated me, we push the container image to quay on each PR merge, which makes it difficult to make any breaking changes anyway. Hence we would need to drop the strict validation of the drivername to end with any trailer.

If that validation is dropped, then backward compatibility is not broken.

Madhu-1 · 2019-03-13T06:26:48Z

@rootfs @ShyamsundarR @gman0 @j-griffith @kfox1111 @humblec updated PR with suggested driver name PTAL

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

humblec · 2019-03-14T05:24:05Z

LGTM. Thanks @Madhu-1 .. This is one of the important changes has to be made without delay.. So requesting review with a timeout :) @rootfs @gman0 @ShyamsundarR

gman0

LGTM, thanks guys!

rootfs · 2019-03-15T22:07:25Z

thanks, can you get this into Rook too?

Madhu-1 · 2019-03-18T04:19:31Z

@rootfs sure i will send a PR for rook

update driver name as per csi spec

Syncing latest changes from upstream devel for ceph-csi

rootfs reviewed Feb 26, 2019

View reviewed changes

j-griffith reviewed Mar 1, 2019

View reviewed changes

ShyamsundarR reviewed Mar 8, 2019

View reviewed changes

Madhu-1 force-pushed the fix-222-1.0 branch from 6850900 to 6dacf64 Compare March 11, 2019 07:06

Madhu-1 changed the title ~~[WIP] [DNM] update driver name as per csi spec~~ update driver name as per csi spec Mar 11, 2019

ShyamsundarR reviewed Mar 11, 2019

View reviewed changes

Madhu-1 force-pushed the fix-222-1.0 branch from 614ed08 to b88ea92 Compare March 13, 2019 06:25

Madhu-1 force-pushed the fix-222-1.0 branch from b88ea92 to f68e075 Compare March 13, 2019 06:34

Madhu-1 added 2 commits March 13, 2019 12:04

Fix driver name as per CSI spec

d61a87b

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

update readme to delete namespace

497411b

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

Madhu-1 force-pushed the fix-222-1.0 branch from f68e075 to 497411b Compare March 13, 2019 06:34

gman0 approved these changes Mar 14, 2019

View reviewed changes

gman0 merged commit a4dd845 into ceph:csi-v1.0 Mar 14, 2019

Madhu-1 mentioned this pull request Mar 18, 2019

rename socket directory to a common name #265

Merged

ShyamsundarR mentioned this pull request Apr 29, 2019

How to migrate from csi-driver 0.3 -> csi-driver csi-1.0 #296

Closed

wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019

Merge pull request ceph#223 from Madhu-1/fix-222-1.0

c6de28e

update driver name as per csi spec

nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 4, 2024

Merge pull request ceph#223 from red-hat-storage/sync_us--devel

36055c9

Syncing latest changes from upstream devel for ceph-csi

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update driver name as per csi spec #223

update driver name as per csi spec #223

Madhu-1 commented Feb 22, 2019

rootfs Feb 26, 2019

kfox1111 Mar 11, 2019

j-griffith left a comment

humblec commented Mar 8, 2019

ShyamsundarR left a comment

ShyamsundarR Mar 8, 2019

Madhu-1 Mar 11, 2019

ShyamsundarR Mar 11, 2019

Madhu-1 Mar 12, 2019

ShyamsundarR Mar 12, 2019

dungdm93 commented Mar 11, 2019

kfox1111 commented Mar 11, 2019

ShyamsundarR Mar 11, 2019

Madhu-1 Mar 12, 2019

ShyamsundarR Mar 12, 2019

ShyamsundarR Mar 11, 2019

Madhu-1 Mar 12, 2019

ShyamsundarR Mar 11, 2019

Madhu-1 Mar 12, 2019

ShyamsundarR Mar 11, 2019

Madhu-1 Mar 12, 2019

ShyamsundarR commented Mar 11, 2019

Madhu-1 commented Mar 12, 2019

Madhu-1 commented Mar 12, 2019

ShyamsundarR commented Mar 12, 2019

Madhu-1 commented Mar 13, 2019

humblec commented Mar 14, 2019

gman0 left a comment

rootfs commented Mar 15, 2019

Madhu-1 commented Mar 18, 2019

		@@ -43,6 +43,13 @@ func init() {

		func main() {

		err := util.ValidateDriverName(*driverName)

update driver name as per csi spec #223

update driver name as per csi spec #223

Conversation

Madhu-1 commented Feb 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

j-griffith left a comment

Choose a reason for hiding this comment

humblec commented Mar 8, 2019

ShyamsundarR 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

dungdm93 commented Mar 11, 2019

kfox1111 commented Mar 11, 2019

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

ShyamsundarR commented Mar 11, 2019

Madhu-1 commented Mar 12, 2019

Madhu-1 commented Mar 12, 2019

ShyamsundarR commented Mar 12, 2019

Madhu-1 commented Mar 13, 2019

humblec commented Mar 14, 2019

gman0 left a comment

Choose a reason for hiding this comment

rootfs commented Mar 15, 2019

Madhu-1 commented Mar 18, 2019