-
Notifications
You must be signed in to change notification settings - Fork 561
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
Conversation
@@ -39,7 +39,11 @@ spec: | |||
lifecycle: | |||
preStop: | |||
exec: | |||
command: ["/bin/sh", "-c", "rm -rf /registration/csi-rbdplugin /registration/csi-rbdplugin-reg.sock"] | |||
command: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc @kfox1111
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks ok to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@Madhu-1 whats pending here ? |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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 )
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Hello guys, My suggestion the better names are:
Because it follow the same pattern as in-tree Kubernetes plugins as well as Rook:
|
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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
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
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
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. |
@rootfs @ShyamsundarR @gman0 @j-griffith @kfox1111 @humblec updated PR with suggested driver name PTAL |
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks guys!
thanks, can you get this into Rook too? |
@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
Closes #222