admin/xDS: prepare for full /config_dump and version support #3199

mattklein123 · 2018-04-24T23:29:07Z

This change does several things:

Clarifies how we handle xDS version_info in responses and sets us up
for both top-level/transactional versions as well as per-resource
versions in the future.
Moves the config_dump admin endpoint to the v2alpha namespace so that
we can iterate on it in the future.
Fills out the config dump proto for the remaining resource types.
These are not implemented but are here to force a discussion about
how we want to handle versions moving forward.
Fixes RDS static config dump to actually work and add better tests.
Wire up version for the RDS config dump on a per-resource basis.

Once we agree on the general version semantics I will be following up
with dump capability of the remaining resource types.

Part of #2421
Part of #2172
Fixes #3141

Risk Level: Low
Testing: Fixed and improved tests
Docs Changes: N/A
Release Notes: N/A

This change does several things: 1) Clarifies how we handle xDS version_info in responses and sets us up for both top-level/transactional versions as well as per-resource versions in the future. 2) Moves the config_dump admin endpoint to the v2alpha namespace so that we can iterate on it in the future. 3) Fills out the config dump proto for the remaining resource types. These are not implemented but are here to force a discussion about how we want to handle versions moving forward. 4) Fixes RDS static config dump to actually work and add better tests. 5) Wire up version for the RDS config dump on a per-resource basis. Once we agree on the general version semantics I will be following up with dump capability of the remaining resource types. Part of #2421 Part of #2172 Fixes #3141 Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-04-24T23:29:52Z

@htuch @nt PTAL. This will hopeful give some context on my concerns about versioning. It became a lot more clear to me as I was figuring out how to implement this change.

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch

Overall, looks good, agree with the approach.

htuch · 2018-04-25T17:50:16Z

api/envoy/admin/v2alpha/config_dump.proto

+
+  // The dynamically loaded draining listeners These are listeners that are currently undergoing
+  // draining in preparation to stop servicing data plane traffic.
+  repeated DynamicListener dynamic_draining_listeners = 5 [(gogoproto.nullable) = false];


I'm wondering how this all ties together with the end objective of being able to dump and then relaunch an Envoy with thee same configuration? It's hard to bring back into the precise warming/draining state, I don't think you want to either. It would be good to document how each field is to be interpreted, e.g. "throw away the draining listeners when recreating config, take the warming and active and static, merge them together and then deliver in static config or LDS", etc.

Sure, I can add some more comments. I do think it's important to dump warming/draining/etc. since it gives a much more complete view of what is going on. I do also think that we should add config_dump query params that allow filtering out certain types of data which might make reload easier.

@htuch "being able to dump and then relaunch an Envoy with thee same configuration" - are we expecting people build tools around this to this work? or what is the exact usecase? Sorry I do not have context of this PR hence this question

@ramaraochavali yes, ultimately, we think it would be pretty awesome if there was a tool that could take the output of config_dump and produce a relatively close approximation of the config using only static resources. Would be great for debugging.

htuch · 2018-04-25T17:50:54Z

api/envoy/admin/v2alpha/config_dump.proto

