mgr/vol: make "snapshot getpath" cmd work with v1 and legacy by rishabh-d-dave · Pull Request #63127 · ceph/ceph

rishabh-d-dave · 2025-05-06T10:37:32Z

Fixes: https://tracker.ceph.com/issues/70834

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

Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2025-05-06T13:44:45Z

jenkins test make check arm64

vshankar · 2025-05-06T13:45:07Z

test_snap_schedules.py handcraft the snapshot path in _get_subvol_snapdir_path(). Suggest, changing that to use this new interface.

rishabh-d-dave · 2025-05-06T14:26:34Z

test_snap_schedules.py handcraft the snapshot path in _get_subvol_snapdir_path(). Suggest, changing that to use this new interface.

@vshankar There must be more such instances where such improvements can be made, let's do that on a separate PR?

vshankar · 2025-05-06T14:31:14Z

test_snap_schedules.py handcraft the snapshot path in _get_subvol_snapdir_path(). Suggest, changing that to use this new interface.

@vshankar There must be more such instances where such improvements can be made, let's do that on a separate PR?

How many are such instances? It should be a straight-forward usage of the new API, isn't it?

rishabh-d-dave · 2025-05-06T14:40:34Z

test_snap_schedules.py handcraft the snapshot path in _get_subvol_snapdir_path(). Suggest, changing that to use this new interface.

@vshankar There must be more such instances where such improvements can be made, let's do that on a separate PR?

How many are such instances? It should be a straight-forward usage of the new API, isn't it?

@vshankar
I think I've seen a few in test_volumes.py as well. Also, it's beyond scope of this PR, we only want to add tests for this command against v1 and legacy.

vshankar · 2025-05-06T14:43:59Z

@vshankar
I think I've seen a few in test_volumes.py as well. Also, it's beyond scope of this PR, we only want to add tests for this command against v1 and legacy.

It's not entirely beyond the scope -- you get tests run for other uses. If its just test_volumes and test_snap_schedule, I suggest changing those to use the new interface.

rishabh-d-dave · 2025-05-06T14:50:10Z

@vshankar
I think I've seen a few in test_volumes.py as well. Also, it's beyond scope of this PR, we only want to add tests for this command against v1 and legacy.

It's not entirely beyond the scope -- you get tests run for other uses. If its just test_volumes and test_snap_schedule, I suggest changing those to use the new interface.

@vshankar The more we make such changes in the tests, the higher will be chance that QA might fail due to some error in making those changes. And that will make it longer to merge this PR. I would really prefer to avoid it in interest of time.

vshankar · 2025-05-06T15:31:23Z

@vshankar
I think I've seen a few in test_volumes.py as well. Also, it's beyond scope of this PR, we only want to add tests for this command against v1 and legacy.

It's not entirely beyond the scope -- you get tests run for other uses. If its just test_volumes and test_snap_schedule, I suggest changing those to use the new interface.

@vshankar The more we make such changes in the tests, the higher will be chance that QA might fail due to some error in making those changes. And that will make it longer to merge this PR. I would really prefer to avoid it in interest of time.

I understand that and I also understand the push that's required to get stuff merged. But when things break (and that has happened before), a lot many things have to be redone. So, let's spend a bit more time in ensuring nothing obvious breaks.

rishabh-d-dave · 2025-05-06T16:59:26Z

But when things break (and that has happened before), a lot many things have to be redone.

@vshankar
AFAIU, test_snap_schedules.py and test_volumes.py will get refactored with this. Nothing will break if don't make that change. Anyways, will proceed to make the change.

rishabh-d-dave · 2025-05-06T18:04:07Z

@vshankar I've made the changes in test_volumes.py. I also went through test_snap_schedules.py, specifically the method you mentioned (_get_subvol_snapdir_path()). This method constructs path for parent directory of snapshot prefix directory and not the path of a specific snapshot directory, therefore this command won't help.

vshankar · 2025-05-06T19:06:27Z

@vshankar I've made the changes in test_volumes.py. I also went through test_snap_schedules.py, specifically the method you mentioned (_get_subvol_snapdir_path()). This method constructs path for parent directory of snapshot prefix directory and not the path of a specific snapshot directory, therefore this command won't help.

Really? If you see:

    def _get_subvol_snapdir_path(self, version, subvol, group):
	args = ['subvolume', 'getpath', 'cephfs', subvol]
        if group:
            args += ['--group_name', group]

        path = self.get_ceph_cmd_stdout("fs", *args).rstrip()
        if version >= 2:
            path += "/.."
        return path[1:]

It first fetches the subvolume path and then for v2 version where the snapshots are taken a level up, /.. is suffixed and for earlier versions, the subvolume path is used as snapshot path. Can you fault my logic here?

rishabh-d-dave · 2025-05-06T19:48:09Z

@vshankar I've made the changes in test_volumes.py. I also went through test_snap_schedules.py, specifically the method you mentioned (_get_subvol_snapdir_path()). This method constructs path for parent directory of snapshot prefix directory and not the path of a specific snapshot directory, therefore this command won't help.

Really? If you see:
    def _get_subvol_snapdir_path(self, version, subvol, group):
	args = ['subvolume', 'getpath', 'cephfs', subvol]
        if group:
            args += ['--group_name', group]

        path = self.get_ceph_cmd_stdout("fs", *args).rstrip()
        if version >= 2:
            path += "/.."
        return path[1:]
It first fetches the subvolume path and then for v2 version where the snapshots are taken a level up, /.. is suffixed

Yes, so parent path of snapshot prefix directory is being derived (/volumes/_nogroup/subvol1) and what snapshot getpath returns is path to a specific snapshot (/volumes/_nogroup/subvol1/<uuid>/.snap/<snap-name>). Both are different. So latter can't replace former.

Also, snapshot getpath requires snap name to be passed. We neither have snap names passed to this method nor in code region that call this method.

and for earlier versions, the subvolume path is used as snapshot path. Can you fault my logic here?

Even for earlier version path derived is not snapshot path. Snapshot prefix directory is discovered and attached to it later.

Overall, goal in both the cases is to find the path to the snapshot prefix directory and not path to a specific snapshot.

/volumes/_nogroup/subvol1 vs /volumes/_nogroup/subvol1/<uuid>/.snap/<snap-name> or /volumes/_nogroup/subvol1/.snap/<snap-name>.

rishabh-d-dave · 2025-05-06T19:50:01Z

jenkins test make check

rishabh-d-dave · 2025-05-06T19:50:08Z

jenkins test make check arm64

rishabh-d-dave · 2025-05-06T20:04:52Z

jenkins test api

vshankar · 2025-05-06T20:31:30Z

@vshankar I've made the changes in test_volumes.py. I also went through test_snap_schedules.py, specifically the method you mentioned (_get_subvol_snapdir_path()). This method constructs path for parent directory of snapshot prefix directory and not the path of a specific snapshot directory, therefore this command won't help.

Really? If you see:
    def _get_subvol_snapdir_path(self, version, subvol, group):
	args = ['subvolume', 'getpath', 'cephfs', subvol]
        if group:
            args += ['--group_name', group]

        path = self.get_ceph_cmd_stdout("fs", *args).rstrip()
        if version >= 2:
            path += "/.."
        return path[1:]
It first fetches the subvolume path and then for v2 version where the snapshots are taken a level up, /.. is suffixed
Yes, so parent path of snapshot prefix directory is being derived (/volumes/_nogroup/subvol1) and what snapshot getpath returns is path to a specific snapshot (/volumes/_nogroup/subvol1/<uuid>/.snap/<snap-name>). Both are different. So latter can't replace former.

Also, snapshot getpath requires snap name to be passed. We neither have snap names passed to this method nor in code region that call this method.

and for earlier versions, the subvolume path is used as snapshot path. Can you fault my logic here?

Even for earlier version path derived is not snapshot path. Snapshot prefix directory is discovered and attached to it later.

Overall, goal in both the cases is to find the path to the snapshot prefix directory and not path to a specific snapshot.

/volumes/_nogroup/subvol1 vs /volumes/_nogroup/subvol1/<uuid>/.snap/<snap-name> or /volumes/_nogroup/subvol1/.snap/<snap-name>.

Ah! I see -- we need the snapshot name. I was thinking in terms of path to snapdir rather than paths to the exact snapshot. Makes sense.

rishabh-d-dave · 2025-05-07T06:38:45Z

@vshankar Does rest of the PR look okay?

qa/tasks/cephfs/test_volumes.py

vshankar

@rishabh-d-dave I'm seeing dup code from existing tests. Better to clean those up.

vshankar · 2025-05-07T14:00:13Z

qa/tasks/cephfs/test_volumes.py

+        from .kernel_mount import KernelMount
+
+        if isinstance(self.mount_a, KernelMount):
+            return self.mount_a.client_config.get('snapdirname', '.snap')


why not use self.mount_a.snapdirname?

It is available in KernelMount instances but not in FuseMount instances.

qa/tasks/cephfs/test_volumes.py

batrick

I guess since this only changes tests then the current code with v1 works?

qa/tasks/cephfs/test_volumes.py

vshankar · 2025-05-07T18:36:55Z

I guess since this only changes tests then the current code with v1 works?

... and legacy too (which gets upgraded to v1 anyway). But yeh, I have that question too :)

legacy subvolumes. Signed-off-by: Rishabh Dave <ridave@redhat.com>

the path to a specific snapshot wherever possible. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2025-05-08T11:13:58Z

I guess since this only changes tests then the current code with v1 works?

Yes, wasn't that why we decided to switch from returning ENOTSUP to supporting it?

rishabh-d-dave · 2025-05-08T11:27:54Z

@vshankar @batrick PTAL

rishabh-d-dave · 2025-05-08T12:05:25Z

jenkins test make check arm64

vshankar · 2025-05-08T12:31:51Z

I guess since this only changes tests then the current code with v1 works?

Yes, wasn't that why we decided to switch from returning ENOTSUP to supporting it?

That was not the question @batrick asked. @batrick wanted to confirm if the only thing required is to add tests since the snapshot getpath operation is also supported by v1/legacy.

rishabh-d-dave · 2025-05-08T12:38:50Z

