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

crush: encode can override weights with weight set #15002

Merged
merged 2 commits into from May 17, 2017

Conversation

Projects
None yet
2 participants
@ghost

ghost commented May 8, 2017

Encode a "legacy" crushmap if (1) we're using weight sets, (2) the
client is lacking features for the weight set, but (3) the weight set
has a single position and no id remapping. Since these maps are only
used by clients (not humans), then there is no need to preserve the
original crush weights. We can just swap them for the real weights and
the legacy clients will behave as expected.

Fixes: http://tracker.ceph.com/issues/19836

Signed-off-by: Loic Dachary loic@dachary.org

@ghost

This comment has been minimized.

ghost commented May 8, 2017

@liewegas it's slightly different from what you suggested in #14959. If that's ok with you I'll add tests.

@@ -1370,6 +1370,33 @@ int CrushWrapper::rebuild_roots_with_classes()
return trim_roots_with_class(true);
}
void CrushWrapper::get_incompat_choose_args(std::map<__u32, crush_choose_arg*> &i2choose_arg)
{
if (HAVE_FEATURE(CRUSH_CHOOSEARGS))

This comment has been minimized.

@liewegas

liewegas May 8, 2017

Member

this is missing a feature arg.. needs to be passed through from encode()?

arg->ids_size == 0)
continue;
id2choose_arg[i] = arg;
}

This comment has been minimized.

@liewegas

liewegas May 8, 2017

Member

it seems like this second part is just repackaging an existing vector/array as a map. the important part is the compatibility check that happens above. maybe recast this function to return bool (can_encode_compat_choose_args), do a check vs features (and assert out if it's not compat but the feature is missing). then in the straw2 encode if a bool should_encode_compat_choose_args is true look up the bucket weights directly in the arg_map[bucketno] array?

@ghost

This comment has been minimized.

ghost commented May 8, 2017

The map was indeed entirely redundant... fixed and repushed.

@ghost ghost changed the title from crush: encode can override weights with weight set to WIP: crush: encode can override weights with weight set May 8, 2017

@ghost ghost changed the title from WIP: crush: encode can override weights with weight set to DNM: crush: encode can override weights with weight set May 8, 2017

@ghost

This comment has been minimized.

ghost commented May 8, 2017

needs rebase once #14959 is merged

@@ -1370,6 +1370,24 @@ int CrushWrapper::rebuild_roots_with_classes()
return trim_roots_with_class(true);
}
bool CrushWrapper::can_encode_compat_choose_args() const
{
if (choose_args.size() != 1)

This comment has been minimized.

@liewegas

liewegas May 8, 2017

Member

1? an empty choose_args is 'compat'...

@@ -1381,6 +1399,15 @@ void CrushWrapper::encode(bufferlist& bl, uint64_t features) const
::encode(crush->max_rules, bl);
::encode(crush->max_devices, bl);
bool encode_compat_choose_args = false;
crush_choose_arg_map arg_map;
if (choose_args.size() > 0 &&

This comment has been minimized.

@liewegas

liewegas May 8, 2017

Member

ah, nm.

@liewegas

This comment has been minimized.

Member

liewegas commented May 8, 2017

yay!

@ghost

This comment has been minimized.

ghost commented May 8, 2017

I'm not sure I get the sequence right.

  • a pre-luminous client connects a does not have the choose_arg feature
  • the cluster has a non compatible crushmap with choose args that can't be converted
  • the assert happens and crashes the monitor :-)

That must not be how it happens

@liewegas

This comment has been minimized.

Member

liewegas commented May 8, 2017

Actually this is possible... we should change that assert to a strong warning.

We don't have a way currently to track the features supported by currently-connected clients; we only control the connection of new clients.

@ghost

This comment has been minimized.

ghost commented May 8, 2017

Does it mean existing client may get an incorrect crushmap as a result ?

@liewegas

This comment has been minimized.

Member

liewegas commented May 8, 2017

@ghost

This comment has been minimized.

ghost commented May 8, 2017

So, the other part of this pull request is to set require_min_compat_client to luminous whenever a crushmap with choose_args is uploaded and it cannot be encoded in a backward compatible way. Right ?

@liewegas

This comment has been minimized.

Member

liewegas commented May 9, 2017

That option is meant to work teh other way around: it would prevent you from uploading a crush map that uses choose_args (or doing any other command that would change the features that clients have to support) until teh admin makes a policy decision and changes the required_min_compat_client option.

I think teh main missing piece here is the get_features() call in OSDMap; I think you should rebase this on top of wip-crush-compat and then make sure it is using the has_incompat_chooseargs() instead of has_chooseargs() helper. That way get_featuers() will not indicate that the new luminous features are required if the choose_args are sufficiently simple...

@liewegas

This comment has been minimized.

Member

liewegas commented May 9, 2017

#14959 is merged; this can be rebased now!

@ghost

This comment has been minimized.

ghost commented May 9, 2017

\o/

@ghost ghost changed the title from DNM: crush: encode can override weights with weight set to crush: encode can override weights with weight set May 9, 2017

@ghost ghost changed the title from crush: encode can override weights with weight set to DNM: crush: encode can override weights with weight set May 9, 2017

@ghost

This comment has been minimized.

ghost commented May 9, 2017

@liewegas is there a way to set the features of the client ? Something like ceph --feature luminous ? I found how to set the mon features :-)

@ghost

This comment has been minimized.

ghost commented May 9, 2017

if there is no way for a client to pretend to be kraken, 59c104e is another possible implementation for the test. The downside is that it takes an hour at least to run which makes debugging longer.

@liewegas

This comment has been minimized.

Member

liewegas commented May 10, 2017

@ghost

This comment has been minimized.

ghost commented May 10, 2017

I'll write a test function in test/crush/CrushWrapper.cc

@ghost ghost changed the title from DNM: crush: encode can override weights with weight set to crush: encode can override weights with weight set May 11, 2017

@ghost

This comment has been minimized.

ghost commented May 11, 2017

@liewegas repushed with tests & caught a bug, useful :-)

@ghost

This comment has been minimized.

ghost commented May 11, 2017

148 - osd-scrub-repair.sh (Failed)

jenkins test this please

ldachary added some commits May 8, 2017

crush: encode can override weights with weight set
Encode a "legacy" crushmap if (1) we're using weight sets, (2) the
client is lacking features for the weight set, but (3) the weight set
has a single position and no id remapping. Since these maps are only
used by clients (not humans), then there is no need to preserve the
original crush weights. We can just swap them for the real weights and
the legacy clients will behave as expected.

Fixes: http://tracker.ceph.com/issues/19836

Signed-off-by: Loic Dachary <loic@dachary.org>
crush,osd: s/chooseargs/choose_args/
Signed-off-by: Loic Dachary <loic@dachary.org>
@ghost

This comment has been minimized.

ghost commented May 12, 2017

rebased & repushed

@ghost ghost added the needs-qa label May 12, 2017

@ghost

This comment has been minimized.

ghost commented May 12, 2017

(for the record make check succeeded before I rebased & repushed but there was a conflict)

@liewegas liewegas merged commit 81bd302 into ceph:master May 17, 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment