mgr: allow disabling always-on modules by rishabh-d-dave · Pull Request #58647 · ceph/ceph

rishabh-d-dave · 2024-07-17T12:54:51Z

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

Original approach archived here: #61043

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

rishabh-d-dave · 2024-07-17T13:44:08Z

@vshankar Had a chat with Venky about the current patch. We want to keep volumes as a always-on module and yet we want to ceph mgr module disable volumes to be successful when `--yes-i-really-mean-it`` is passed. Proceeding to make this change.

src/mon/MgrMonitor.cc

rishabh-d-dave · 2024-07-19T11:40:07Z

@vshankar I've made the change as per our last discussion -- we now allow volumes to disabled through ceph mgr module disable volumes --yes-i-really-mean-it even though it is an always-on module. PTAL.

Let me know if this PR is moving right direction, I'll wait before proceeding until then. If it looks okay, I'll begin with remainder bits of work left.

rishabh-d-dave · 2024-07-23T11:07:00Z

@vshankar I've made the change as per our last discussion -- we now allow volumes to disabled through ceph mgr module disable volumes --yes-i-really-mean-it even though it is an always-on module. PTAL.

Let me know if this PR is moving right direction, I'll wait before proceeding until then. If it looks okay, I'll begin with remainder bits of work left.

@vshankar ping

rishabh-d-dave · 2024-07-23T12:55:02Z

I had requested review from ceph/cephfs before here, since Github removes it from "reviewers" after first review, adding it again at Venky's suggestion

rishabh-d-dave · 2024-07-25T13:48:41Z

@vshankar I've made the change as per our last discussion -- we now allow volumes to disabled through ceph mgr module disable volumes --yes-i-really-mean-it even though it is an always-on module. PTAL.

Let me know if this PR is moving right direction, I'll wait before proceeding until then. If it looks okay, I'll begin with remainder bits of work left.

Pinging again. Please check and let me know if this PR is moving in right direction and if I should continue building on top of this.

vshankar

Need to include

Tests
Documentation
Note in PendingReleaseNotes

vshankar · 2024-07-26T05:22:50Z

src/mon/MgrMonitor.cc

-      ss << "module '" << module << "' is already disabled";
-      r = 0;
-      goto out;
+    if (module != "volumes") {


Should this allow disabling any always-on module?

It depends on whether or not maintainers of other MGR modules want it. I don't know if allowing disabling them too is appropriate. Especially on this PR.

cc @ljflores

vshankar · 2024-07-26T05:23:27Z

src/mon/MgrMonitor.cc

+
+      dout(8) << __func__ << " disabling module '" << module << "'" << dendl;
+      pending_map.force_disabled_modules.insert(module);
+    } else if (module != "volumes" && confirmation_flag) {


It doesn't harm to pass --yes... flag to modules that can be disabled IMO.

#58647 (comment)

github-actions · 2024-07-29T05:00:26Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

rishabh-d-dave · 2024-10-19T11:28:16Z

Rebased to resolve the conflict.

vshankar · 2024-10-22T12:46:33Z

@rishabh-d-dave - Can you rebase as I am seeing errors due to conflicts -

@hkadam134 Rebased.

Test Jobs in-progress :
admin - https://pulpito.ceph.com/hyelloji-2024-10-15_06:07:49-fs-wip-hemanth-testing-2024-10-14-1559-testing-default-smithi/
fs:functional - https://pulpito.ceph.com/hyelloji-2024-10-15_06:08:53-fs-wip-hemanth-testing-2024-10-14-1559-testing-default-smithi/

@hkadam134 please review the fs suite for any known failures and if any of the failures are related to this change. Also, I commented about running through rados suite (not full suite but at least the mgr tests). Has that been in the works?

@vshankar - Apart from these two remaining failures are part of known issue list : https://tracker.ceph.com/issues/68568 https://tracker.ceph.com/issues/68567

I will follow up on rados(mgr) tests and update.

@hkadam134 any update on this?

ljflores · 2024-10-23T08:27:14Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues67974

rishabh-d-dave · 2024-10-23T10:52:56Z

Rados approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues67974

Thank you.

@vshankar Should we proceed to merge or some bit of CephFS review is left? Let me know if I can help.

vshankar · 2024-10-23T11:11:25Z

Good to merge! Nice work @rishabh-d-dave

Backport the "Disabling Volumes Plugin" section of doc/cephfs/fs-volumes.rst, which was folded into a packet of code changes in ceph#58647 and therefore not possible to backport in the normal way. This is one of a series of similar pull requests that will result in this file being congruent across release branches. Signed-off-by: Zac Dover <zac.dover@proton.me>

zdover23 · 2024-10-25T20:36:09Z

#60496 - Squid backport (documentation)
#60497 - Reef backport (documentation)
#60498 - Quincy backport (documentation)

rishabh-d-dave · 2024-10-28T11:43:40Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23
cc @anthonyeleven
Why were docs backported separately? Merging doc changes without rest of the changes in older versions makes no sense.

zdover23 · 2024-10-28T13:19:14Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23 cc @anthonyeleven Why were docs backported separately? Merging doc changes without rest of the changes in older versions makes no sense.

The file fs-volumes.rst had become inconsistent between main and the release branches squid, reef, and quincy, making backports impossible without in each case resolving the merge conflicts that arose from the inconsistency.

Should the Volumes plugins docs be removed from Squid, Reef, and Quincy?

rishabh-d-dave · 2024-10-30T05:53:20Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23 cc @anthonyeleven Why were docs backported separately? Merging doc changes without rest of the changes in older versions makes no sense.

The file fs-volumes.rst had become inconsistent between main and the release branches squid, reef, and quincy, making backports impossible without in each case resolving the merge conflicts that arose from the inconsistency.

Okay, I see. The issue is now docs for Squid, Reef and Quincy tell disabling volumes plugin is possible but that is not the case for these releases yet. So users are misinformed.

Should the Volumes plugins docs be removed from Squid, Reef, and Quincy?

I guess let it be now since this PR has to be backported to all 3 releases. Older releases will be incorrect in the mean time.

zdover23 · 2024-10-30T06:10:50Z

@rishabh-d-dave, if you tell me how you want the docs to look, I will make them that way. It's not a big deal to get them right.

rishabh-d-dave · 2024-10-30T06:40:33Z

@rishabh-d-dave, if you tell me how you want the docs to look, I will make them that way. It's not a big deal to get them right.

The doc changes introduced by the three backports your raised needs to be removed entirely.

rishabh-d-dave · 2024-10-30T06:41:03Z

@rishabh-d-dave, if you tell me how you want the docs to look, I will make them that way. It's not a big deal to get them right.

The doc changes introduced by the three backports your raised needs to be removed entirely.

But I guess let's keep it as it is. It will be taken care of in few weeks when rest of this PR is also backported.

rishabh-d-dave · 2024-10-30T15:05:15Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23 @anthonyeleven We should avoid backporting docs (or any other bits of a PR) separately in future. Not only that puts docs for previous releases in an incorrect state (since docs were backported without backporting the feature/bug fix) but also makes PRs more difficult to backport (due to conflicts) and harder to review (since reviewer now has to say mindful of missing patches and commits as well).

zdover23 · 2024-10-30T15:09:46Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23 @anthonyeleven We should avoid backporting docs (or any other bits of a PR) separately in future. Not only that puts docs for previous releases in an incorrect state (since docs were backported without backporting the feature/bug fix) but also makes PRs more difficult to backport (due to conflicts) and harder to review (since reviewer now has to say mindful of missing patches and commits as well).

I think that the docs that accompany code changes should be put in a PR separate from the code changes (but, of course, cross-referencing one another). If this is done, then we can avoid situations in which accidentally incomplete backports result in divergent documentation in release branches.

rishabh-d-dave · 2024-10-30T15:17:28Z

#60496 - Squid backport (documentation) #60497 - Reef backport (documentation) #60498 - Quincy backport (documentation)

@zdover23 @anthonyeleven We should avoid backporting docs (or any other bits of a PR) separately in future. Not only that puts docs for previous releases in an incorrect state (since docs were backported without backporting the feature/bug fix) but also makes PRs more difficult to backport (due to conflicts) and harder to review (since reviewer now has to say mindful of missing patches and commits as well).

I think that the docs that accompany code changes should be put in a PR separate from the code changes (but, of course, cross-referencing one another). If this is done, then we can avoid situations in which accidentally incomplete backports result in divergent documentation in release branches.

I don't see the point in separating backport PR for docs changes. That increasing the risk of reaching an inconsistent state. For example, if backport doc PR is merged but backport code changes PR is later rejected, our docs are incorrect for that particular release. There are many other similar cases. For example doc changes backports much sooner than code changes (which will always be the case). The user of older release will be misinformed about a feature or bug fixes in such cases, making our docs unreliable.

rishabh-d-dave requested a review from a team as a code owner July 17, 2024 12:54

github-actions bot added common core mon labels Jul 17, 2024

rishabh-d-dave marked this pull request as draft July 17, 2024 13:02

rishabh-d-dave changed the title ~~mgr: allow disalbing volumes module~~ mgr: allow disabling volumes module Jul 17, 2024

rishabh-d-dave requested review from a team and vshankar July 17, 2024 13:39

vshankar reviewed Jul 17, 2024

View reviewed changes

src/mon/MgrMonitor.cc Show resolved Hide resolved

rishabh-d-dave force-pushed the mgr-vol-mod-disable branch from 9ee81c4 to 6188a3b Compare July 19, 2024 11:28

github-actions bot added the mgr label Jul 19, 2024

rishabh-d-dave added cephfs Ceph File System needs-review labels Jul 19, 2024

rishabh-d-dave marked this pull request as ready for review July 19, 2024 11:29

rishabh-d-dave requested a review from a team July 23, 2024 12:53

vshankar requested changes Jul 26, 2024

View reviewed changes

rishabh-d-dave force-pushed the mgr-vol-mod-disable branch from 6188a3b to 37fca79 Compare July 29, 2024 05:00

rishabh-d-dave requested a review from a team as a code owner July 29, 2024 05:00

github-actions bot added documentation tests needs-rebase labels Jul 29, 2024

rishabh-d-dave requested a review from vshankar July 29, 2024 05:01

vshankar requested review from a team and batrick July 29, 2024 05:35

rishabh-d-dave force-pushed the mgr-vol-mod-disable branch from 7d96673 to 3232f6b Compare October 19, 2024 11:28

github-actions bot removed the needs-rebase label Oct 19, 2024

vshankar approved these changes Oct 23, 2024

View reviewed changes

hkadam134 merged commit fca07e9 into ceph:main Oct 23, 2024

This was referenced Oct 25, 2024

squid: doc/cephfs: backport "Disabling Volumes Plugin" #60493

Closed

reef: doc/cephfs: backport "Disabling Volumes Plugin" #60494

Closed

quincy: doc/cephfs: backport "Disabling Volumes Plugin" #60495

Closed

rishabh-d-dave deleted the mgr-vol-mod-disable branch October 28, 2024 11:43

This was referenced Oct 30, 2024

squid: mgr: allow disabling always-on modules #60562

Merged

reef: mgr: allow disabling always-on modules #60563

Merged

quincy: mgr: allow disabling always-on modules #60564

Closed

rishabh-d-dave mentioned this pull request Mar 14, 2025

doc/cephfs: link section for pausing async threads in section for... #62300

Merged

14 tasks

Conversation

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

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

rishabh-d-dave commented Jul 17, 2024

Uh oh!

Uh oh!

rishabh-d-dave commented Jul 19, 2024

Uh oh!

rishabh-d-dave commented Jul 23, 2024

Uh oh!

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

Uh oh!

Uh oh!

rishabh-d-dave commented Jul 25, 2024

Uh oh!

vshankar left a comment

Choose a reason for hiding this comment

Uh oh!

vshankar Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Jul 26, 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!

vshankar Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

vshankar Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

rishabh-d-dave Jul 26, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jul 29, 2024

Uh oh!

rishabh-d-dave commented Oct 19, 2024

Uh oh!

vshankar commented Oct 22, 2024

Uh oh!

ljflores commented Oct 23, 2024

Uh oh!

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

Uh oh!

Uh oh!

vshankar commented Oct 23, 2024

Uh oh!

zdover23 commented Oct 25, 2024

Uh oh!

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

Uh oh!

Uh oh!

zdover23 commented Oct 28, 2024

Uh oh!

rishabh-d-dave commented Oct 30, 2024

Uh oh!

zdover23 commented Oct 30, 2024

Uh oh!

rishabh-d-dave commented Oct 30, 2024

Uh oh!

rishabh-d-dave commented Oct 30, 2024

Uh oh!

rishabh-d-dave commented Oct 30, 2024

Uh oh!

zdover23 commented Oct 30, 2024

Uh oh!

rishabh-d-dave commented Oct 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

rishabh-d-dave commented Jul 17, 2024 •

edited

Loading

rishabh-d-dave commented Jul 23, 2024 •

edited

Loading

rishabh-d-dave Jul 26, 2024 •

edited

Loading

rishabh-d-dave commented Oct 23, 2024 •

edited

Loading

rishabh-d-dave commented Oct 28, 2024 •

edited

Loading