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

rename socket directory to a common name #265

Merged
merged 1 commit into from
Mar 22, 2019

Conversation

Madhu-1
Copy link
Collaborator

@Madhu-1 Madhu-1 commented Mar 18, 2019

as the socket directory will be created inside the container no need to follow the plugin name in for the directory creation, this will also reduce the code changes if we want to change driver name.

Fixes: #223 (comment)
Signed-off-by: Madhu Rajanna madhupr007@gmail.com

@ShyamsundarR
Copy link
Contributor

Current changes look fine. Are we not handling the helm change for the same in this PR?

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 18, 2019

@ShyamsundarR if I change that value I need to make a lot of changes in helm folder, so I kept it as it is, changing this value should be enough for the users to deploy helm chats

most of the fields are derived from the values so changing value should be sufficient,

@kfox1111 any comments related to this one?

@ShyamsundarR
Copy link
Contributor

@ShyamsundarR if I change that value I need to make a lot of changes in helm folder, so I kept it as it is, changing this value should be enough for the users to deploy helm chats

most of the fields are derived from the values so changing value should be sufficient,

Just for clarification, I was suggesting to just change that default value in the variable to what is now the default, and NOT make it static and remove it from the variables yaml for Helm. Does that require changes beyond the change to the variable in the file in Helm?

@humblec
Copy link
Collaborator

humblec commented Mar 18, 2019

@Madhu-1 eventhough I see the intention of this PR, I doubt this is really necessary. At times , the container or mount path 'at same location' gives more clarity. If its causing confusion to someone/developer, I think thats still fine/ok to have as they will eventually find out the change should be necessary or not :).

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 18, 2019

@ShyamsundarR if I change that value I need to make a lot of changes in helm folder, so I kept it as it is, changing this value should be enough for the users to deploy helm chats
most of the fields are derived from the values so changing value should be sufficient,

Just for clarification, I was suggesting to just change that default value in the variable to what is now the default, and NOT make it static and remove it from the variables yaml for Helm. Does that require changes beyond the change to the variable in the file in Helm?

yes, this is the variable used for binding to the hostpath also. https://github.com/ceph/ceph-csi/blob/csi-v1.0/deploy/rbd/helm/templates/nodeplugin-daemonset.yaml#L109

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.

Looks fine.

@humblec
Copy link
Collaborator

humblec commented Mar 18, 2019

@Madhu-1 eventhough I see the intention of this PR, I doubt this is really necessary. At times , the container or mount path 'at same location' gives more clarity. If its causing confusion to someone/developer, I think thats still fine/ok to have as they will eventually find out the change should be necessary or not :).

@ShyamsundarR
Copy link
Contributor

@Madhu-1 eventhough I see the intention of this PR, I doubt this is really necessary. At times , the container or mount path 'at same location' gives more clarity. If its causing confusion to someone/developer, I think that's still fine/ok to have as they will eventually find out the change should be necessary or not :).

Considering I raised this issue, here is what I think,

  • This mount path at the same location is actually not required, and is adding verbosity without any need
  • The socket dir need not take in the CSI name at all, so why add that and maintain it for any revision of the CSI name (thinking multi instance CSI if any in the future)
  • The current scheme just makes the socket look special, whereas it can just be at a well known path

The existing scheme does not provide any clarity or is not required, IMO simplifying it is better than retaining it with these long names.

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.

Seems like a resonable simplification to me without any real negative side effects

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 19, 2019

@gman0 @rootfs PTAL

@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 21, 2019

@rootfs @gman0 can you guys merge this one if it looks good

@rootfs
Copy link
Member

rootfs commented Mar 21, 2019

@Madhu-1 what about attacher in rbd plugin?

@rootfs
Copy link
Member

rootfs commented Mar 21, 2019

cc @kfox1111

as the socket directory will be created
inside the container no need to follow
the plugin name in for the directory
creation, this will also reduce the code
changes if we want to change driver name.

Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@Madhu-1
Copy link
Collaborator Author

Madhu-1 commented Mar 22, 2019

@Madhu-1 what about attacher in rbd plugin?

@rootfs Thanks, updated

@rootfs rootfs merged commit 5c6d200 into ceph:csi-v1.0 Mar 22, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
rename socket directory to a common name
nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 12, 2024
Syncing latest changes from 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.

5 participants