caching: Stream cached responses in chunks and handle downstream backpressure by yosrym93 · Pull Request #13054 · envoyproxy/envoy

yosrym93 · 2020-09-11T00:04:19Z

Commit Message:
The CacheFilter now streams cached responses in chunks and handles downstream backpressure.
Signed-off-by: Yosry Ahmed yosryahmed@google.com

Additional Description:
The CacheFilter now:

Does not stop headers encoding until the cached response is fetched.
Streams the cached response downstream in chunks, according to the encoding buffer size.
Subscribes to downstream watermark events and handles downstream backpressure by stopping fetching the response body from the cache.

Risk Level: Low
Testing: Unit / Integration
Docs Changes: N/A
Release Notes: N/A
Fixes #9835

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

…ized according to the buffer limit Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 · 2020-09-11T00:16:01Z

Background:

When the CacheFilter served validated cached responses during encoding, it used to stop headers iteration until the cached body is fetched. This incurs an unnecessary delay to headers encoding.
The CacheFilter used to fetch the whole cached response from the cache in one go and send it downstream. This may cause delays and buffer overflows if the cached response is large.
The CacheFilter did not handle backpressure from downstream.

In this PR:

The CacheFilter does not stop headers iteration when encoding a cached response, made possible by http: Allow http filters to add a body that's not readily available to a headers-only request/response #12832.
The CacheFilter fetches the body from the cache in several chunks, the chunk size is based on the encoding buffer size to avoid overflows.
The CacheFilter subscribes to downstream watermark events and uses them to handle downstream backpressure, by stopping fetching body chunks when the high watermark callback is invoked, and resuming when the low watermark callback is invoked.

1 & 2 are already implemented and tested.
3 is implemented and was tested manually (by adding trace statements and running the integration test), but is still missing actual unit/integration tests. To test this, we need to make sure that when the high watermark callback is invoked we do not fetch more body chunks from the cache (maybe check that encodeData / injectEncodedDataToFilterChain were not called?), and that when the low watermark callback is invoked, we continue fetching data. This already implicitly happens in the integration test, but we need to make sure it is invoked correctly (by unit and integration tests).

yosrym93 · 2020-09-11T00:19:52Z

To subscribe to receive watermark events we need to call StreamDecoderCallbacks::addDownstreamWatermarkCallbacks(), and give it an object of DownstreamWatermarkCallbacks.The passed DownstreamWatermarkCallbacks object should implement onAboveWriteBufferHighWatermark and onBelowWriteBufferHighWatermark to handle downstream watermark events (in this case, stop and resume fetching body from the cache).

Currently, the CacheFilter inherits from DownstreamWatermarkCallbacks, and calls addDownstreamWatermarkCallbacks(*this) in encodeCachedResponse, before it starts fetching the cached body. We should make sure this is the correct place to call it.

Alternatively, the CacheFilter could create an internal class/struct that inherits from DownstreamWatermarkCallbacks and pass it to addDownstreamWatermarkCallbacks.

yosrym93 · 2020-09-11T00:22:20Z

The chunk size used to fetch the body from the cache is equal to the encoding buffer limit. This way, we fetch as much data in every request to the cache as we can send downstream.

yosrym93 · 2020-09-11T00:25:46Z

If the high watermark callback was invoked AFTER a body chunk is already request from the cache, this request will be completed, and that chunk will be sent downstream. This may result in crossing the encoding buffer limit, but it should be okay as this is only a soft limit, and this should not happen frequently.

…essure

yosrym93 · 2020-09-11T01:06:47Z

According to https://github.com/envoyproxy/envoy/blob/master/test/integration/protocol_integration_test.cc#L1036 we don't need to handle non-compliant 304 bodies. Envoy already handles them.

source/extensions/filters/http/cache/cache_filter.cc

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

yosrym93 · 2020-09-11T04:23:51Z

@toddmgreer I think this addresses all your comments.

toddmgreer · 2020-09-11T17:02:44Z

tools/spelling/spelling_dictionary.txt

 dir
 dirname
 djb
+dont


Why is "dont" added?

The comment that has ContinueAndDontEndStream. "Dont" is seen as a spelling error.

https://github.com/envoyproxy/envoy/pull/13054/files#diff-35f4cb0d915fd79ca0fc7937b5be1d89R365

This is a spelling error. "Don't"

source/extensions/filters/http/cache/cache_filter.cc

source/extensions/filters/http/cache/cache_filter.h

source/extensions/filters/http/cache/cache_filter.cc

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

mattklein123 · 2020-09-14T18:30:54Z

