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.

Sign up for GitHub

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

Jump to bottom

Support storing cephfs omap data store in radosnamespace via cli argument #4652

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Support storing cephfs omap data store in radosnamespace via cli argument #4652

merged 2 commits into from
Jul 30, 2024

Conversation

zerotens
Copy link
Contributor

Describe what this PR does

Fixes #4599

This PR adds a new cli argument to let users override the current hardcoded radosNamespace "csi" used in CephFS.

ceph-csi/internal/cephfs/util/util.go

Line 24 in ce3ec6a

RadosNamespace = "csi"

Needed is this for multitenant environments where one ceph cluster with one cephfs filesystem is used by multiple kubernetes clusters.
This allows storing the state of every cephfs instance / kubernetes cluster in their own radosNamespaces and allows finer controlled access for cephfs.

This PR also makes those two needed parameters --instanceid (rbd & cephfs Helm Chart) and --radosnamespacecephfs (cephfs Helm Chart) configurable via helm chart.

Is there anything that requires special attention

Is the change backward compatible?

Yes

Are there concerns around backward compatibility?

No, the default values in code have not been changed but are now overrideable via helm and cli arguments.

Future concerns

Depending on future usage of the NFS Controller, currently the NFS controller is using the CephFS variables cephfs.CSIInstanceID and fsutil.RadosNamespace which is now overrideable with the cli argument --radosnamespacecephfs and --instanceid.

ceph-csi/internal/nfs/controller/controllerserver.go

Line 48 in ce3ec6a

store.VolJournal = journal.NewCSIVolumeJournalWithNamespace(cephfs.CSIInstanceID, fsutil.RadosNamespace)

ceph-csi/internal/nfs/controller/controllerserver.go

Line 49 in ce3ec6a

store.SnapJournal = journal.NewCSISnapshotJournalWithNamespace(cephfs.CSIInstanceID, fsutil.RadosNamespace)

ceph-csi/internal/nfs/controller/volume.go

Line 291 in ce3ec6a

j, err := store.VolJournal.Connect(nv.mons, fsutil.RadosNamespace, nv.cr)

ceph-csi/internal/nfs/controller/volume.go

Line 329 in ce3ec6a

j, err := store.VolJournal.Connect(nv.mons, fsutil.RadosNamespace, nv.cr)

Testing

As i don't have a nice way of testing it and this is my first PR and go code for some time i would appreciate an detailed review.

@zerotens zerotens force-pushed the cephfs-namespace branch from de2a831 to 23fe029 Compare May 30, 2024 12:23
@zerotens zerotens changed the title Cephfs namespace Support storing cephfs omap in a different namespace in rados under same filesystem metadata pool May 30, 2024
@nixpanic nixpanic added component/cephfs Issues related to CephFS component/deployment Helm chart, kubernetes templates and configuration Issues/PRs labels May 31, 2024
@zerotens zerotens force-pushed the cephfs-namespace branch from 23fe029 to ebaa395 Compare May 31, 2024 14:24
@zerotens
Copy link
Contributor Author

@nixpanic Linting was hopefully fixed
/retest please

@zerotens zerotens marked this pull request as draft June 5, 2024 17:49
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 5, 2024

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two weeks if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale label Jul 5, 2024
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 5, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

@zerotens zerotens force-pushed the cephfs-namespace branch from ebaa395 to 6704586 Compare July 6, 2024 06:01
@zerotens zerotens changed the title Support storing cephfs omap in a different namespace in rados under same filesystem metadata pool Support storing cephfs omap data store in radosnamespace via cli argument Jul 6, 2024
@zerotens zerotens marked this pull request as ready for review July 6, 2024 06:07
@zerotens zerotens requested a review from Rakshith-R July 6, 2024 06:07
@zerotens zerotens mentioned this pull request Jul 6, 2024
Closed
@github-actions github-actions bot removed the stale label Jul 6, 2024
@zerotens
Copy link
Contributor Author

@Rakshith-R @iPraveenParihar

Is this going to be available in the next release?
Any concerns?

iPraveenParihar
iPraveenParihar previously approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@iPraveenParihar iPraveenParihar 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 @zerotens

@Rakshith-R Rakshith-R requested a review from a team July 24, 2024 09:34
Madhu-1
Madhu-1 requested changes Jul 26, 2024
View reviewed changes
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

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

one nit.

docs/deploy-cephfs.md Outdated Show resolved Hide resolved
@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 26, 2024

This pull request now has conflicts with the target branch. Could you please resolve conflicts and force push the corrected changes? 🙏

zerotens added 2 commits July 27, 2024 22:22
@zerotens
cephfs: support omap data store in radosnamespace via cli argument
6bf03f9 
Signed-off-by: Andreas <zerotens@users.noreply.github.com>
@zerotens
deploy: support omap data store in radosnamespace via cli argument
dc0ddeb 
Signed-off-by: Andreas <zerotens@users.noreply.github.com>
@mergify mergify bot dismissed iPraveenParihar’s stale review July 27, 2024 20:26

Pull request has been modified.

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Jul 30, 2024

@Mergifyio queue

@mergify Mergify
Copy link
Contributor

mergify bot commented Jul 30, 2024 •
edited
Loading

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 7afddb4

@mergify mergify bot added the ok-to-test Label to trigger E2E tests label Jul 30, 2024
@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/upgrade-tests-rbd

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.27

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/k8s-e2e-external-storage/1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e-helm/k8s-1.28

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.29

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.30

@ceph-csi-bot
Copy link
Collaborator

/test ci/centos/mini-e2e/k8s-1.28

@ceph-csi-bot ceph-csi-bot removed the ok-to-test Label to trigger E2E tests label Jul 30, 2024
@mergify mergify bot merged commit 7afddb4 into ceph:devel Jul 30, 2024
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iPraveenParihar iPraveenParihar iPraveenParihar approved these changes

@Madhu-1 Madhu-1 Madhu-1 approved these changes

@Rakshith-R Rakshith-R Awaiting requested review from Rakshith-R

Assignees
No one assigned
Labels
component/cephfs Issues related to CephFS component/deployment Helm chart, kubernetes templates and configuration Issues/PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support storing cephfs omap in a different namespace in rados under same filesystem metadata pool
6 participants
@zerotens @Madhu-1 @ceph-csi-bot @iPraveenParihar @Rakshith-R @nixpanic