server: add handler for dumping out eds by Rainton · Pull Request #11577 · envoyproxy/envoy

Rainton · 2020-06-12T22:24:36Z

Commit Message:
/config_dump API now supports dumping out EDS while using parameter ?include_eds
Add help method dumpEndpointConfigs() to dump out EDS in /config_dump by calling this method in the handler handlerConfigDump()
This will dump out envoy::admin::v3::EndpointsConfigDump by generating envoy::config::endpoint::v3::ClusterLoadAssignment based on data stored in server_.clusterManager().clusters()

Missing Field:

ClusterLoadAssignment
- Policy
  - endpoint_stale_after

StaticEndpointConfig
- last_updated

DynamicEndpointConfig
- version_info
- last_updated

Additional Description:
Risk Level: Medium
Testing: add unit test, integration test
Docs Changes: operations_admin_interface
Release Notes: N/A

Part of fixing #3362

/cc @fuqianggao @alexburnos

Signed-off-by: Yutong Li yutongli@google.com

Signed-off-by: Yutong Li <yutongli@google.com>

repokitteh-read-only · 2020-06-12T22:24:42Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/.
CC @envoyproxy/api-watchers: FYI only for changes made to api/.

🐱

Caused by: #11577 was opened by Rainton.

see: more, trace.

ramaraochavali · 2020-06-13T04:44:45Z

Should we add additional parameter like ?includeEds and dump this only when it is there?

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton · 2020-06-15T00:42:56Z

Should we add additional parameter like ?includeEds and dump this only when it is there?

For now, just like other xDS, it is implemented as automatically dumping out.
Maybe I can ask @htuch if it is necessary to add this parameter.

Signed-off-by: Yutong Li <yutongli@google.com>

fuqianggao · 2020-06-15T17:41:17Z

source/server/admin/admin.cc

-  for (const auto& key_callback_pair : config_tracker_.getCallbacksMap()) {
+  Envoy::Server::ConfigTracker::CbsMap callbacks_map = std::move(config_tracker_.getCallbacksMap());
+  if (!server_.clusterManager().clusters().empty()) {
+    callbacks_map.emplace("endpoint", [this] { return dumpEndpointConfigs(); });


since dumpEndpontConfigs is not fully completed, should we defer this and changes in addResourceToDump to the next PR? Otherwise, anyone doing config dump will get incomplete EDS. @htuch WDYT?

Yeah, that's fair enough. Or you could use a runtime feature flag to control this behavior. https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#runtime-guarding. Might be overkill.

htuch

It might be worth just doing the full EDS dump support in a single PR. In general, small PRs are good, but it's hard to see how this is going to be tested etc. until we have the more complete implementation.

htuch · 2020-06-16T14:50:31Z

source/server/admin/admin.cc

-  for (const auto& key_callback_pair : config_tracker_.getCallbacksMap()) {
+  Envoy::Server::ConfigTracker::CbsMap callbacks_map = std::move(config_tracker_.getCallbacksMap());
+  if (!server_.clusterManager().clusters().empty()) {
+    callbacks_map.emplace("endpoint", [this] { return dumpEndpointConfigs(); });


Yeah, that's fair enough. Or you could use a runtime feature flag to control this behavior. https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#runtime-guarding. Might be overkill.

htuch · 2020-06-16T14:53:38Z

@Rainton the suggestion from @ramaraochavali is reasonable, since this might dump a ton of data, so being able to make it opt-in is good.

Signed-off-by: Yutong Li <yutongli@google.com>

This reverts commit 2d434b0. Signed-off-by: Yutong Li <yutongli@google.com>

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton · 2020-06-18T06:25:55Z

/retest

repokitteh-read-only · 2020-06-18T06:25:58Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11577 (comment) was created by @Rainton.

see: more, trace.

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton · 2020-06-18T18:15:53Z

@htuch I've updated a more complete version. For now, EDS config can be dumped completely except the missing fields with the parameter ?includeEds.

As mentioned in the description, these fields are missing in the ClusterManager. Will it be worth adding track to them?

ClusterLoadAssignment
- Policy
  - endpoint_stale_after

StaticEndpointConfig
- last_updated

DynamicEndpointConfig
- version_info
- last_updated

fuqianggao · 2020-06-19T00:46:16Z

api/envoy/admin/v3/config_dump.proto


  message DynamicEndpointConfig {
-    // This is the per-resource version information. This version is currently taken from the
+    // [#not-implemented-hide:] This is the per-resource version information. This version is currently taken from the


If this is not implemented, probably the second sentence can be removed?

source/server/admin/admin.cc

test/server/admin/admin_test.cc

param, rename shouldIncludeEdsInDump() Signed-off-by: Yutong Li <yutongli@google.com>

Signed-off-by: Yutong Li <yutongli@google.com>

htuch

LGTM modulo remaining nits.

source/server/admin/admin.h

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton · 2020-06-25T06:33:04Z

/retest

repokitteh-read-only · 2020-06-25T06:33:07Z

🐴 hold your horses - no failures detected, yet.

🐱

Caused by: a #11577 (comment) was created by @Rainton.

see: more, trace.

Signed-off-by: Yutong Li <yutongli@google.com>

htuch

LGTM, thanks!

EDS config has been added to config_dump in envoyproxy#11425 and implemented in envoyproxy#11577 . Risk Level: Low Testing: N/A Signed-off-by: Yutong Li <yutongli@google.com>

/config_dump API now supports dumping out EDS while using parameter ?include_eds Add help method dumpEndpointConfigs() to dump out EDS in /config_dump by calling this method in the handler handlerConfigDump() This will dump out envoy::admin::v3::EndpointsConfigDump by generating envoy::config::endpoint::v3::ClusterLoadAssignment based on data stored in server_.clusterManager().clusters() Missing Field: - ClusterLoadAssignment - Policy - endpoint_stale_after - StaticEndpointConfig - last_updated - DynamicEndpointConfig - version_info - last_updated Risk Level: Medium Testing: add unit test, integration test Docs Changes: operations_admin_interface Release Notes: N/A Part of fixing envoyproxy#3362 Signed-off-by: Yutong Li <yutongli@google.com>

EDS config has been added to config_dump in envoyproxy#11425 and implemented in envoyproxy#11577 . Risk Level: Low Testing: N/A Signed-off-by: Yutong Li <yutongli@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

/config_dump API now supports dumping out EDS while using parameter ?include_eds Add help method dumpEndpointConfigs() to dump out EDS in /config_dump by calling this method in the handler handlerConfigDump() This will dump out envoy::admin::v3::EndpointsConfigDump by generating envoy::config::endpoint::v3::ClusterLoadAssignment based on data stored in server_.clusterManager().clusters() Missing Field: - ClusterLoadAssignment - Policy - endpoint_stale_after - StaticEndpointConfig - last_updated - DynamicEndpointConfig - version_info - last_updated Risk Level: Medium Testing: add unit test, integration test Docs Changes: operations_admin_interface Release Notes: N/A Part of fixing envoyproxy#3362 Signed-off-by: Yutong Li <yutongli@google.com> Signed-off-by: yashwant121 <yadavyashwant36@gmail.com>

Added an node field to csds request to identify the CSDS client to the CSDS server, and removed the [#not-implemented-hide:] for the endpoint_config since it has been implemented in #11577 Risk Level: Low Testing: N/A Docs Changes: N/A Release Notes: N/A Signed-off-by: Yutong Li <yutongli@google.com>

admin: add handler for dumping out eds

50d00dd

Signed-off-by: Yutong Li <yutongli@google.com>

repokitteh-read-only bot added the api label Jun 12, 2020

mattklein123 assigned htuch Jun 14, 2020

admin: add exist check before dumping out eds

3e2771a

Signed-off-by: Yutong Li <yutongli@google.com>

admin: update cluster_name in eds dump handler

f3acc0f

Signed-off-by: Yutong Li <yutongli@google.com>

fuqianggao reviewed Jun 15, 2020

View reviewed changes

htuch reviewed Jun 16, 2020

View reviewed changes

Rainton added 6 commits June 17, 2020 03:16

server: complete eds dump handler in config dump

821c3a8

Signed-off-by: Yutong Li <yutongli@google.com>

api: fix [#not-implemented-hide:] in /config_dump

9092bc4

Signed-off-by: Yutong Li <yutongli@google.com>

test: fix unit test and integration test of config dump

69c3406

Signed-off-by: Yutong Li <yutongli@google.com>

test: fix integration test for config_dump with eds

b0adffe

Signed-off-by: Yutong Li <yutongli@google.com>

Merge branch 'feature' of https://github.com/Rainton/envoy into feature

2a1e441

server: fix format issue

2d434b0

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton requested a review from dio as a code owner June 17, 2020 18:06

Rainton added 4 commits June 17, 2020 18:13

Revert "server: fix format issue"

385c4d6

This reverts commit 2d434b0. Signed-off-by: Yutong Li <yutongli@google.com>

server: fix format issue in BUILD

2a86785

Signed-off-by: Yutong Li <yutongli@google.com>

server: add pointer check to eds dump handler

725c5e2

Signed-off-by: Yutong Li <yutongli@google.com>

server: add pointer check and unit test for eds config dump

4011428

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton added 2 commits June 18, 2020 06:29

Kick CI

5d17439

Signed-off-by: Yutong Li <yutongli@google.com>

server: fix admin test

06ca891

Signed-off-by: Yutong Li <yutongli@google.com>

Rainton requested a review from htuch June 18, 2020 18:16

fuqianggao reviewed Jun 19, 2020

View reviewed changes

Rainton mentioned this pull request Jun 19, 2020

api: add eds config to csds proto #11662

Merged

update docs, invert logic in dumpEndpointConfigs(), remove const on bool

adfffa4

param, rename shouldIncludeEdsInDump() Signed-off-by: Yutong Li <yutongli@google.com>

Rainton reopened this Jun 24, 2020

repokitteh-read-only bot removed the waiting label Jun 24, 2020

Rainton added 2 commits June 24, 2020 12:07

Merge branch 'master' into feature

db4e23a

fix logic

3723189

Signed-off-by: Yutong Li <yutongli@google.com>

htuch reviewed Jun 24, 2020

View reviewed changes

source/server/admin/admin.h Outdated Show resolved Hide resolved

Rainton added 2 commits June 24, 2020 23:19

Kick CI

6d31bcc

Signed-off-by: Yutong Li <yutongli@google.com>

flip the logic and drop the const

3a39461

Signed-off-by: Yutong Li <yutongli@google.com>

Kick CI

14938c8

Signed-off-by: Yutong Li <yutongli@google.com>

htuch approved these changes Jun 25, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jun 25, 2020

htuch merged commit 3cec62a into envoyproxy:master Jun 25, 2020

lizan mentioned this pull request Jul 10, 2020

use include_eds in config_dump for proxy istio/istio#25359

Closed

Rainton mentioned this pull request Aug 6, 2020

api: add node field to csds request #12522

Merged

htuch mentioned this pull request Sep 16, 2020

Add EDS to /config_dump #3362

Closed

Conversation

Rainton commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jun 12, 2020

Uh oh!

ramaraochavali commented Jun 13, 2020

Uh oh!

Rainton commented Jun 15, 2020

Uh oh!

fuqianggao Jun 15, 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!

htuch Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

htuch commented Jun 16, 2020

Uh oh!

Rainton commented Jun 18, 2020

Uh oh!

repokitteh-read-only bot commented Jun 18, 2020

Uh oh!

Rainton commented Jun 18, 2020

Uh oh!

fuqianggao Jun 19, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Rainton commented Jun 25, 2020

Uh oh!

repokitteh-read-only bot commented Jun 25, 2020

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Rainton commented Jun 12, 2020 •

edited

Loading

fuqianggao Jun 15, 2020 •

edited

Loading