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: making a streaming version of /clusters #32055

Open
jmarantz opened this issue Jan 25, 2024 · 8 comments
Open

admin: making a streaming version of /clusters #32055

jmarantz opened this issue Jan 25, 2024 · 8 comments
Assignees
Labels
area/admin area/perf enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@jmarantz
Copy link
Contributor

/clusters can produce a remarkably huge response. This will need to be fully buffered as an in-memory continuous string. For large configs, this will be slow and may exert memory pressure as well.

See also #31755 as well as prior work streaming admin stats in #19693. This work is not trivial and has risks. See also the difficult attempt to make /stats/prometheus stream as well: #24998 which needed to be rolled back due to complexities in the Prom format that made streaming difficult.

See also #32054 which is a similar request for /config_dump

@jmarantz jmarantz added enhancement Feature requests. Not bugs or questions. triage Issue requires triage area/perf area/admin help wanted Needs help! and removed triage Issue requires triage labels Jan 25, 2024
@jmarantz
Copy link
Contributor Author

Quick thoughts on how to do this:

/clusters impl is here: https://github.com/envoyproxy/envoy/blob/main/source/server/admin/clusters_handler.cc

first place to look for the streaming API this should implement:

class Request {

The current implementation builds a gigantic json structure that copies all the cluster information, and then builds a string that has another copy of that. So if you are generating 1G of data from /clusters it will hold 2G of that in memory. Goal is to represent none of that in memory, maybe a 100k buffer or so.

One building block I recommend is a streaming json seriazlier, so you can incrementally write out chunks of json without holding the whole model in memory. see https://github.com/envoyproxy/envoy/blob/main/source/common/json/json_streamer.h for a class to help you with that.

/config_dump is similar except that the data model exposed by /clusters is a lot simpler, so I think it will be easier to convert it to a seriazlied model.

@miroswan
Copy link
Contributor

miroswan commented Apr 1, 2024

Hey @jmarantz. Thanks for assigning me this issue and providing some insight into the components to consider.

It appears that the main work to be done is to implement Envoy::Sever::Admin::Request for the Envoy::Sever::ClustersHandler. This will provide implementations for the start and nextChunk member functions on the Envoy::Sever::Admin::Request that processes the relevant clusters data and builds intermediate encoded JSON data into an Envoy::Buffer::Instance using an Envoy::Json::Streamer.

Looking at the OneStatJson stats request test, the test calls response which seems to call nextChunk until nextChunk returns false. It then calls toString on the buffer for verification of the JSON output in the test. Does this mean that we should not be calling drain on the Envoy::Buffer::Instance in each call to nextChunk? My understanding here is that drain truncates the buffer so doing so would result in invalid JSON.

@jmarantz
Copy link
Contributor Author

jmarantz commented Apr 1, 2024

I think from the test perspective you can keep calling nextChunk and not drain the buffer, or you can alternatively drain the buffer on each call to nextChunk, and append to a string in the test. Then you can validate the compete JSON for functionality. You can also test the streaming functionality just by looking at the chunk sizes coming out one at a time.

From an implementation perspective you need to be able to emit partal Json which is why I wrote source/common/json/json_streamer.h .

@miroswan
Copy link
Contributor

miroswan commented Apr 5, 2024

High Level Design Considerations

I have two high-level approaches to this problem. One presents a stateful iteration-based solution that is simple to implement but provides somewhat minimal memory pressure relief. The second provides substantial memory pressure relief modeled as a token stream using simple pub/sub at the cost of a second temporary thread.

Stateful Cluster Iteration

My first approach employs stateful iteration. The working proof of concept can be found in this diff. The idea is to transform the ClusterInfoMap into a vector of reference wrapped Upstream::Cluster instances. A ClusterRenderer is satisfied by two classes: JsonClusterRenderer and TextClusterRenderer. In each implementation, its nextChunk member function loops through an index stored as a class member. If the index is less than the size of the vector and the data added to the Buffer::instance is less than the chunk limit, then process the next Upstream::Cluster and add its data to the Buffer::Instance. At the end of nextChunk, return true if there are still Upstream::Cluster instances remaining to be processed. Otherwise, return false.

The remainder of the implementation follows pretty similarly to the class collaborators in streaming stats rendering but uses the logic of the existing clusters handler.

Advantages

  • Easy to implement
  • One thread of execution

Disadvantages

  • Only provides memory pressure relief if there are many clusters
  • If there are several large clusters, this solution could consume as much memory as the 1 or two largest adjacent clusters in the worst cases.

Token-Like Pub/Sub Stream

The alternative approach employs token-like pub/sub stream processing. In this solution, for each Upstream::Cluster a producer thread iterates through each relevant data point to be emitted and constructs a variant structure to emplace on a queue and places said data point into a queue. The queue can be bound by some small max element quantity. In the consumer, a data payload is popped of the queue. Like in the previous solution, the while loop checks if the current Buffer::instance has exceeded its chunk limit. If not, it calls the appropriate member function to process the matching payload type which will load the Buffer::Instance with the appropriate data. When the size of the Buffer::Instance exceeds the chunk limit or we have no more data to process return true if there are remaining elements to process or false if not.

Advantages

  • Since smaller payloads of data are processed at a time, the chunk limit will only be exceeded by the largest data payload in collection of payloads streamed by the producer in the worst case. This more significantly reduces memory pressure since the data payloads comprise a partition element of the data found in a single Upstream::Cluster.
  • Closer to streaming than batching.

Disadvantages

  • Harder to implement, although the pub/sub pattern is generally well-understood
  • A second thread would be required throughout the duration of all calls to nextChunk until no more data is left

I could probably develop a proof-of-concept of the second option in a branch off of the diff supplied above.

@tonya11en
Copy link
Member

@miroswan chatted with me about this offline, so I wanted to post the relevant bits here.

I think "Stateful Cluster Iteration" is the way to go here, since its simpler and the disadvantages are moot. AFAIK, the memory usage problems are not caused by small numbers of really big clusters, so this will almost certainly be an improvement where we need it.

I can help review the PR whenever you want feedback.

@jmarantz
Copy link
Contributor Author

+1: stateful cluster iteration is a lot more like the /stats request system, which would also be defeated by small numbers of huge clusters. I think we want to avoid more threads if possible, especially if there might be a thread per active request.

@miroswan
Copy link
Contributor

Excellent. I proceed with stateful cluster iteration this week. Thanks for the feedback folks.

@miroswan
Copy link
Contributor

Going to solve this issue and come back once it's merged: #33752

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/admin area/perf enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

3 participants