-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Changes from 38 commits
Commits
Show all changes
44 commits
Select commit
Hold shift + click to select a range
2cb2617
add streaming variant of the admin API
jmarantz d2412c6
remove superfluous diff
jmarantz 48f114d
add streaming test
jmarantz d890eb3
add cancellation semantics and tests.
jmarantz b56ecb4
try to fix coverage and compiling without admin.
jmarantz aed8263
checkpoint
jmarantz 3973c92
comment and cleanup
jmarantz 9237ff9
format
jmarantz 895e552
add asserts, refactor tests, and add quit/cancel race.
jmarantz 4784a23
add cancel/quit race test
jmarantz d252659
tighten up logic and asserts.
jmarantz 8ba301d
Cover a few missing lines.
jmarantz e6a10f3
format
jmarantz 05c1127
hit one more early-exit line in main_common with a contrived test.
jmarantz c40e30a
improve consitency and handling of cancel function.
jmarantz 8cfed12
add early exit in handleHeaders to force it to be tested, and refacto…
jmarantz ce30118
add comments.
jmarantz cd88721
cover more lines.
jmarantz 4262d8d
tighten up coverage
jmarantz 699f357
Merge branch 'main' into admin-streaming-c++-api
jmarantz 47a51ca
mutex-protect destructor accesss to AdminResponse::terminated_. Make …
jmarantz f6edee6
add a TerminateNotifier to provide a context with appropriate lifetim…
jmarantz eda6834
remove superfluous function, fix compile-time issue with admin disabled.
jmarantz 7239d03
add more tests and cleanup
jmarantz 7fe67f3
fix initialization and expected results.
jmarantz 7a433d1
Merge branch 'main' into admin-streaming-c++-api
jmarantz aa70f33
Split out AdminContext into its own source, hdr, test.
jmarantz a6d49b1
remove no-longer-needed 'friend' declarations and rename TerminateNot…
jmarantz 153b2a2
format
jmarantz 36195fe
cleanup & format
jmarantz 4fb7898
fix admin-disabled build
jmarantz 0cf32c1
address some review comments (others remain)
jmarantz 03ce2c7
review comments
jmarantz 48ece2d
remove ifdef'd out test helper.
jmarantz b006d79
add reference to FSM drawing
jmarantz f799b84
grammar
jmarantz e1640d3
fix race.
jmarantz d957b58
Merge branch 'main' into admin-streaming-c++-api
jmarantz 06df4ec
Merge branch 'main' into admin-streaming-c++-api
jmarantz 0e6a275
Better interlock for test infrastructure, hopefully still hitting all…
jmarantz 23caf5e
remove run_before_resume hack.
jmarantz 2427170
clean up
jmarantz bd8db80
Remove half-baked README.md, and add Tianyu's note to PtrSet comment.
jmarantz 4fa21ea
Merge branch 'main' into admin-streaming-c++-api
jmarantz File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
State Action Next State Side Effects | ||
|
||
Ground adminRequest(u,m) ResponseInitial | ||
ResponseInitial getHeaders HeadersWait post to main thread to get headers | ||
HeadersWait postComplete HeadersSent call HeadersFn | ||
HeadersSent | ||
|
||
|
||
ResponseInitial cancel ResponseCancelled | ||
Terminated adminRequest(u,m) ResponseTerminated |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,191 @@ | ||
#include "source/exe/admin_response.h" | ||
|
||
#include "envoy/server/admin.h" | ||
|
||
#include "source/server/admin/admin_filter.h" | ||
#include "source/server/admin/utils.h" | ||
|
||
namespace Envoy { | ||
|
||
AdminResponse::AdminResponse(Server::Instance& server, absl::string_view path, | ||
absl::string_view method, SharedPtrSet response_set) | ||
: server_(server), opt_admin_(server.admin()), shared_response_set_(response_set) { | ||
request_headers_->setMethod(method); | ||
request_headers_->setPath(path); | ||
} | ||
|
||
AdminResponse::~AdminResponse() { | ||
cancel(); | ||
shared_response_set_->detachResponse(this); | ||
jmarantz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
void AdminResponse::getHeaders(HeadersFn fn) { | ||
auto request_headers = [response = shared_from_this()]() { response->requestHeaders(); }; | ||
|
||
// First check for cancelling or termination. | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
ASSERT(headers_fn_ == nullptr); | ||
if (cancelled_) { | ||
return; | ||
} | ||
headers_fn_ = fn; | ||
if (terminated_ || !opt_admin_) { | ||
sendErrorLockHeld(); | ||
return; | ||
} | ||
} | ||
server_.dispatcher().post(request_headers); | ||
} | ||
|
||
void AdminResponse::nextChunk(BodyFn fn) { | ||
auto request_next_chunk = [response = shared_from_this()]() { response->requestNextChunk(); }; | ||
|
||
// Note the caller may race a call to nextChunk with the server being | ||
// terminated. | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
ASSERT(body_fn_ == nullptr); | ||
if (cancelled_) { | ||
return; | ||
} | ||
body_fn_ = fn; | ||
if (terminated_ || !opt_admin_) { | ||
sendAbortChunkLockHeld(); | ||
return; | ||
} | ||
} | ||
|
||
// Note that nextChunk may be called from any thread -- it's the callers choice, | ||
// including the Envoy main thread, which would occur if the caller initiates | ||
// the request of a chunk upon receipt of the previous chunk. | ||
// | ||
// In that case it may race against the AdminResponse object being deleted, | ||
// in which case the callbacks, held in a shared_ptr, will be cancelled | ||
// from the destructor. If that happens *before* we post to the main thread, | ||
// we will just skip and never call fn. | ||
server_.dispatcher().post(request_next_chunk); | ||
} | ||
|
||
// Called by the user if it is not longer interested in the result of the | ||
// admin request. After calling cancel() the caller must not call nextChunk or | ||
// getHeaders. | ||
void AdminResponse::cancel() { | ||
absl::MutexLock lock(&mutex_); | ||
cancelled_ = true; | ||
headers_fn_ = nullptr; | ||
body_fn_ = nullptr; | ||
} | ||
|
||
bool AdminResponse::cancelled() const { | ||
absl::MutexLock lock(&mutex_); | ||
return cancelled_; | ||
} | ||
|
||
// Called from terminateAdminRequests when the Envoy server | ||
// terminates. After this is called, the caller may need to complete the | ||
// admin response, and so calls to getHeader and nextChunk remain valid, | ||
// resulting in 503 and an empty body. | ||
void AdminResponse::terminate() { | ||
ASSERT_IS_MAIN_OR_TEST_THREAD(); | ||
absl::MutexLock lock(&mutex_); | ||
if (!terminated_) { | ||
terminated_ = true; | ||
sendErrorLockHeld(); | ||
sendAbortChunkLockHeld(); | ||
} | ||
} | ||
|
||
void AdminResponse::requestHeaders() { | ||
ASSERT_IS_MAIN_OR_TEST_THREAD(); | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
if (cancelled_ || terminated_) { | ||
return; | ||
} | ||
} | ||
Server::AdminFilter filter(*opt_admin_); | ||
filter.decodeHeaders(*request_headers_, false); | ||
request_ = opt_admin_->makeRequest(filter); | ||
code_ = request_->start(*response_headers_); | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
if (headers_fn_ == nullptr || cancelled_) { | ||
return; | ||
} | ||
Server::Utility::populateFallbackResponseHeaders(code_, *response_headers_); | ||
headers_fn_(code_, *response_headers_); | ||
headers_fn_ = nullptr; | ||
} | ||
} | ||
|
||
void AdminResponse::requestNextChunk() { | ||
jmarantz marked this conversation as resolved.
Show resolved
Hide resolved
|
||
ASSERT_IS_MAIN_OR_TEST_THREAD(); | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
if (cancelled_ || terminated_ || !more_data_) { | ||
return; | ||
} | ||
} | ||
ASSERT(response_.length() == 0); | ||
more_data_ = request_->nextChunk(response_); | ||
{ | ||
absl::MutexLock lock(&mutex_); | ||
if (sent_end_stream_ || cancelled_) { | ||
return; | ||
} | ||
sent_end_stream_ = !more_data_; | ||
body_fn_(response_, more_data_); | ||
ASSERT(response_.length() == 0); | ||
body_fn_ = nullptr; | ||
} | ||
} | ||
|
||
void AdminResponse::sendAbortChunkLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_) { | ||
if (!sent_end_stream_ && body_fn_ != nullptr) { | ||
response_.drain(response_.length()); | ||
body_fn_(response_, false); | ||
sent_end_stream_ = true; | ||
} | ||
body_fn_ = nullptr; | ||
} | ||
|
||
void AdminResponse::sendErrorLockHeld() ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_) { | ||
if (headers_fn_ != nullptr) { | ||
code_ = Http::Code::InternalServerError; | ||
Server::Utility::populateFallbackResponseHeaders(code_, *response_headers_); | ||
headers_fn_(code_, *response_headers_); | ||
headers_fn_ = nullptr; | ||
} | ||
} | ||
|
||
void AdminResponse::PtrSet::terminateAdminRequests() { | ||
ASSERT_IS_MAIN_OR_TEST_THREAD(); | ||
|
||
absl::MutexLock lock(&mutex_); | ||
accepting_admin_requests_ = false; | ||
for (AdminResponse* response : response_set_) { | ||
// Consider the possibility of response being deleted due to its creator | ||
// dropping its last reference right here. From its destructor it will call | ||
// detachResponse(), which is mutex-ed against this loop, so before the | ||
// memory becomes invalid, the call to terminate will complete. | ||
response->terminate(); | ||
} | ||
response_set_.clear(); | ||
} | ||
|
||
void AdminResponse::PtrSet::attachResponse(AdminResponse* response) { | ||
absl::MutexLock lock(&mutex_); | ||
if (accepting_admin_requests_) { | ||
response_set_.insert(response); | ||
} else { | ||
response->terminate(); | ||
} | ||
} | ||
|
||
void AdminResponse::PtrSet::detachResponse(AdminResponse* response) { | ||
absl::MutexLock lock(&mutex_); | ||
response_set_.erase(response); | ||
} | ||
|
||
} // namespace Envoy |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like you might be able to do this as a mermaid sequence diagram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'll leave this pending for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm thinking the graphical FSM is enough and I should revert this. @ravenblackx WDYT?
There was a problem hiding this comment.
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.