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

sit.cephfs: Add share configured with vfs_ceph_new(non-mgr) #116

Merged
merged 1 commit into from
Aug 28, 2024

Conversation

anoopcs9
Copy link
Collaborator

@anoopcs9 anoopcs9 commented Aug 5, 2024

depends on #117

@spuiuk
Copy link
Collaborator

spuiuk commented Aug 7, 2024

I think instead of adding a new variable, we should just add to the list of variants. "vfs_new"?

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Aug 9, 2024

I think instead of adding a new variable, we should just add to the list of variants. "vfs_new"?

There are multiple aspects to be considered here:

  • As of now smb mgr module based configuration is based on backend=cephfs.mgr.vfs which indicates the need for ceph native method using the second part of the backend specification. This looked more relatable and meaningful.
  • Going forward we may include another configuration using proxy. If go with vfs_new we end up with vfs_new_proxy. I think cephfs.vfs.new_proxy will be more sensible. Of course this will end up as cephfs.mgr.vfs_new_proxy for the other case.
  • Sooner or later we would have only two: cephfs and cephfs.mgr. All other variations are supposed to be clubbed together. Till then it might be easier to put anything extra(new vfs module, proxy requirement) other than the setup method choices separately to avoid any complication in intelligently deriving everything from a single variable.

Let me know if you had something else in your mind.

@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Aug 9, 2024

I think instead of adding a new variable, we should just add to the list of variants. "vfs_new"?

I think I'll reverse the order here to first make the change to group possible shares(starting with cephfs) and then rework this PR to add another share(using vfs_ceph_new) on top of it.

@spuiuk
Copy link
Collaborator

spuiuk commented Aug 9, 2024

I think the code is far too complex now with a number of if conditions causing it to split into multiple code paths. This change adds another such conditions. I think it would be far easier to go straight to grouping now.

@anoopcs9 anoopcs9 changed the title sit.cephfs: Add optional support to access shares via vfs_ceph_new sit.cephfs: Add share configured with vfs_ceph_new Aug 10, 2024
@anoopcs9 anoopcs9 marked this pull request as draft August 10, 2024 11:15
Copy link

dpulls bot commented Aug 10, 2024

⚠️ Dpulls not installed on repository ceph/ceph. Checkout our quickstart for how to install.

@anoopcs9 anoopcs9 force-pushed the vfs-ceph-new branch 2 times, most recently from 43f75e1 to 89271ac Compare August 14, 2024 06:08
@anoopcs9
Copy link
Collaborator Author

anoopcs9 commented Aug 14, 2024

/retest centos-ci/xfs

@anoopcs9 anoopcs9 changed the title sit.cephfs: Add share configured with vfs_ceph_new sit.cephfs: Add share configured with vfs_ceph_new(non-mgr) Aug 14, 2024
@anoopcs9 anoopcs9 marked this pull request as ready for review August 14, 2024 12:14
xhernandez
xhernandez previously approved these changes Aug 28, 2024
Copy link

dpulls bot commented Aug 28, 2024

🎉 All dependencies have been resolved !

Signed-off-by: Anoop C S <anoopcs@cryptolab.net>
@anoopcs9 anoopcs9 dismissed xhernandez’s stale review August 28, 2024 09:05

The merge-base changed after approval.

@xhernandez xhernandez merged commit dfccf34 into samba-in-kubernetes:main Aug 28, 2024
11 checks passed
@anoopcs9 anoopcs9 deleted the vfs-ceph-new branch August 28, 2024 10:23
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.

3 participants