messages/MForward: fix encoding features #11180
Conversation
|
@gregsfortytwo @jecluis Do you mind looking at this? I'm a little surprised this hasn't bitten us before. Am I missing something? |
There was a problem hiding this comment.
Yeah, I'm pretty sure this is (partly) deliberate. We can encode the MForward itself with whatever features we want, but the forwarded message needs to be encoded the way it was originally by the client — otherwise the receiving monitor might not know when it should and should not fill in fields in specific ways.
Similarly, on reply the leader needs to encode the message for the client, not for the forwarding monitor.
We've definitely had bugs around encoding here before, so I wouldn't toss out the entire thing without searching git logs and the tracker for how it happened. Did you see an actual bug or just notice it on inspection?
|
Oh, and indeed that's how the existing implementation seems to work. I guess if both the client and the peon monitor have newer features+code than the leader, we could get in trouble. So we might want to do a logical AND of the client's features and the quorum features for encoding, but that's the only issue I see with current implementation. |
|
The memory management commits seem to be
2731b19 which originally introduced this,
and f2e240a, which changed the reference
counting. This is where the bug was introduced (encoding with client--not
target--features).
There was another fix ac10aa5 which
addressed the target mon knowing the original client's features. This is
what's used to populate the synthetic Connection on the receiving end.
AFAICS there isn't any dependency on how the payload message is
encoded/decoded as it passes over the wire between monitors...
I hit this with the upgrade test on wip-addrvec. The receiving mon
couldn't decode the payload message. There is something else going on
there, but I think it's independent of this bug.
|
|
Yeah, the scenario you describe is exactly what's going on (leader mon is older than peon and client). So the question is, should we AND the features or just use the target? I suppose there's no harm in ANDing them, and it's possible there's some subtle dependency. I'll change that. Unrelated weirdness, though: in this particular case the message came from loopback, and for those virtual Connections we set features to the feature mask passed into the messenger ctor... which is 0 in ceph_mon.cc (and most other places). The defautl arg is 0, and everyone passes 0, except one caller that passes CEPH_FEATURES_ALL. And the value is only used by the messenger for this loopback connection. I'm going to do a separate PR to just rip that all out... |
liewegas
force-pushed
the
wip-mon-forward
branch
2 times, most recently
from
9464b0f
to
77d5730
Compare
|
@gregsfortytwo It's doing the & now on the feature set. And passed the jewel-x test that it was previously failing due to the encoding disparity (http://pulpito.ceph.com/yuriw-2016-09-27_21:00:16-upgrade:jewel-x-wip-mon-forward_2016_09_27-distro-basic-smithi/) |
| public: | ||
| void encode_payload(uint64_t features) { | ||
| ::encode(tid, payload); | ||
| ::encode(client, payload, features); | ||
| ::encode(client_caps, payload, features); | ||
| payload.append(msg_bl); | ||
| // encode client message with intersection of target and source | ||
| // features. this probably doesn't matter, but might. |
There was a problem hiding this comment.
This is not a helpful comment. Say when it does matter rather than that it probably doesn't.
We were encoding the message with the sending client's features, which makes no sense: we need to encode with the recipient's features so that it can decode the message. The simplest way to fix this is to rip out the bizarre msg_bl handling code and simply keep a decoded Message reference, and encode it when we send. We encode the encapsulated message with the intersection of the target mon's features and the sending client's features. This probably doesn't matter, but it's conceivable that there is some feature-dependent behavior in the message encode/decode that is important. Fixes: http://tracker.ceph.com/issues/17365 Signed-off-by: Sage Weil <sage@redhat.com>
liewegas
force-pushed
the
wip-mon-forward
branch
from
77d5730
to
d4f5e88
Compare
|
@gregsfortytwo updated |
We were encoding the message with the sending client's
features, which makes no sense: we need to encode with
the recipient's features so that it can decode the
message.
The simplest way to fix this is to rip out the bizarre
msg_bl handling code and simply keep a decoded Message
reference, and encode it when we send.
Fixes: http://tracker.ceph.com/issues/17365
Signed-off-by: Sage Weil sage@redhat.com