core: Extracting scrub-related code from the PG & friends - w.i.p#37317
Closed
ronen-fr wants to merge 17 commits intoceph:masterfrom
Closed
core: Extracting scrub-related code from the PG & friends - w.i.p#37317ronen-fr wants to merge 17 commits intoceph:masterfrom
ronen-fr wants to merge 17 commits intoceph:masterfrom
Conversation
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
athanatos
reviewed
Sep 22, 2020
Contributor
|
I think this approach in general should be viable. @dzafman should probably take a look. |
Contributor
|
This would be greatly easier to review if perhaps you could sequence the commits so that there's a single commit that moves the main logic into pg_scrubber* prior to re-factoring the state machine. I understand if you don't want to take the effort to do that, however. |
Contributor
Author
|
"This would be greatly easier to review if perhaps you could sequence the commits so that there's a single commit that moves the main logic into pg_scrubber* prior to re-factoring the state machine. I understand if you don't want to take the effort to do that, however." I will try to improve on that in the rebased version (which I plan to do line-by-line). |
ronen-fr
commented
Sep 30, 2020
Contributor
Author
|
jenkins retest this please |
Contributor
Author
|
jenkins retest this please unrelated issues in the makecheck. All but one test OK on the 1st round. Strane results on the 2nd.
|
Member
|
rados run: https://pulpito.ceph.com/nojha-2020-09-30_21:58:00-rados-wip-37317-distro-basic-smithi/ @ronen-fr The following failures look related: |
dzafman
suggested changes
Oct 1, 2020
Contributor
dzafman
left a comment
There was a problem hiding this comment.
I have more reviewing to do, but I wanted to submit these comments.
src/osd/PG.cc
Outdated
| queue_scrub(); | ||
| m_planned_scrub.must_deep_scrub = true; | ||
| m_planned_scrub.check_repair = true; | ||
| m_planned_scrub.must_scrub = true; // RRR added this line, as I don't see how the comment in PG::sched_scrub_initial() is true otherwise |
Contributor
There was a problem hiding this comment.
Setting must_scrub isn't required in the original code because queue_scrub() knows we are scrubbing, so it doesn't matter in that function if this is a scheduled scrub or user initiated scrub.
Contributor
There was a problem hiding this comment.
This is why req_scrub is added, to remember whether original scrub was user requested. Also, the comment in sched_scrub_initial() is true at a higher level. Internally, it doesn't have to be so strict, but maybe it is ok to set it here as long as it is cleared. We don't want that flag on during scrubbing. We don't want to show MUST_SCRUB generally in the log, so it is cleared once scrub starts.
Contributor
Author
There was a problem hiding this comment.
Thanks for the clarification.
(req_scrub wasn't there yet when I added the comment you ref'ed to.)
I have found the flags handling the hardest to follow, and made the following changes - mostly for structure & legibility - while following all the details of the existing implementation.
The flags are now divided into the following categories/structures:
(A) Flags managed by the PG to affect the next scrub parameters (and scheduling):
Grouped in PG.m_planned_scrub of type requested_scrub_t.
- Primary only;
- all
next-scrub((next commit push later today)) flags are cleared onPG::sched_scrub(), after affecting the running scrub flags (B below).
Note that this is different from the existing implementation, though I think I maintained the existing behaviour.
Specifically:- must_scrub: was cleared in the original code. A copy is now maintained in marked_must (see below).
- need_auto, must_deep_scrub: cleared in the original code too;
- must_repair: same. State is remembered in PG_STATE_REPAIR.
- deep_scrub_on_error: copied to the running flags. That copy is only cleared at
scrub_finish(), after initiating an auto deep-scrub. - check_repair: original behavior is maintained.
- time_for_deep: TBD... I am verifying the code now...
- auto_repair: state affects running scrub 'auto_repair' flag; also saved in PG_STATE_REPAIR. Clearing matches original code.
- req_scrub - note that the new version uses
m_planned_scrub.req_scrubfor the role played by save_req_scrub:
whenscrub_finish()determines that a scrub_after_recovery is in order, it copies ('or'ing, actually) the current
required flag (what was req_scrub when we started) back into the next-scrub's flag inm_planned_scrub.
- note the detailed description of each flag, per my understanding of the code, in scrub_common.h;
(B) Flags used by the executing scrub. These are owned by the PgScrubber.
Grouped in PgScrubber.m_flags, of type scrub_flags_t.
- used by both primary and replicas;
- set in
PgScrubber::set_op_parameters()(for a Primary), which is called from:PG::sched_scrub()- and note that we never reach here again after the scrubbing starts; andPG::queue_scrub_after_recovery()
and by PgScrubber::replica_scrub_op()
- a new marked_must flag captures the state of must_scrub at start time, to affect the scrubber periodic sleeps before each NewChunk (note that I didn't see that being handled in the original code.
Seems like it usesmust_scrubthat was already cleared inPG::sched_scrub()). - cleared when scrubbing finishes, in
PgSCrubber::reset_internal_state().
Contributor
There was a problem hiding this comment.
Please note that req_scrub stands for requested scrub which is the same as marked_must. req_scrub was only set in limited cases where a new scrub would happen and we needed to keep track of whether the instigating scrub was user requested. If you set req_scrub early in the scrub process, you just need to be careful not to clear it in the cases where it is needed.
Contributor
Author
There was a problem hiding this comment.
Currently there are some differences in the ways req_scrub vs mark_scrub affect scrubbing behaviour. After discussing this with others - I will merge the two anyway.
This comment has been minimized.
This comment has been minimized.
Contributor
Author
|
jenkins retest this please |
Contributor
Author
|
jenkins retest this please (previous round, b4 the last commit, was OK. Last check round fails for what seem to be unrelated) |
athanatos
requested changes
Oct 21, 2020
Contributor
athanatos
left a comment
There was a problem hiding this comment.
I've done a review pass on the first few files. If there are still outstanding questions you need help with, perhaps it would help to compile those in one place so that we can get them answered?
What
src/osd/PeeringState.h
Outdated
| virtual void on_info_history_change() = 0; | ||
| /// Notify that a scrub has been requested | ||
| virtual void scrub_requested(bool deep, bool repair, bool need_auto = false) = 0; | ||
| virtual void scrub_requested(scrub_level_t is_deep, scrub_type_t is_repair) = 0; |
Contributor
Author
There was a problem hiding this comment.
Done. But like I said - I don't really like the result re the 'is_repair'. Looking for a better name.
src/osd/PrimaryLogPG.cc
Outdated
| << "," << scrubber.end << ")" << dendl; | ||
| } | ||
| } | ||
| m_scrubber->add_stats_if_lower(delta_stats, soid); |
Contributor
There was a problem hiding this comment.
add_stats_if_lower isn't particularly descriptive. How about notify_object_stats_changed(delta_stats, soid)?
Contributor
There was a problem hiding this comment.
The lower part is an implementation detail of the scrubber, it simply reflects that objects < scrubber.start are reflected in the compiled scrub stats.
Contributor
Author
There was a problem hiding this comment.
I wouldn't use 'notify', as no notification takes place. Added comments to the PrimaryLogScrub (copied from Josh's notes). And renaming to stats_of_handled_objects() (?)
Contributor
|
These 2 subtests failed with the same assert. qa/standalone/scrub/osd-scrub-test.sh TEST_deep_scrub_abort |
Contributor
Author
Thanks. Removed the lock by mistake. |
Contributor
|
The following test using b22a806060c3ffdffa4e085cb0eb75e749947e9e PASSED! I'll run all standalone tests now. ../qa/run-standalone.sh scrub |
Objectives:
- identify and extract scrub code, as a first step into sharing scrub code between classic
and Crimson implementations;
- refactor the code as needed for clarity;
Main changes:
- PgScrubber (pg_scrubber.{h,cc}) implements the scrub code that resided in PG.cc. And:
- PrimaryLogScrub.{h,cc} now implement the parts of the scrub code that resided in PrimaryLogPG.
(note that I didn't see much point in the separation, but changing that was outside the scope of this PR)
- The flags that govern scrub scheduling/operation were separated into two structures: one is maintained
by the PG, and details the "next scrub" parameters. The other is PgScrubber-owned, and determines the
operation options for the running scrub. This collection of flags is set (and is "frozen") at
PG:queue_scrub(), following the logic of the existing code.
- the scrubbing is now implemented as an explicit state-machine.
- and the events that trigger state changes are now translated into separate specific messages, that
are initiated using specific calls;
- the priorities used when queuing and handling those events were reworked, trying to match the original
semantics.
Design decisions:
- the new scrub FSM (in scrub_machine.*) does not contain PG-specific code. All code that requires knowledge
of the PG internals (or PrimaryLogPG internals) is in PgScrubber & PrimaryLogScrub. That, to achieve:
- easy switching from (the slow) boost::state_chart to something better (I promised the SML author to give
it a second try, after he has incorporated some needed changes);
- ease of adaptation to Crimson;
- clearer separation of concerns.
Not done yet:
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
a set of shards" Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Objectives:
- identify and extract scrub code, as a first step into sharing scrub code between classic
and Crimson implementations;
- refactor the code as needed for clarity;
Main changes:
- PgScrubber (pg_scrubber.{h,cc}) implements the scrub code that resided in PG.cc. And:
- PrimaryLogScrub.{h,cc} now implement the parts of the scrub code that resided in PrimaryLogPG.
(note that I didn't see much point in the separation, but changing that was outside the scope of this PR)
- The flags that govern scrub scheduling/operation were separated into two structures: one is maintained
by the PG, and details the "next scrub" parameters. The other is PgScrubber-owned, and determines the
operation options for the running scrub. This collection of flags is set (and is "frozen") at
PG:queue_scrub(), following the logic of the existing code.
- the scrubbing is now implemented as an explicit state-machine.
- and the events that trigger state changes are now translated into separate specific messages, that
are initiated using specific calls;
- the priorities used when queueing and handling those events were reworked, trying to match the original
semantics.
- Scrub-related resources (the OSD-owned 'local' and 'remote' scrub resources) are managed using RAII
wrappers, hopefully guaranteeing no leakage.
Design decisions:
- the new scrub FSM (in scrub_machine.*) does not contain PG-specific code. All code that requires knowledge
of the PG internals (or PrimaryLogPG internals) is in PgScrubber & PrimaryLogScrub. That, to achieve:
- easy switching from (the slow) boost::state_chart to something better (I promised the SML author to give
it a second try, after he has incorporated some needed changes);
- ease of adaptation to Crimson;
- clearer separation of concerns.
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
object that manages the status of all maps. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
- static anal. warnings regarding state-chart post-event() of non-heap allocated events
- some scrubbing state transitions, esp re error events
- a temporary fix to osd-scrub-test.sh (the problem encountered is this:
when testing 'deep-scrub abort', once the nodeepscrub conf is released - the
OSD might immediately schedule the regular (non-deep) scrub on that PG.
The test is not expecting that.
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Clarifying the distinction between the two "scrub_requested()" flavours: the "external" one, triggered by a command, vs the 'auto' request that may be triggered at scrub termination (and which sets 'need_auto'). That one is internal to the PgScrubber object. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
of scrub flags when scrubbing after recovery. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
- refactoring sched_scrub_initial() (now - verify_scrub_mode()) - cosmetics Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Also - multiple minor changes following code-review comments. Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
…ions Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
…maps() and multiple minor logging/formatting changes Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
(and some minor formatting) Signed-off-by: Ronen Friedman <rfriedma@redhat.com>
Member
|
superseded by #38111 |
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.
DNM as:
Objectives: