balancer: support injection of per-call metadata from LB policies

[easwars]

Summary of changes:

• New field added to balancer.PickResult. LB policies can use this to inject per-call arbitrary metadata.
• Arbitrary metadata returned by LB policies is then plumbed all the way to the place where new streams are created, and these are sent out as part of the HEADERS frame.

RELEASE NOTES:

• balancer: support injection of per-call metadata from LB policies

[easwars]

@dfawley : Based on our offline discussion, I've made changes to merge metadata from LB policies with existing call metadata instead of overwriting existing one. PTAL. Thanks.

[dfawley]

LGTM modulo this one potential concern.

[dfawley]

Should this not update a.ctx in place instead? I'm a bit worried if we ever use it again later, it will be the wrong one.

[easwars]

• clientStream.ctx is the RPC's context (from what I can see, it is used directly and not derived from it)
• csAttempt.ctx is derived from the above context, and is used in the following places
  • getTransport(): is passed to Pick()
  • sending/receiving messages and when the attempt is closed, the context is passed to stats handler methods

I feel that by not modifying the csAttempt's context and instead deriving a new one (with metadata added by the LB policy) to be passed to NewStream(), we can ensure that we don't have any surprises when the attempt is retried. If we modify the csAttempt's context and the attempt is retried, and we call Pick() with the modified context, the LB policy's metadata might show up multiple times in there.

[dfawley]

Each attempt creates a new csAttempt, so that should not be a concern. Technically the attempt is not what's retried, but the RPC (clientStream).

[easwars]

Each attempt creates a new csAttempt, so that should not be a concern.

Oh yes, that's right.

I'm a bit worried if we ever use it again later, it will be the wrong one.

When will we ever use it again? From what I see, the context is used in getTransport() and while invoking stats handler methods. Even if we add few usages for this context, why would it be helpful to pass the extra metadata added by the LB policy?

I'm not opposed to updating the attempt's context in place with metadata from the LB policy. But, I thought it was safer to derive one from it and pass it to NewStream and continue using the existing one for everything else, since that will minimize the effect of this change.

[dfawley]

I'm not sure we ever will use it again. But if ClientStream.Context() doesn't contain the true outgoing metadata, then that could potentially be considered a bug.

[easwars]

clientStream.Context() returns the context from the underlying transport stream.

grpc-go/stream.go

Line 701 in e53d28f

if cs.attempt.s != nil {

And we use the modified context (with all outgoing metadata) when creating the transport stream. So, it looks like we are good here too.

[dfawley]

Honestly it's still feeling wrong to not update the attempt's context when the metadata is updated by the LB policy. Unless there's a strong reason not to do this, then I'd rather do it and be safe vs. sorry. The concerns about it affecting subsequent attempts are a misunderstanding of the design of attempts (everything in an attempt should be local to that one attempt), and not a good reason to not update in place.

[easwars]

Done.

[dfawley]

LGTM (modulo a todo comment suggestion), thanks!

[dfawley]

Maybe add to this sentence: ..., or create a version of AppendToOutgoingContext that accepts a metadata.MD. ?

[easwars]

Done.