+
+  // Describes a dynamically loaded cluster via the LDS API.
+  message DynamicListener {
+    // This is the top level, "transactional," version information in the last processed LDS


Why do dynamic listeners have the top-level transactional version, rather than the per-resource version?

Typo, will fix.

htuch · 2018-04-25T17:53:37Z

api/envoy/admin/v2alpha/config_dump.proto

+
+  // Describes a dynamically loaded cluster via the CDS API.
+  message DynamicCluster {
+    // This is the per-resource version information. This version is currently taken from the


... as opposed to DynamicListener, where it's still transactional?

Typo in the other place.

htuch · 2018-04-25T17:54:17Z

api/envoy/admin/v2alpha/config_dump.proto

+
+  // The dynamically loaded route configs.
+  repeated DynamicRouteConfig dynamic_route_configs = 3 [(gogoproto.nullable) = false];
+}


No ClusterLoadAssignment (i.e. EDS) yet?

I would like to think about adding that in a follow up, as we need to reconcile against the existing /clusters endpoint.

htuch · 2018-04-25T17:55:10Z

include/envoy/router/rds.h

@@ -13,31 +13,31 @@ namespace Router {
 */
 class RouteConfigProvider {
 public:
+  struct ConfigInfo {
+    // A reference to the currently loaded route configuration. Do not hold this reference beyond
+    // the caller's scope.


Nit: which caller?

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-04-25T21:30:33Z

@htuch PTAL

junr03 · 2018-04-25T22:59:43Z

source/common/router/rds_impl.cc

-    dynamic_configs->Add()->MergeFrom(provider->configAsProto());
+    auto config_info = provider->configInfo();
+    if (config_info) {
+      auto* dynamic_config = config_dump->mutable_dynamic_route_configs()->Add();


quick c++ question here. In #3129 Matt grabbed the object by reference from the pointer instead of going along with the returned pointer like you are doing. Can you comment on your preference?

It doesn't really matter, what he did is probably more Envoy style. I will change.

htuch · 2018-04-26T18:43:17Z

api/envoy/admin/v2alpha/config_dump.proto

+message ClustersConfigDump {
+  // This is the top level, "transactional," version information in the last processed CDS
+  // discovery response. If there are only static bootstrap clusters, this field will be "".
+  string version_info = 1;


In a world of incremental xDS, this transaction value might only refer to a CDS response with a single cluster, with no bearing on the other clusters that are present. Am I reading this right?

Yes correct. It's basically the last received response in the top level version_info field (per my suggestion of keeping that field a singleton and adding per-version resources additionally). If a deployment doesn't care about this they can leave it blank.

We can discuss at the meeting today, but I do have some concern about this being racy.

@htuch @nt my plan after our meeting is to not really change this proto, but to change the comments a bit. I will do that sometime this week. Let me know if that sounds wrong.

SGTM, @nt is also planning on updating the doc based on the meeting.

@mattklein123 do you still have other work planned here to followup on comments?

Yes I will update comments today.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-05-07T16:17:08Z

@htuch @nt PTAL. I cleaned up the language a bit and made it more general. I think this will be compatible with wherever we end up taking the incremental API.

htuch

@nt not sure if the public doc is updated?

htuch · 2018-05-07T21:28:36Z

api/envoy/admin/v2alpha/config_dump.proto

+  // This map is serialized and dumped in its entirety at the /config_dump endpoint.
+  //
+  // Keys should be a short descriptor of the config object they map to. For example, Envoy's HTTP
+  // routing subsystem might use "routes" as the key for its config, for which it uses the message


Did we ever discuss using a type URL here instead? Seems like one less need for magic constants.

The type URL doesn't really map exactly to this output, and end-users are not going to be familiar with that detail, so IMO it's best to keep it human readable and disjoint.

What do you think about making it a type URL for machines (e.g. when dumping in pure proto form) and then having a human friendly representation for HTML output?

Sorry I just realize this is an Any. It already has the type in it. Is that good enough?

Yes, so should it just be a repeated instead of map?

The map gives us the friendly name. And in the future I think we want to be able to do something like:
config_dump?name=routes

I suppose we could document all of the full type names, but that seems kind of ugly. TBH I don't feel that strongly about it. If you would rather lose the friendly name for now I can kill it, make it repeated, and we can figure out a friendly name later if needed.

htuch · 2018-05-07T21:30:04Z

api/envoy/admin/v2alpha/config_dump.proto

+
+// This message describes the bootstrap configuration that Envoy was started with. This includes
+// any CLI overrides that were merged. Bootstrap configuration information can be used to recreate
+// an Envoy configuration by reusing the output as the bootstrap configuration for another Envoy.


Maybe explain that this only recreates the static aspects of configuration (e.g. it's not like a checkpoint restore).

htuch · 2018-05-07T21:31:00Z

api/envoy/admin/v2alpha/config_dump.proto

+  // The statically loaded listener configs.
+  repeated envoy.api.v2.Listener static_listeners = 2 [(gogoproto.nullable) = false];
+
+  // The dynamically loaded active listeners These are listeners that are available to service


Nit: Missing punctuation here and below at end of first sentence.

htuch · 2018-05-07T21:32:06Z

api/envoy/admin/v2alpha/config_dump.proto

+
+  // The dynamically loaded warming listeners These are listeners that are currently undergoing
+  // warming in preparation to service data plane traffic. Note that if attempting to recreate an
+  // Envoy configuration from a configuration dump, the warming listeners should generally be


Creating a Python script to rebuild an Envoy configuration would be a very useful exercise in hammering out the intended use patterns for this new API.

I agree, but I'm not going to do that as part of this change. There are a ton of users (including Lyft) who just want to see the current status and don't care about rebuilding anything so my goal is to get that working first.

OK, maybe open an issue around this and that's fine.

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch

LGTM

htuch · 2018-05-07T23:14:47Z

source/common/router/rds_impl.cc

@@ -168,11 +168,15 @@ std::vector<RouteConfigProviderSharedPtr>
 RouteConfigProviderManagerImpl::getStaticRouteConfigProviders() {
  std::vector<RouteConfigProviderSharedPtr> providers_strong;
  // Collect non-expired providers.
-  std::transform(static_route_config_providers_.begin(), static_route_config_providers_.end(),
-                 providers_strong.begin(), [](auto&& weak) { return weak.lock(); });
+  for (const auto& weak_provider : static_route_config_providers_) {


Is there a reason for this change? Is is that the assign line was not right?

The above transform call was not actually filtering on not nullptr, so wasn't doing what it was supposed to do.

Even apart from (1), it was crashing, and I have no idea why. I didn't debug.

IMO the new code is easier to understand and has the nice property of actually working. :)

htuch · 2018-05-07T23:18:53Z

api/envoy/admin/v2alpha/config_dump.proto

+// This message describes the bootstrap configuration that Envoy was started with. This includes
+// any CLI overrides that were merged. Bootstrap configuration information can be used to recreate
+// the static portions of an Envoy configuration by reusing the output as the bootstrap
+// configuration for another Envoy.


Should these be [#not-implemented-hide:]?

There are no docs being output for this page currently, and I'm working on the follow up change right now to add the rest. In that change I will figure out how to print out the docs, so I think probably OK to leave for now?

Sure, up to you.

mattklein123 · 2018-05-08T00:53:41Z

In working on the 2nd part of this change, I think some of the version info handling needs to change here slightly. I will push another update to this tomorrow.

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 · 2018-05-08T01:30:29Z

@htuch updated again. I brought back versionInfo() for just CDS API and LDS API objects which I will end up using in the follow up change.

fix

aa82b53

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch self-assigned this Apr 25, 2018

htuch suggested changes Apr 25, 2018

View reviewed changes

mattklein123 added 2 commits April 25, 2018 16:55

Merge branch 'master' into config_dump

be0034e

Signed-off-by: Matt Klein <mklein@lyft.com>

comments

635bed1

Signed-off-by: Matt Klein <mklein@lyft.com>

junr03 reviewed Apr 25, 2018

View reviewed changes

htuch reviewed Apr 26, 2018

View reviewed changes

mattklein123 added 4 commits May 4, 2018 06:26

Merge branch 'master' into config_dump

37f1987

Signed-off-by: Matt Klein <mklein@lyft.com>

fix

f1be665

Signed-off-by: Matt Klein <mklein@lyft.com>

Merge branch 'master' into config_dump

1082178

Signed-off-by: Matt Klein <mklein@lyft.com>

comments

ed3aae8

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch reviewed May 7, 2018

View reviewed changes

comments

8c41849

Signed-off-by: Matt Klein <mklein@lyft.com>

htuch previously approved these changes May 7, 2018

View reviewed changes

fix

a9ff047

Signed-off-by: Matt Klein <mklein@lyft.com>

mattklein123 dismissed htuch’s stale review via a9ff047 May 8, 2018 01:25

htuch approved these changes May 8, 2018

View reviewed changes

mattklein123 merged commit ada7587 into master May 8, 2018

mattklein123 deleted the config_dump branch May 8, 2018 17:36

mattklein123 mentioned this pull request May 9, 2018

crashed when click config_dump #3329

Closed

admin/xDS: prepare for full /config_dump and version support #3199

admin/xDS: prepare for full /config_dump and version support #3199

Conversation

mattklein123 commented Apr 24, 2018

mattklein123 commented Apr 24, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented Apr 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented May 7, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 commented May 8, 2018

mattklein123 commented May 8, 2018