mds: Fix incorrect Importer MDS recovery during EImportStart journal replay by sidharthanup · Pull Request #36227 · ceph/ceph

sidharthanup · 2020-07-21T13:15:38Z

Signed-off-by: Sidharth Anupkrishnan sanupkri@redhat.com
Fixes: https://tracker.ceph.com/issues/46535

Checklist

References tracker ticket
Updates documentation if necessary
Includes tests for new functionality or reproducer for bug

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 backend
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox

ukernel · 2020-07-21T13:25:28Z

src/mds/Server.cc

 		      << " seconds during MDS startup";

-    if (g_conf()->mds_session_blacklist_on_timeout) {
+    if (g_conf()->mds_session_blacklist_on_timeout && !session->is_importing()) {


I don't think this works

Why not? In Server::prepare_force_open_sessions(), the count gets incremented (https://github.com/sidharthanup/ceph/blob/wip-fix-import-recovery/src/mds/Server.cc#L937) . Then this session is persisted in EImportStart. During replay when sessionmap.replay_open_sessions() is called this information is relayed and then Server::reconnect_tick is going to see this and kill it instead of blacklisting.

import_count is always 0 after log replay.

Ack. Thanks @ukernel. I have updated the code to set a flag on EImportStart replay to avoid blacklisting it. Yet to test it but don't you think this should work?

ukernel · 2020-07-29T14:25:24Z

src/mds/Server.cc

 		      << " seconds during MDS startup";

-    if (g_conf()->mds_session_blacklist_on_timeout) {
+    if (g_conf()->mds_session_blacklist_on_timeout && !session->is_import_replayed()) {


for this implementation, any session opened by replay_open_sessions will not get blacklisteded. It's not good.

It will not get blacklisted only when replay_open_sessions is called in EImportStart journal replay: https://github.com/ceph/ceph/pull/36227/files#diff-e18d1bb1d5f6388430b7705cf32eac6dR2991, due to import_replayed boolean being set to true. In this scenario either 2 things happen: the session was not actually opened by the client (before the MDS failed) and so it gets killed (instead of blacklisted) during recconnect, or if the client had opened the session before the MDS went down then during reconnect the connection is established again and the boolean is set back to false: https://github.com/ceph/ceph/pull/36227/files#diff-eecf840e09d7481f7b8f415789852c7dR1501.

Is there an issue that you see with this approach?

needs to handle the case that client has actually opened session before MDS went down and the client failed to reconnect

@ukernel Tried working on this today but I'm still not sure how to handle the above case (the session still gets killed, but not blacklisted, which is what I think you mean should happen). Problem is the journalling for the opening of connection from the MDS side (ESession) comes after EImportStart is journalled. Now during failover journal replay, the MDS at EImportStart replay does not have any information on whether the session was really opened by the client or not. That information is in the next ESession replay. Any thoughts on how to handle this? I may be missing something here.

for any session opened by EImportStart, set a client_not_open flag. If ESession is countered later, clear the flag. If client does not reconnect to session with the flag, just kill the session (don't blacklist)

@ukernel Thanks! I missed your comment before. Clearing the flag on ESession seems like a good fix. I have updated the code.

batrick · 2020-08-18T18:07:47Z

status on this?

sidharthanup · 2020-08-20T14:08:16Z

Will update by the weekend. Was slightly busy with the other pending work.

sidharthanup · 2020-09-22T12:28:43Z

@ukernel @batrick PTAL.

src/mds/SessionMap.cc

ukernel · 2020-09-22T13:21:36Z

src/mds/journal.cc

      session = mds->sessionmap.get_or_add_session(client_inst);
      mds->sessionmap.set_state(session, Session::STATE_OPEN);
      session->set_client_metadata(client_metadata);
+      session->set_client_not_open(false);


no code submits the ESession event

Here It's just clearing the client_not_open flag set during EImportStart replay. Meaning the client had actually opened the connection and so we don't need this flag to be set. So during reconnect if the flag is set and the client failed to reconnect, then we kill the session and if it is not set and client failed to reconnect, then blacklist.

check Server::handle_client_session(). if session is already being opened by Server::prepare_force_open_sessions. it does not submit ESession event

stale · 2020-12-04T22:03:02Z

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.

batrick · 2020-12-04T23:56:05Z

@sidharthanup status?

sidharthanup · 2021-02-18T14:04:44Z

jenkins test make check

github-actions · 2021-03-05T19:48:31Z

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

sidharthanup · 2021-04-21T18:22:02Z

jenkins test make check

…replay https://tracker.ceph.com/issues/46535 Signed-off-by: Sidharth Anupkrishnan <sanupkri@redhat.com>

stale · 2021-07-21T03:00:31Z

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 · 2022-04-25T12:41:13Z

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

github-actions · 2022-09-13T16:02:15Z

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 · 2022-10-28T21:11:00Z

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!

sidharthanup self-assigned this Jul 21, 2020

sidharthanup added the cephfs Ceph File System label Jul 21, 2020

sidharthanup requested review from batrick and ukernel July 21, 2020 13:16

ukernel reviewed Jul 21, 2020

View reviewed changes

sidharthanup added the DNM label Jul 21, 2020

sidharthanup force-pushed the wip-fix-import-recovery branch from 26444a4 to 13a4197 Compare July 28, 2020 12:14

ukernel reviewed Jul 29, 2020

View reviewed changes

sidharthanup force-pushed the wip-fix-import-recovery branch 2 times, most recently from 60467f2 to 5c807f9 Compare September 22, 2020 12:25

sidharthanup force-pushed the wip-fix-import-recovery branch from 5c807f9 to 775e281 Compare September 22, 2020 12:33

ukernel reviewed Sep 22, 2020

View reviewed changes

src/mds/SessionMap.cc Show resolved Hide resolved

ukernel reviewed Sep 22, 2020

View reviewed changes

sidharthanup force-pushed the wip-fix-import-recovery branch from 775e281 to c860bc5 Compare September 22, 2020 16:44

stale bot added the stale label Dec 4, 2020

stale bot removed the stale label Dec 4, 2020

sidharthanup force-pushed the wip-fix-import-recovery branch from c860bc5 to b5bf0a6 Compare January 27, 2021 08:54

sidharthanup removed the DNM label Jan 27, 2021

sidharthanup requested a review from a team January 27, 2021 08:56

sidharthanup force-pushed the wip-fix-import-recovery branch from b5bf0a6 to 240bfbd Compare January 27, 2021 11:02

github-actions bot added the needs-rebase label Mar 5, 2021

sidharthanup force-pushed the wip-fix-import-recovery branch from 240bfbd to 371abc4 Compare April 14, 2021 10:38

github-actions bot removed the needs-rebase label Apr 14, 2021

sidharthanup force-pushed the wip-fix-import-recovery branch 2 times, most recently from 7ff106a to 06e120e Compare April 22, 2021 13:03

mds: Fix incorrect Importer MDS recovery during EImportStart journal …

cab9792

…replay https://tracker.ceph.com/issues/46535 Signed-off-by: Sidharth Anupkrishnan <sanupkri@redhat.com>

sidharthanup force-pushed the wip-fix-import-recovery branch from 06e120e to cab9792 Compare April 22, 2021 13:54

batrick mentioned this pull request Apr 26, 2021

qa: enable MDS export killpoint tests #28004

Closed

3 tasks

stale bot added the stale label Jul 21, 2021

github-actions bot added the needs-rebase label Apr 25, 2022

djgalloway changed the base branch from master to main July 8, 2022 00:00

github-actions bot removed the stale label Jul 15, 2022

github-actions bot added the stale label Sep 13, 2022

github-actions bot closed this Oct 28, 2022

Conversation

sidharthanup commented Jul 21, 2020

Checklist

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!

Choose a reason for hiding this comment

Uh oh!

sidharthanup Aug 26, 2020 • 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!

ukernel Aug 28, 2020 • 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!

batrick commented Aug 18, 2020

Uh oh!

sidharthanup commented Aug 20, 2020

Uh oh!

sidharthanup commented Sep 22, 2020

Uh oh!

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!

stale bot commented Dec 4, 2020

Uh oh!

batrick commented Dec 4, 2020

Uh oh!

sidharthanup commented Feb 18, 2021

Uh oh!

github-actions bot commented Mar 5, 2021

Uh oh!

sidharthanup commented Apr 21, 2021

Uh oh!

stale bot commented Jul 21, 2021

Uh oh!

github-actions bot commented Apr 25, 2022

Uh oh!

github-actions bot commented Sep 13, 2022

Uh oh!

github-actions bot commented Oct 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sidharthanup Aug 26, 2020 •

edited

Loading

ukernel Aug 28, 2020 •

edited

Loading