cephfs,mon: fs rename must require FS to be offline and refuse_client_session to be set by rishabh-d-dave · Pull Request #53899 · ceph/ceph

rishabh-d-dave · 2023-10-09T20:01:49Z

Before renaming a CephFS, make sure that the CephFS is offline and refuse_client_session is set for it.

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

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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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 retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

qa/tasks/cephfs/test_admin.py

vshankar · 2023-10-10T08:40:23Z

@rishabh-d-dave ping back when ready for another round of review.

dparmar18 · 2023-10-10T09:58:35Z

why not enable fs rename code to do it on its own? i.e. fail fs before renaming and then post rename set joinable to true? this can save operator's time and reduce complexity for a naive user i guess

vshankar · 2023-10-10T10:21:04Z

why not enable fs rename code to do it on its own? i.e. fail fs before renaming and then post rename set joinable to true?

Because its destructive and doing it by default is not a good idea.

this can save operator's time and reduce complexity for a naive user i guess

Its the naive users we need to give extra care at times :)

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

qa/tasks/cephfs/test_admin.py

rishabh-d-dave · 2023-10-12T06:41:11Z

jenkins test make check

rishabh-d-dave · 2023-10-12T06:43:22Z

https://jenkins.ceph.com/job/ceph-pull-requests/123343/

rishabh-d-dave · 2023-10-12T06:43:44Z

https://jenkins.ceph.com/job/ceph-api/63035/

rishabh-d-dave · 2023-10-12T06:43:50Z

jenkins test api

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

rishabh-d-dave · 2023-11-01T06:04:56Z

test_volumes: https://pulpito.ceph.com/rishabh-2023-11-01_05:37:22-fs:volumes-rishabh-fs-rename-testing-2-testing-default-smithi
test_admin: https://pulpito.ceph.com/rishabh-2023-11-01_05:41:27-fs-rishabh-fs-rename-testing-2-testing-default-smithi

EDIT -
Both tests passed, the bug has been fixed.

vshankar · 2023-11-02T04:14:46Z

http://pulpito.front.sepia.ceph.com/rishabh-2023-10-28_17:50:16-fs:volumes-rishabh-fs-rename-testing-testing-default-smithi/

EDIT

This job failed. It also failed during integration test runs. This is failing due to a case that needs to be covered in mgr/vol plugin due to this changes in this PR.

Could you describe the failure and the commit diff that subsequently fixed it?

rishabh-d-dave · 2023-11-02T10:20:53Z

@vshankar

Could you describe the failure and the commit diff that subsequently fixed it?

subvolume rename would fail because the code for it wouldn't check if the FS is offline. Here's the fix for it - 45b0336

vshankar · 2023-11-02T11:50:38Z

@rishabh-d-dave As discussed in slack, it would suffice with the existing checks in the monitor to disallow rename if the file system is still online.

EDIT: s/IRC/slack

rishabh-d-dave · 2023-11-02T15:00:04Z

I've moved the commit (ca0633b) from PR #54306 to here and closed that PR.

Reject the attempt to rename the CephFS is the CephFS is not offline. Add new tests for this and update current tests (test_admin.py and test_volumes.py) accordingly. Fixes: https://tracker.ceph.com/issues/63154 Signed-off-by: Rishabh Dave <ridave@redhat.com>

Move tests for "ceph fs volume rename" command to a new class. This makes it possible to run this group of tests in a single command. This provides a convenient way to execute these tests which is necessary after the changes has been made to the code for the "ceph fs volume rename" command. Signed-off-by: Rishabh Dave <ridave@redhat.com>

client_refuse_session must be set for a CephFS before an attempt to rename a CephFS can be made. Add a new test for this, and update current tests (test_admin.py and test_volumes.py) accordingly. Fixes: https://tracker.ceph.com/issues/63154 Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2023-11-03T16:36:32Z

test_admin: https://pulpito.ceph.com/rishabh-2023-11-03_16:31:20-fs:functional-rishabh-fs-rename-abc-testing-default-smithi

test_volumes: https://pulpito.ceph.com/rishabh-2023-11-03_16:34:54-fs:volumes-rishabh-fs-rename-abc-testing-default-smithi

vshankar · 2023-11-06T07:20:49Z

@rishabh-d-dave also requires a note in PendingReleaseNotes.

vshankar · 2023-11-06T07:26:50Z

qa/tasks/cephfs/test_volumes.py

        # data pool names unchanged
        self.assertCountEqual(orig_data_pool_names, list(self.fs.data_pools.values()))

+    def test_rename_when_fs_is_online(self):


Is this test really required since test_admin.test_rename_when_fs_is_online exists?

In mgr/volumes, the code for renaming volumes is more than 100 LoC. IMHO, it is a nice idea to keep this test here so that we catch issues sooner.

We discussed yesterday that the nature of this test isn't clear since we already have the corresponding test in test_admin. What specific 100 LoC is this test validating? Sorry, but this isn't an excuse to writing duplicate tests.

Hi @vshankar. This was a mistake. I was supposed to merge "fs swap" PR and leave this one unmerged but I made a mistake while merging the batch of PRs and ended up with the reverse. You may check the fs swap PR, it's still unmerged - #50212. I'll open a PR to delete the test.

Sorry to get you angry. :)

Hi @vshankar. This was a mistake. I was supposed to merge "fs swap" PR and leave this one unmerged but I made a mistake while merging the batch of PRs and ended up with the reverse. You may check the fs swap PR, it's still unmerged - #50212. I'll open a PR to delete the test.

Sorry to get you angry. :)

I was not angry - just that I didn't find an explanation regarding the pending comments :)

rishabh-d-dave

CephFS integration tests were successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#7-Nov-2023

vshankar · 2023-11-08T04:15:13Z

@rishabh-d-dave What about this comment - #53899 (comment) ?

With merging of PR ceph#53899 it is mandatory now for CephFS to be offline and refuse_client_session to be set before an attempt to rename the CephFS is made. Fixes: ceph#53899 Fixes: https://tracker.ceph.com/issues/63154 Signed-off-by: Rishabh Dave <ridave@redhat.com>

With merging of PR ceph#53899 it is mandatory now for CephFS to be offline and refuse_client_session to be set before an attempt to rename the CephFS is made. Add a release note for this. Fixes: ceph#53899 Fixes: https://tracker.ceph.com/issues/63154 Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave requested a review from a team as a code owner October 9, 2023 20:01

rishabh-d-dave added cephfs Ceph File System needs-review labels Oct 9, 2023

rishabh-d-dave requested a review from a team October 9, 2023 20:01

github-actions bot added core mon tests labels Oct 9, 2023

rishabh-d-dave requested a review from batrick October 9, 2023 20:02

batrick approved these changes Oct 9, 2023

View reviewed changes

vshankar requested changes Oct 10, 2023

View reviewed changes

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

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

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch from 45b19a9 to dc494c1 Compare October 10, 2023 06:20

qa/cephfs: import only sleep() from time

4fe08f1

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

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch from dc494c1 to 117b0a6 Compare October 10, 2023 19:55

rishabh-d-dave requested a review from vshankar October 10, 2023 19:57

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch 3 times, most recently from c5e9d0f to aa3232f Compare October 10, 2023 20:04

vshankar reviewed Oct 11, 2023

View reviewed changes

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

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch 2 times, most recently from 7f51106 to 5d5419a Compare October 11, 2023 10:09

vshankar approved these changes Oct 11, 2023

View reviewed changes

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Oct 12, 2023

batrick approved these changes Oct 12, 2023

View reviewed changes

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch 2 times, most recently from eac7a7b to 2e63452 Compare October 31, 2023 20:18

dparmar18 reviewed Oct 31, 2023

View reviewed changes

src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved

rishabh-d-dave changed the title ~~cephfs,mon: check if FS is down before renaming an FS~~ cephfs,mon: allow FS rename only if FS is down Nov 1, 2023

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch 2 times, most recently from c3119b9 to 78d9e6d Compare November 1, 2023 13:15

rishabh-d-dave mentioned this pull request Nov 2, 2023

cephfs: allow FS rename only if refuse_client_session is set #54306

Closed

14 tasks

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch from 78d9e6d to ed9cb3a Compare November 2, 2023 10:23

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch 2 times, most recently from cfe0fc7 to ca0633b Compare November 2, 2023 13:42

rishabh-d-dave changed the title ~~cephfs,mon: allow FS rename only if FS is down~~ cephfs,mon: fs rename must require FS to be offline and refuse_client_session to be set Nov 2, 2023

rishabh-d-dave added 3 commits November 2, 2023 22:11

rishabh-d-dave force-pushed the before-fs-rename-fail-fs branch from ca0633b to 3f93d74 Compare November 2, 2023 16:44

vshankar reviewed Nov 6, 2023

View reviewed changes

rishabh-d-dave commented Nov 7, 2023

View reviewed changes

rishabh-d-dave merged commit 380ac6c into ceph:main Nov 7, 2023

rishabh-d-dave deleted the before-fs-rename-fail-fs branch November 7, 2023 17:53

rishabh-d-dave mentioned this pull request Jan 16, 2025

reef: cephfs,mon: fs rename must require FS to be offline and refuse_client_session to be set #61410

Merged

Conversation

rishabh-d-dave commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

vshankar commented Oct 10, 2023

Uh oh!

dparmar18 commented Oct 10, 2023

Uh oh!

vshankar commented Oct 10, 2023

Uh oh!

Uh oh!

rishabh-d-dave commented Oct 12, 2023

Uh oh!

rishabh-d-dave commented Oct 12, 2023

Uh oh!

rishabh-d-dave commented Oct 12, 2023

Uh oh!

rishabh-d-dave commented Oct 12, 2023

Uh oh!

Uh oh!

rishabh-d-dave commented Nov 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Nov 2, 2023

Uh oh!

rishabh-d-dave commented Nov 2, 2023

Uh oh!

vshankar commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented Nov 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rishabh-d-dave commented Nov 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Nov 6, 2023

Uh oh!

vshankar Nov 6, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Nov 7, 2023

Choose a reason for hiding this comment

Uh oh!

vshankar Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

vshankar Nov 8, 2023

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar commented Nov 8, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rishabh-d-dave commented Oct 9, 2023 •

edited

Loading

rishabh-d-dave commented Nov 1, 2023 •

edited

Loading

vshankar commented Nov 2, 2023 •

edited

Loading

rishabh-d-dave commented Nov 2, 2023 •

edited

Loading

rishabh-d-dave commented Nov 3, 2023 •

edited

Loading