@yosrym93 is this still a draft or is it ready for review? I can help review the watermark logic (or @alyssawilk potentially) but want to make sure this is ready for review.

/wait-any

yosrym93 · 2020-09-14T21:25:08Z

@yosrym93 is this still a draft or is it ready for review? I can help review the watermark logic (or @alyssawilk potentially) but want to make sure this is ready for review.

The PR is only missing tests for the watermark logic, but the logic code should be complete.
You can review it now or wait after tests are added, whichever you see fit.

mattklein123 · 2020-09-14T21:28:41Z

I will wait for the tests, thanks.

/wait

Patching envoyproxy#13054 Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

…essure Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

mattklein123 · 2020-10-27T16:12:04Z

@yosrym93 is this ready for review? If so can you mark it non-draft?

/wait-any

zyshi · 2020-10-29T21:01:41Z

I added a test in cache_filter_test.cc which should test the downstream watermark handling. Should be ready for review now.

A side note: it looks like the test dispatcher once run, cannot be interrupted (unless I miss something). It would be nice if there's a mode to run the event loop one by one, something similar to chromium's TestTestRunner::RunNextTask(). That made me wonder how Envoy test complicated scheduling logic, i.e., verifying callbacks are posted with right delay.

zyshi · 2020-10-29T21:36:21Z

@yanavlasov could you help set this PR ready for review? I don't have permission to do that. Besides, I might also need you to kick off the retry on presubmit to see if infra failures goes away. Thanks!

@yosrym93 If you happen to see this message early, it would be nice to set the PR ready for review. Thanks!

yosrym93 · 2020-10-30T02:43:15Z

@yosrym93 If you happen to see this message early, it would be nice to set the PR ready for review. Thanks!

Done. Glad to see progress on this PR!

jmarantz · 2020-10-30T02:46:22Z

Happy to review; please ping once @toddmgreer finishes review. Thanks!

yanavlasov · 2020-10-30T14:19:20Z

/azp run

azure-pipelines · 2020-10-30T14:19:31Z

Azure Pipelines successfully started running 1 pipeline(s).

zyshi · 2020-10-30T17:11:58Z

@toddmgreer the change is ready for review, PTAL, thanks!

jmarantz

basically looks great; need to do another quick pass over the tests.

Some minor nits.

test/extensions/filters/http/cache/cache_filter_integration_test.cc

test/extensions/filters/http/cache/cache_filter_test.cc

jmarantz · 2020-11-03T01:01:11Z

test/extensions/filters/http/cache/cache_filter_test.cc

+
+    // Make sure validation conditional headers are added
+    const Http::TestRequestHeaderMapImpl injected_headers = {
+        {"if-none-match", etag_}, {"if-modified-since", response_last_modified_}};


To keep it simple and verbose, is that okay to keep it as is rather than using values in headers.h? The dic needs std::string pairs, so we will need to use something like Http::CustomHeaders.get().ifNoneMatch.get(), which is not clean.

Besides, I checked other places where TestRequestHeaderMapImpl, it seems we generally follow a explicit key-value pair setting. WDYT?

I'm OK either way. One way to make this a little less verbose than what you suggest is to make a temp for Http::CustomHeaders.get() and use that throughout this function. But it's up to you.

test/extensions/filters/http/cache/cache_filter_test.cc

Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

zyshi · 2020-11-04T07:27:14Z

Comments addressed, PATL, thanks!

Sorry if I didn't figure out the best way to use the tool (bear with me if the comments notifications are not bundled). I am still trying to get familiar with this git native PR process (compared to a extremely convenient Gerrit workflow).

zyshi · 2020-11-04T23:55:36Z

@jmarantz This change is ready for review. BTW, I don't have permission to resolve conversations, please do resolve if you have that button. Thanks!

jmarantz · 2020-11-05T00:30:54Z

test/extensions/filters/http/cache/cache_filter_test.cc

+
+    // Make sure validation conditional headers are added
+    const Http::TestRequestHeaderMapImpl injected_headers = {
+        {"if-none-match", etag_}, {"if-modified-since", response_last_modified_}};


I'm OK either way. One way to make this a little less verbose than what you suggest is to make a temp for Http::CustomHeaders.get() and use that throughout this function. But it's up to you.

jmarantz · 2020-11-05T00:37:07Z

test/extensions/filters/http/cache/cache_filter_test.cc

