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

admin: streaming C++ API #31755

Closed
jmarantz opened this issue Jan 10, 2024 · 2 comments · Fixed by #32346
Closed

admin: streaming C++ API #31755

jmarantz opened this issue Jan 10, 2024 · 2 comments · Fixed by #32346
Assignees
@jmarantz
Labels
area/admin area/perf enhancement Feature requests. Not bugs or questions. no stalebot Disables stalebot from closing an issue

Comments

@jmarantz
Copy link
Contributor

Currently the admin API is designed so the implementation must collect and return the entire response in one contiguous block. This means that very long responses such as /stats and /config_dump may introduce significant delay and memory pressure.

Specificallly:

envoy/source/exe/main_common.h

Line 52 in 7bd0c0f

void adminRequest(absl::string_view path_and_query, absl::string_view method,

envoy/envoy/server/admin.h

Line 254 in 7bd0c0f

virtual Http::Code request(absl::string_view path_and_query, absl::string_view method,

These two interfaces should be changed so they allow for streaming. Note that we have a dependence on the current non-streaming versions so we might need to have both old and new versions at least temporarily.

This is related to #31082
@jmarantz jmarantz added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Jan 10, 2024
@phlax phlax added area/perf area/admin and removed triage Issue requires triage labels Jan 11, 2024
This was referenced Jan 25, 2024
Open
Open
@jmarantz jmarantz self-assigned this Jan 25, 2024
@jmarantz
Copy link
Contributor Author

I have a prototype of this interface locally. Want to get some internal review then I'll publish a PR.

@jmarantz jmarantz mentioned this issue Feb 21, 2024
Merged
@github-actions GitHub Actions
Copy link

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 "no stalebot" or other activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Feb 25, 2024
@jmarantz jmarantz added no stalebot Disables stalebot from closing an issue and removed stale stalebot believes this issue/PR has not been touched recently labels Feb 25, 2024
jmarantz added a commit that referenced this issue Mar 18, 2024
@jmarantz
admin: add streaming variant of the admin API (#32346)
87655e5 
Commit Message: In #19693 a streaming stats implementation was introduced, dramatically reducing the amount of time and memory it takes to generate an admin http response.  /config_dump (#32054) and /clusters_ (#32054) can also be improved in this way.

However, in some environments we do not want to expose an HTTP port for admin requests, and we must use the C++ API. However that API is not streaming: it buffers the entire content. This PR adds a new streaming API.

Mechanically this new functionality was easy to add as an externally callble C++ API as the Admin class already supported this model on behalf of /stats.

However there's a fair amount of complexity managing potential races between active streaming requests, server exit, and explicit cancellation of an in-progress request. So much of the effort and complexity in this PR is due to making that thread-safe and testing it.

Additional Description:

![Life of an AdminResponse (1)](https://github.com/envoyproxy/envoy/assets/1942589/28387991-406c-45d4-812e-ccd1be910c36)


Note to reviewers: if this PR is too big it can be broken into 2. The significant additions to main_common.cc, main_common.h, and main_common_test.cc need to stay together in one PR, the rest of the changes are non-functional refactors which are needed for the larger changes to work; they could be reviewed and merged first.

Risk Level: medium -- this is a new API but it does refactor some existing functionality. Using the new functionality will add some risk also as there is some careful thought required to believe we have protected against all possibilities of shutdown/cancel/admin-request races.
Testing: //test/..., and lots of tsan repeated tests for the new streaming tests. Lots of iteration to ensure every new line of main_common.cc is hit by coverage tests.
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes: #31755

Signed-off-by: Joshua Marantz <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@jmarantz jmarantz

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

Successfully merging a pull request may close this issue.

admin: add streaming variant of the admin API jmarantz/envoy
2 participants
@phlax @jmarantz