Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mon: remove osd_epoch to avoid out-dated osdmap_cache #5315

Merged
merged 3 commits into from Aug 21, 2015

Conversation

tchaikov
Copy link
Contributor

  • replace the OSDMonitor::osd_epoch with the MonSession::osd_epoch
  • apply this optimisation to all MonClients
  • remove the duplicated code of the two OSDMonitor::send_incremental()

due to the refactory, one of the OSDMonitor::send_incremental() is not able to mark op events anymore.

FIxes: #10930 http://tracker.ceph.com/issues/10930

@tchaikov
Copy link
Contributor Author

PASS: unittest_erasure_code_shec_all
+ test 124 = 124
+ display_logs ../../ceph-centos-7-jenkins
+ local dir=../../ceph-centos-7-jenkins
+ xargs grep -l FAIL
+ find ../../ceph-centos-7-jenkins -name '*.trs'
+ read file
+ echo abort by timeout after 1h
abort by timeout after 1h

the test failed due to timeout. will rebase and repush to see if it's a false negative.

@ghost
Copy link

ghost commented Jul 22, 2015

@tchaikov thanks for repushing. I'm a little concerned, this is the second timeout run in 24h. I'll investigate.

@liewegas
Copy link
Member

liewegas commented Aug 4, 2015

This looks great. I think the thing to watch out for is when we need an older osd map before s->osd_epoch.

  • preprocess_get_osdmap() is used if we get a bad crc and want an old map. This is untouched by this patch and doesn't use or modify s->osd_epock. Ok.
  • We set s->osd_epoch to some new map in a lazy fashion (there are a few paths that just send the latest incremental to make sure the client knows there is a new map), but the osd goes back and wants earlier ones. In this case, Objecter will send a subscribe message to an earlier map. I think this is broken. I suggest
  1. we consistently make send_incremental turn into a no-op if s->osd_epoch says they have it
  2. in check_sub(), we reset s->osd_epoch back in time if they requested an older map.

What do you think?

* remove osd_epoch<osd, epoch> from OSDMonitor
* add osd_epoch to MonSession to track the latest osdmap epoch
  OSDMonitor sends to a mon client
* do not remove osd_epoch entries if an OSD is down, or
  max_osd > osd_id

Fixes: ceph#10930
Signed-off-by: Kefu Chai <kchai@redhat.com>
previously, we only track the osd_epoch for OSD peers. but other
MonClients who receives osdmaps can also benefit from osd_epoch.

Signed-off-by: Kefu Chai <kchai@redhat.com>
@tchaikov
Copy link
Contributor Author

@liewegas i rethought the earlier-osdmap problem:

in check_sub(), we reset s->osd_epoch back in time if they requested an older map.

we'd better reset s->osd_epoch when session_map.add_update_sub(). if we reset it afterwards, chances are that we've sent some osdmaps to the subscriber over this session already, and after resetting s->osd_epoch, these osdmaps will be re-sent.

i was thinking if we should rename osd_epoch into Subscription instead, this sounds more natural to me. but seems we can't. as we have two client types here 1) dedicated subscribers 2) other clients should be updated with latest osdmaps. in the later case, the session is not associated with a Subscription.

* remove the duplicated part of the two implementations of
  OSDMonitor::send_incremental()

Signed-off-by: Kefu Chai <kchai@redhat.com>
@liewegas
Copy link
Member

Yeah, I think s->osd_epoch is fine. The current patches look right to me...

@tchaikov
Copy link
Contributor Author

thanks for your review, sage. will merge it after the mon:thrash qa run.

@ghost
Copy link

ghost commented Aug 12, 2015

@tchaikov please ignore the bot false negative, it is being fixed by @dillaman at http://tracker.ceph.com/issues/12664

@liewegas liewegas added this to the infernalis milestone Aug 12, 2015
@tchaikov
Copy link
Contributor Author

@liewegas
Copy link
Member

@tchaikov this passed my wip-sage-testing run. we can wait for yours to go as well or just merge, up to you!

@tchaikov
Copy link
Contributor Author

@liewegas thanks for testing. the monthrash tests are also green. merging.

tchaikov added a commit that referenced this pull request Aug 21, 2015
mon: remove osd_epoch to avoid out-dated osdmap_cache

Reviewed-by: Sage Weil <sage@redhat.com>
@tchaikov tchaikov merged commit b33209b into ceph:master Aug 21, 2015
@tchaikov tchaikov assigned liewegas and unassigned tchaikov Aug 21, 2015
@tchaikov tchaikov deleted the wip-10930 branch August 21, 2015 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants