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: track features from connect clients, and use it to gate set-require-min-compat-client #15371

Merged
merged 6 commits into from Jun 7, 2017

Conversation

Projects
None yet
3 participants
@liewegas
Member

liewegas commented May 30, 2017

As an aside, I kind of hate the 'ceph osd set-require-min-compat-client' naming. Any better ideas?

@liewegas liewegas changed the title from mon: track features from connect clients, and use it to gate set-require-min-compat-client to DNM mon: track features from connect clients, and use it to gate set-require-min-compat-client May 30, 2017

liewegas added some commits May 30, 2017

mon/mon_types: move C_MonOp to MonOpRequest.h
Fixes a circular #include of
 mon_types -> MonOpRequest -> Session -> mon_types

Signed-off-by: Sage Weil <sage@redhat.com>
mon: track connected mon client features
Track counts of entities by their supported features.

Signed-off-by: Sage Weil <sage@redhat.com>
mon: share connected features with leader
Signed-off-by: Sage Weil <sage@redhat.com>

@liewegas liewegas changed the title from DNM mon: track features from connect clients, and use it to gate set-require-min-compat-client to mon: track features from connect clients, and use it to gate set-require-min-compat-client Jun 1, 2017

@liewegas liewegas requested review from jdurgin, tchaikov and jecluis Jun 1, 2017

@@ -206,6 +206,8 @@ COMMAND("df name=detail,type=CephChoices,strings=detail,req=false", \
COMMAND("report name=tags,type=CephString,n=N,req=false", \
"report full status of cluster, optional title tag strings", \
"mon", "r", "cli,rest")
COMMAND("features", "report of connected featuers", \

This comment has been minimized.

@jdurgin

jdurgin Jun 7, 2017

Member

typo: 'featuers'

liewegas added some commits May 30, 2017

mon: add 'features' command to show connected client features
for whole mon cluster

Signed-off-by: Sage Weil <sage@redhat.com>
common: ceph_release_[from_]features
Map releases to client features (and back again).

Signed-off-by: Sage Weil <sage@redhat.com>
mon/OSDMonitor: validate set-require-min-compat-client against connec…
…ted clients

Prevent the user from setting a require_min_compat_client if a disallowed
client is currently connected to the mon cluster.  Instead, you'll get an
error like

Error EPERM: cannot set require_min_compat_client to luminous: 1 connected client(s) look like jewel (missing 0x800000000200000); add --yes-i-really-mean-it to do it anyway

Signed-off-by: Sage Weil <sage@redhat.com>
@jdurgin

jdurgin approved these changes Jun 7, 2017

maybe set-min-client for a shorter command name?

@liewegas liewegas merged commit 94d17fb into ceph:master Jun 7, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
Details

@liewegas liewegas deleted the liewegas:wip-mon-client-features branch Jun 7, 2017

con(c), inst(i), closed(false), item(this),
con(c),
con_type(c->get_peer_type()),
con_features(0),

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

no need to initialize it with 0. we already do this in-class.

int ceph_release_from_features(uint64_t features)
{
int r = 1;
while (true) {

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

i am still not able to turn my head around this. i thought it should be something like:

for (int r = 1; r < CEPH_RELEASE_MAX; r++) {
  if (ceph_release_features(r) & features == features) {
    return r;
  }
}
return -1;

but i guess i am just wrong.

This comment has been minimized.

@liewegas

liewegas Jun 7, 2017

Member

I banged my head against this for more than an hour, and the problem is very subtle because there are some feature bits that are there to indicate that other bits have new meanings (the CEPH_FEATURE_INCARNATION_* bits), which makes the trivial solution not work. At the end of the day I tried 4 or 5 different appraoches and settled with this because the unit test worked (see unittest_features).

@@ -118,6 +121,9 @@ class MMonPaxos : public Message {
::decode(latest_version, p);
::decode(latest_value, p);
::decode(values, p);
if (header.version >= 4) {
::decode(feature_map, p);
}

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

could clear the feature_map if header.version < 4.

This comment has been minimized.

@liewegas

liewegas Jun 7, 2017

Member

it's decode() so we can assume it's prior state is MMonPaxos()

return;
}
if (!is_leader()) {
forward_request_leader(op);

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

i don't really understand why only the leader's featuremap matters?

the peon has its own session_map and keeps track of the quorum also. but i agree, its session_map is different from the leader's. the question is why we should use the leader's session_map but not the peon's?

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

scratch that, seems we are collecting all mons' featuremaps.

@@ -39,6 +40,8 @@ struct Subscription {
struct MonSession : public RefCountedObject {
ConnectionRef con;
int con_type = 0;

This comment has been minimized.

@tchaikov

tchaikov Jun 7, 2017

Contributor

can we use entity_type_t ?

@liewegas

This comment has been minimized.

Member

liewegas commented Jun 7, 2017

or set-min-compat-client?

The reason I added 'require' is because OSDMap::print/dump also report min_compat_client, which is the actual oldest client that can connect given the features used by the current osdmap and crush map. This setting, on the other hand, is advisory/policy and will prevent the admin from enabling a feature that breaks old clients.

Maybe

  • rename require_min_compat_client -> target_min_compat_client
  • rename min_compat_client -> actual_min_compat_client

?

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