Ceph

Sage asked on irc about just setting it up on the initial create_empty. The problem with that the only data which is sent out to the other Monitors is the PGMap::Incremental, and (before this change) it's only filled in on changes.
We could special-case the first epoch, but that just seems like it's asking for trouble, and doesn't really help any.

pushed a patch that confines the logic of when to update this into a single bit of code. look okay?

i think the best solution might be to make this not a function of config (except maybe at mkfs time) at all, but to create an explicit mon command to do it. similar to the osd_pg_bits stuff. but i'm not sure it makes sense to go there yet until we clean up the mon interfaces some. this works for me, for now. what do you think?

I'm looking at your patch and it doesn't make a lot of sense to me.
First off, when do you think that peon monitors are going to get their ratios set upon creation?
Second, it looks like there's nothing preventing a peon from making a local change to ratios that doesn't get propagated (this may have been an issue before).
Third, what's the penalty involved with filling in the incremental's ratios every time? (Since you got rid of that.)

Greg Farnum wrote:

I'm looking at your patch and it doesn't make a lot of sense to me.
First off, when do you think that peon monitors are going to get their ratios set upon creation?

It'll be zero. The first master will notice it's zero and set it to something non-zero using the normal update mechanism.

Second, it looks like there's nothing preventing a peon from making a local change to ratios that doesn't get propagated (this may have been an issue before).

There is no such thing as a local update now.. the only way it changes is via the propose in tick()...?

Third, what's the penalty involved with filling in the incremental's ratios every time? (Since you got rid of that.)

When it changes you ahve to recalculate the full and nearfull sets.. definitely not something you want to every time.

We want this to behave like any other field in the map, with updates via the incremental. We want to confine any weirdness to the logic that causes a new inc proposal. Because the data isn't fed into mkfs, just have an early monitor fill it in using a normal update.

Oh, I see...I wasn't following that need_*_ratio_update stuff properly. And update_full_ratios() will be called on the initial startup, right?

You'll notice that I set up the apply_incremental() to only actually change the map's ratios if they differ from what's in the incremental. ;)

Anyway, this patch is a mostly orthogonal solution compared to mine. We should probably just make one that inits everything to zeros and does that check in update_from_paxos().

yeah. actually, i think the check should go in tick() inside the is_leader() block, and not update_from_paxos().

Greg Farnum wrote:

Hmm. I looked at redoing this and got stuck on the semantics we want. If we're interested in full_ratio == 0 being an off switch, we need to be able to propagate that — which means that whenever the incremental's ratios don't match the current map's ratios, we need to take the Incremental's (including for 0, implying we fill it in all the time). I think this is fine (see my first patch) but you didn't seem to like the idea.

i think it's okay, now that i see the sets are recalculated, although:

There's another problem with our current setup, which is that right now I don't think we can avoid reverting a change somebody's made online if they don't put it into their config. So I think we should divorce the post-creation changes from g_conf's settings. But I think we can accomplish the first version with just 3ec6425f412a802ccd3ab3fd2fb4d5f3d35d6c94 and 983ab06776168031dd75800e4549f5fae9b2b149. (I just don't see that your middle commit gets us anything, and in fact I think the current use of need_full_ratio_update could be greatly simplified.)

yeah, i think that's the right thing to do, and we may as well do it now. which means we could do:

- if it's 0, then set it to g_conf.
- otherwise, only change it via a mon command (not g_conf)

then we don't have to set it every time. or maybe we should anyway... but i'm still hesitant to do that if only because it diverges from the usual pattern of optional updates for everything in the incremental.

Okay, pretty sure this is a result of a pre-upgrade PGMap::Incremental being committed by post-upgrade code. Sage pointed out we can fix this by changing the encoding version and doing a selective interpretation on it, so I'll do that and push a fix.