osd/PeeringState: fix acting_set_writeable min_size check#40572
Merged
tchaikov merged 2 commits intoceph:masterfrom Apr 6, 2021
Merged
osd/PeeringState: fix acting_set_writeable min_size check#40572tchaikov merged 2 commits intoceph:masterfrom
tchaikov merged 2 commits intoceph:masterfrom
Conversation
acting has placeholders for ec, need to use actingset. Signed-off-by: Samuel Just <sjust@redhat.com>
Contributor
Author
|
I think this explains both https://tracker.ceph.com/issues/48613 and the original lrc problem https://tracker.ceph.com/issues/48417 |
Contributor
Author
|
Commit message has an explanation. |
Contributor
Author
|
@ideepika Can you run this through the teuthology suite sequence you've been using to reproduce the original lrc problem? |
Member
@athanatos this makes sense, will test them! |
tchaikov
reviewed
Apr 5, 2021
ideepika
approved these changes
Apr 5, 2021
Member
ideepika
left a comment
There was a problem hiding this comment.
passes with reproducer I was using: https://pulpito.ceph.com/ideepika-2021-04-05_06:44:27-rados:thrash-erasure-code-wip-deepika-testing-2021-04-05-0643-distro-basic-gibba/
running with: #40593 on a full suite run
here: http://pulpito.front.sepia.ceph.com/ideepika-2021-04-05_14:32:13-rados-wip-deepika-testing-2021-04-05-0643-distro-basic-smithi/
acting.size() >= pool.info.min_size is meant to check min_size against
acting set participants, but acting is a vector with placeholders.
actingset is the representation with placeholders removed.
The upshot of this bug is that the activation process will basically
ignore min_size for an ec pool allowing writes in cases where it
shouldn't. PastIntervals::check_new_interval, however, performs
the check correctly, and will therefore discount intervals in which
we really did serve writes as not writeable. This can trigger many
different problem conditions including but not limited to:
- Unfound objects due to accepting a last_update with insufficient
osds
- Lost writes
- Crashes due to peering rules being violated
This bug was originally introduced with recovery below min_size in
e5a96fd, and then preserved through refactors in 749a13d and 95bec9.
7cb818a exposed it with with expansion of recovery below min_size
to include ec pools (acting.size() is sufficient for replicated
pools).
Fixes: https://tracker.ceph.com/issues/48613
Fixes: https://tracker.ceph.com/issues/48417
Signed-off-by: Samuel Just <sjust@redhat.com>
f962c63 to
642a1c1
Compare
Contributor
3 tasks
dvanders
added a commit
to dvanders/ceph
that referenced
this pull request
Jun 1, 2021
This is a nautilus only manual backport of ceph#40572 which is itself composed of commits 7b2e0f4 642a1c1 The backport did not apply cleanly because these call have been factored out into PeeringState.cc in octopus and newer. The original callers have been fixed in PG.cc. Fixes: https://tracker.ceph.com/issues/50153 Signed-off-by: Dan van der Ster <daniel.vanderster@cern.ch>
mkogan1
pushed a commit
to mkogan1/ceph
that referenced
this pull request
Apr 24, 2022
Fix backport of Greg's Farnum's PeeringState changes to correctly arrive at the valid implementation, accounting for Dan van der Ster's bugfix for this: commit f3b15f1 Author: Dan van der Ster <daniel.vanderster@cern.ch> Date: Tue Jun 1 11:14:10 2021 +0200 nautilus: osd/PeeringState: fix acting_set_writeable min_size check This is a nautilus only manual backport of ceph#40572 which is itself composed of commits 7b2e0f4 642a1c1 The backport did not apply cleanly because these call have been factored out into PeeringState.cc in octopus and newer. The original callers have been fixed in PG.cc. Fixes: https://tracker.ceph.com/issues/50153 Signed-off-by: Dan van der Ster <daniel.vanderster@cern.ch> Cross checked with Greg, mistakes mine if any. Signed-off-by: Matt Benjamin <mbenjamin@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.