Incremental XDS proposal. by nt · Pull Request #3470 · envoyproxy/envoy

nt · 2018-05-23T13:57:51Z

This PR includes proposed additions to the XDS discovery protos as discussed in this design doc.

The implementation is not included and will be done in a follow up PRs.

The tacking bug id is #3229

zuercher

Looks good. One minor comment and some copy editing. (I'm happy to take a closer copy editing pass later, if you'd like.)

zuercher · 2018-05-23T17:33:19Z

api/envoy/api/v2/discovery.proto

typos: known and corresponding. CDS should be XDS, I think.

I think the type/field name are also swapped here.

Re "I think the type/field name are also swapped here.": I'm not sure what you mean here. My intention is to have keys be resource names and values be their version.

zuercher · 2018-05-23T17:48:10Z

api/envoy/api/v2/discovery.proto

Is the intention here to try to keep implementations from relying on Envoy setting this value in requests?

I think this documentation should just describe the client behavior (e.g., the client returns the value it saw in the last ACKed response) and leave off the "debugging purposes only" statements.

See my comment below, I don't think it is needed.

I updated the comment to match having it in responses only.

mattklein123 · 2018-05-23T19:38:10Z

Please merge master to make sure you pick up the formatter fixes.

htuch · 2018-05-24T21:00:52Z

api/envoy/api/v2/discovery.proto

Do we need to supply this at all? I realize this is what we do in v1, but what is the function in v2?

IMO this is still useful even in the incremental case to cover the add/remove case. What version are we at if we have no resources? Obviously a deployment can leave this empty, but I would suggest we don't remove the possibility of populating it.

Oh right, this is the request. Yes probably don't need that. We should leave in the response though...

Done, kept it in the response.

htuch · 2018-05-24T21:01:20Z

api/envoy/api/v2/discovery.proto

See my comment below, I don't think it is needed.

htuch · 2018-05-24T21:02:12Z

api/envoy/api/v2/discovery.proto

When we get agreement on the basic protos and sequencing, we should add sequence diagrams and a section to https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md as part of this PR.

htuch · 2018-05-24T21:02:50Z

api/envoy/api/v2/discovery.proto

Nit: s/GRPC/gRPC/, s/XDS/xDS/

htuch · 2018-05-25T15:14:22Z

api/envoy/api/v2/discovery.proto

Nit: be consistent with whether it is "incremental xDS" or "Incremental xDS".

htuch · 2018-05-28T18:59:29Z

api/envoy/api/v2/discovery.proto

How does this work in an incremental sense? If an incremental request has the empty list does that then become "subscribe all"?

I removed that comment and made the field resource_name_subscribe.

htuch · 2018-05-28T19:00:12Z

api/envoy/api/v2/discovery.proto

I think the type/field name are also swapped here.

htuch · 2018-05-28T19:01:40Z

api/envoy/api/v2/discovery.proto

Is there a better name than "system version"? It's more like "upstream config pipeline provenance version", which is a bit of a mouthful..

I'm open to suggestions, if we can move away form the version keyword that might help avoid confusion. Something like short_description maybe?

mattklein123 · 2018-05-30T21:22:48Z

@nt can you please merge master, fix DCO, and look at CI issues?

htuch · 2018-05-31T20:48:49Z

api/envoy/api/v2/discovery.proto

Can you describe the nonce value in this case here?Should it match the last nonce we received from the server? If not, how do we avoid it being conflated with ACK?

I updated the PR. The nonce should only be included when the Requests is an ACK / NACK. otherwise it is omitted.

htuch · 2018-05-31T20:49:46Z

api/envoy/api/v2/discovery.proto

Nit: needs blank line separator.

htuch · 2018-05-31T20:49:57Z

api/envoy/api/v2/discovery.proto

Nit: needs blank line separator.

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt · 2018-06-05T23:06:04Z

re: @mattklein123 I rebased on master and fixed DCO. PTAL.

mattklein123

Overall looks great. Just some docs nits and more comment asks. @htuch can you also take a look?

mattklein123 · 2018-06-07T04:13:37Z

api/envoy/api/v2/discovery.proto

