Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement incremental xDS in Envoy #4991

Closed
fredlas opened this issue Nov 7, 2018 · 9 comments · Fixed by #7293
Closed

Implement incremental xDS in Envoy #4991

fredlas opened this issue Nov 7, 2018 · 9 comments · Fixed by #7293
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Milestone

Comments

@fredlas
Copy link
Contributor

fredlas commented Nov 7, 2018

The incremental xDS design has been decided on, and added to the appropriate .proto file (in #3470). Now, incremental xDS clients should be implemented in Envoy. I am currently getting started on CDS.

Relevant Links: an early draft proposal for incremental xDS, still relevant for examples and overview of goals: https://docs.google.com/document/d/1dh8ACRzNNUkVYB3exepTPlLQK6CyDrOaP2OwwGlxDB4/edit#heading=h.yfahbzhf2783

@mattklein123 mattklein123 added the enhancement Feature requests. Not bugs or questions. label Nov 8, 2018
@mattklein123 mattklein123 added this to the 1.9.0 milestone Nov 8, 2018
@stale
Copy link

stale bot commented Dec 8, 2018

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 8, 2018
@fredlas
Copy link
Contributor Author

fredlas commented Dec 10, 2018

ongoing

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Dec 10, 2018
@hzxuzhonghu
Copy link
Contributor

Looking forward to this.

@stale
Copy link

stale bot commented Jan 18, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Jan 18, 2019
@fredlas
Copy link
Contributor Author

fredlas commented Jan 18, 2019

In progress! #5466 is going to need a refactor, to happen in a separate PR, to unblock a good deal of additional work that #5466 needs, but things are moving forward.

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 18, 2019
@fredlas
Copy link
Contributor Author

fredlas commented Jan 28, 2019

A list of subtasks to be done in the course of "fully implement incremental xDS", starting with what I am currently working on:

  • Merge a working implementation of incremental CDS in Envoy (WIP: config: implement delta CDS client in Envoy #5466)
  • Implement the delta-style onConfigUpdate callback in all xDS implementations
  • Change the request/response protobufs to support some form of partial resource rejection (config: partial rejections in incremental xDS #5739)
  • Implement ADS, and presumably rewrite non-aggregated incremental xDSes as "ADS that happens to only carry one xDS", like how standard xDS currently is
  • Unify implementations (including aggregated) of state-of-the-world and delta

@stale
Copy link

stale bot commented Feb 27, 2019

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or other activity occurs. Thank you for your contributions.

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 27, 2019
@snowp snowp added the no stalebot Disables stalebot from closing an issue label Mar 1, 2019
@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Mar 1, 2019
htuch pushed a commit that referenced this issue Mar 7, 2019
Description: Code for Envoy to speak delta CDS with a management server. DELTA_GRPC added to config_source.proto's ApiTypes, to allow bootstrap configs to ask for incremental xDS.

Part of #4991. Was #5466; giving up on broken DCO craziness.

Risk Level: medium
Testing: new integration test

Signed-off-by: Fred Douglas <fredlas@google.com>
@mattklein123 mattklein123 modified the milestones: 1.10.0, 1.11.0 Mar 20, 2019
htuch pushed a commit that referenced this issue Apr 19, 2019
…otocol spec (#6545)

 realized that, with the unreliable queue implementation copied from SotW xDS, delta xDS could get into a state where Envoy thinks it has subscribed, but the server hasn't heard the subscription, with no way for either to realize the mistake. I fixed that by converting the queue setup to a cleaner "do I currently want to send a request?" with the request's (un)subscriptions only populated immediately before the request is actually sent into gRPC.

While doing that, I further realized there was a problem when a given resource was subscribed then unsubscribed (or reversed), all in between request sends. I made sure Envoy handles that sensibly, and added explicit requirements to the xDS protocol spec to ensure servers will also handle it sensibly.

Added unit tests for those fixes.

Risk Level: low
Testing: added unit tests for bugs uncovered

#4991

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue May 5, 2019
Split out about half of DeltaSubscriptionImpl into DeltaSubscriptionState, one of which is owned by DeltaSubscriptionImpl. The upcoming aggregated implementation will aggregate-ize the remaining code of DeltaSubscriptionImpl (in a new class with a new name), and replace the current single DeltaSubscriptionState instance with a map of them.

Risk Level: low
Testing: existing delta xDS unit test is almost entirely for the split out logic

#4991 #5270

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue May 15, 2019
Risk Level: none, adding unused proto field

#4991

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue May 20, 2019
Introduce a unified "generic" version of compareDiscoveryRequest and sendDiscoveryResponse for use in integration tests, allowing parameterization of SotW/delta. #4991

Risk Level: none, test only
Testing: modified cds_ and ads_integration_test, integration/integration.cc.

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue May 23, 2019
Adding support for the new delta-style onConfigUpdate everywhere. Going by the comments, we had planned to have just this onConfigUpdate, rather than both it and the state-of-the-world style one. However, when I investigated unifying them it didn't make sense; I think both are needed. Part of #4991.

Risk Level: low
Testing: added tests for the delta style onConfigUpdate in CDS and EDS

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue May 31, 2019
Delta services other than DeltaClusters were missing. Also added those services to proto_descriptors.cc. Also added some other things that proto_descriptors.cc was missing. #4991

Risk Level: low

Signed-off-by: Fred Douglas <fredlas@google.com>
htuch pushed a commit that referenced this issue Jun 4, 2019
#7092)

Previously, constant strings like "envoy.api.v2.EndpointDiscoveryService.FetchEndpoints" were all over eds.cc, rds_impl.cc, etc, being fed into SubscriptionFactory. Now, they are all centralized in SubscriptionFactory, the only place that actually uses them. #4991

Risk Level: low

Signed-off-by: Fred Douglas <fredlas@google.com>
@mattklein123
Copy link
Member

@fredlas @htuch what's the current status of this issue?

@fredlas
Copy link
Contributor Author

fredlas commented Jul 3, 2019

#7293, which depends on #7108, will be marking this issue fixed. As for the nice-to-have "Unify implementations (including aggregated) of state-of-the-world and delta" checkbox item, I have some not-yet-in-PR code written up that will accomplish that cleanly and simply.

@alyssawilk alyssawilk modified the milestones: 1.11.0, 1.12.0 Jul 10, 2019
htuch pushed a commit that referenced this issue Aug 13, 2019
To be used with delta ADS. Could probably be used with the current GrpcMuxImpl. Has the SubscriptionCallbacks interface, so a GrpcMux can just directly pass onConfigUpdate() calls through to the WatchMap, which will then appropriately distribute the various resources to the various watches' SubscriptionCallbacks. #4991

Risk Level: none, not yet built into Envoy
Testing: unit tests for the new class

Signed-off-by: Fred Douglas <fredlas@google.com>
lizan pushed a commit that referenced this issue Oct 1, 2019
Delta ADS/xDS support. Added NewGrpcMuxImpl, a GrpcMux implementation that supports delta (and is intended to soon take over for state-of-the-world). DeltaSubscriptionImpl (which can soon be renamed to/replace GrpcSubscriptionImpl) now uses that new class. NewGrpcMuxImpl is the first user of the recently added WatchMap.

Risk Level: medium (huge new thing that people aren't using yet)
Testing: added delta param variant to ads_integration_test
Docs Changes: added source/common/config/README.md and accompanying diagram, added to root/configuration/overview.rst and root/intro/arch_overview/operations/dynamic_configuration.rst.
Release Notes: added support for delta xDS (including ADS) delivery
Fixes #4991

Signed-off-by: Fred Douglas <fredlas@google.com>
danzh2010 pushed a commit to danzh2010/envoy that referenced this issue Oct 4, 2019
Delta ADS/xDS support. Added NewGrpcMuxImpl, a GrpcMux implementation that supports delta (and is intended to soon take over for state-of-the-world). DeltaSubscriptionImpl (which can soon be renamed to/replace GrpcSubscriptionImpl) now uses that new class. NewGrpcMuxImpl is the first user of the recently added WatchMap.

Risk Level: medium (huge new thing that people aren't using yet)
Testing: added delta param variant to ads_integration_test
Docs Changes: added source/common/config/README.md and accompanying diagram, added to root/configuration/overview.rst and root/intro/arch_overview/operations/dynamic_configuration.rst.
Release Notes: added support for delta xDS (including ADS) delivery
Fixes envoyproxy#4991

Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants