pybind/mgr/volumes: configure case sensitivity by xhernandez · Pull Request #62105 · ceph/ceph

xhernandez · 2025-03-04T16:08:44Z

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins test classic perf Jenkins Job | Jenkins Job Definition
jenkins test crimson perf Jenkins Job | Jenkins Job Definition
jenkins test signed Jenkins Job | Jenkins Job Definition
jenkins test make check Jenkins Job | Jenkins Job Definition
jenkins test make check arm64 Jenkins Job | Jenkins Job Definition
jenkins test submodules Jenkins Job | Jenkins Job Definition
jenkins test dashboard Jenkins Job | Jenkins Job Definition
jenkins test dashboard cephadm Jenkins Job | Jenkins Job Definition
jenkins test api Jenkins Job | Jenkins Job Definition
jenkins test docs ReadTheDocs | Github Workflow Definition
jenkins test ceph-volume all Jenkins Jobs | Jenkins Jobs Definition
jenkins test windows Jenkins Job | Jenkins Job Definition
jenkins test rook e2e Jenkins Job | Jenkins Job Definition

src/pybind/mgr/volumes/fs/operations/versions/subvolume_v2.py

src/pybind/mgr/volumes/fs/operations/versions/subvolume_v1.py

src/pybind/mgr/volumes/fs/operations/subvolume.py

src/pybind/mgr/volumes/fs/volume.py

src/pybind/mgr/volumes/module.py

src/pybind/mgr/volumes/fs/volume.py

avanthakkar · 2025-03-05T12:44:45Z

mypy failure in make check :

volumes/module.py:9: note: In module imported here,
volumes/__init__.py:2: note: ... from here:
volumes/fs/volume.py: note: In member "create_subvolume" of class "VolumeClient":
volumes/fs/volume.py:276: error: Name "casesensitive" is not defined  [name-defined]
Found 1 error in 1 file (checked 32 source files)

avanthakkar

Can you update qa suites too just like for earmarking

batrick

Is there a reason you want to do this at subvolume creation time rather than add fix it in the second RPC ceph fs subvolume charmap ...?

xhernandez · 2025-03-13T08:14:29Z

Is there a reason you want to do this at subvolume creation time rather than add fix it in the second RPC ceph fs subvolume charmap ...?

Initially I thought that doing something similar to earmarking would be ok. Then I realized that there exists this command and some internal helpers that could be used, but I didn't know how or from where to call them. Can you give me some hints about where to call this command when a samba share is created ?

phlogistonjohn · 2025-03-13T20:08:52Z

Is there a reason you want to do this at subvolume creation time rather than add fix it in the second RPC ceph fs subvolume charmap ...?

I'll inject myself into this conversation to add my 2c:
There's a case for having all the major properties of a subvolume available at creation time for an admin to do create a subvolume for a specific purpose (smb shares) in one single action. CLI users get to avoid having to look up a 2nd command and there's no chance of the volume ever being in an in-between state.
In the case of UI the GUI can call a single backend API for all these properties.

PS. If we don't do this now I feel a PM I know will eventually ask for it ;-)

Re-reading my comment I don't think I was being particularly clear. The value of one RPC/API having this is (a) discoverability - when I look up the create subvolume command docs I see an option about setting case insentivity rather than having to look it up seperately and (b) no gaps - not creating the volume sharing it out in the "wrong" state and then needing to go back and change it later

batrick · 2025-03-17T16:33:11Z

Is there a reason you want to do this at subvolume creation time rather than add fix it in the second RPC ceph fs subvolume charmap ...?

Initially I thought that doing something similar to earmarking would be ok. Then I realized that there exists this command and some internal helpers that could be used, but I didn't know how or from where to call them. Can you give me some hints about where to call this command when a samba share is created ?

Well, I don't think there is a straightforward way to adapt the "charmap" commands to this particular flag you want to use. You just want a way to set casesensitive on a the subvolume so what you have is probably fine.

batrick · 2025-03-17T16:36:36Z

Is there a reason you want to do this at subvolume creation time rather than add fix it in the second RPC ceph fs subvolume charmap ...?

I'll inject myself into this conversation to add my 2c: There's a case for having all the major properties of a subvolume available at creation time for an admin to do create a subvolume for a specific purpose (smb shares) in one single action. CLI users get to avoid having to look up a 2nd command and there's no chance of the volume ever being in an in-between state. In the case of UI the GUI can call a single backend API for all these properties.

PS. If we don't do this now I feel a PM I know will eventually ask for it ;-)

Re-reading my comment I don't think I was being particularly clear. The value of one RPC/API having this is (a) discoverability - when I look up the create subvolume command docs I see an option about setting case insentivity rather than having to look it up seperately and (b) no gaps - not creating the volume sharing it out in the "wrong" state and then needing to go back and change it later

If you think there is a real danger of a subvolume being used in a "half-way" state, then this change is fine with me. I think however we should go ahead and add "normalization" now in addition to "casesensitive". "encoding" can be skipped.

xhernandez · 2025-03-19T12:29:58Z

I've addressed the comments and also added a normalization option.

batrick

Please update the documentation too.

otherwise LGTM

src/pybind/mgr/volumes/fs/operations/versions/subvolume_base.py

src/pybind/mgr/volumes/fs/operations/versions/subvolume_v1.py

avanthakkar

Looks good overall! Could you move it out of draft state? I've also triggered a shaman build for the changes. Once the image is ready, I'll test it locally and attempt to run teuthology as well.

comments are addressed

avanthakkar · 2025-03-20T15:25:39Z

make check failure seems related:

flake8: commands[0] /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox
./tasks/cephfs/test_volumes.py:2569:13: F841 local variable 'case_sensitive' is assigned to but never used
./tasks/cephfs/test_volumes.py:2612:13: F841 local variable 'normalization' is assigned to but never used
flake8: exit 1 (6.61 seconds) /home/jenkins-build/build/workspace/ceph-pull-requests/qa> flake8 --select=F,E9 --exclude=venv,.tox pid=3809804
flake8: FAIL ✖ in 10.36 seconds

src/pybind/mgr/volumes/module.py

xhernandez · 2025-03-21T16:24:30Z

I've updated the way the options are used:

There's an optional --normalization option that takes as an argument a unicode normalization form. There's also an optional --case_insensitive boolean option (instead of the original --case-sensitive <true|false>).

Using --case_insensitive also enables --normalization implicitly (this is inherited from the charset behavior).

With these two options we can define all possible configurations:

No normalization, case sensitive: This is the default, no option should be provided
Normalization, case sensitive: Only --normalization option is required
Normalization, case insensitive: --case_insensitive is enough, unless the default normalization is not what you want. In this case the --normalization option can also be specified.

anoopcs9

In general the case insensitive option is still displayed and meant to be used as --case-sensitive and not --case_sensitive, right?

avanthakkar · 2025-03-25T13:10:15Z

Teuthology failure :