I guess since this only changes tests then the current code with v1 works?

Yes, wasn't that why we decided to switch from returning ENOTSUP to supporting it?

That was not the question @batrick asked. @batrick wanted to confirm if the only thing required is to add tests since the snapshot getpath operation is also supported by v1/legacy.

Yes, only thing required is to add tests. That is, the current code for v1 and legacy already supports snapshot getpath command. This is exactly what led us to change our decision and close the previous PR.

rishabh-d-dave · 2025-05-08T13:35:36Z

jenkins test make check

rishabh-d-dave · 2025-05-08T14:53:54Z

jenkins test make check

batrick · 2025-05-08T14:56:34Z

I guess since this only changes tests then the current code with v1 works?

Yes, wasn't that why we decided to switch from returning ENOTSUP to supporting it?

That was not the question @batrick asked. @batrick wanted to confirm if the only thing required is to add tests since the snapshot getpath operation is also supported by v1/legacy.

Yes, only thing required is to add tests. That is, the current code for v1 and legacy already supports snapshot getpath command. This is exactly what led us to change our decision and close the previous PR.

If this is only adding tests then, do we need the extreme urgency of getting this merged?

batrick

fine w/ me

rishabh-d-dave · 2025-05-09T08:13:28Z

jenkins test make check

rishabh-d-dave · 2025-05-09T08:13:32Z

jenkins test make check arm64

rishabh-d-dave

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#rishabh-vols-snappath3

Requested changes were incorporated

rishabh-d-dave added cephfs Ceph File System tests pybind needs-review labels May 6, 2025

rishabh-d-dave requested review from a team and vshankar May 6, 2025 10:37

rishabh-d-dave force-pushed the vols-snappath-v1-and-legacy branch 2 times, most recently from 390a884 to 2b785d2 Compare May 6, 2025 12:02

rishabh-d-dave mentioned this pull request May 6, 2025

mgr/vol: make "snapshot getpath" cmd return ENOTSUP for v1 #63123

Closed

14 tasks

rishabh-d-dave force-pushed the vols-snappath-v1-and-legacy branch from 2b785d2 to 222be9d Compare May 6, 2025 12:59

rishabh-d-dave added 2 commits May 6, 2025 19:07

qa/cephfs: move tests for "snapshot getpath" cmd to a separate class

c176143

Signed-off-by: Rishabh Dave <ridave@redhat.com>

qa/cephfs: add a helper method to construct the snapshot path

9e39e5c

Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave force-pushed the vols-snappath-v1-and-legacy branch from 222be9d to b2213ec Compare May 6, 2025 14:20

vshankar reviewed May 7, 2025

View reviewed changes

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

vshankar previously requested changes May 7, 2025

View reviewed changes

batrick requested changes May 7, 2025

View reviewed changes

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

rishabh-d-dave added 2 commits May 8, 2025 16:29

qa/cephfs: add tests for "snapshot getpath" cmd against v1 and...

8cecbc2

legacy subvolumes. Signed-off-by: Rishabh Dave <ridave@redhat.com>

qa/cephfs: use "snapshot getpath" cmd instead of constructing...

3e70066

the path to a specific snapshot wherever possible. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave force-pushed the vols-snappath-v1-and-legacy branch from e7744d5 to 3e70066 Compare May 8, 2025 11:13

rishabh-d-dave requested review from batrick and vshankar May 8, 2025 11:27

batrick approved these changes May 8, 2025

View reviewed changes

rishabh-d-dave closed this May 9, 2025

rishabh-d-dave reopened this May 9, 2025

rishabh-d-dave commented May 12, 2025

View reviewed changes

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label May 12, 2025

rishabh-d-dave merged commit beed49e into ceph:main May 12, 2025
18 checks passed

This was referenced May 12, 2025

squid: mgr/vol: make "snapshot getpath" cmd work with v1 and legacy #63222

Merged

tentacle: mgr/vol: make "snapshot getpath" cmd work with v1 and legacy #63223

Merged

reef: mgr/vol: make "snapshot getpath" cmd work with v1 and legacy #63224

Open

rishabh-d-dave deleted the vols-snappath-v1-and-legacy branch May 12, 2025 06:07

Conversation

rishabh-d-dave commented May 6, 2025

Contribution Guidelines

Checklist

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 6, 2025

Uh oh!

vshankar commented May 6, 2025

Uh oh!

rishabh-d-dave commented May 7, 2025

Uh oh!

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar May 7, 2025

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave May 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vshankar commented May 7, 2025

Uh oh!

rishabh-d-dave commented May 8, 2025

Uh oh!

rishabh-d-dave commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented May 8, 2025

Uh oh!

vshankar commented May 8, 2025

Uh oh!

rishabh-d-dave commented May 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented May 8, 2025

Uh oh!

rishabh-d-dave commented May 8, 2025

Uh oh!

batrick commented May 8, 2025

Uh oh!

batrick left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented May 9, 2025

Uh oh!

rishabh-d-dave commented May 9, 2025

rishabh-d-dave commented May 8, 2025 •

edited

Loading

rishabh-d-dave commented May 8, 2025 •

edited

Loading