A40 - xDS Configuration Dump via Client Status Discovery Service in gRPC by lidizheng · Pull Request #223 · grpc/proposal

lidizheng · 2021-02-16T20:16:54Z

gRFC: https://github.com/lidizheng/proposal/blob/csds/A40-csds-support.md

srini100

LGTM but wait for @markdroth's review and approval.

lidizheng · 2021-02-19T21:09:49Z

+    "clientFeatures": ["envoy.lb.does_not_support_overprovisioning"]
+  }
+}
+```


The requirement of Node information is updated.
CC @sergiitk @menghanl

Thanks for capturing this! Glad we've figured it out.

As I mentioned above, I don't see any compelling reason to bother with populating this field.

@markdroth: Earlier last week @lidizheng @menghanl and I discussed this in a chat. To summarize, arguments for populating node info:

Parity with istioctl proxy-config bootstrap

Some fields are generated after on-the-fly, and sent to the control plane in discovery requests: user_agent_name, user_agent_version, and, more importantly clientFeatures. Exposing them can be helpful for debugging. Note this is different from regular CSDS behavior to dump configs as is. The argument for treating the node info differently is that it's loaded from bootstrap and sent to the control plane, as opposed to it's received from the control plane

If there's desire to expose the client's node information for debugging purposes, then that's a perfectly reasonable use-case, and we should design something to address it. However, CSDS was not designed to solve that problem, and IMHO it would be a very clunky way to solve that problem. If the mechanism we provide for accessing the bootstrap info is for a client to query CSDS, that means that a client that wants only the bootstrap info has no way to get it that does not also send the entire set of known xDS resources. That's sending a potentially very large amount of completely unnecessary data.

If we want a way to expose the client's bootstrap info for debugging purposes, let's add a separate RPC service for that. I don't think it should be part of this design.

I think I might be missing your point. Could you please clarify what data (if any) should be in ClientStatusResponse. ClientConfig.node?

it would be a very clunky way to solve that problem

I agree the Node information will be sent along other xDS config dumps. And users might not need them all the time. But the same argument could apply to all xDS configs, like users just updated listeners and would like to see the listeners, and we are sending routes, clusters, endpoints in the CSDS response.

When the xDS implements get popularity soon, we might have users posting issues, and with the node information we could tell which gRPC language/release it is using, and (possibly) which control plane it is trying to talk to.

@sergiitk

Could you please clarify what data (if any) should be in ClientStatusResponse. ClientConfig.node?

What I am proposing is that we do not populate that field at all.

@lidizheng

But the same argument could apply to all xDS configs, like users just updated listeners and would like to see the listeners, and we are sending routes, clusters, endpoints in the CSDS response.

I don't see what that statement has to do with the argument I'm making here. It's true that CSDS does not provide the ability to filter which set of configs are returned, but that's equally true whether CSDS is being served by the control plane or the client. (And this would be trivial to add if someone needed it just by adding a resource type filter to the CSDS request proto.)

The argument I am making here is that the reason that the node field exists in the CSDS response is that CSDS was designed to be served by control planes, and the control plane will be returning data for multiple clients and needs a way to indicate which client each set of data is relevant to. But when CSDS is sent on a client, all of the data is by definition for the same client, so there's no reason to populate this field. In other words, the node field needs to be populated only when the CSDS server is a control plane; it is simply not relevant when the CSDS server is a client.

When the xDS implements get popularity soon, we might have users posting issues, and with the node information we could tell which gRPC language/release it is using, and (possibly) which control plane it is trying to talk to.

IMHO, the fact that this could conceivably be useful to someone someday is not enough justification to add it. If that was a good enough justification, why isn't it standard practice for every API to include the gRPC language and release? Why doesn't channelz include this info? Why doesn't every single RPC service designed by applications provide this info?

In case it's not clear, the answer is that if it's not what a given API is designed to do, it shouldn't be part of that API. The microservices approach means that each API should provide only the data that is necessary for the problem that service is designed to solve. If that wasn't the case, then all APIs would be huge, because everyone would throw everything they could think of into every one of them.

CSDS is designed to show the set of resources seen by the client, not the client's node information. If we want something to expose the client's node information, that's IMHO a separate use-case, and we should design a separate service to solve that problem.

In any case, I don't think this is important enough to justify spending more time debating it, so I'll leave it up to you.

markdroth

Thanks for doing this! My comments are mostly minor.

Please let me know if you have any questions. Thanks!

markdroth · 2021-02-26T08:51:27Z

+    "clientFeatures": ["envoy.lb.does_not_support_overprovisioning"]
+  }
+}
+```


