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

Merge branch 'main' into admin-streaming-c++-api

4fa21ea
Select commit
Loading
Failed to load commit list.
Merged

admin: add streaming variant of the admin API #32346

Merge branch 'main' into admin-streaming-c++-api
4fa21ea
Select commit
Loading
Failed to load commit list.
CI (Envoy) / Envoy/Publish and verify succeeded Mar 7, 2024 in 1h 21m 50s

Envoy/Publish and verify (success)

Check has finished

Details

Check run finished (success ✔️)

The check run can be viewed here:

Envoy/Publish and verify (pr/32346/main@4fa21ea)

Check started by

Request (pr/32346/main@4fa21ea)

jmarantz @jmarantz 4fa21ea #32346 merge main@8318716

admin: add streaming variant of the admin API

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:

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

Environment

Request variables

Key Value
ref 5979107d40770cba652d4f53861057da487c6c58
sha 4fa21ea
pr 32346
base-sha 8318716
actor jmarantz @jmarantz
message admin: add streaming variant of the admin API...
started 1709832711.180559
target-branch main
trusted false
Build image

Container image/s (as used in this CI run)

Key Value
default envoyproxy/envoy-build-ubuntu:0ca52447572ee105a4730da5e76fe47c9c5a7c64
mobile envoyproxy/envoy-build-ubuntu:mobile-0ca52447572ee105a4730da5e76fe47c9c5a7c64
Version

Envoy version (as used in this CI run)

Key Value
major 1
minor 30
patch 0
dev true