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: fix commands advertised during mon cluster upgrade #16871

Merged
merged 4 commits into from Aug 7, 2017

Conversation

liewegas
Copy link
Member

@liewegas liewegas commented Aug 7, 2017

While we have mixed mons we need to advertise the right set of commands
so that thing in PGmonitor and later mgr will be usable.

@liewegas
Copy link
Member Author

liewegas commented Aug 7, 2017

I'm a bit conflicted here. We didn't have the PGMonitorCommands explicitly broken out before.. it seems like it might be better to advertise and use those until the require luminous bit is set (and then flip over to mgr mode) but that's a slightly bigger change.

@liewegas liewegas removed this from the luminous milestone Aug 7, 2017
@liewegas
Copy link
Member Author

liewegas commented Aug 7, 2017

http://pulpito.ceph.com/sage-2017-08-07_15:29:52-upgrade:jewel-x-wip-20920-distro-basic-smithi/

looks much better (altho there are still 2 tests going). cls_rgw is unrelated, and the feature bit failure i'm fixing in a separate pr.

Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

This is fine, just one real question. I was left a little confused by the first commit message ("We also drop the win_election command args because when we win we get to set the leader commands to our commands") but the code is good.

// determine whether we are a dumpling mon due to some crufty old
// code. It only needs to see this buffer non-empty, so put
// something useless there.
m->sharing_bl.append("I am not dumpling");
Copy link
Member

Choose a reason for hiding this comment

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

Is there a point to this? The bandwidth savings hardly seems worth the slight risk of change.

Copy link
Member Author

Choose a reason for hiding this comment

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

I originally removed it because the command list is a function of which mon features the quorum has, and we don't know that here. But then I added this back because then jewel thought we were dumpling mons.

Copy link
Member Author

Choose a reason for hiding this comment

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

I can go back to hard coding the old command set here for defensive purposes.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, I'm good with whatever you decide.


/* no guard; may be included multiple times */

// see MonCommands.h
Copy link
Member

Choose a reason for hiding this comment

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

I assume this file was created by copying from somewhere?

The old code was pretty messy. This is standardizes on std::vector
throughout.  We also drop the win_election command args because
when we win an election we always set the leader commands to our
commands, and we can do that inside win_command() without passing
them in from here.

Signed-off-by: Sage Weil <sage@redhat.com>
While we have a mixed version cluster, we have to advertise our
PGMonitor commands to our peons or else commands like 'pg dump'
won't work.

Once the mon feature flag is set, we can drop that because each
mon will include the mgr commands (either those stored in paxos
or the statically compiled ones until that point).

Signed-off-by: Sage Weil <sage@redhat.com>
It's deprecated.

Also, this avoids a dup when we have an upgrading mon cluster
and it's also in PGMonitorCommands.

Signed-off-by: Sage Weil <sage@redhat.com>
This code runs on the mgr.

Signed-off-by: Sage Weil <sage@redhat.com>
Copy link
Member

@gregsfortytwo gregsfortytwo left a comment

Choose a reason for hiding this comment

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

Reviewed-by: Greg Farnum gfarnum@redhat.com

@liewegas liewegas merged commit 973772c into ceph:master Aug 7, 2017
@liewegas liewegas deleted the wip-20920 branch August 7, 2017 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants