mds: update older versioned sr_t struct(s) to incorporate snapshot visibility flag by dparmar18 · Pull Request #65960 · ceph/ceph

dparmar18 · 2025-10-15T11:50:45Z

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

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

You must only issue one Jenkins command per-comment. Jenkins does not understand
comments with more than one command.

dparmar18 · 2025-10-15T11:52:41Z

@vshankar should i also add an upgrade test case?

vshankar · 2025-10-21T11:30:41Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

vshankar

LGTM.

dparmar18 · 2025-10-26T14:15:13Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

hard to include in an existing test case because we need to: install squid -> create a subvol -> check the visibility flag -> upgrade to tentacle -> verify the visibility flag. I think this needs a separate sub-suite of its own.

vshankar · 2025-10-28T12:52:09Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

hard to include in an existing test case because we need to: install squid -> create a subvol -> check the visibility flag -> upgrade to tentacle -> verify the visibility flag. I think this needs a separate sub-suite of its own.

OK. I think you can put it under fs:bugs with a similar flow for upgrade.

dparmar18 · 2025-10-28T13:17:34Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

hard to include in an existing test case because we need to: install squid -> create a subvol -> check the visibility flag -> upgrade to tentacle -> verify the visibility flag. I think this needs a separate sub-suite of its own.

OK. I think you can put it under fs:bugs with a similar flow for upgrade.

sure, i'll a new sub-suite under fs:bugs.

vshankar · 2025-11-10T04:21:54Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

hard to include in an existing test case because we need to: install squid -> create a subvol -> check the visibility flag -> upgrade to tentacle -> verify the visibility flag. I think this needs a separate sub-suite of its own.

OK. I think you can put it under fs:bugs with a similar flow for upgrade.

sure, i'll a new sub-suite under fs:bugs.

Gentle nudge on this @dparmar18

dparmar18 · 2025-11-14T10:53:54Z

@vshankar should i also add an upgrade test case?

I think it's nice to have a test that verifies this change. We can include it in one of the existing fs:upgrade tests if possible.

hard to include in an existing test case because we need to: install squid -> create a subvol -> check the visibility flag -> upgrade to tentacle -> verify the visibility flag. I think this needs a separate sub-suite of its own.

OK. I think you can put it under fs:bugs with a similar flow for upgrade.

sure, i'll a new sub-suite under fs:bugs.

Gentle nudge on this @dparmar18

on it now

dparmar18 · 2025-11-15T18:24:54Z

@vshankar this passes in teuthology - https://pulpito.ceph.com/dparmar-2025-11-15_17:54:48-fs:bugs-i73550-distro-default-smithi/

qa/suites/fs/bugs/snapshot_visibility_on_upgrade/cluster/1-node.yaml

.../bugs/snapshot_visibility_on_upgrade/tasks/1-test-subvolume-snapshot-default-visibility.yaml

vshankar · 2025-11-18T08:34:37Z

@dparmar18 also check the jenkins failure regarding .qa links as qa code has been added in this change. The test checks is all there is a valid .qa symlink.

dparmar18 · 2025-11-18T11:26:02Z

@dparmar18 also check the jenkins failure regarding .qa links as qa code has been added in this change. The test checks is all there is a valid .qa symlink.

fixed

vshankar · 2025-11-20T06:53:16Z

jenkins test docs

vshankar · 2025-11-20T07:37:55Z

jenkins retest this please

vshankar · 2025-11-20T08:37:20Z

This PR is under test in https://tracker.ceph.com/issues/73938.

vshankar · 2025-12-01T13:07:43Z

Need to retest with https://tracker.ceph.com/issues/74026

qa/suites/fs/bugs/snapshot_visibility_on_upgrade/tasks/0-install.yaml

vshankar

@dparmar18 PTAL (unrelated failure due to another recent merge)

dparmar18 · 2025-12-01T18:24:46Z

@dparmar18 PTAL (unrelated failure due to another recent merge)

https://github.com/ceph/ceph/compare/8e06b78e46fb3e1675b264a5ed8d768d260963ae..27d76ea8f6289b1c9f96480c5a5a8b139e13f180

…sibility flag - If there is cluster upgrade, the older sr_t structs' flags would not have the SNAPSHOT_VISIBILITY flag bit set which means the existing subvolumes would report 0 instead of 1 for the vxattr ceph.dir.subvolume.snaps.visible which although doesn't lead to any actual denial of access to subvolume snapshots since the client config to respect snapshot visibility is set to false as default but might come as a surprise to any cluster operator if the client config is toggled to true to see the snapshot visibility being denied(since the `flags` is reporting to be 0). So, change the in-memory flags while decoding the sr_t members. - bump up struct_v to 8 while encoding and while decoding, for all the structs older than 8 be assigned SNAPSHOT_VISIBILITY flag in-memory accordingly. Fixes: https://tracker.ceph.com/issues/73550 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

the default flag value should return 1 Fixes: https://tracker.ceph.com/issues/73550 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2025-12-01T18:25:39Z

last push was a rebase

* refs/pull/65960/head: Reviewed-by: Venky Shankar <vshankar@redhat.com>

vshankar

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20251120083629-debug

vshankar · 2025-12-10T04:57:33Z

https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-vshankar-testing-20251120083629-debug

Note that fs:upgrade fails currently due to reason mentioned in the above run wiki. However, I have tested this PR locally. Tests will be rerun when fs:upgrade issues is fixed with #66475

github-actions bot added the cephfs Ceph File System label Oct 15, 2025

dparmar18 requested a review from a team October 15, 2025 11:51

vshankar reviewed Oct 21, 2025

View reviewed changes

github-actions bot added the tests label Nov 14, 2025

dparmar18 force-pushed the i73550 branch 8 times, most recently from c3f5b6d to e25c9f3 Compare November 15, 2025 17:47

dparmar18 requested review from a team and vshankar November 15, 2025 18:24

vshankar requested changes Nov 17, 2025

View reviewed changes

qa/suites/fs/bugs/snapshot_visibility_on_upgrade/cluster/1-node.yaml Show resolved Hide resolved

.../bugs/snapshot_visibility_on_upgrade/tasks/1-test-subvolume-snapshot-default-visibility.yaml Show resolved Hide resolved

dparmar18 force-pushed the i73550 branch from e25c9f3 to 8e06b78 Compare November 18, 2025 11:22

dparmar18 requested a review from vshankar November 18, 2025 11:24

vshankar approved these changes Nov 19, 2025

View reviewed changes

vshankar added the wip-vshankar-testing label Nov 19, 2025

vshankar reviewed Dec 1, 2025

View reviewed changes

qa/suites/fs/bugs/snapshot_visibility_on_upgrade/tasks/0-install.yaml Show resolved Hide resolved

vshankar requested changes Dec 1, 2025

View reviewed changes

dparmar18 force-pushed the i73550 branch from 8e06b78 to 27d76ea Compare December 1, 2025 18:24

dparmar18 added 2 commits December 1, 2025 23:55

qa: test subvolume snapshot visibility flag post upgrade

d6c53c5

the default flag value should return 1 Fixes: https://tracker.ceph.com/issues/73550 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the i73550 branch from 27d76ea to d6c53c5 Compare December 1, 2025 18:25

dparmar18 requested a review from vshankar December 1, 2025 18:25

vshankar added a commit to vshankar/ceph that referenced this pull request Dec 2, 2025

Merge PR ceph#65960 into wip-vshankar-testing-20251202.044501

7d2d332

* refs/pull/65960/head: Reviewed-by: Venky Shankar <vshankar@redhat.com>

vshankar self-assigned this Dec 2, 2025

vshankar approved these changes Dec 5, 2025

View reviewed changes

vshankar merged commit ba6e1ce into ceph:main Dec 5, 2025
13 checks passed

vshankar removed the wip-vshankar-testing label Dec 5, 2025

dparmar18 mentioned this pull request Jan 21, 2026

tentacle: mds: update older versioned sr_t struct(s) to incorporate snapshot visibility flag #67017

Closed

14 tasks

Conversation

dparmar18 commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

dparmar18 commented Oct 15, 2025

Uh oh!

vshankar commented Oct 21, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Oct 26, 2025

Uh oh!

vshankar commented Oct 28, 2025

Uh oh!

dparmar18 commented Oct 28, 2025

Uh oh!

vshankar commented Nov 10, 2025

Uh oh!

dparmar18 commented Nov 14, 2025

Uh oh!

dparmar18 commented Nov 15, 2025

Uh oh!

Uh oh!

Uh oh!

vshankar commented Nov 18, 2025

Uh oh!

dparmar18 commented Nov 18, 2025

Uh oh!

vshankar commented Nov 20, 2025

Uh oh!

vshankar commented Nov 20, 2025

Uh oh!

vshankar commented Nov 20, 2025

Uh oh!

vshankar commented Dec 1, 2025

Uh oh!

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

dparmar18 commented Dec 1, 2025

Uh oh!

dparmar18 commented Dec 1, 2025

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vshankar commented Dec 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dparmar18 commented Oct 15, 2025 •

edited

Loading