Adding ECDS config dump support.#23902
Conversation
Signed-off-by: Yanjun Xiang <[email protected]>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
|
/assign @kyessenov @adisuissa |
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Thanks!
Overall the API LGTM.
2 high level comments/questions:
- Memory consumption could be non-negligible here. Is there anything we can do to only impact those that want to use this feature?
- Should there be a way to filter for a specific ECDS config (e.g., using https://www.envoyproxy.io/docs/envoy/latest/operations/admin#get--config_dump?name_regex=)?
Signed-off-by: Yanjun Xiang <[email protected]>
…lter config dump case Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
Signed-off-by: Yanjun Xiang <[email protected]>
…the for loop in the dumpEcdsConfig() function Signed-off-by: Yanjun Xiang <[email protected]>
Merge branch 'main' of https://github.com/envoyproxy/envoy into ecds_config_dump Signed-off-by: Yanjun Xiang <[email protected]>
|
/retest |
|
Retrying Azure Pipelines: |
|
@envoyproxy/api-shepherds PTAL |
|
/assign @yanavlasov |
adisuissa
left a comment
adisuissa
left a comment
There was a problem hiding this comment.
Overall code changes LGTM.
I would suggest rethinking about the tests, as they seem to be complex and validate multiple things. See if you can make them more hermetic by targeting specific validations.
/lgtm api
test/integration/listener_extension_discovery_integration_test.cc
Outdated
Show resolved
Hide resolved
|
/retest |
|
Retrying Azure Pipelines: |
…config_dump Signed-off-by: Yanjun Xiang <[email protected]>
|
/retest |
|
Retrying Azure Pipelines: |
|
/retest |
|
Retrying Azure Pipelines: |
|
@kyessenov @adisuissa The PR had been updated. Please approve again if no further comments. |
|
@yanavlasov PTAL |
|
I believe this was the other half of breaking coverage for main |
This reverts commit 3752119.
When working on #23902 , we found the code : UpstreamHttpFilterConfigProviderManagerImpl in config_discovery_impl.h is not tested, which is causing the test coverage in that file is low. |
|
Coverage passed on main before your PR, and your PR made additions to a file. I have never seen this then fail without the reason being your new code lowered overall coverage. I'm sorry about this mix up and happy to point you at the coverage results when you try to reinstate it and help you sort out how to do it. The greater problem was this PR was unintentionally merged without the correct approvals so we have to fix that up as well. |
|
yeah looks like you were penalized for 2 comments (stupid sorry) but also didn't test an early return: https://storage.googleapis.com/envoy-postsubmit/main/coverage/source/common/filter/config_discovery_impl.h.gcov.html |
|
Thanks for sharing the coverage link. Yes, let me refactor the code to remove that early return. This is done in most other config dump code. Also removing those two comments line to increase the coverage. |
|
Hi, @alyssawilk , I quickly created this PR to address the test flaky issue in config_discovery_impl.h: #24362 |
History: ECDS config dump is first committed by : envoyproxy#23902. There are some tests issues which is fixed by: envoyproxy#24362 This PR is to: revert envoyproxy#24354, at same time add a couple lines added in envoyproxy#24362, to re-commit ECDS config dump functionality. Signed-off-by: Yanjun Xiang <[email protected]>
…verted by (envoyproxy#24354) This reverts commit c5d6160. Signed-off-by: Yanjun Xiang <[email protected]>
* Adding back ECDS config dump support. (#23902)" which is reverted by (#24354) This reverts commit c5d6160. * Fixing test coverage issue due to an early return and a couple of comment lines. Signed-off-by: Yanjun Xiang <[email protected]>
5 participants
For issue: #20086
Signed-off-by: Yanjun Xiang [email protected]
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]