2025-03-25T11:07:58.122 INFO:tasks.cephfs_test_runner:test_subvolume_create_with_normalization (tasks.cephfs.test_volumes.TestSubvolumes) ... ERROR
2025-03-25T11:07:58.122 INFO:tasks.cephfs_test_runner:
2025-03-25T11:07:58.122 INFO:tasks.cephfs_test_runner:======================================================================
2025-03-25T11:07:58.122 INFO:tasks.cephfs_test_runner:ERROR: test_subvolume_create_with_normalization (tasks.cephfs.test_volumes.TestSubvolumes)
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_15c6874445d8abf7144ed4b12e051bd3e1578f55/qa/tasks/cephfs/test_volumes.py", line 2712, in test_subvolume_create_with_normalization
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:    subvolpath = self._get_subvolume_path(self.volname, subvolume, "--normalization", "nfc")
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:TypeError: TestVolumesHelper._get_subvolume_path() takes from 3 to 4 positional arguments but 5 were given
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:Ran 96 tests in 4312.596s
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:FAILED (errors=1)
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:
2025-03-25T11:07:58.123 INFO:tasks.cephfs_test_runner:======================================================================
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:ERROR: test_subvolume_create_with_normalization (tasks.cephfs.test_volumes.TestSubvolumes)
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_15c6874445d8abf7144ed4b12e051bd3e1578f55/qa/tasks/cephfs/test_volumes.py", line 2712, in test_subvolume_create_with_normalization
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:    subvolpath = self._get_subvolume_path(self.volname, subvolume, "--normalization", "nfc")
2025-03-25T11:07:58.124 INFO:tasks.cephfs_test_runner:TypeError: TestVolumesHelper._get_subvolume_path() takes from 3 to 4 positional arguments but 5 were given

https://pulpito.ceph.com/avan-2025-03-25_09:29:02-fs:volumes-wip-athakkar-testing-2025-03-25-1217-distro-default-smithi/

qa/tasks/cephfs/test_volumes.py

When a new subvolume is created, optionally set the ceph.dir.normalization xattr to the root directory. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

Add an option to explicitly set the unicode normalization form to use on a CephFS subvolume. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

Add an option to explicitly set the case sensitivity of a CephFS subvolume. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

avanthakkar · 2025-03-25T17:35:32Z

jenkins test api

avanthakkar · 2025-03-25T17:35:45Z

jenkins test windows

avanthakkar · 2025-03-25T20:10:05Z

Teuthology run passed:
https://pulpito.ceph.com/avan-2025-03-25_17:37:31-fs:volumes-wip-athakkar-testing-2025-03-25-1937-distro-default-smithi/

avanthakkar · 2025-03-25T20:10:19Z

jenkins test make check

avanthakkar · 2025-03-26T06:44:16Z

jenkins test make check

avanthakkar · 2025-03-26T07:36:01Z

jenkins test make check

avanthakkar · 2025-03-26T12:10:55Z

jenkins test make check

avanthakkar · 2025-03-26T19:38:17Z

jenkins test make check

anoopcs9

lgtm.

ceph/ceph#62105 is now merged which introduces the '--case-insensitive' subvolume option to make it case insensitive to suite SMB use cases. Signed-off-by: Anoop C S <anoopcs@cryptolab.net>

ceph/ceph#62105 is now merged which introduces the `--case-insensitive` subvolume option to make it case insensitive to suite SMB use cases. Signed-off-by: Anoop C S <anoopcs@cryptolab.net>

batrick · 2025-04-18T01:11:40Z

followup: #62872

batrick · 2025-04-28T20:05:33Z

backported to squid via #63023

github-actions bot added cephfs Ceph File System pybind labels Mar 4, 2025

spuiuk requested review from avanthakkar and phlogistonjohn March 4, 2025 16:11

avanthakkar requested a review from a team March 4, 2025 16:30

avanthakkar reviewed Mar 5, 2025

View reviewed changes

avanthakkar added the wip-athakkar-testing label Mar 5, 2025

avanthakkar previously requested changes Mar 5, 2025

View reviewed changes

batrick requested changes Mar 12, 2025

View reviewed changes

xhernandez force-pushed the configure-case-sensitivity branch from 4811ac6 to 7fcb5ab Compare March 19, 2025 12:28

batrick requested changes Mar 20, 2025

View reviewed changes

avanthakkar reviewed Mar 20, 2025

View reviewed changes

avanthakkar reviewed Mar 21, 2025

View reviewed changes

src/pybind/mgr/volumes/module.py Outdated Show resolved Hide resolved

xhernandez force-pushed the configure-case-sensitivity branch from 7fcb5ab to fa8bb8d Compare March 21, 2025 16:17