+    EXPECT_CALL(encoder_callbacks_,
+                injectEncodedDataToFilterChain(
+                    testing::Property(&Buffer::Instance::toString,
+                                      testing::Eq(std::string(buffer_limit_, 'a'))),


I was thinking about this testcase. I think it would give us more confidence in the correctness of the system if you used a buffer_limit of something much smaller, say 6. Then you can use a payload of "1234567890" and make sure the first chunk is "123456" and the second is "7890".

Right now you are comparing against a bunch of 'a's and I'm wondering if a bug could creep in where we take the wrong span of that string in the logic, but your test doesn't care.

WDYT?

Good point! Thanks for doing a thorough review. I didn't pay much attention on the existing code but focused on downstream watermark handling test :P

jmarantz · 2020-11-05T00:38:23Z

test/extensions/filters/http/cache/cache_filter_test.cc

+// Test that a body with size exactly equal to the buffer limit will be encoded in 1 chunk.
+TEST_F(CacheChunkSizeTest, EqualBufferLimit) {
+  request_headers_.setHost("EqualBufferLimit");
+  const std::string body = std::string(buffer_limit_, 'a');


again I'd consider using smaller limits and more interesting strings to compare.

jmarantz

btw this is great code, super clean and easy to read. Thanks for doing this!

I'm really just having nits and testing questions that are probably academic.

Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

zyshi · 2020-11-05T03:58:54Z

Thanks for the thorough review, really appreciate it! It was @yosrym93 did the great work, I only added one additional test to ensure there's test coverage on downstream watermark handling. LMK if there's any other concern. Thanks!

yosrym93 · 2020-11-05T10:56:13Z

Thanks @zyshi for completing this and @jmarantz for reviewing it. Happy to see the Cache Filter one step closer to being production-ready!

jmarantz · 2020-11-05T13:19:56Z

test/extensions/filters/http/cache/cache_filter_test.cc

  request_headers_.setHost("DownstreamPressureHandling");
  const int chunks_count = 3;
-  const uint64_t body_size = buffer_limit_ * chunks_count;
+  const uint64_t body_size = getBufferLimit() * chunks_count;


We are still just testing a bunch of "a"s below; should we change these too?

jmarantz

Modulo my previous comment I'll pass this to @envoyproxy/senior-maintainers in parallel with possibly addressing that.

Nice job (all who worked on it)!

mattklein123

Thanks for this. A few questions/comments to get started.

/wait

mattklein123 · 2020-11-05T16:20:46Z

source/extensions/filters/http/cache/cache_filter.cc

+  // TODO(yosrym93): Make sure this is the right place to add the callbacks.
+  decoder_callbacks_->addDownstreamWatermarkCallbacks(*this);


I would recommend initializing this as early as possible (in setEncoderFilterCallbacks)

I assume you really meant setEncoderFilterCallbacks rather than a typo here. Since I'm stepping up on the implementation code, I did get confused on why the registration is only provided available via StreamDecoderFilterCallbacks. What if a pure encoder filter (who doesn't have decoder_callbacks) wants to listen to downstream watermark changes? In other words, there's no way you could call: encoder_callbacks_->addDownstreamWatermarkCallbacks(*this).

I think it's just an implementation oversight because no one has needed it before.

mattklein123 · 2020-11-05T16:21:11Z

source/extensions/filters/http/cache/cache_filter.h

  bool request_allows_inserts_ = false;

+  // These are used to keep track of whether we should fetch more data from the cache.
+  int high_watermark_calls_ = 0;


nit: unsigned

mattklein123 · 2020-11-05T16:22:00Z

tools/spelling/spelling_dictionary.txt

 dir
 dirname
 djb
+dont


This is a spelling error. "Don't"

mattklein123 · 2020-11-05T16:23:41Z

source/extensions/filters/http/cache/cache_filter.cc

+    // Continue encoding the headers but do not end the stream as the response body is yet to be
+    // injected.
+    return Http::FilterHeadersStatus::ContinueAndDontEndStream;


Does this correctly handle the case where the cached response is a header only response?

mattklein123 · 2020-11-05T16:24:46Z

source/extensions/filters/http/cache/cache_filter.cc

-  if (filter_state_ == FilterState::EncodeServingFromCache) {
-    // Stop the encoding stream until the cached response is fetched & added to the encoding stream.
-    return Http::FilterDataStatus::StopIterationAndBuffer;
-  }


Why is this removed? Can this no longer happen? Should the enum be removed?

mattklein123 · 2020-11-05T16:26:49Z

source/extensions/filters/http/cache/cache_filter.cc

+void CacheFilter::onBelowWriteBufferLowWatermark() {
+  ASSERT(high_watermark_calls_ > 0);
+  --high_watermark_calls_;
+  if (!remaining_ranges_.empty()) {
+    // Fetching the cached response body was stopped, continue if possible.
+    maybeGetBody();
+  }
+}


Are there cases in which we stream through a router response and this might get triggered? I don't see any guards in maybeGetBody() to confirm that we are actually doing a fetch?

github-actions · 2020-12-09T03:11:40Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

github-actions · 2020-12-16T04:03:39Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

yosrym93 added 3 commits September 10, 2020 23:48

CacheFilter does not stop headers iteration when encoding a response

872bf95

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Divide the body fetched from cache and encoded downstream to chunks s…

8550240

…ized according to the buffer limit Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

Handled downstream watermark callbacks

5a2672c

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

htuch requested a review from toddmgreer September 11, 2020 00:20

Merge remote-tracking branch 'upstream/master' into downstream-backpr…

fe1bdd7

…essure

toddmgreer suggested changes Sep 11, 2020

View reviewed changes

Fixed problems with chunk size and watermark callbacks handling

facd0c8

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

toddmgreer suggested changes Sep 11, 2020

View reviewed changes

mattklein123 self-assigned this Sep 11, 2020

Minor correctness and readability improvements

137fa78

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>

toddmgreer previously approved these changes Sep 11, 2020

View reviewed changes

repokitteh-read-only bot added the waiting:any label Sep 14, 2020

repokitteh-read-only bot removed the waiting:any label Sep 14, 2020

repokitteh-read-only bot added the waiting label Sep 14, 2020

zyshi added a commit to zyshi/envoy that referenced this pull request Oct 22, 2020

CacheFilter does not stop headers iteration when encoding a response

c0d9501

Patching envoyproxy#13054 Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

zyshi mentioned this pull request Oct 22, 2020

cloned PR: caching: Stream cached responses in chunks and handle downstream backpressure #13714

Closed

Merge remote-tracking branch 'upstream/master' into downstream-backpr…

81e6248

…essure Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

zyshi dismissed toddmgreer’s stale review via 81e6248 October 26, 2020 19:57

repokitteh-read-only bot removed the waiting label Oct 26, 2020

repokitteh-read-only bot added the waiting:any label Oct 27, 2020

yosrym93 marked this pull request as ready for review October 30, 2020 02:41

yosrym93 requested a review from jmarantz as a code owner October 30, 2020 02:41

jmarantz self-assigned this Oct 30, 2020

toddmgreer previously approved these changes Nov 3, 2020

View reviewed changes

jmarantz reviewed Nov 3, 2020

View reviewed changes

Address comments from jmarantz@.

046beae

Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

zyshi dismissed toddmgreer’s stale review via 046beae November 4, 2020 07:15

repokitteh-read-only bot removed the waiting label Nov 4, 2020

jmarantz reviewed Nov 5, 2020

View reviewed changes

Use non-repeated data chunks to test strict ordering.

ce72485

Signed-off-by: Zhongyi Shi <zhongyi@chromium.org>

jmarantz reviewed Nov 5, 2020

View reviewed changes

jmarantz approved these changes Nov 5, 2020

View reviewed changes

mattklein123 requested changes Nov 5, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 5, 2020

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 9, 2020

github-actions bot closed this Dec 16, 2020

		// TODO(yosrym93): Make sure this is the right place to add the callbacks.
		decoder_callbacks_->addDownstreamWatermarkCallbacks(*this);

Conversation

yosrym93 commented Sep 11, 2020

Uh oh!

yosrym93 commented Sep 11, 2020

Uh oh!

yosrym93 commented Sep 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yosrym93 commented Sep 11, 2020

Uh oh!

yosrym93 commented Sep 11, 2020

Uh oh!

yosrym93 commented Sep 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yosrym93 commented Sep 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 commented Sep 14, 2020

Uh oh!

yosrym93 commented Sep 14, 2020

Uh oh!

mattklein123 commented Sep 14, 2020

Uh oh!

mattklein123 commented Oct 27, 2020

Uh oh!

zyshi commented Oct 29, 2020

Uh oh!

zyshi commented Oct 29, 2020

Uh oh!

yosrym93 commented Oct 30, 2020

Uh oh!

jmarantz commented Oct 30, 2020

Uh oh!

yanavlasov commented Oct 30, 2020

Uh oh!

azure-pipelines bot commented Oct 30, 2020

Uh oh!

zyshi commented Oct 30, 2020

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zyshi commented Nov 4, 2020

Uh oh!

zyshi commented Nov 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yosrym93 commented Sep 11, 2020 •

edited

Loading