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.

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

admin: add streaming variant of the admin API #32346

Merged
merged 44 commits into from
Mar 18, 2024

Conversation

jmarantz
Copy link
Contributor

@jmarantz jmarantz commented Feb 13, 2024

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)

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

Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #32346 was opened by jmarantz.

see: more, trace.

Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #32346 was synchronize by jmarantz.

see: more, trace.

@jmarantz jmarantz marked this pull request as ready for review February 21, 2024 03:21
@jmarantz
Copy link
Contributor Author

@RyanTheOptimist the coverage change is actually to increase the coverage expectation. I assume that should be an easy one.

Copy link
Contributor

@RyanTheOptimist RyanTheOptimist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

coverage lgtm!

source/exe/main_common.cc Outdated Show resolved Hide resolved
source/exe/main_common.cc Outdated Show resolved Hide resolved
source/exe/main_common.cc Outdated Show resolved Hide resolved
Signed-off-by: Joshua Marantz <[email protected]>
@jmarantz jmarantz dismissed ravenblackx’s stale review March 1, 2024 04:11

addressed comments

Copy link
Contributor

@ravenblackx ravenblackx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Realizing posting a comment just as a comment in a thread doesn't act like a review and doesn't show up in the right place in the conversation timeline so it's easily missed/lost. So this is a request for change connected to this long comment.

jmarantz added 2 commits March 6, 2024 19:48
Signed-off-by: Joshua Marantz <[email protected]>
Signed-off-by: Joshua Marantz <[email protected]>
@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 7, 2024

I got the thread-synchronizer strategy to work but decided it was more verbose and harder to read than the interlock call, so I dropped it.

Copy link
Contributor Author

@jmarantz jmarantz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ptal

source/exe/admin_response.cc Show resolved Hide resolved
@@ -0,0 +1,10 @@
State Action Next State Side Effects
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually the mermaid diagrams look pretty cool and I will make a note to try them next time, but I propose that what I have is sufficient, and I think it might take a while to get the mermaid diagram to express that cyclic behavior and all the annotations I added to the graphics.

@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 7, 2024

Filed #32769 for test flake in ext_proc

/retest

@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 7, 2024

/retest

@jmarantz jmarantz dismissed ravenblackx’s stale review March 7, 2024 17:26

addressed comment

@jmarantz
Copy link
Contributor Author

jmarantz commented Mar 7, 2024

/retest

@jmarantz
Copy link
Contributor Author

@tyxia @ravenblackx are there more comments? Thanks!

Copy link
Member

@tyxia tyxia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great!
Thanks

Copy link
Contributor

@ravenblackx ravenblackx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the fixes.

@jmarantz
Copy link
Contributor Author

Thansk for the great comments, @ravenblackx and @tyxia .

Greg can you do a senior maintainer pass? Thanks!

Copy link
Contributor

@ggreenway ggreenway left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great documentation and test coverage.

Long ago, we had a discussion about creating an admin http filter in a normal (non-main-thread) http listener. It looks possible that we could use the chunking/buffering mechanism you've built here to implement such a thing. WDYT?

@jmarantz jmarantz merged commit 87655e5 into envoyproxy:main Mar 18, 2024
53 checks passed
@jmarantz jmarantz deleted the admin-streaming-c++-api branch March 18, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

admin: streaming C++ API
6 participants