github-actions bot added documentation tests labels Mar 21, 2025

xhernandez force-pushed the configure-case-sensitivity branch 2 times, most recently from 7c21d28 to d974f89 Compare March 21, 2025 22:52

xhernandez marked this pull request as ready for review March 22, 2025 07:16

xhernandez requested a review from a team as a code owner March 22, 2025 07:16

anoopcs9 reviewed Mar 24, 2025

View reviewed changes

avanthakkar reviewed Mar 25, 2025

View reviewed changes

qa/tasks/cephfs/test_volumes.py Outdated Show resolved Hide resolved

xhernandez added 4 commits March 25, 2025 14:45

pybind/mgr: create normalization xattr in subvolumes

4a414a5

When a new subvolume is created, optionally set the ceph.dir.normalization xattr to the root directory. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

pybind/mgr: add option to set unicode normalization to a subvolume

b8b0f1f

Add an option to explicitly set the unicode normalization form to use on a CephFS subvolume. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

qa: add tests for normalization configuration on subvolume creation

66caf74

Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

pybind/mgr: add option to set case sensitivity to a subvolume

87bb089

Add an option to explicitly set the case sensitivity of a CephFS subvolume. Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

xhernandez force-pushed the configure-case-sensitivity branch from 2651412 to 0440009 Compare March 25, 2025 13:46

xhernandez added 2 commits March 25, 2025 14:52

qa: add tests for case sensitivity configuration on subvolume creation

b8a7764

Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

doc: update documentation for normalization and case sensitivity

d9704c0

Signed-off-by: Xavi Hernandez <xhernandez@gmail.com>

xhernandez force-pushed the configure-case-sensitivity branch from 0440009 to d9704c0 Compare March 25, 2025 13:52

avanthakkar requested a review from anoopcs9 March 27, 2025 06:41

anoopcs9 approved these changes Mar 27, 2025

View reviewed changes

avanthakkar merged commit 210a00a into ceph:main Mar 27, 2025
12 checks passed

xhernandez deleted the configure-case-sensitivity branch March 27, 2025 11:46

anoopcs9 mentioned this pull request Mar 28, 2025

cephfs: Create and test against case insensitive subvolumes samba-in-kubernetes/sit-environment#134

Merged

batrick mentioned this pull request Apr 30, 2025

squid: pybind/mgr/volumes: fix typo in casesensitive vxattr #63023

Open

Conversation

xhernandez commented Mar 4, 2025

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avanthakkar commented Mar 5, 2025

Uh oh!

avanthakkar left a comment

Choose a reason for hiding this comment

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

xhernandez commented Mar 13, 2025

Uh oh!

phlogistonjohn commented Mar 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

batrick commented Mar 17, 2025

Uh oh!

batrick commented Mar 17, 2025

Uh oh!

xhernandez commented Mar 19, 2025

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

avanthakkar left a comment

Choose a reason for hiding this comment

Uh oh!

avanthakkar commented Mar 20, 2025

Uh oh!

Uh oh!

xhernandez commented Mar 21, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

avanthakkar commented Mar 25, 2025

Uh oh!

Uh oh!

avanthakkar commented Mar 25, 2025

Uh oh!

avanthakkar commented Mar 25, 2025

Uh oh!

avanthakkar commented Mar 25, 2025

Uh oh!

avanthakkar commented Mar 25, 2025

Uh oh!

avanthakkar commented Mar 26, 2025

Uh oh!

avanthakkar commented Mar 26, 2025

Uh oh!

avanthakkar commented Mar 26, 2025

Uh oh!

avanthakkar commented Mar 26, 2025

Uh oh!

anoopcs9 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

batrick commented Apr 18, 2025

Uh oh!

batrick commented Apr 28, 2025

Uh oh!

Reviewers

Assignees

phlogistonjohn commented Mar 13, 2025 •

edited

Loading