-
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
Add support for cephfs helm chat #218
Conversation
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@gman0 PTAL |
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.
All in all, this looks really good. Thanks for contributing it.
There are two files removed that I think you didn't intend to:
deploy/rbd/helm/templates/provisioner-role.yaml
deploy/rbd/helm/templates/provisioner-rolebinding.yaml
A few other comments inline
deploy/rbd/helm/README.md
Outdated
|
||
```bash | ||
helm install --namespace "ceph-csi-rbd" --name "ceph-csi-rbd" ceph-csi/ceph-csi-rbd | ||
helm install --name "ceph-csi-rbd" ceph-csi/ceph-csi-rbd |
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.
Why remove the namespace? This puts it in "default"
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 templates in kubernetes folder deploy everything in the default namespace, to keep consistency between both I deleted helm deployment in a different namespace
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 don't think installing it in "default" is a good recommendation for the users. Please undo.
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.
undo done
resources: ["volumeattachments"] | ||
verbs: ["get", "list", "watch", "update"] | ||
- apiGroups: [""] | ||
resources: ["configmaps"] |
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.
Can you please double check the permssions for cephfs? rbd needs configmap reading, but I'm not sure cephfs does.
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 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.
Ok. I'll talk with rootfs about it. But I don't think that should block this pr.
as we are creating cluster-role and binding cluster-role to a service-account, a normal role is not required, tested this and everything is working fine |
You tested out the rbd driver without those two files and provisioning rbd volumes still worked? |
here are the logs
|
I think it will start, but it won't function properly without permissions to read/write the configmaps. The rbd helm rbac is a little different then the static manifests. The security is tighter as it only has permissions to the configmaps in the specific namespace rather then for the entire cluster. Its a change I wouild like to propose to the static manifests too, but haven't had time yet. |
means volume mount won't work as expected?
i am not aware of this issue. to make this PR completed, do i need to bring back |
Yeah, I don't think it will work properly to provision the volumes without the role. I think it best if the PR only added the new chart, not touched the other chart. Its mergeable in its current state without the rbd changes. |
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
Signed-off-by: Madhu Rajanna <madhupr007@gmail.com>
@kfox1111 reverted back the changes for role and role binding |
/lgtm |
thanks @kfox1111 |
Add support for cephfs helm chat
Syncing latest changes from upstream devel for ceph-csi
This PR adds the support for cephfs helm chat
logs