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

Support storing cephfs omap in a different namespace in rados under same filesystem metadata pool #4599

Open
Madhu-1 opened this issue May 6, 2024 · 4 comments · May be fixed by #4652 or #4661
Open
Assignees
Labels
component/cephfs Issues related to CephFS enhancement New feature or request

Comments

@Madhu-1
Copy link
Collaborator

Madhu-1 commented May 6, 2024

Describe the feature you'd like to have

Support storing cephfs omap in a different namespace in rados under the same filesystem metadata pool

What is the value to the end user? (why is it a priority?)

With this feature, even the metadata of the pvc/subvolumes will be stored under a different namespace, where security/isolation is the priority the user/customers can configure csi to use a different namespace.

How will we know we have a good solution? (acceptance criteria)

We can have a new field in the configmap where we have an entry for the subvolumegroup name.

@Madhu-1 Madhu-1 added component/cephfs Issues related to CephFS enhancement New feature or request labels May 6, 2024
@iPraveenParihar iPraveenParihar self-assigned this May 15, 2024
@zerotens
Copy link

zerotens commented May 30, 2024

Hello together, funny story.
I wrote today a PR, but i didn't see the issue and that @iPraveenParihar was already working on it.
https://github.com/ceph/ceph-csi/compare/devel...zerotens:cephfs-namespace?expand=1

Is this reasonable to you or do you really want to make it configurable via configmap which would need many function calls as the constant is used in many places.

@iPraveenParihar
Copy link
Contributor

@zerotens, Thanks for the PR 😃
I had already started working on this issue and was testing the same, and have opened a PR now #4661

Is this reasonable to you or do you really want to make it configurable via configmap which would need many function calls as the constant is used in many places.

This issue actually needs radosNamespace option in the csi ConfigMap as this will allow users to use different radosNamespace for different clusterID, similar to what we have in RBD config section.

However, your PR allow users to overwrite the default radosNamespace which at the point is not the requirement I guess.
@Rakshith-R, do you think overwriting the default radosNamespace via flag will be useful?

@Rakshith-R
Copy link
Contributor

@zerotens, Thanks for the PR 😃 I had already started working on this issue and was testing the same, and have opened a PR now #4661

Is this reasonable to you or do you really want to make it configurable via configmap which would need many function calls as the constant is used in many places.

This issue actually needs radosNamespace option in the csi ConfigMap as this will allow users to use different radosNamespace for different clusterID, similar to what we have in RBD config section.

However, your PR allow users to overwrite the default radosNamespace which at the point is not the requirement I guess. @Rakshith-R, do you think overwriting the default radosNamespace via flag will be useful?

Making it configurable via configmap allows us to implement multi tenancy with the same ceph filesystem so it is definitely required as praveen mentioned.

But if you would like to have an option to pass it through cli arguement and change the default, that would be welcome as well.

@zerotens
Copy link

zerotens commented Jun 5, 2024

@zerotens, Thanks for the PR 😃 I had already started working on this issue and was testing the same, and have opened a PR now #4661

Is this reasonable to you or do you really want to make it configurable via configmap which would need many function calls as the constant is used in many places.

This issue actually needs radosNamespace option in the csi ConfigMap as this will allow users to use different radosNamespace for different clusterID, similar to what we have in RBD config section.
However, your PR allow users to overwrite the default radosNamespace which at the point is not the requirement I guess. @Rakshith-R, do you think overwriting the default radosNamespace via flag will be useful?

Making it configurable via configmap allows us to implement multi tenancy with the same ceph filesystem so it is definitely required as praveen mentioned.

Okay, i agree if the user needs to access multiple ceph clusters from 1 kubernetes cluster.

But if you would like to have an option to pass it through cli arguement and change the default, that would be welcome as well.

Well depending on the usecase i would say it would be enough to just configure it at the configmap level.

I have seen the draft PR of @iPraveenParihar.
There are still many fsutil.RadosNamespace constant access calls in the PR #4661
For example:
https://github.com/iPraveenParihar/ceph-csi/blob/fd4e89ed004357c97e7c528af72c6f3e7010c713/internal/cephfs/driver.go#L125
https://github.com/iPraveenParihar/ceph-csi/blob/fd4e89ed004357c97e7c528af72c6f3e7010c713/internal/cephfs/driver.go#L131

I would feel much safer if the constant would be configurable.
Any future code change which could rely on this constant could break the support of storing csi metadata in a different namespace.
If everything get's refactored to calling the new function GetCephFSRadosNamespace instead of relying on the constant to retrieve the namespace, my PR would be obsolete.

Otherwise i would still like to follow up on the constant to cli configurable global variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/cephfs Issues related to CephFS enhancement New feature or request
Projects
None yet
4 participants