CSDS: add new config status ACKED by lidizheng · Pull Request #13121 · envoyproxy/envoy

lidizheng · 2020-09-15T22:36:12Z

Signed-off-by: Lidi Zheng lidiz@google.com

Commit Message: Add new config status for CSDS: ACKED.
Additional Description: As the CSDS service definition described, it has the potential to be used to expose xDS config from a client or proxy. gRPC wants to utilize this service to improve its debuggability. But the ConfigStatus is designed from the control plane point of view. Especially, the client cannot predict if there is new config on its way, so it can't accurately claim any xDS config status as SYNCED. We need another config status to indicate the status that the client received the status and sent out ACK. The actual wording is up to debate.
Risk Level: Low
Testing: Not needed
Docs Changes: Inline with proto definition
Release Notes: New config status for CSDS: ACKED.

CC @fuqianggao @markdroth @menghanl

Signed-off-by: Lidi Zheng <lidiz@google.com> Signed-off-by: Lidi Zheng <lidiz@google.com>

repokitteh-read-only · 2020-09-15T22:36:19Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy[\w/]*/(v1alpha\d?|v1|v2alpha\d?|v2))|(api/envoy/type/(matcher/)?\w+.proto).
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #13121 was opened by lidizheng.

see: more, trace.

api/envoy/service/status/v2/csds.proto

htuch · 2020-09-16T14:39:22Z

api/envoy/service/status/v3/csds.proto

+  // client has received the config but replied with NACK.
  ERROR = 4;
+
+  // The client has received the config and repied with ACK.


I'm almost thinking we want separate status enum messages or very explicit field naming to help make clear which one (client vs. server status) we're talking about.

Yeah, seeing these comments, I guess I do think that having a separate set of enums would be clearer. We can call them something like CLIENT_REQUESTED, CLIENT_ACKED, and CLIENT_NACKED.

We can either add these as a new values in this existing enum, or we can add a whole separate enum field for use in clients.

Adding a new proto enum struct named ClientSideStatusConfig. It's a bit verbose, but I think it needs to have a very clear naming.

markdroth

Thanks for doing this, Lidi!

api/envoy/service/status/v2/csds.proto

api/envoy/service/status/v3/csds.proto

markdroth · 2020-09-16T15:16:16Z

api/envoy/service/status/v3/csds.proto

+  // client has received the config but replied with NACK.
  ERROR = 4;
+
+  // The client has received the config and repied with ACK.


Yeah, seeing these comments, I guess I do think that having a separate set of enums would be clearer. We can call them something like CLIENT_REQUESTED, CLIENT_ACKED, and CLIENT_NACKED.

We can either add these as a new values in this existing enum, or we can add a whole separate enum field for use in clients.

htuch · 2020-09-16T16:35:25Z

Let's split this out as a separate enum. I feel it's going to be necessary for any CSDS client to have some logic to make sense of whether they are interpreting a management or client status here.

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng

@markdroth PTALA.

api/envoy/service/status/v2/csds.proto

lidizheng · 2020-09-16T17:26:19Z

api/envoy/service/status/v3/csds.proto

+  // client has received the config but replied with NACK.
  ERROR = 4;
+
+  // The client has received the config and repied with ACK.


Adding a new proto enum struct named ClientSideStatusConfig. It's a bit verbose, but I think it needs to have a very clear naming.

markdroth · 2020-09-16T17:27:33Z

api/envoy/service/status/v3/csds.proto

+  // Client received the config and replied with ACK.
+  CLIENT_ACKED = 2;
+
+  // Client received the config and replied with NACK.


Let's explicitly document that the resource content that is returned in this case will be the most recent version accepted, if any, not the version that was NACKed.

Documented the expected config dump behavior here and in the PerXdsConfig message.

markdroth · 2020-09-16T17:29:23Z

api/envoy/service/status/v3/csds.proto

@@ -73,6 +89,7 @@ message PerXdsConfig {
      "envoy.service.status.v2.PerXdsConfig";

  ConfigStatus status = 1;


It's probably worth adding a comment here that status will be populated by xDS servers but client_side_config_status will be populated by xDS clients.

Comments added.

(It's too bad that we can't use a oneof here, but that would be a breaking change for the go proto API.)

Suggest adding (udpa.annotations.field_migrate).oneof_promotion annotations.

Annotation added, TIL.

@htuch Can you advice what should we use in v4alpha? The format suggests that the migration annotation is expected to be expanded into real oneof in v4alpha. But that will break backward compatibility.

That's fine -- v4alpha will be a new major version (if it ever actually happens), so breaking changes are okay. This is actually the whole point of the annotation, to make sure this change happens when the next major version bump occurs.

Oh, but it looks like this didn't actually work for some reason. The annotation was carried forward to v4alpha instead of being turned into a oneof. Maybe the annotation has the wrong syntax or something?

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng

@markdroth PTALAA.

lidizheng · 2020-09-16T17:55:11Z

api/envoy/service/status/v3/csds.proto

+  // Client received the config and replied with ACK.
+  CLIENT_ACKED = 2;
+
+  // Client received the config and replied with NACK.


Documented the expected config dump behavior here and in the PerXdsConfig message.

lidizheng · 2020-09-16T17:55:47Z

api/envoy/service/status/v3/csds.proto

@@ -73,6 +89,7 @@ message PerXdsConfig {
      "envoy.service.status.v2.PerXdsConfig";

  ConfigStatus status = 1;


Comments added.

markdroth

Please also change the comment on line 23-26 to indicate that we are now going to use this on the xDS client side.

Thanks!

api/envoy/service/status/v3/csds.proto

markdroth · 2020-09-16T18:08:03Z

api/envoy/service/status/v3/csds.proto

 }

+// Config status from a client-side view.
+enum ClientSideConfigStatus {


Suggest calling this ClientConfigStatus.

Changed to ClientConfigStatus.

In this proto definition, there is the ClientConfig message. It might be a bit confusing to readers that why ClientConfigStatus is not the status for the ClientConfig message, but the client-side view of config status. On the other hand, the ClientSideConfigStatus is not much better.

Better naming is still welcomed. Picking ClientConfigStatus since it is simpler.

markdroth · 2020-09-16T18:08:55Z

api/envoy/service/status/v3/csds.proto

@@ -73,6 +89,7 @@ message PerXdsConfig {
      "envoy.service.status.v2.PerXdsConfig";

  ConfigStatus status = 1;


(It's too bad that we can't use a oneof here, but that would be a breaking change for the go proto API.)

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng

@markdroth Thanks for the quick review! PTALAAA.

lidizheng · 2020-09-16T18:26:03Z

api/envoy/service/status/v3/csds.proto

 }

+// Config status from a client-side view.
+enum ClientSideConfigStatus {


Changed to ClientConfigStatus.

In this proto definition, there is the ClientConfig message. It might be a bit confusing to readers that why ClientConfigStatus is not the status for the ClientConfig message, but the client-side view of config status. On the other hand, the ClientSideConfigStatus is not much better.

Better naming is still welcomed. Picking ClientConfigStatus since it is simpler.

api/envoy/service/status/v3/csds.proto

markdroth · 2020-09-16T18:50:50Z

Please also change the comment on line 23-26 to indicate that we are now going to use this on the xDS client side. Thanks!

Signed-off-by: Lidi Zheng <lidiz@google.com>

markdroth

This looks great!

I'll recuse myself and let @htuch or one of the other API shepherds give API approval.

markdroth · 2020-09-16T19:22:15Z

(Whoops -- I didn't realize that my approving the PR would satisfy the API approval requirement; I thought I had to say "/lgtm api". But since one of the maintainers has to review before merging anyway, that's probably okay.)

htuch

Needs a fix_format.

htuch · 2020-09-16T19:59:44Z

api/envoy/service/status/v3/csds.proto

@@ -73,6 +89,7 @@ message PerXdsConfig {
      "envoy.service.status.v2.PerXdsConfig";

  ConfigStatus status = 1;


Suggest adding (udpa.annotations.field_migrate).oneof_promotion annotations.

htuch · 2020-09-16T20:04:25Z

api/envoy/service/status/v3/csds.proto

+
+  // Client received the config and replied with NACK. Notably, the attached
+  // config dump is not the NACKed version, but the most recent accepted one. If
+  // no config is accepted yet, the attached config dump will be empty.


Can we have a similar statement added about control plane behavior for configs with a negative status, i.e. "this is always the latest successful config?" Or maybe that's not even desired, we might want the latest successful and unsuccessful config? Would that also make sense for client? CC @fuqianggao

This behavior wasn't clearly described in this proto definition before, or in other documentation. Could it be a control-plane specific detail?

Hi @fuqianggao, can you help to explain the designed behavior of config dump when client replied with NACK?

For control plane, it will be the latest config that control plane has. If status is NACK, it means the config is sent to client, but rejected.

Makes sense. I think the actionable thing is for @lidizheng to update the management server wording to reflect this. I'm still wondering in this discussion thread, is there value in having both active and failed configs reported?

I suspect that for both xDS servers and xDS clients, there would be additional overhead in storing old versions of configs. In general, both sides will store only the latest version that they care about, so they don't have anything else to report here.

And if all clients implement CSDS, the CLI can get the active config from the clients, and the most up-to-date config from the management server (if they are different).

I have updated the comment for ConfigStatus.ERROR to note that the management server will dump the latest config in case of client replying NACK.

The behavior described by @menghanl is implemented by istioctl's proxy-status command.

I like the idea of being able to query both client and server CSDS and effectively determine the failing and active config. @lidizheng can you please file an issue for Envoy to add support for CSDS? You don't have to do the implementation, but we should track this as an open feature request that we can share with the community.

Filed as an issue: #13181. For Envoy, it overlaps with the admin config dump feature. Today, tools like istioctl is fetching xDS config from server via CSDS, and from client via admin config dump (HTTP).

Signed-off-by: Lidi Zheng <lidiz@google.com>

htuch · 2020-09-17T16:00:48Z

api/envoy/service/status/v3/csds.proto

  // the CSDS server is an xDS server. No matter what the client config status
  // is, xDS clients should always dump the most recent accepted xDS config.
-  ClientConfigStatus client_status = 7;
+  ClientConfigStatus client_status = 7


I think you might need to have a oneof existing. I suggest moving this field lexically before the existing status and wrapping it in a oneof status_config. That should allow the v4alpha generation to work.

Lidi, I think you still need to address this comment.

According to the style in the code base, the oneof_promotion annotation seems appear in pairs (example 1, example 2). Splitting the oneof options into different layers might break the consistency, and slightly weaken the semantic intention of the upcoming migration.

So, in my mind, keeping oneof_promotion in v3 and real oneof in v4alpha is the better way.

Yes, that's what I meant.

The v4alpha file looks right now. The last time I looked, I thought it was still wrong, but maybe I was looking at an old snapshot.

Anyway, this looks great. Thanks!

htuch

LGTM modulo the thread on clarifying management server behavior on stale/nacked config. I'd like there to be consistency, and ideally we should have a plan for how to support reporting both last known good and bad config (at least on management server side), since both are useful, CC @fuqianggao

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng · 2020-09-17T22:55:59Z

@htuch PTALAA. All tests are passing, and the mentioned behavior is documented.

Add new config status for CSDS: ACKED

e142072

Signed-off-by: Lidi Zheng <lidiz@google.com> Signed-off-by: Lidi Zheng <lidiz@google.com>

repokitteh-read-only bot added v2-freeze api labels Sep 15, 2020

asraa assigned markdroth Sep 16, 2020

htuch self-assigned this Sep 16, 2020

htuch reviewed Sep 16, 2020

View reviewed changes

markdroth reviewed Sep 16, 2020

View reviewed changes

Split the client-side config status into another enum

78c118c

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng commented Sep 16, 2020

View reviewed changes

markdroth reviewed Sep 16, 2020

View reviewed changes

Enrich the documentation to describe expected config dump behavior

ef87599

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng commented Sep 16, 2020

View reviewed changes

markdroth reviewed Sep 16, 2020

View reviewed changes

Simplify the new enum's naming and improve documentation

8f2fb64

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng commented Sep 16, 2020

View reviewed changes

lidizheng added 2 commits September 16, 2020 11:59

Update the service description to include getting configs from clients

15a4491

Signed-off-by: Lidi Zheng <lidiz@google.com>

Make formatter happy

abd2351

Signed-off-by: Lidi Zheng <lidiz@google.com>

markdroth previously approved these changes Sep 16, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 16, 2020

htuch reviewed Sep 16, 2020

View reviewed changes

Add UDPA oneof annotation

5fb88f0

Signed-off-by: Lidi Zheng <lidiz@google.com>

lidizheng dismissed markdroth’s stale review via 5fb88f0 September 16, 2020 21:18

repokitteh-read-only bot added the api label Sep 16, 2020

Import migrate.proto to use UDPA oneof

87b889f

Signed-off-by: Lidi Zheng <lidiz@google.com>

htuch reviewed Sep 17, 2020

View reviewed changes

lidizheng added 2 commits September 17, 2020 10:26

Use oneof_promotion in v3 and real oneof in v4alpha

0ba9254

Signed-off-by: Lidi Zheng <lidiz@google.com>

Notes down the expected behavior for management server saw NACK

f7f38af

Signed-off-by: Lidi Zheng <lidiz@google.com>

htuch approved these changes Sep 18, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Sep 18, 2020

htuch merged commit 22061a2 into envoyproxy:master Sep 18, 2020

lidizheng mentioned this pull request Sep 18, 2020

[Feature] Support Client Status Discovery Service (CSDS) #13181

Open

		@@ -73,6 +89,7 @@ message PerXdsConfig {
		"envoy.service.status.v2.PerXdsConfig";

		ConfigStatus status = 1;

Conversation

lidizheng commented Sep 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Sep 15, 2020

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!

markdroth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch commented Sep 16, 2020

Uh oh!

lidizheng left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidizheng left a comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lidizheng Sep 16, 2020 • 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!

lidizheng left a comment

Choose a reason for hiding this comment

Uh oh!

lidizheng Sep 16, 2020 • 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!

Uh oh!

lidizheng commented Sep 15, 2020 •

edited

Loading

lidizheng Sep 16, 2020 •

edited

Loading

lidizheng Sep 16, 2020 •

edited

Loading

lidizheng Sep 17, 2020 •

edited

Loading