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.

Sign up for GitHub

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

Jump to bottom

mon: remove pre-luminous compat cruft (2 of many) #17322

Merged
merged 38 commits into from Sep 7, 2017
Merged

mon: remove pre-luminous compat cruft (2 of many) #17322

merged 38 commits into from Sep 7, 2017

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Aug 28, 2017 •
edited

This rips out a ton of pre-luminous compat cruft from the monitor, mainly
include PGMonitor.

  • rebase on top of pt 1
  • update incompat flags in object corpus for PGMap[::Incremental]

@liewegas liewegas force-pushed the wip-post-luminous-mon branch 2 times, most recently from 9ea1782 to 6032ac0 Compare August 29, 2017 21:59
@tchaikov tchaikov self-requested a review August 30, 2017 14:59
@liewegas liewegas mentioned this pull request Aug 30, 2017
Merged
tchaikov
tchaikov reviewed Aug 31, 2017
View reviewed changes
src/mon/HealthMonitor.h Outdated

class HealthMonitor : public PaxosService
{
map<int,HealthService*> services;
version_t version = 0;
map<int,health_check_map_t> quorum_checks; // for each quorum member
health_check_map_t leader_checks; // leader only

public:
HealthMonitor(Monitor *m, Paxos *p, const string& service_name);
~HealthMonitor() override {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit. just remove this dtor.

src/mon/MonCommands.h
*/

// note: this should be replaced shortly!
COMMAND_WITH_FLAG("pg force_create_pg name=pgid,type=CephPgid", \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you remove the reference of force_create_pg, set_full_ratio and set_nearfull_ratio in ceph.rst as well?

src/mon/PGMap.cc Outdated
// or old mon running.
}
epoch_t e;
::decode(e, p);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no need to decode the epoch if we don't check it.

src/mon/PGMap.cc Outdated
@@ -936,85 +936,35 @@ void PGMap::get_rules_avail(const OSDMap& osdmap,
// ---------------------
// PGMap

/*
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why shall we keep this around instead of just yanking it?

src/mon/OSDMonitor.cc
@@ -9072,10 +9072,6 @@ bool OSDMonitor::prepare_command_impl(MonOpRequestRef op,
<< " < firefly, which is required for primary-temp";
err = -EPERM;
goto reply;
} else if (!g_conf->mon_osd_allow_primary_temp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are covered by require_osd_release in the OSDMap.

i understand that OSDs are required to be all luminous+ in post luminous. but how can we educate old clients to understand this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These options are superceded by the new osdmap require-min-compat-client which controls which newish incompat things are allowed to be used.

@liewegas liewegas removed the DNM label Sep 1, 2017
tchaikov
tchaikov reviewed Sep 1, 2017
View reviewed changes
qa/standalone/ceph-helpers.sh Outdated
fi
fi
if [ "$cores" = "yes" -o "$dumplogs" = "1" ]; then
if [ "$dumplogs" = "1" ]; then
Copy link
Contributor

@tchaikov tchaikov Sep 1, 2017 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the EXPECT_DEATH tests should not create cores. the last of them who were dumping cores were fixed by 6beaf0b.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas @tchaikov
I have not find the time to actually make the EXPECT_DEATH tests not dump cores.
So although I agree with you, factual this is not the case on FreeBSD.
Perhaps excluding *unittest* cores while grepping?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@liewegas @wjwithagen since #17447 has been merged. can we drop this commit?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tchaikov
I think you also have to ask @dzafman, since he is the original submitter of this part.
The problem with this fix I had is amended by #17447, because there are no more cores.
And I'm very much in favor of making dumplogs optional, since it really bloats output, also in cases where there are other means of capturing the logs..

tchaikov
tchaikov approved these changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@tchaikov tchaikov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aside from f7f4e8b, lgtm. i think the core check feature is still useful for us to check for regressions in ceph-helper based tests. and the false alarms should have been taken care of. so i guess it's safe to keep it?

@wjwithagen
Copy link
Contributor

@liewegas @tchaikov
Running this one thru jenkins/FreeBSB-master.

@wjwithagen wjwithagen self-requested a review September 6, 2017 12:11
wjwithagen
wjwithagen approved these changes Sep 6, 2017
View reviewed changes
Copy link
Contributor

@wjwithagen wjwithagen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

apart from f7f4e8b, which I needed to remove, it tested oke for FreeBSD. But that is cause of other patches.

@liewegas
mon: require SERVER_LUMINOUS from mons, osds
ac8548c 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: CEPH_MON_FEATURE_INCOMPAT_MIMIC
59325c4 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: remove legacy get_health infrastructure
729a089 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: remove OldHealthMonitor
7b4a741 
The only service this provided was mon space notifications, which
are now handled explicitly by the new HealthMonitor's
check_member_health(), and communited by the new MMonHealthChecks.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: remove PGMonitor
f85e097 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: remove some upgrade-specific bits of PGStatService
81bee4c 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: collapse MonPGStatService into PGStatService
cea8ca9 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: drop hide_mgr_flag behavior
c16566c 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: add MIMIC mon feature
d9cd2d7 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/OSDMonitor: drop legacy 'osd df'
c4728db 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: drop PGMapStatService
1dcab1c 
There is only one user and one implementation; this just makes the
code hard to read.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/PGMap: remove unused functions
ebe59db 
These are left over from health cleanup

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/PGMap: streamline encoding
ef5cd82 
The only user left is 'pg getmap' and ceph-dencoder; we can drop
the compat cruft.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/PGMap: remove legacy encode/decode methods
fced000 
These were used by PGMonitor when stored in per-pg and per-osd k/v
pairs.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
osd/OSDMap: remove unused static
3ac23d8 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/PGMap: remove old PGMapUpdater methods
4b3e399 
These were used by PGMonitor.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/Elector: drop pre-luminous elector compat
33606ae 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: remove mon_osd_allow_primary_{temp,affinity}
2587abc 
These are covered by require_osd_release in the OSDMap.

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon/OSDMonitor: encode without MIMIC if require_osd_release < mimic
7649062 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: do not join quorum unless luminous upgrade has completed
71d1dcb 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
ceph-object-corpus: PGMap encoding change
2cf04b1 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
doc/man/8/ceph: remove old 'ceph pg ' force_create_pg, set_[near]full…
e467873 
…_ratio

Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
test/osd/CMakeLists: fix test name
e840d38 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
doc/man/8/ceph.rst: remove pg set_*full_ratio
b198236 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
qa/workunits/cephtool/test.sh: drop 'pg set_full_ratio'
e21e63c 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
mon: s/mon_debug_no_require_luminous/mon_debug_no_require_mimic/
0761108 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
qa/standalone/mon/misc.sh: fix mon feature test
e2bc888 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

liewegas commented Sep 6, 2017

@tchaikov ok thanks, dropped that commit and rebased!

@dzafman
Copy link
Contributor

dzafman commented Sep 6, 2017 •
edited

@liewegas (re: f7f4e8b) We could just "rm -rf td" in run-standalone.sh to clear any previous testing using "td" the test directory. This would clear any cores generated by unit tests. Presumably, those tests aren't using run-standalone.sh because they would always fail. If you wanted to make them runnable with run-standalone, just preserve the core_pattern then set it to something that doesn't begin or end with "core". After the test runs set it back. This would only work as long as run-standalone continues to be single threaded.

@liewegas
qa/workunits/*: require-osd-release mimic, not luminous
e715533 
Signed-off-by: Sage Weil <sage@redhat.com>
@liewegas
Copy link
Member Author

liewegas commented Sep 7, 2017

http://pulpito.ceph.com/sage-2017-09-07_15:28:50-rados-wip-sage-testing2-2017-09-07-0919-distro-basic-smithi/

@liewegas liewegas merged commit 1006b62 into ceph:master Sep 7, 2017
@liewegas liewegas deleted the wip-post-luminous-mon branch September 7, 2017 18:47
@gregsfortytwo
Copy link
Member

-5k lines w00t!

@liewegas
Copy link
Member Author

liewegas commented Sep 7, 2017 via email

@renhwztetecs
Copy link
Contributor

renhwztetecs commented Sep 18, 2017 •
edited

@liewegas @tchaikov
whether we consider this part of the commit into the Luminous, because like c4728db can solve http://tracker.ceph.com/issues/21300.

@tchaikov
Copy link
Contributor

@renhwztetecs i am not sure that change is backward compatible. instead, i think we should fix http://tracker.ceph.com/issues/21300

@renhwztetecs
Copy link
Contributor

@tchaikov

  1. start Mon and Mgr
  2. stop Mgr
  3. stop Mon
  4. start Mon
  5. use command "ceph --verbose osd df", you can get Mon Coredump info

@tchaikov
Copy link
Contributor

@renhwztetecs thanks for the reproducer! will leave this to Joao.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tchaikov tchaikov tchaikov approved these changes

@wjwithagen wjwithagen wjwithagen approved these changes

Assignees
No one assigned
Labels
cleanup core mon needs-qa wip-sage2-testing
Projects
None yet
Milestone
No milestone
6 participants
@liewegas @wjwithagen @dzafman @gregsfortytwo @renhwztetecs @tchaikov