+//   2. As a ACK or NACK response to a previous IncrementalDiscoveryResponse.
+//      In this case the response_nonce is set to the nonce value in the Response.
+//      ACK or NACK is determined by the absence or presence of error_detail.
+//      In the case of a ACK the system_version_info must match the value in the


I think this is out of date, right? We no longer send system_version_info?

You are right, removed.

mattklein123 · 2018-06-07T04:15:03Z

api/envoy/api/v2/discovery.proto

+  repeated string resource_names_unsubscribe = 4;
+
+  // This map must be populated when the IncrementalDiscoveryRequest is the
+  // first in a stream. The keys are know resources names to the xDS client and


Typo "keys are know" / a bit confusing in general. Clarify?

I rewrote this, let me know if it make better sense now.

mattklein123 · 2018-06-07T04:18:07Z

api/envoy/api/v2/discovery.proto

+  // required for ADS.
+  string type_url = 2;
+
+  // A list of Resource names to add to the list of tracked resources.


Can we clarify here when this should be empty? For example, on an initial request for CDS when Envoy wants to be told about all relevant clusters by the management server and isn't doing any Envoy side dynamic loading stuff? I guess in general, given that we send this message in multiple different situations, it would be useful for each field to have more detail on when it is and is not set. I think it would make the documentation a lot better.

I added a paragraph to clarify the use of these two fields. Let me know if it makes sense to you.

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123

This looks great. Two more comments for discussion but I think in general LGTM.

mattklein123 · 2018-06-07T17:42:48Z

api/envoy/api/v2/discovery.proto

+  // All resource names in the resource_names_subscribe list are added to the
+  // set of tracked resources and all resource names in the resource_names_unsubscribe
+  // list are removed from the set of tracked resources.
+  // Unlike in non incremental xDS, an empty resource_names_subscribe or


I think we should still describe here this is effectively a hint, right? Meaning, the management server is still allowed to send additional resources? (Though it must send the requested ones) I think it's worth clarifying this as it's similar to non-incremental in this regard?

mattklein123 · 2018-06-07T17:43:42Z

api/envoy/api/v2/discovery.proto

+  // first in a stream. The keys are the resources names of the xDS resources
+  // known to the xDS client. The values in the map are the associated resource
+  // level version info.
+  map<string, string> resource_versions = 5;


WDYT about calling this initial_local_resource_versions just so the intent is more clear? I don't feel strongly about this. Open to other names too.

I think that is a great idea. Renamed it initial_resource_versions. I think the "local" part should be obvious.

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123

LGTM, nice work! Will defer to @htuch for further review.

htuch

Looks good, but would like to get the protocol spelled out in detail in XDS_PROTOCOL.md.

htuch · 2018-06-07T23:13:31Z

api/envoy/api/v2/discovery.proto

+//      ACK or NACK is determined by the absence or presence of error_detail.
+//   3. Spontaneous IncrementalDiscoveryRequest from the client.
+//      This can be done to dynamically add or remove elements from the tracked
+//      resource_names set. In this case response_nonce must be omitted.


It would be good to convince and explain why this doesn't have the racy behavior that originally motivated the nonce. I.e. we need sequence diagrams considering races. Can we make the changes to https://github.com/envoyproxy/envoy/blob/master/api/XDS_PROTOCOL.md as part of this PR?

Done. Let me know what you think.

Looks good, I think that even if a nonce-less incremental subscribe races with some other request/response, it's largely harmless; the set of resources being tracked by the management server should eventually converge on what the client is interested in tracking.

Consider this example (M is management server, E is Envoy, X/Y/Z are resources):

M->E (CDS): {X}

E-> M (CDS): subscribe Y

E->M (CDS): unsubscribe Y

M->E (CDS): {X, Y, Z}

What if steps 3 and 4 race? I don't think there is a problem here, since we eventually converge on the right thing, but there will be periods of inconsistency perhaps.

If someone can persuade me that the above is valid, I think we're good to ship, but want to just raise this.

stale · 2018-06-18T21:39:28Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

htuch · 2018-06-26T19:47:19Z

api/XDS_PROTOCOL.md

+    all 100k clusters when a single cluster is modified, the management server
+    only needs to deliver the single cluster that changed.
+
+An xDS Incremental session is always in the context of a gRPC bidirectional


Nit: s/Incremental/incremental/ here, incremental is an adjective not a proper noun.

htuch · 2018-06-26T19:48:07Z

api/XDS_PROTOCOL.md

+connected to it. There is no REST version of Incremental xDS.
+
+In Incremental xDS the nonce field is required and used to pair
+IncrementalDiscoveryResponse to a IncrementalDiscoveryRequest ACK or NACK.


Can you add docs links (as done with DiscoveryRequest/Response) above? Also, use quote style for proto message names as done for these messages above.

htuch · 2018-06-26T19:49:15Z

api/XDS_PROTOCOL.md

+
+IncrementalDiscoveryRequest can be sent in 3 situations:
+  1. Initial message in a xDS bidirectional gRPC stream.
+  2. As a ACK or NACK response to a previous IncrementalDiscoveryResponse.


Nit: s/a ACK/an ACK/

htuch · 2018-06-26T19:49:39Z

api/XDS_PROTOCOL.md

+IncrementalDiscoveryRequest can be sent in 3 situations:
+  1. Initial message in a xDS bidirectional gRPC stream.
+  2. As a ACK or NACK response to a previous IncrementalDiscoveryResponse.
+     In this case the response_nonce is set to the nonce value in the Response.


Nit: use quote style for response_nonce and error_detail here.

htuch · 2018-06-26T19:50:30Z

api/XDS_PROTOCOL.md

+     This can be done to dynamically add or remove elements from the tracked
+     resource_names set. In this case response_nonce must be omitted.
+
+In this first example the client connects as receive a first update that it ACKs.


s/as receive/and receives/

htuch · 2018-06-26T19:53:18Z

api/XDS_PROTOCOL.md

+
+In this first example the client connects as receive a first update that it ACKs.
+The second update fails and the client NACKs the update. Later the xDS client
+spontaneously requests the "wc" resource.


I think the wc resource_list_subscribe arrow is the wrong way around in the diagram.

htuch · 2018-06-26T20:01:21Z

api/envoy/api/v2/discovery.proto

+//      ACK or NACK is determined by the absence or presence of error_detail.
+//   3. Spontaneous IncrementalDiscoveryRequest from the client.
+//      This can be done to dynamically add or remove elements from the tracked
+//      resource_names set. In this case response_nonce must be omitted.


Looks good, I think that even if a nonce-less incremental subscribe races with some other request/response, it's largely harmless; the set of resources being tracked by the management server should eventually converge on what the client is interested in tracking.

Consider this example (M is management server, E is Envoy, X/Y/Z are resources):

M->E (CDS): {X}

E-> M (CDS): subscribe Y

E->M (CDS): unsubscribe Y

M->E (CDS): {X, Y, Z}

What if steps 3 and 4 race? I don't think there is a problem here, since we eventually converge on the right thing, but there will be periods of inconsistency perhaps.

If someone can persuade me that the above is valid, I think we're good to ship, but want to just raise this.

htuch

Thanks, LGTM modulo remaining comments.

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt · 2018-06-28T17:47:12Z

For some reason I can not directly reply to your race comment about nonce-less spontaneous subscribes/unsubscribes:

subscribes: optionally the client can wait for the update for that specific resource before sending more sub/unsub. but really I should just wait to eventually receive the update.
unsubscribes: the client can locally ignore resources it no longer cares about.

I do not think this is a problem as these are "hints" to the XDS server.

htuch

LGTM, this is great, modulo two nits. @mattklein123 any further thoughts before merging?

htuch · 2018-06-29T17:48:36Z

api/XDS_PROTOCOL.md

+[`IncrementalDiscoveryResponse`]](https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/discovery.proto#discoveryrequest)
+to a [`IncrementalDiscoveryRequest`](https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/discovery.proto#discoveryrequest)
+ACK or NACK.
+Optionaly, a response message level system_version_info is present for


Nit: s/Optionaly/Optionally/

htuch · 2018-06-29T17:48:58Z

api/XDS_PROTOCOL.md

+connected to it. There is no REST version of Incremental xDS.
+
+In incremental xDS the nonce field is required and used to pair a
+[`IncrementalDiscoveryResponse`]](https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/discovery.proto#discoveryrequest)


Additional bracket here - you can preview MD generation by clicking the View button on GH (pro tip).

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt · 2018-06-29T19:01:15Z

fixed both nits.

mattklein123

LGTM, one question. Thank you!

mattklein123 · 2018-06-29T23:17:58Z

api/XDS_PROTOCOL.md

+
+![Incremental reconnect example](diagrams/incremental-reconnect.svg)
+
+The incremental xDS method may be added to CDS and RDS in the future.


Why wouldn't this just be supported out of the box? It's not clear to me how any of this is ADS specific?

I thought adding it on a need-for basis made more sense to keep complexity down. We can add them now if you prefer. Let me know.

IMO I would just add it now. Unless I'm misunderstanding the implementation, I think it will basically "just work." @htuch thoughts?

Yep, it will Just Work (tm).

mattklein123 · 2018-06-29T23:19:15Z

api/envoy/api/v2/discovery.proto

+  //
+  // A list of Resource names to add to the list of tracked resources.
+  repeated string resource_names_subscribe = 3;
+  // A list of Resource names to remove from the list of tracked resources.


nit: newline before this line

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

htuch · 2018-07-02T20:28:30Z

@nt can you fix_format? Ready to merge when that's done.

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

htuch

Awesome, thanks!

htuch self-assigned this May 23, 2018

zuercher reviewed May 23, 2018

View reviewed changes

htuch reviewed May 25, 2018

View reviewed changes

htuch reviewed May 28, 2018

View reviewed changes

htuch reviewed May 31, 2018

View reviewed changes

Nicolas Thiebaud added 2 commits June 5, 2018 13:24

Incremental XDS proposal.

2e7532a

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

Fixes.

6aa0c42

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123 reviewed Jun 7, 2018

View reviewed changes

mattklein123 self-assigned this Jun 7, 2018

Addressed Matt's comments.

c7ae0ca

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123 reviewed Jun 7, 2018

View reviewed changes

Matt review.

baac96a

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123 previously approved these changes Jun 7, 2018

View reviewed changes

htuch suggested changes Jun 7, 2018

View reviewed changes

kyessenov mentioned this pull request Jun 14, 2018

Incremental xDS istio/istio#6299

Closed

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jun 18, 2018

nt dismissed mattklein123’s stale review via 2d13318 June 21, 2018 23:18

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jun 21, 2018

Nicolas Thiebaud added 2 commits June 21, 2018 16:22

Updated XDS_PROTOCOL.md.

9364862

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

Fix formatting.

538d5cf

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

htuch reviewed Jun 26, 2018

View reviewed changes

htuch suggested changes Jun 26, 2018

View reviewed changes

htuch review.

afdee76

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

htuch previously approved these changes Jun 29, 2018

View reviewed changes

two nits.

39a4327

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt dismissed htuch’s stale review via 39a4327 June 29, 2018 18:59

htuch previously approved these changes Jun 29, 2018

View reviewed changes

mattklein123 reviewed Jun 29, 2018

View reviewed changes

Added new line.

0a13894

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt dismissed htuch’s stale review via 0a13894 July 2, 2018 17:22

Added to CDS and RDS.

b1c0ccd

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

mattklein123 previously approved these changes Jul 2, 2018

View reviewed changes

Fix format.

03bbd30

Signed-off-by: Nicolas Thiebaud <nicothieb@google.com>

nt dismissed mattklein123’s stale review via 03bbd30 July 2, 2018 21:00

htuch approved these changes Jul 2, 2018

View reviewed changes

mattklein123 approved these changes Jul 2, 2018

View reviewed changes

mattklein123 merged commit e341004 into envoyproxy:master Jul 2, 2018

rlenglet mentioned this pull request Jul 30, 2018

Support Envoy's Incremental xDS protocol in Cilium Agent's xDS server cilium/cilium#5051

Open

2 tasks

fredlas mentioned this pull request Nov 7, 2018

Implement incremental xDS in Envoy #4991

Closed


		![Incremental reconnect example](diagrams/incremental-reconnect.svg)

		The incremental xDS method may be added to CDS and RDS in the future.

Conversation

nt commented May 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zuercher 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!

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!

mattklein123 commented May 23, 2018

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!

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!

mattklein123 commented May 30, 2018

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!

nt commented Jun 5, 2018

Uh oh!

mattklein123 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nt commented May 23, 2018 •

edited

Loading