src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON #55103

kamoltat · 2024-01-09T10:44:19Z

Problem:

In stretch mode, we encountered
an assert failure when checking for
dead crush zones when we have a none-existing
CRUSH bucket.

Solution:

Ignore the non-existent crush bucket, instead
of asserting. We then warn the user about a particular
MON contains a non-existent CRUSH bucket.
The tiebreaker monitor is the only exception where
we would allow having non-existent Crush location

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

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

ronen-fr · 2024-01-10T16:34:52Z

src/mon/MonmapMonitor.cc

+    if (monmap.stretch_mode_enabled) {
+      for (const auto &p : loc) {
+        if (!mon.osdmon()->osdmap.crush->name_exists(p.second)) {
+          ss << "location doesn't belong to any existing crush buckets!"


One very minor comment:
one long warning line? shouldn't it be broken down into multiple output lines?

Several lines above also have one long warning line, IMHO the warning isn't so large that we need multiple lines.

nvm I'm changing the output to be shorter, thank you for the review though.

kamoltat · 2024-01-18T07:02:38Z

jenkins test api

kamoltat · 2024-01-18T07:03:06Z

jenkins test windows

ljflores · 2024-01-18T21:24:27Z

jenkins test api

ljflores · 2024-01-23T18:23:24Z

jenkins test api

ljflores · 2024-01-23T18:23:26Z

Rados-approved: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrellocomc79noWWpu1934-wip-yuri7-testing-2024-01-18-1327

gregsfortytwo · 2024-01-24T15:49:31Z

src/mon/MonmapMonitor.cc

+        if (!mon.osdmon()->osdmap.crush->name_exists(p.second)) {
+          ss << "location doesn't belong to any existing crush buckets!"
+          << " If you are trying to replace an arbiter mon, please use the command:"
+          <<  " ceph mon set_new_tiebreaker";


Don't you need to add the tiebreaker before you can configure it as the tiebreaker? Or did we move that to be a special command to avoid all this?

You're right the output is misleading, you need to do ceph mon add and then ceph mon set_new_tiebreaker. I'll just remove that part of the output.

I mean the problem is the tiebreaker monitor's bucket cannot exist in the crush map (or at least, there can't be OSDs there, so having the map reflect it is not idiomatic). And the documentation would need to be changed even if accepting that solution.

And once in stretch mode you are blocked from adding monitors without a location, if that wasn't clear. So this commit actually prevents replacing the tiebreaker at all.

gregsfortytwo · 2024-01-24T15:50:01Z

src/mon/MonmapMonitor.cc

      goto reply_no_propose;
    }
+    if (monmap.stretch_mode_enabled) {
+      for (const auto &p : loc) {


There's a TODO comment several lines up that should be removed if this actually works.

Okay will remove that

gregsfortytwo · 2024-01-24T15:53:32Z

The documentation certainly still suggests you need to create a monitor and then set it as a tiebreaker.

Which means that since this passed testing, it should also add some tests around that. 😮

yuriw · 2024-01-24T21:54:45Z

@kamoltat @gregsfortytwo This was tested ref: https://trello.com/c/79noWWpu
pls merge or add "needs-qa" if more tests needed

kamoltat · 2024-02-08T20:41:09Z

The documentation certainly still suggests you need to create a monitor and then set it as a tiebreaker.

Which means that since this passed testing, it should also add some tests around that. 😮

@gregsfortytwo You're right, I should have added a test to at least qa/standalone. I was just testing the change in vstart.sh. Will create a test for this.

gregsfortytwo · 2024-02-08T23:31:33Z

The documentation certainly still suggests you need to create a monitor and then set it as a tiebreaker.

Which means that since this passed testing, it should also add some tests around that. 😮

@gregsfortytwo You're right, I should have added a test to at least qa/standalone. I was just testing the change in vstart.sh. Will create a test for this.

How did you test this in vstart? Am I parsing things wrong, given my assumption it actually blocks switching to a new monitor in real deployments?

kamoltat · 2024-02-09T15:40:07Z

@gregsfortytwo I'll test it one more time, been a while since I tested this PR.

yuriw · 2024-03-14T19:45:51Z

@kamoltat any update?

github-actions · 2024-05-13T21:01:30Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2024-06-12T22:01:38Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

kamoltat · 2024-08-21T12:59:33Z

@gregsfortytwo ping

kamoltat · 2024-08-26T13:21:18Z

jenkins test make check

github-actions · 2024-10-25T14:02:21Z

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

github-actions · 2024-11-14T03:24:09Z

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

Problem: In a stretch cluster, we encountered an assert failure when checking for dead crush zones when we have a none-existing CRUSH bucket. Solution: Ignore the none-existing crush bucket, instead of assert. Fixes: https://tracker.ceph.com/issues/63861 Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>

gregsfortytwo

This seems fine
Reviewed-by: Greg Farnum gfarnum@redhat.com

gregsfortytwo · 2024-11-25T20:35:50Z

src/mon/HealthMonitor.cc

+void HealthMonitor::check_mon_crush_loc_stretch_mode(health_check_map_t *checks)
+{
+  // Check if the CRUSH location exists for all MONs
+  ceph_assert(mon.monmap->stretch_mode_enabled);


Rather than asserting, why not just return (healthy) if stretch mode is off?
I put in a lot of asserts that caused crashes in my first stretch implementation, and assert crashes are better than data corruption, but not being able to fail is even better!

The reason I have the assert is because the callee is under the condition:
if (mon.monmap->stretch_mode_enabled)
so if we enter check_mon_crush_loc_stretch_mode and stretch mode is off, then something must have gone wrong. But I understand your point, we can potentially modify this such that we call check_mon_crush_loc_stretch_mode but then exit if stretch mode is not enabled, I can see this being a cleaner approach.

In streth mode, warn the user when we encounter a MON that has nonexistent crush location, with the tiebreaker MON being the only exception to this. Fixes: https://tracker.ceph.com/issues/63861 Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>

…TRETCH_MODE Added NONEXISTENT_MON_CRUSH_LOC_STRETCH_MODE to the documentation. Fixes: https://tracker.ceph.com/issues/63861 Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>

kamoltat · 2024-11-25T21:55:31Z

Pushed new change regarding Greg's comment above

kamoltat · 2025-01-07T09:51:12Z

jenkins test make check

kamoltat · 2025-02-23T07:12:42Z

RADOS APPROVED: https://tracker.ceph.com/projects/rados/wiki/MAIN#httpstrackercephcomissues69792

anthonyeleven · 2025-02-23T20:29:10Z

doc/rados/operations/health-checks.rst

+
+The CRUSH location specified for the monitor must belong to one of the dividing
+buckets when stretch mode is enabled. With the ``tiebreaker`` monitor being the
+only exception.


Comma not period. The latter part here is not a sentence.

@anthonyeleven Ah okay, I guess I'll file a new PR for this.

or I can do so, let's get in it while we can.

@anthonyeleven okay I think you'll be faster, thank you!

src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON Reviewed-by: Ronen Friedman <rfriedma@redhat.com> Reviewed-by: Greg Farnum <gfarnum@redhat.com> Reviewed-by: Anthony D'Atri <anthony.datri@gmail.com>

kamoltat added core needs-review mon labels Jan 9, 2024

kamoltat self-assigned this Jan 9, 2024

kamoltat requested a review from a team as a code owner January 9, 2024 10:44

ronen-fr approved these changes Jan 10, 2024

View reviewed changes

neha-ojha added needs-qa and removed needs-review labels Jan 10, 2024

ljflores added the wip-yuri7-testing label Jan 18, 2024

batrick requested a review from gregsfortytwo January 24, 2024 13:43

gregsfortytwo requested changes Jan 24, 2024

View reviewed changes

yuriw added TESTED ready-to-merge and removed wip-yuri7-testing labels Jan 24, 2024

kamoltat force-pushed the wip-ksirivad-fix-63861 branch from 532dd19 to 29bddbf Compare February 8, 2024 20:28

github-actions bot added the stale label May 13, 2024

github-actions bot closed this Jun 12, 2024

kamoltat force-pushed the wip-ksirivad-fix-63861 branch 2 times, most recently from 8d446e8 to 10926fd Compare August 8, 2024 21:01

kamoltat requested a review from a team as a code owner August 8, 2024 21:13

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

anthonyeleven approved these changes Aug 8, 2024

View reviewed changes

github-actions bot added the stale label Oct 25, 2024

kamoltat removed the stale label Oct 29, 2024

github-actions bot added the needs-rebase label Nov 14, 2024

kamoltat force-pushed the wip-ksirivad-fix-63861 branch from d4fc990 to a9c0abc Compare November 25, 2024 20:22

github-actions bot removed the needs-rebase label Nov 25, 2024

gregsfortytwo approved these changes Nov 25, 2024

View reviewed changes

kamoltat added 2 commits November 25, 2024 21:54

doc/rados/operations/health-checks: Added NONEXISTENT_MON_CRUSH_LOC_S…

97b815c

…TRETCH_MODE Added NONEXISTENT_MON_CRUSH_LOC_STRETCH_MODE to the documentation. Fixes: https://tracker.ceph.com/issues/63861 Signed-off-by: Kamoltat Sirivadhna <ksirivad@redhat.com>

kamoltat force-pushed the wip-ksirivad-fix-63861 branch from a9c0abc to 97b815c Compare November 25, 2024 21:55

kamoltat added needs-qa and removed needs-review labels Nov 25, 2024

ljflores added the wip-yuri6-testing label Feb 4, 2025

anthonyeleven reviewed Feb 23, 2025

View reviewed changes

yuriw merged commit 5f9fa43 into ceph:main Feb 24, 2025
12 checks passed

This was referenced Feb 27, 2025

squid: src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON #62039

Merged

reef: src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON #62040

Merged

src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON #55103

src/mon/OSDMonitor.cc: [Stretch Mode] WRN non-existent CRUSH location assigned to MON #55103

Uh oh!

Conversation

kamoltat commented Jan 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

ronen-fr Jan 10, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kamoltat commented Jan 18, 2024

Uh oh!

kamoltat commented Jan 18, 2024

Uh oh!

ljflores commented Jan 18, 2024

Uh oh!

ljflores commented Jan 23, 2024

Uh oh!

ljflores commented Jan 23, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gregsfortytwo commented Jan 24, 2024

Uh oh!

yuriw commented Jan 24, 2024

Uh oh!

kamoltat commented Feb 8, 2024

Uh oh!

gregsfortytwo commented Feb 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kamoltat commented Feb 9, 2024

Uh oh!

yuriw commented Mar 14, 2024

Uh oh!

github-actions bot commented May 13, 2024

Uh oh!

github-actions bot commented Jun 12, 2024

Uh oh!

kamoltat commented Aug 21, 2024

Uh oh!

kamoltat commented Aug 26, 2024

Uh oh!

github-actions bot commented Oct 25, 2024

Uh oh!

github-actions bot commented Nov 14, 2024

Uh oh!

gregsfortytwo left a comment

Choose a reason for hiding this comment

Uh oh!

gregsfortytwo Nov 25, 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!

Choose a reason for hiding this comment

Uh oh!

kamoltat commented Nov 25, 2024

Uh oh!

kamoltat commented Jan 7, 2025

Uh oh!

kamoltat commented Feb 23, 2025

Uh oh!

Choose a reason for hiding this comment

kamoltat commented Jan 9, 2024 •

edited

Loading

ronen-fr Jan 10, 2024 •

edited

Loading

gregsfortytwo commented Feb 8, 2024 •

edited

Loading

gregsfortytwo Nov 25, 2024 •

edited

Loading