Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update driver name as per csi spec #223

Merged
merged 2 commits into from
Mar 14, 2019
Merged

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Feb 22, 2019

Closes #222

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks ok to me.

Copy link

@j-griffith j-griffith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@humblec
Copy link
Collaborator

humblec commented Mar 8, 2019

@Madhu-1 whats pending here ?

Copy link
Contributor

@ShyamsundarR ShyamsundarR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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
Copy link
Contributor

Does this have any effect on existing instances?

@@ -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 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

func main() {

err := util.ValidateDriverName(*driverName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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])?)*')

}
//update plugin name
cephfs.PluginFolder = cephfs.PluginFolder + *driverName

cp, err := util.CreatePersistanceStorage(cephfs.PluginFolder, *metadataStorage, *driverName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

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
Copy link
Collaborator Author

Madhu-1 commented Mar 12, 2019

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
Copy link
Collaborator Author

Madhu-1 commented Mar 12, 2019

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

@ShyamsundarR
Copy link
Contributor

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
Copy link
Collaborator Author

Madhu-1 commented Mar 13, 2019

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

Madhu-1 added 2 commits March 13, 2019 12:04
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@humblec
Copy link
Collaborator

humblec commented Mar 14, 2019

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

Copy link
Contributor

@gman0 gman0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks guys!

@gman0 gman0 merged commit a4dd845 into ceph:csi-v1.0 Mar 14, 2019
@rootfs
Copy link
Member

rootfs commented Mar 15, 2019

thanks, can you get this into Rook too?

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 18, 2019

@rootfs sure i will send a PR for rook

wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
update driver name as per csi spec
nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 4, 2024
Syncing latest changes from upstream devel for ceph-csi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants