mon,cephfs: require confirmation when changing max_mds on unhealthy cluster by rishabh-d-dave · Pull Request #59420 · ceph/ceph

rishabh-d-dave · 2024-08-23T13:27:56Z

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

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 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
jenkins test rook e2e

qa/tasks/cephfs/test_admin.py

joscollin · 2024-08-26T05:13:52Z

@rishabh-d-dave Otherwise, LGTM

qa/tasks/cephfs/test_admin.py

src/mon/FSCommands.cc

qa/tasks/cephfs/filesystem.py

qa/tasks/cephfs/test_admin.py

src/mon/FSCommands.cc

rishabh-d-dave · 2024-08-27T08:26:30Z

@joscollin @batrick Thanks for the quick review. I've made the recommended changes now. PTAL.

src/mon/MDSMonitor.cc

src/mon/FSCommands.cc

qa/tasks/cephfs/test_admin.py

vshankar · 2024-08-29T09:52:15Z

doc/cephfs/troubleshooting.rst


  That prevents any clients from establishing new sessions with the MDS.

+* **Dont tweak max_mds** Changing value of ``max_mds`` FS setting variable is


I would add this as a precautionary note rather than an assertive statement.

Please suggest better a wording. How is **Be cautious tweaking max_mds**?

See wording above

qa/tasks/cephfs/test_admin.py

src/mon/MDSMonitor.cc

rishabh-d-dave · 2024-09-02T13:23:24Z

.

anthonyeleven

suggestions

PendingReleaseNotes

doc/cephfs/administration.rst

anthonyeleven · 2024-09-02T15:12:30Z

doc/cephfs/troubleshooting.rst


  That prevents any clients from establishing new sessions with the MDS.

+* **Dont tweak max_mds** Changing value of ``max_mds`` FS setting variable is


See wording above

src/mon/FSCommands.cc

rishabh-d-dave · 2024-09-09T18:49:55Z

@vshankar i've made the changes last week that were recommended by you. PTAL when you find some time. :)

vshankar · 2024-09-10T16:45:58Z

@vshankar i've made the changes last week that were recommended by you. PTAL when you find some time. :)

I'll have a look tomorrow. Meanwhile requesting a review from @ceph/cephfs.

rishabh-d-dave · 2024-09-10T17:22:53Z

@vshankar i've made the changes last week that were recommended by you. PTAL when you find some time. :)

I'll have a look tomorrow. Meanwhile requesting a review from @ceph/cephfs.

Thank you!

vshankar

Minor nit, otherwise LGTM.

src/mon/MDSMonitor.cc

rishabh-d-dave · 2024-09-30T12:41:54Z

Had a discussion wit Venky here, have made the relevant changes.

vshankar

Otherwise LGTM.

src/mon/MDSMonitor.cc

…luster User must pass the confirmation flag (--yes-i-really-mean-it) to change the value of CephFS setting variable "max_mds" when the Ceph cluster is unhealthy. This measure was decided upon to prevent users from changing "max_mds" as a measure of troubleshotoing unhealthy cluster. Fixes: https://tracker.ceph.com/issues/66301 Signed-off-by: Rishabh Dave <ridave@redhat.com>

Add tests to ensure that when cluster has any health warning, especially MDS_TRIM, confirmation flag is mandatory to change max_mds. Signed-off-by: Rishabh Dave <ridave@redhat.com>

Update the documentation for CephFs admininstration as well troubleshooting. Signed-off-by: Rishabh Dave <ridave@redhat.com>

Add a release note for the fact that users now need to pass the confirmation flag for modifying "max_mds" when cluster is unhealthy. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave · 2024-10-01T14:14:34Z

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

rishabh-d-dave · 2024-10-01T14:15:47Z

jenkins test api

rishabh-d-dave

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/Main#2024-Oct-18

rishabh-d-dave · 2024-10-18T11:45:51Z

jenkins test api

rishabh-d-dave · 2024-10-18T11:46:58Z

Ready for merge but waiting for Ceph API CI job to pass

Changes requested have been incorporated.

rishabh-d-dave requested a review from a team as a code owner August 23, 2024 13:27

rishabh-d-dave added cephfs Ceph File System tests needs-review labels Aug 23, 2024

rishabh-d-dave requested review from a team, batrick and vshankar August 23, 2024 13:27

github-actions bot added core mon labels Aug 23, 2024

rishabh-d-dave changed the title ~~mon,cephfs: require confirmation when changing max_mds onunhealthy cluster~~ mon,cephfs: require confirmation when changing max_mds on unhealthy cluster Aug 23, 2024

rishabh-d-dave force-pushed the max-mds-confirm branch from ee33e86 to d978576 Compare August 23, 2024 13:41

joscollin reviewed Aug 26, 2024

View reviewed changes

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

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

joscollin previously requested changes Aug 26, 2024

View reviewed changes

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

src/mon/FSCommands.cc Show resolved Hide resolved

batrick requested changes Aug 26, 2024

View reviewed changes

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

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

src/mon/FSCommands.cc Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the max-mds-confirm branch 2 times, most recently from 5f3467d to 2de7b1d Compare August 27, 2024 08:25

rishabh-d-dave requested a review from a team as a code owner August 27, 2024 08:25

github-actions bot added the documentation label Aug 27, 2024

rishabh-d-dave requested review from batrick and joscollin August 27, 2024 08:25

rishabh-d-dave force-pushed the max-mds-confirm branch from 2de7b1d to 1985b7a Compare August 27, 2024 11:00

batrick approved these changes Aug 27, 2024

View reviewed changes

vshankar requested changes Aug 29, 2024

View reviewed changes

vshankar reviewed Aug 29, 2024

View reviewed changes

src/mon/MDSMonitor.cc Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the max-mds-confirm branch 2 times, most recently from 7704ae9 to 33bb3cb Compare September 2, 2024 13:09

rishabh-d-dave force-pushed the max-mds-confirm branch from 33bb3cb to 7ea5fdb Compare September 2, 2024 13:30

anthonyeleven reviewed Sep 2, 2024

View reviewed changes

rishabh-d-dave force-pushed the max-mds-confirm branch 2 times, most recently from 2b68078 to 75b6224 Compare September 2, 2024 18:52

vshankar requested a review from a team September 10, 2024 09:52

vshankar reviewed Sep 12, 2024

View reviewed changes

src/mon/MDSMonitor.cc Outdated Show resolved Hide resolved

src/mon/MDSMonitor.cc Outdated Show resolved Hide resolved

rishabh-d-dave force-pushed the max-mds-confirm branch from 75b6224 to 6002ced Compare September 30, 2024 12:40

rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Sep 30, 2024

vshankar approved these changes Sep 30, 2024

View reviewed changes

src/mon/MDSMonitor.cc Outdated Show resolved Hide resolved

rishabh-d-dave added 4 commits October 1, 2024 12:45

qa/cephfs: add tests for confirmationn required to change max_mds

4d5ec87

Add tests to ensure that when cluster has any health warning, especially MDS_TRIM, confirmation flag is mandatory to change max_mds. Signed-off-by: Rishabh Dave <ridave@redhat.com>

doc/cephfs: update about changing max_mds FS setting variable

2d28faa

Update the documentation for CephFs admininstration as well troubleshooting. Signed-off-by: Rishabh Dave <ridave@redhat.com>

PendingReleaseNotes: add a release note about confirm flag for max_mds

a71c8e8

Add a release note for the fact that users now need to pass the confirmation flag for modifying "max_mds" when cluster is unhealthy. Signed-off-by: Rishabh Dave <ridave@redhat.com>

rishabh-d-dave force-pushed the max-mds-confirm branch from 6002ced to a71c8e8 Compare October 1, 2024 08:20

rishabh-d-dave commented Oct 18, 2024

View reviewed changes

rishabh-d-dave merged commit 8978b85 into ceph:main Oct 18, 2024

rishabh-d-dave deleted the max-mds-confirm branch October 18, 2024 14:35

This was referenced Oct 18, 2024

reef: mon,cephfs: require confirmation when changing max_mds on unhealthy cluster #60397

Open

squid: mon,cephfs: require confirmation when changing max_mds on unhealthy cluster #60398

Merged


		That prevents any clients from establishing new sessions with the MDS.

		* Dont tweak max_mds Changing value of ``max_mds`` FS setting variable is

Conversation

rishabh-d-dave commented Aug 23, 2024

Contribution Guidelines

Checklist

Uh oh!

Uh oh!

Uh oh!

joscollin commented Aug 26, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rishabh-d-dave commented Aug 27, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vshankar Aug 29, 2024

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonyeleven Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rishabh-d-dave commented Sep 2, 2024

Uh oh!

anthonyeleven left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

anthonyeleven Sep 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rishabh-d-dave commented Sep 9, 2024

Uh oh!

vshankar commented Sep 10, 2024

Uh oh!

rishabh-d-dave commented Sep 10, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

rishabh-d-dave commented Sep 30, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rishabh-d-dave commented Oct 1, 2024

Uh oh!

rishabh-d-dave commented Oct 1, 2024

Uh oh!

rishabh-d-dave left a comment

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave commented Oct 18, 2024

Uh oh!

rishabh-d-dave commented Oct 18, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rishabh-d-dave Sep 2, 2024 •

edited

Loading