Skip to content

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

Closed
rishabh-d-dave wants to merge 3 commits intoceph:mainfrom
rishabh-d-dave:vols-snap-getpath-for-v1
Closed

mgr/vol: make "snapshot getpath" cmd return ENOTSUP for v1#63123
rishabh-d-dave wants to merge 3 commits intoceph:mainfrom
rishabh-d-dave:vols-snap-getpath-for-v1

Conversation

@rishabh-d-dave
Copy link
Contributor

Return ENOTSUP when "ceph fs subvolume snapshot getpath" command is run
for subvolume v1.

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)
Show available Jenkins commands

@rishabh-d-dave rishabh-d-dave requested review from a team and vshankar May 5, 2025 18:37
@rishabh-d-dave rishabh-d-dave force-pushed the vols-snap-getpath-for-v1 branch from 3ba1f7e to 7c80e22 Compare May 5, 2025 18:39
@rishabh-d-dave
Copy link
Contributor Author

With vstart_runner.py, tests ran fine. I am getting them to run in Sepia lab as well now.

Return ENOTSUP when "ceph fs subvolume snapshot getpath" command is run
for subvolume v1.

Fixes: https://tracker.ceph.com/issues/70834
Signed-off-by: Rishabh Dave <ridave@redhat.com>
Signed-off-by: Rishabh Dave <ridave@redhat.com>
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave rishabh-d-dave force-pushed the vols-snap-getpath-for-v1 branch from 7c80e22 to 7cdbbef Compare May 5, 2025 19:07
if subvol.version() == 1:
ret = (-errno.ENOTSUP, '',
'command not supported for v1 subvolumes')
return ret
Copy link
Member

Choose a reason for hiding this comment

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

Normally I would expect v1 to implement snapshot_data_path as a method which always returns -ENOTSUP rather than special case here.

Won't stop approval though. Please clean up in a future PR.

Copy link
Member

Choose a reason for hiding this comment

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

Also, let's add another TODO to have some command from the volumes plugin tell us how many v1/v2/v3 subvols there (any upgrades done, if applicable). That could be collected by telemetry.

Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave May 5, 2025

Choose a reason for hiding this comment

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

Normally I would expect v1 to implement snapshot_data_path as a method which always returns -ENOTSUP rather than special case here.

snapshot_data_path() is already implemented which is why I added this block of code here - https://github.com/ceph/ceph/blob/main/src/pybind/mgr/volumes/fs/operations/versions/subvolume_v1.py#L84-L86

Copy link
Member

Choose a reason for hiding this comment

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

Wait I'm confused now. You're saying v1 could support the command but we're choosing not to?

https://github.com/ceph/ceph/blob/main/src/pybind/mgr/volumes/fs/operations/versions/subvolume_v1.py#L84-L86

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the concern was something related to retained snaps in v1.

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check arm64

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

jenkins test make check arm64

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented May 6, 2025

Decision was made to support snapshot getpath command for v1 (and legacy too). Therefore, closing this in the favour of #63127.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants