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.

Sign up for GitHub

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

Jump to bottom

Add EDS to /config_dump #3362

Closed
mattklein123 opened this issue May 11, 2018 · 12 comments
Closed

Add EDS to /config_dump #3362

mattklein123 opened this issue May 11, 2018 · 12 comments
Assignees
@Rainton
Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@mattklein123
Copy link
Member

This is a follow up from #3340. We should add EDS support also to the config_dump admin endpoint. Some thought needs to be put into how this will be managed given that on its own showing EDS output disjoint from cluster output is not that useful. If a cluster is an EDS cluster, should we dump the EDS configuration as a nested item in the cluster output?

Note that host information is available in the /clusters endpoint which is why this was not implemented with as high of an urgency.
@mattklein123 mattklein123 added enhancement Feature requests. Not bugs or questions. help wanted Needs help! labels May 11, 2018
@mattklein123 mattklein123 mentioned this issue May 11, 2018
Merged
mattklein123 added a commit that referenced this issue May 11, 2018
@mattklein123
admin: complete /config_dump (#3340)
a2a5fc1 
This completes initial implementation of the /config_dump admin endpoint,
allowing for detailed display of the currently loaded Envoy configuration,
including versions of each resource. Note that this does not include output
for EDS. That work is tracked in #3362.

Fixes #2421
Fixes #2172

*Risk Level*: Low 
*Testing*: Unit/integration
*Docs Changes*: Added
*Release Notes*: Added

Signed-off-by: Matt Klein <[email protected]>
mattklein123 pushed a commit to envoyproxy/data-plane-api that referenced this issue May 11, 2018
admin: complete /config_dump (#3340)
2bbed54 
This completes initial implementation of the /config_dump admin endpoint,
allowing for detailed display of the currently loaded Envoy configuration,
including versions of each resource. Note that this does not include output
for EDS. That work is tracked in envoyproxy/envoy#3362.

Fixes envoyproxy/envoy#2421
Fixes envoyproxy/envoy#2172

*Risk Level*: Low
*Testing*: Unit/integration
*Docs Changes*: Added
*Release Notes*: Added

Signed-off-by: Matt Klein <[email protected]>

Mirrored from https://github.com/envoyproxy/envoy @ a2a5fc1df1541ebe0b0cd761f3e77212dbf70e5f
@plombardi89 plombardi89 mentioned this issue May 21, 2018
Closed
@ramaraochavali ramaraochavali mentioned this issue Jun 27, 2018
Closed
@liamawhite liamawhite mentioned this issue Oct 27, 2019
Merged
@hennna
Copy link
Contributor

hennna commented Dec 5, 2019 •
edited
Loading

@htuch looks like there is an existing issue regarding eds config dump. @mattklein123 I was looking into getting a dump of effective eds weights but it seems that the /clusters endpoint dumps the endpoint weight. What would it take to get more eds info into the /clusters endpoint?

@htuch
Copy link
Member

htuch commented Dec 5, 2019

I've had other folks ask for this as well. Would be great to get someone to take on this work.

I think even disjoint from cluster, there is some value here.

@mattklein123
Copy link
Member Author

Agreed we should fix this and get EDS into config_dump. @hennna do you want to pick this up?

@hennna
Copy link
Contributor

hennna commented Dec 5, 2019

It doesn't look like there's much time in the coming weeks. But are there parts of the code I can familiarize myself with to see how much work is involved?

@hennna
Copy link
Contributor

hennna commented Dec 5, 2019

We also may be able to dump this information in the control plane.

@mattklein123
Copy link
Member Author

mattklein123 commented Dec 6, 2019 •
edited
Loading

@hennna take a look at ConfigTracker and its use around the code base for allowing dumping of RDS, etc. When I originally opened this issue, I think I was concerned about EDS being disjoint from CDS, but TBH, I don't think it matters as the cluster_name is embedded in the EDS/CLA response. I think "all you need to do" is just plumb the right tracker into the EDS machinery and then allow dumping.

@incfly
Copy link
Contributor

incfly commented Mar 23, 2020

/cc @incfly

@HeathLee
Copy link

Any update on the progress?

@htuch
Copy link
Member

htuch commented May 27, 2020

@Rainton will be working on this over the summer.

@htuch htuch added no stalebot Disables stalebot from closing an issue and removed help wanted Needs help! labels May 27, 2020
Rainton added a commit to Rainton/envoy that referenced this issue Jun 2, 2020
@Rainton
config dump: add EDS (envoyproxy#3362)
ff04940 
Signed-off-by: Yutong Li <[email protected]>
@htuch
Copy link
Member

htuch commented Jun 3, 2020

@mattklein123 re: "If a cluster is an EDS cluster, should we dump the EDS configuration as a nested item in the cluster output?"

How about we make EDS standalone ClusterLoadAssignment and only do this for EDS clusters in the config dump?

@mattklein123
Copy link
Member Author

How about we make EDS standalone ClusterLoadAssignment and only do this for EDS clusters in the config dump?

Yes see #3362 (comment). I think we should just do an independent dump here as the dump will have the cluster name, so IMO it's not needed to have it be nested.

@Rainton Rainton mentioned this issue Jun 3, 2020
Merged
htuch pushed a commit that referenced this issue Jun 4, 2020
api: add eds to config dump (#11425)
192cd13 
Add EndpointsConfigDump message to support EDS in config_dump.proto
(not implemented in Envoy)

Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

This is the first step to solve #3362

Signed-off-by: Yutong Li <[email protected]>
@Rainton Rainton mentioned this issue Jun 12, 2020
Merged
htuch pushed a commit that referenced this issue Jun 25, 2020
@Rainton
server: add handler for dumping out eds (#11577)
3cec62a 
/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 #3362

Signed-off-by: Yutong Li <[email protected]>
songhu pushed a commit to songhu/envoy that referenced this issue Jun 25, 2020
@Rainton @songhu
server: add handler for dumping out eds (envoyproxy#11577)
7009f49 
/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 <[email protected]>
yashwant121 pushed a commit to yashwant121/envoy that referenced this issue Jul 24, 2020
@yashwant121
api: add eds to config dump (envoyproxy#11425)
a526ee7 
Add EndpointsConfigDump message to support EDS in config_dump.proto
(not implemented in Envoy)

Additional Description:
Risk Level: Low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A

This is the first step to solve envoyproxy#3362

Signed-off-by: Yutong Li <[email protected]>
Signed-off-by: yashwant121 <[email protected]>
yashwant121 pushed a commit to yashwant121/envoy that referenced this issue Jul 24, 2020
@Rainton @yashwant121
server: add handler for dumping out eds (envoyproxy#11577)
19ac001 
/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 <[email protected]>
Signed-off-by: yashwant121 <[email protected]>
@htuch
Copy link
Member

htuch commented Sep 16, 2020

#11577 implemented this capability.

@htuch htuch closed this as completed Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@Rainton Rainton

Labels
enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@incfly @mattklein123 @hennna @HeathLee @htuch @Rainton