As I mentioned above, I don't see any compelling reason to bother with populating this field.

lidizheng

Thanks for the detailed review! PTALA

lidizheng · 2021-02-26T19:20:27Z

+    "clientFeatures": ["envoy.lb.does_not_support_overprovisioning"]
+  }
+}
+```


it would be a very clunky way to solve that problem

I agree the Node information will be sent along other xDS config dumps. And users might not need them all the time. But the same argument could apply to all xDS configs, like users just updated listeners and would like to see the listeners, and we are sending routes, clusters, endpoints in the CSDS response.

When the xDS implements get popularity soon, we might have users posting issues, and with the node information we could tell which gRPC language/release it is using, and (possibly) which control plane it is trying to talk to.

voidzcy · 2021-03-01T20:15:36Z

+
+* If the entire message won’t parse: no need to record anything in CSDS, since
+  we don’t know what type of xDS config or what resources are being updated;
+* If one resource won’t parse: don’t abort the parsing, record the error in the


The handling is still a bit ambiguous. I guess we would only record the first error, so the parsing does not need to continue. But would attach the error message to all previously accepted resources that the current response is attempting to update.

Can you specify the cases that are ambiguous here? I guess the wording here is not accurate enough, if one resource (e.g., a cluster) has validation error, the parsing should continue; or one of its field has parsing error, e.g. a config in Any failed to deserialize, the parsing should continue. In other word, the parsing should continue when possible.

If you need a reference PR, please see grpc/grpc#25329.

Looks C-core is concatenating errors together if there are more than one error resources. I don't see much benefit of doing so, other than showing a giant error message while may still not showing all errors (e.g., multiple validation errors for a single resource). Anyway, I am fine with what's being implemented in C-core.

I think the important part is peeking into the ADS response to see what set of resources is being updated, then pasting the error details to their error_state field. I'm fine with one error, as long as they are logged in the right place in the CSDS response, so it's actionable for users.

menghanl · 2021-03-01T23:12:26Z

+existing service protocol, hence we merged several updates (see above) to
+improve the CSDS service to meet our standard. The updates are made to xDS v3
+and xDS v4 (alpha) only, since xDS v2 is in deprecated state. This doc proposes
+to not support xDS v2 for CSDS.


What to do if the underlying client is speaking v2?
Return an error? What error code to use? FailedPrecondition?

By underlying client, do you mean the gPRC application? I thought gRPC processes all xDS config into v3, so the dumped xDS config is agnostic to v2/v3. If the external CSDS client is only speaking v2, they won't be able to invoke the v3 method, because the fully-qualified-method-name includes the service name which has "v3" in it. So, the v2 external CSDS client will get UNIMPLEMENTED by default, in theory.

Here's what TD does for v3 (currently only v2 supported):

❯ csds-client -request_file request.yaml -api_version v3 2021/03/01 18:20:55 rpc error: code = Unimplemented desc = The GRPC target is not implemented on the server, host: trafficdirector.googleapis.com, method: /envoy.service.status.v3.ClientStatusDiscoveryService/StreamClientStatus.

Nice, so in practicce, the old client will also get UNIMPLEMENTED.

Note that the CSDS RPC service name is different between v2 and v3, because the major version is part of the proto package name. So I think all we need to do here is to implement only the v3 service, not the v2 service. Then clients will get UNIMPLEMENTED if they try to use the v2 service, just like they would for any other unimplemented RPC service.

I think it's fine to send v2 protos inside of the Any fields in v3 CSDS. In xDS terms, the transport protocol version and the data model version do not have to be the same. I don't think we need to do any conversion here.

Do you have an example that where the v2 resources might slip through

It's not one type of resource. All the resources in xds v2 are v2. E.g. v2.Cluster vs v3.Cluster, v2.Endpoint vs v3.Endpoint.

In xDS terms, the transport protocol version and the data model version do not have to be the same.

That's right.
The difference would be on the csds client side. If we send v2 protos as Any in the csds responses, the csds client needs to depend on v2 protos (not that they will be used, but their types need to be registered) so the client can unmarshal those Any.

I think it's fine for CSDS client to need v2 protos if the client is using v2. That's independent of the fact that we're only supporting v3 of the CSDS RPC service itself.

This is no different than ADS -- it's possible for a client to use v3 of ADS but v2 of the resource protos, or vice versa. In fact, in Envoy, those are two independent knobs. In gRPC, we just simplified it and avoided one of the knobs by depending on the fact that we didn't happen to use any fields in v2 that were being removed in v3, so we could just unconditionally treat all of the resource protos as v3.

This SGTM (and is actually easier to do :))

Let's update the gRFC to clarify this? This doesn't sound accurate:

This doc proposes to not support xDS v2 for CSDS.

Updated to

For xDS CSDS v2 (the RPC service itself), this doc proposes to not support, so
we only provide v3 CSDS service and CSDS client requesting for v2 service will
get UNIMPLEMENTED error.

There are edge cases that v2 and v3 xDS resources are mixed together. This doc
proposes to keep them as-is in the CSDS responses. So, the CSDS service can
accurately reflect the xDS configs received from the control plane.

The section title also updated to xDS v2/v3/v4, since it's hard to describe our handling of v2 in the title.

lidizheng

@markdroth PTALAA. Is there any blocking issues?

sergiitk · 2021-03-06T01:41:27Z

🥳

Add gRFC for CSDS

9b49087

lidizheng requested a review from markdroth February 16, 2021 20:16

lidizheng assigned markdroth Feb 16, 2021

Add discussion links and update status

1270633

srini100 reviewed Feb 18, 2021

View reviewed changes

Comment thread A40-csds-support.md Outdated

srini100 requested review from menghanl and sergiitk February 18, 2021 00:08

srini100 approved these changes Feb 18, 2021

View reviewed changes

Remove the extra left parens

e263e6a

menghanl reviewed Feb 18, 2021

View reviewed changes

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md Outdated

Adopt reviewer's advice

90b438e

sergiitk approved these changes Feb 19, 2021

View reviewed changes

lidizheng added 2 commits February 18, 2021 16:55

Adopt reviewer's advices

d6564eb

Update what Node information should CSDS return

5d9cd4a

lidizheng commented Feb 19, 2021

View reviewed changes

Fix grammer mistakes

dcdd1e3

markdroth reviewed Feb 26, 2021

View reviewed changes

Adopt reviewer's suggestions

d8b5cc2

lidizheng commented Feb 26, 2021

View reviewed changes

voidzcy reviewed Mar 1, 2021

View reviewed changes

markdroth reviewed Mar 1, 2021

View reviewed changes

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md

menghanl reviewed Mar 1, 2021

View reviewed changes

Update implemented language & fix a typo

b6f5539

lidizheng commented Mar 2, 2021

View reviewed changes

Comment thread A40-csds-support.md Outdated

Comment thread A40-csds-support.md Outdated

markdroth approved these changes Mar 2, 2021

View reviewed changes

Update the description about xDS v2 handling

cad45d5

menghanl approved these changes Mar 3, 2021

View reviewed changes

Update the status to approved

aadbb4c

lidizheng merged commit d57cb87 into grpc:master Mar 5, 2021

Uh oh!

Conversation

lidizheng commented Feb 16, 2021

Uh oh!

Uh oh!

srini100 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergiitk Feb 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

lidizheng left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergiitk Feb 26, 2021 •

edited

Loading