xds: use a standalone Context for xDS control plane RPCs

[voidzcy]

Control plane RPCs are independent of application RPCs, they can stand for completely different lifetime. So the context for making application RPCs should not be propagated to control plane RPCs.

[ejona86]

Will you do a follow-up with a test?

[voidzcy]

Will you do a follow-up with a test?

Well, there isn't a good way to test this unless we have something end to end. I was able to reproduce the issue with XdsClient's test, by attaching a CancellableContext before making the xDS RPC and then close the context to let the watcher receives the error as we saw. But after the change for using a standalone context, that kind of test code looks very weird, more of lying to ourselves.

[ejona86]

We could have a test in a ClientXdsClientTest potentially. Watch a resource, then cancel the Context, then watch another resource and see if it succeeds. You'd arrange it to make sure the backoff doesn't trigger during the test.

But since we have a manual test verification, if that unit test is too much effort/takes too long then it is better to merge as-is.

[voidzcy]

Watch a resource, then cancel the Context, then watch another resource and see if it succeeds.

That's what I did for reproducing the issue, something like:

CancellableContext cancellableContext = Context.current().withCancellation();
Context baseContext = cancellableContext.attach();

try {
   // watch to some resource
   cancellableContext.close();
   // verify watcher receives the error
} finally {
  cancellableContext.detach(baseContext);
}

It's simple as that. But after the fix, such a test to verify things working correctly looks very strange, isn't it? As just from the code, it's quite unintuitive/non-obvious to relate the context here to that in application RPCs. It's weak and tests very little. So I'd lean toward not put such a test.

[ejona86]

But after the fix, such a test to verify things working correctly looks very strange, isn't it?

Nah, that looks fine. It guarantees that the Context is handled properly by the implementation. As far as non-obvious, put a comment on it or some such.

[voidzcy]

But after the fix, such a test to verify things working correctly looks very strange, isn't it?

Nah, that looks fine. It guarantees that the Context is handled properly by the implementation. As far as non-obvious, put a comment on it or some such.

Well, still. Now we are injecting a Context to the XdsClient, and that's just the ROOT in test. So if we test that, we are basically testing the test code :). I'd say this is a more like a static behavior from the scope of XdsClient, so just testing such would not be useful.

[ejona86]

So if we test that, we are basically testing the test code :).

No, we are testing that "the xds client uses context a and not context b" sort of thing. If xds client ignores the Context.ROOT argument, that would trigger a test failure, so it seems fair.

[voidzcy]

No, we are testing that "the xds client uses context a and not context b" sort of thing. If xds client ignores the Context.ROOT argument, that would trigger a test failure, so it seems fair.

Okay, added a simple test case for it.