mon: more aggressively convert crush rulesets -> distinct rules #17508

liewegas · 2017-09-05T22:48:45Z

The existing mon code only makes a half-hearted attempt to adjust crush rules so that ruleset == rule id. This PR

renumbers as needed so that they always match
improves validation on injected crush maps

Mimic will eventually remove the rule mask crap from the decompiled crush maps with the addition of a tunable flag, but it's too late to do that in luminous. Instead, let's just make sure that luminous clusters normalize the ruleset/ruleid business completely.

liewegas · 2017-09-06T18:43:15Z

http://pulpito.ceph.com/sage-2017-09-06_14:17:17-rados-wip-sage-testing-2017-09-05-2251-distro-basic-smithi/

tchaikov · 2017-09-07T02:23:53Z

src/mon/OSDMonitor.cc

+    // Now, go ahead and renumber all the rules so that their
+    // rule_id field corresponds to their position in the array
+    auto old_to_new = newcrush.renumber_rules();
+    dout(1) << __func__ << "Rewrote " << old_to_new << " crush IDs:" << dendl;


nit, add a space before "Rewrote".

tchaikov · 2017-09-07T02:24:15Z

src/crush/CrushWrapper.h

+   * Return a map of old ID -> new ID.  Caller must update OSDMap
+   * to use new IDs.
+   */
+  std::map<int, int> renumber_rules();


nit, make this method a const.

it's not const.. it modifies the ruleset field in the mask

tchaikov · 2017-09-07T02:47:34Z

src/osd/OSDMap.cc

+    auto& pool = i.second;
+    int ruleno = pool.get_crush_rule();
+    if (!newcrush->rule_exists(ruleno)) {
+      if (ss)


ss is always valid, right?

tchaikov · 2017-09-07T02:52:05Z

src/osd/OSDMap.cc

+    if (newcrush->get_rule_mask_ruleset(ruleno) != ruleno) {
+      if (ss)
+	*ss << "rule " << ruleno << " mask ruleset does not match rule id";
+      return -EINVAL;


can we also check

if (!newcrush->check_crush_rule(ruleno, pool.get_type(), pool.get_size(), *ss)) { return -EINVAL; }

? so we can verify the type and size range of the rule are sane.

but we might need to update check_crush_rule(), so it prints out the ruleid in the error message.

updated = validate to make sure min/max size are ok

gregsfortytwo

I haven't followed along in the earlier drafts here.

Have we already made it impossible for admins to have set multiple rules to a ruleset?
Is there anything we can do about existing ceph-external databases referencing rules by the wrong number, once renumbered? If we're just doing it automatically that seems a bit cruel (especially if we want to backport this to Luminous in a point release).

gregsfortytwo · 2017-09-07T14:14:49Z

src/mon/OSDMonitor.cc

+
+  // Rewrite CRUSH rule IDs if they are using legacy "ruleset"
+  // structure.  Only do this if there is no pending crush map or pool
+  // creations/deletions/updates and we don't have to worry about


There doesn't seem to be any logic enforcing this constraint that we not have other stuff going on?

oops, that comment is obsolete from when this code was elsewehre. it's now in create_pending(), so by definition there is nothing (yet) going on. fixed

liewegas · 2017-09-07T14:30:24Z

Before we only fixed the rulesets in the trivial case; this patch does it for any cluster (by also fixing up pg_pool_t::crush_rule as needed). So now we will prevent multiple rules in a ruleset.

I think kraken had a weak guard to steer users away from doing that, but notably jewel allowed it.

What kind of external database do you have in mind? tendrl or something? The "good news" is that we've also changed the interface for setting this (ceph osd pool set $pool crush_rule ... instead of crush_ruleset), and the prevent any incoming crush map from doing anything untoward, so I don't think an unmodified external thing could accidentally do the wrong thing here (it would fail outright instead).

liewegas · 2017-09-07T14:31:41Z

Also notably I guess 12.2.0 doesn't allow these to be created so this conversion won't trigger for earlier luminous folks.. it will only trigger for clusters that upgraded from an older version.

tchaikov · 2017-09-07T16:28:06Z

src/osd/OSDMap.cc

+	  << " fall within rule " << ruleno
+	  << " min_size " << newcrush->get_rule_mask_min_size(ruleno)
+	  << " and max_size " << newcrush->get_rule_mask_max_size(ruleno);
+      return -EINVAL;


can we check poo's type against rule's type as well?

good call, fixed!

gregsfortytwo · 2017-09-07T19:50:04Z

Before we only fixed the rulesets in the trivial case; this patch does it for any cluster (by also fixing up pg_pool_t::crush_rule as needed). So now we will prevent multiple rules in a ruleset.

Do we know anything about the ordering on existing rules, though? Maybe there aren't actually any clusters with multiple rules in a ruleset, but if there are I presume this is not going to play nicely for them as we forcibly renumber stuff...?

What kind of external database do you have in mind? tendrl or something? The "good news" is that we've also changed the interface for setting this (ceph osd pool set $pool crush_rule ... instead of crush_ruleset), and the prevent any incoming crush map from doing anything untoward, so I don't think an unmodified external thing could accidentally do the wrong thing here (it would fail outright instead).

It doesn't need to be anything as sophisticated as tendrl if they've got cookbooks or playbooks that automatically set up new pools when hardware pods are added or something.

liewegas · 2017-09-07T19:54:38Z

Can you explain what might not play nicely? We're renumbering the ruleset field to match the rule id, and then changing each pool to make sure it maps to the new ruleset (and ruleid) that it used before. No net change except the ruleset ids are different.

Any external tool that was fiddling with ruleset before will fail and need changes because the crush_ruleset pool property is now called crush_rule, so I don't think we need to worry about misbehavior there.

gregsfortytwo · 2017-09-07T23:54:04Z

Ah okay, I don't think I don't actually know how the IDs associate with rules and rule sets. Was concerned we could go from having e.g. rules 1,3,5 to 1,2,3.
(I told you I hadn't followed along with the earlier crush work!)

jcsp · 2017-09-11T10:17:44Z

src/mon/OSDMonitor.cc

+	      << osdmap.get_pool_name(pool_id) << " crush ruleset "
+	      << pool.crush_rule << " -> rule id " << new_rule_id << dendl;
+      pg_pool_t &updated = pending_inc.new_pools[pool_id];
+      updated = pool;


I think this comment still applies? https://github.com/ceph/ceph/pull/17255/files#r135267195

Probably.. i was focusing just on the bit that I'd like to get backported to luminous.

Signed-off-by: John Spray <john.spray@redhat.com>

Previously, the rules were only modified in the trivial case, so we continued to potentially have CRUSH maps with the legacy ruleset functionality in use. In order to ultimately remove rulesets entirely, we need to do this more aggressively, renumbering all the rules and then updating any pools as needed. Signed-off-by: John Spray <john.spray@redhat.com>

- move into OSDMap method - ensure that rules exist for each pool - ensure pool type matches rule type - ensure rule mask min/max size cover the pool size Signed-off-by: Sage Weil <sage@redhat.com>

liewegas · 2017-09-15T20:35:43Z

http://pulpito.ceph.com/sage-2017-09-15_15:50:19-rados-wip-sage-testing2-2017-09-14-1256-distro-basic-smithi/

liewegas · 2017-09-15T20:36:08Z

I think this is ready to merge; final review please?

liewegas added cleanup mon needs-backport labels Sep 5, 2017

liewegas requested review from gregsfortytwo and jcsp September 5, 2017 22:53

liewegas force-pushed the wip-crushrule-convert branch from ab535b0 to 5dc3ab0 Compare September 6, 2017 02:48

liewegas added the wip-sage-testing label Sep 6, 2017

liewegas added needs-review and removed wip-sage-testing labels Sep 6, 2017

tchaikov reviewed Sep 7, 2017

View reviewed changes

liewegas force-pushed the wip-crushrule-convert branch from 5dc3ab0 to f382847 Compare September 7, 2017 13:45

gregsfortytwo reviewed Sep 7, 2017

View reviewed changes

liewegas force-pushed the wip-crushrule-convert branch from f382847 to a4c4978 Compare September 7, 2017 14:30

tchaikov reviewed Sep 7, 2017

View reviewed changes

liewegas force-pushed the wip-crushrule-convert branch 2 times, most recently from 59ef476 to 5f4c8fc Compare September 7, 2017 19:08

liewegas added the wip-sage2-testing label Sep 7, 2017

jcsp reviewed Sep 11, 2017

View reviewed changes

liewegas force-pushed the wip-crushrule-convert branch from 5f4c8fc to 337975c Compare September 14, 2017 13:53

liewegas added needs-qa wip-sage2-testing and removed wip-sage2-testing labels Sep 14, 2017

John Spray added 2 commits September 14, 2017 12:30

osd: s/crush_ruleset_in_use/crush_rule_in_use/

675483e

Signed-off-by: John Spray <john.spray@redhat.com>

liewegas force-pushed the wip-crushrule-convert branch from 337975c to 3823fc6 Compare September 14, 2017 16:30

mon/OSDMonitor: improve crush map validation

8f8f0b2

- move into OSDMap method - ensure that rules exist for each pool - ensure pool type matches rule type - ensure rule mask min/max size cover the pool size Signed-off-by: Sage Weil <sage@redhat.com>

liewegas force-pushed the wip-crushrule-convert branch from 3823fc6 to 8f8f0b2 Compare September 14, 2017 16:31

liewegas removed needs-qa wip-sage2-testing labels Sep 15, 2017

tchaikov self-requested a review September 16, 2017 02:35

tchaikov approved these changes Sep 19, 2017

View reviewed changes

tchaikov removed the needs-review label Sep 19, 2017

tchaikov merged commit f59dc0a into ceph:master Sep 19, 2017

tchaikov mentioned this pull request Sep 19, 2017

luminous: mon: more aggressively convert crush rulesets -> distinct rules #17805

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mon: more aggressively convert crush rulesets -> distinct rules #17508

mon: more aggressively convert crush rulesets -> distinct rules #17508

liewegas commented Sep 5, 2017 •

edited

liewegas commented Sep 6, 2017

tchaikov Sep 7, 2017

liewegas Sep 7, 2017

tchaikov Sep 7, 2017

liewegas Sep 7, 2017

tchaikov Sep 7, 2017

liewegas Sep 7, 2017

tchaikov Sep 7, 2017

liewegas Sep 7, 2017

gregsfortytwo left a comment

gregsfortytwo Sep 7, 2017

liewegas Sep 7, 2017 •

edited

liewegas commented Sep 7, 2017

liewegas commented Sep 7, 2017

tchaikov Sep 7, 2017

liewegas Sep 7, 2017

gregsfortytwo commented Sep 7, 2017

liewegas commented Sep 7, 2017

gregsfortytwo commented Sep 7, 2017

jcsp Sep 11, 2017

liewegas Sep 11, 2017

liewegas commented Sep 15, 2017

liewegas commented Sep 15, 2017

mon: more aggressively convert crush rulesets -> distinct rules #17508

mon: more aggressively convert crush rulesets -> distinct rules #17508

Conversation

liewegas commented Sep 5, 2017 • edited

liewegas commented Sep 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsfortytwo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas Sep 7, 2017 • edited

Choose a reason for hiding this comment

liewegas commented Sep 7, 2017

liewegas commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gregsfortytwo commented Sep 7, 2017

liewegas commented Sep 7, 2017

gregsfortytwo commented Sep 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liewegas commented Sep 15, 2017

liewegas commented Sep 15, 2017

liewegas commented Sep 5, 2017 •

edited

liewegas Sep 7, 2017 •

edited