crush: encode can override weights with weight set#15002
Conversation
src/crush/CrushWrapper.cc
Outdated
|
|
||
| void CrushWrapper::get_incompat_choose_args(std::map<__u32, crush_choose_arg*> &i2choose_arg) | ||
| { | ||
| if (HAVE_FEATURE(CRUSH_CHOOSEARGS)) |
There was a problem hiding this comment.
this is missing a feature arg.. needs to be passed through from encode()?
src/crush/CrushWrapper.cc
Outdated
| arg->ids_size == 0) | ||
| continue; | ||
| id2choose_arg[i] = arg; | ||
| } |
There was a problem hiding this comment.
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?
|
The map was indeed entirely redundant... fixed and repushed. |
|
needs rebase once #14959 is merged |
src/crush/CrushWrapper.cc
Outdated
|
|
||
| bool CrushWrapper::can_encode_compat_choose_args() const | ||
| { | ||
| if (choose_args.size() != 1) |
There was a problem hiding this comment.
1? an empty choose_args is 'compat'...
src/crush/CrushWrapper.cc
Outdated
|
|
||
| bool encode_compat_choose_args = false; | ||
| crush_choose_arg_map arg_map; | ||
| if (choose_args.size() > 0 && |
|
yay! |
|
I'm not sure I get the sequence right.
That must not be how it happens |
|
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. |
|
Does it mean existing client may get an incorrect crushmap as a result ? |
|
Yeah. This is an existing problem we don't have a good solution for. If
someone is already connected and you switch the crush tunables the
connected client might break.
We should put a card for this on the backlog.. it's hard, though, since
even if the monitor tracked all connected clients (it pretty easily could)
it's also possible that a client temporarily disconnected with the mon due
to network blip or something, so it would be 100% effective. (Much better
than nothing at all, though!)
|
|
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 ? |
|
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... |
|
#14959 is merged; this can be rebased now! |
|
\o/ |
|
@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 :-) |
|
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. |
|
For manual testing i'd build ceph jewel or kraken in a different source
checkout and pass -c /path/to/luminous/build/ceph.conf.
|
|
I'll write a test function in test/crush/CrushWrapper.cc |
|
@liewegas repushed with tests & caught a bug, useful :-) |
|
148 - osd-scrub-repair.sh (Failed) jenkins test this please |
Signed-off-by: Loic Dachary <loic@dachary.org>
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>
|
rebased & repushed |
|
(for the record make check succeeded before I rebased & repushed but there was a conflict) |
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