Implemented H2 stream level buffer accounting. by KBaichoo · Pull Request #16218 · envoyproxy/envoy

KBaichoo · 2021-04-28T18:27:16Z

Signed-off-by: Kevin Baichoo kbaichoo@google.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Implement H2 stream level buffer accounting.
Additional Description: This is runtime guarded, disabled by default since only tests use the new features provided. As this gets developed it'll become enabled by default.
Risk Level: Medium
Testing: Included unit test / integration tests
Docs Changes: N/A
Release Notes: N/A (not end-user accessible yet)
Platform Specific Features: N/A
Runtime guard: Yes, envoy.test_only.per_stream_buffer_accounting
Related Issue #15791

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-04-28T18:27:46Z

/assign @antoniovicente

Can you take a look? Thanks

antoniovicente

Great start. See comments below.

antoniovicente · 2021-04-28T23:52:52Z

source/common/http/http2/codec_impl.h

+      return buffer_memory_account_;
+    }
+    void setAccount(Buffer::BufferMemoryAccountSharedPtr) override {
+      RELEASE_ASSERT(


I'm wondering if for consistency we should have something outside the codec create the account and call setAccount on the serverside connections. I think that a possible place is ConnectionManagerImpl::createCodec which creates the server connection.

Also consider moving the implementation of these methods to the StreamImpl base class if possible.

I modified ConnectionManagerImpl::newStream to create the account and set it on the newly created downstream request object. This has allowed me to push the implementations to StreamImpl as suggested, and get rid of getAccount() as part of the stream api.

antoniovicente · 2021-04-28T23:56:00Z

source/common/http/conn_manager_impl.cc

  ENVOY_CONN_LOG(debug, "new stream", read_callbacks_->connection());
-  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit()));
+  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit(),
+                                              response_encoder.getStream().getAccount()));


We could also say that the connection manager is the owner of the account and feed the account directly from the connection manager instead of asking the stream. I think we can remove getAccount() from the stream API if we do this change.

See related comment in http2/codec_impl.h setAccount

See response to comment in http2/codec_impl.h

antoniovicente · 2021-04-29T00:13:32Z

source/common/http/async_client_impl.h

  Upstream::ClusterInfoConstSharedPtr clusterInfo() override { return parent_.cluster_; }
  void clearRouteCache() override {}
  uint64_t streamId() const override { return stream_id_; }
+  // TODO(kbaichoo): Implement this?


I think that some users of async client are filters like the RBAC filter. We should plumb accounts to those eventually. Other users may involve config plane operations which are not associated with clients. Still it may be worth associating with an account owned by the config system, as a way to track memory usage by config plane components.

Could you change the TODO to something like: Plumb account from owning request filter.

Updated the comment.

antoniovicente · 2021-04-29T03:28:28Z

test/integration/tracked_watermark_buffer.cc

+    }
+
+    for (auto& acc : account_infos_) {
+      if (static_cast<Buffer::BufferMemoryAccountImpl*>(acc.first.get())->balance() < byte_size) {


Is this call to balance() happening in the right thread? Remember that buffer_memory_allocated_ is not atomic.

Hmm, I've changed how this is done so it should be running in the worker thread.

antoniovicente · 2021-04-29T03:32:01Z

test/integration/tracked_watermark_buffer.cc

+    }
+    return true;
+  };
+  return mutex_.AwaitWithTimeout(absl::Condition(&predicate), absl::Milliseconds(timeout.count()));


mutex AwaitWithTimeout only works correctly if changes to the predicate condition can only happen during times the mutex is held. Accounts are not changed with the mutex above held, therefore this await won't work correctly.

antoniovicente · 2021-04-29T03:37:55Z

source/common/buffer/watermark_buffer.h

  void appendSliceForTest(absl::string_view data) override;

-  void setWatermarks(uint32_t watermark) override { setWatermarks(watermark / 2, watermark); }
-  void setWatermarks(uint32_t low_watermark, uint32_t high_watermark);


test/common/buffer/watermark_buffer_test.cc seems to be failing to build because the 2 argument version of setWatermarks is missing, please look into it.

was unnecessary. Fixed potential race condition in checking balances. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the review @antoniovicente

KBaichoo · 2021-04-29T18:07:10Z

source/common/buffer/watermark_buffer.h

  void appendSliceForTest(absl::string_view data) override;

-  void setWatermarks(uint32_t watermark) override { setWatermarks(watermark / 2, watermark); }
-  void setWatermarks(uint32_t low_watermark, uint32_t high_watermark);


KBaichoo · 2021-04-30T13:13:00Z

source/common/http/http2/codec_impl.h

+      return buffer_memory_account_;
+    }
+    void setAccount(Buffer::BufferMemoryAccountSharedPtr) override {
+      RELEASE_ASSERT(


I modified ConnectionManagerImpl::newStream to create the account and set it on the newly created downstream request object. This has allowed me to push the implementations to StreamImpl as suggested, and get rid of getAccount() as part of the stream api.

KBaichoo · 2021-04-30T15:06:43Z

source/common/http/conn_manager_impl.cc

  ENVOY_CONN_LOG(debug, "new stream", read_callbacks_->connection());
-  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit()));
+  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit(),
+                                              response_encoder.getStream().getAccount()));


See response to comment in http2/codec_impl.h

KBaichoo · 2021-04-30T20:22:16Z

test/integration/tracked_watermark_buffer.cc

+    }
+
+    for (auto& acc : account_infos_) {
+      if (static_cast<Buffer::BufferMemoryAccountImpl*>(acc.first.get())->balance() < byte_size) {


Hmm, I've changed how this is done so it should be running in the worker thread.

KBaichoo · 2021-04-30T20:22:30Z

test/integration/tracked_watermark_buffer.cc

+    }
+    return true;
+  };
+  return mutex_.AwaitWithTimeout(absl::Condition(&predicate), absl::Milliseconds(timeout.count()));


test/integration/tracked_watermark_buffer_test.cc

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

source/common/http/http2/codec_impl.cc

antoniovicente · 2021-05-04T01:05:15Z

include/envoy/buffer/buffer.h

+   * @param low_watermark supplies the buffer low watermark size threshold, in bytes.
+   * @param high_watermark supplies the buffer high watermark size threshold, in bytes.
+   */
+  virtual void setWatermarks(uint32_t low_watermark, uint32_t high_watermark) PURE;


Fairly sure we don't need this as part of the public API. The setWatermarks 2 arg method is on the way out, would it be helpful to get it removed everywhere? Probably as part of a separate PR.

It was needed as it broke some tests. It doesn't seem like this is the right PR to also remove that older interface.

I'll create another PR removing it.

source/common/http/conn_manager_impl.cc

antoniovicente · 2021-05-04T01:14:16Z

source/common/http/http2/codec_impl.h

-      pending_recv_data_.setWatermarks(low_watermark, high_watermark);
-      pending_send_data_.setWatermarks(low_watermark, high_watermark);
+      pending_recv_data_->setWatermarks(low_watermark, high_watermark);
+      pending_send_data_->setWatermarks(low_watermark, high_watermark);


This should really transition to the 1 argument version of setWatermarks. There's only 1 call to setWriteBufferWatermarks and it uses buffer_limit / 2, buffer_limit as arguments.

Will do removing of setWatermark(low,high) in another PR as mentioned above.

test/integration/tracked_watermark_buffer.cc

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the review @antoniovicente

test/integration/tracked_watermark_buffer.cc

KBaichoo · 2021-05-04T13:47:55Z

include/envoy/buffer/buffer.h

+   * @param low_watermark supplies the buffer low watermark size threshold, in bytes.
+   * @param high_watermark supplies the buffer high watermark size threshold, in bytes.
+   */
+  virtual void setWatermarks(uint32_t low_watermark, uint32_t high_watermark) PURE;


It was needed as it broke some tests. It doesn't seem like this is the right PR to also remove that older interface.

I'll create another PR removing it.

KBaichoo · 2021-05-04T13:52:53Z

source/common/http/http2/codec_impl.h

-      pending_recv_data_.setWatermarks(low_watermark, high_watermark);
-      pending_send_data_.setWatermarks(low_watermark, high_watermark);
+      pending_recv_data_->setWatermarks(low_watermark, high_watermark);
+      pending_send_data_->setWatermarks(low_watermark, high_watermark);


Will do removing of setWatermark(low,high) in another PR as mentioned above.

source/common/http/conn_manager_impl.cc

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente

What is left before this PR can be consider no longer WIP?

source/common/buffer/buffer_impl.cc

antoniovicente · 2021-05-07T22:39:52Z

source/common/http/async_client_impl.h

  Upstream::ClusterInfoConstSharedPtr clusterInfo() override { return parent_.cluster_; }
  void clearRouteCache() override {}
  uint64_t streamId() const override { return stream_id_; }
+  // TODO(kbaichoo): Implement this?


Could you change the TODO to something like: Plumb account from owning request filter.

source/common/http/conn_manager_impl.cc

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo · 2021-05-10T22:38:31Z

The only thing that was keeping it still WIP is lack of a runtime guard, which is now included. This is disabled by default since only tests use the new features right now.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only · 2021-05-17T22:51:46Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16218 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-05-18T15:15:50Z

/retest

repokitteh-read-only · 2021-05-18T15:15:54Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16218 (comment) was created by @KBaichoo.

see: more, trace.

antoniovicente · 2021-05-18T23:37:31Z

source/common/http/conn_manager_impl.cc

+    response_encoder.getStream().setAccount(downstream_request_account);
+  }
+  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit(),
+                                              std::move(downstream_request_account)));


Thinking a bit about the future:

It would be useful to have a way to go from Account to ActiveStream. Possible strawperson:

class BufferMemoryAccount { + // Methods to set and clear the account owner. + virtual void setOwner(AccountOwner& owner) PURE; + virtual void clearOwner() PURE; }; class AccountOwner { public: // Reset the stream owned by this account. // Better method names may be possible. Reason argument seems optional since we expect this // to be called only when under memory pressure. void resetStream(Reason reason) PURE; // Dump debug information about the stream. void dumpState(std::ostream& os) PURE; };

The account interface could wrap the reset and dump methods so they are no-ops if the account has no owner, or have method to get an optional reference to the owner that the per-worker overload trackers can use to run those operations on the relevant owner objects.

Yes, we'll need something like this later down the line, and will address this then.

include/envoy/http/codec.h

antoniovicente · 2021-05-19T00:48:16Z

test/integration/buffer_accounting_integration_test.cc

+  // Wait for all requests to buffered the response from upstream.
+  if (streamBufferAccounting()) {
+    EXPECT_TRUE(
+        buffer_factory_->waitForExpectedAccountBalanceWithTimeout(TestUtility::DefaultTimeout))


It would be nice to pass the expected body sizes and requests to waitForExpectedAccountBalanceWithTimeout instead of requiring a separate call to setExpectedAccountBalance

Not sure that sending requests should really be the job of the buffer_factory. That seems a bit strange.

This comment is about the wait with timeout API provided by the buffer factory.

Perhaps we're miscommunicating.

I see the issue as follows:

The test thread has to send the requests

The test thread also has to set these expectations and wait on them

If the test thread runs (1) followed by (2), there's a possibility of a race causing the wait to timeout even though accounts are correctly sized due to the accounts notifying if expectations are met in the worker when updating accounts. i.e. if all accounts are updated, and we then set the expectation it won't fire since none of them have any additional events.

So instead this is split into

Send request

Set expectation

Wait for expectation

And we can do 2, 1, and 3. Alternatively we can push those into a single function, as what I think you'd suggest would require, but it seems strange for the buffer factory to be involved with 2.

Yeah, I wasn't understanding why you saw the need to mix expectations, sending requests and waiting for expectations. Hopefully the info below will make things more clear:

We have ways to query the factory about the state of buffers created by it. For example, TrackedWatermarkBufferFactory::maxBufferSize

That method is implemented by storing some information about the buffer in an atlernate data structure kept by the factory which is updated from the check watermark overrides. It seems to me that your extensions to the tracked buffer factory to also gather information about accounts can make some basic assumptions about check watermark functions being called after changes to buffers which result in updates to the number of bytes tracked by the accounts.

Therefore, it is possible to keep a map from account to account size which is updated on each call to check watermark functions which could be used to get the expected size of each account under a lock. Given that map with a mirrored version of the account sizes, you can implement methods to get info about the size of accounts which are implemented using mechanisms similar to those used in the implementation of TrackedWatermarkBufferFactory::maxBufferSize and TrackedWatermarkBufferFactory::waitUntilTotalBufferedExceeds (via Mutex::AwaitWithTimeout, see https://github.com/envoyproxy/envoy/pull/14714/files#diff-9f19f8978f392143007cd837f851e2674b7b61227f71d6f907a0e60a72cd0b4b)

Using this technique would allow sending requests in the normal way and implement a version of wait for expectation that takes the expectations as function arguments.

Duplicating the internal information of the account doesn't seem right -- seems like a leaky abstraction.

I also prefer using the notification as IIRC AwaitWithTimeout doesn't have a signaling mechanism while the notification does, awaking blocked threads.

AwaitWithTimeout checks every time that the mutex is released. But there's a requirement that updates happen when the mutex is held.

Anyway, I think we can start with this test framework API and improve it in the future as needed.

test/integration/tracked_watermark_buffer.cc

antoniovicente · 2021-05-19T00:56:02Z

test/integration/tracked_watermark_buffer.cc

+
+bool TrackedWatermarkBufferFactory::waitForExpectedAccountBalanceWithTimeout(
+    std::chrono::milliseconds timeout) {
+  return expected_balances_met_.WaitForNotificationWithTimeout(absl::FromChrono(timeout));


A potential drawback of the Notification approach vs mutex_.AwaitWithTimeout is that the notification approach can only be used once per test. Should the mutex_.AwaitWithTimeout approach be used here instead? See also comments about passing in expectations to waitForExpectedAccountBalanceWithTimeout instead of using a separate method to set expectations.

I remember I explicitly choose to use Notification over mutex_.AwaitWithTimeout. It's been a while, but I think my original reasoning is that only the worker thread should do accumulations and notify the test thread when the condition is met rather than try to have the test thread access the account balances which might change in the worker thread.

For now Notification has what we need.

antoniovicente · 2021-05-19T00:58:07Z

test/integration/tracked_watermark_buffer.cc

+  }
+
+  removeDanglingAccounts();
+  if (account_infos_.size() < expected_balances_->num_accounts_) {


Should this be an exact comparison? Or should we allow for there to be extra accounts as long as at least num_accounts_ have balance_per_account_ in them?

Also, should we extend these concepts to allow waiting for accounts to be drained? I guess waitUntilExpectedNumberOfAccountsAndBoundBuffers with 0 accounts could be used to tell when all requests are complete.

Done.

I think using the existing mechanism that most test do (if the responses are completed at the endpoint) is sufficient.

test/integration/buffer_accounting_integration_test.cc

test/integration/tracked_watermark_buffer.cc

antoniovicente · 2021-05-19T01:15:21Z

test/integration/tracked_watermark_buffer.cc

+  while (accounts_it != account_infos_.end()) {
+    auto next = std::next(accounts_it);
+
+    // Remove all "dangling" accounts.


Should you consider accounts with no associated buffers as dangling instead of looking at the ref count?

Consider the cases where slices point to accounts while all the associated buffers no longer exist. Checking the ref count tells us that no other entity should have access to the account.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo

Thanks for the review @antoniovicente .

KBaichoo · 2021-05-19T19:00:58Z

source/common/http/conn_manager_impl.cc

+    response_encoder.getStream().setAccount(downstream_request_account);
+  }
+  ActiveStreamPtr new_stream(new ActiveStream(*this, response_encoder.getStream().bufferLimit(),
+                                              std::move(downstream_request_account)));


Yes, we'll need something like this later down the line, and will address this then.

test/integration/buffer_accounting_integration_test.cc

include/envoy/http/codec.h

test/integration/tracked_watermark_buffer.cc

KBaichoo · 2021-05-19T19:19:06Z

test/integration/tracked_watermark_buffer.cc

+  while (accounts_it != account_infos_.end()) {
+    auto next = std::next(accounts_it);
+
+    // Remove all "dangling" accounts.


Consider the cases where slices point to accounts while all the associated buffers no longer exist. Checking the ref count tells us that no other entity should have access to the account.

test/integration/tracked_watermark_buffer.cc

KBaichoo · 2021-05-19T19:46:57Z

test/integration/tracked_watermark_buffer.cc

+
+bool TrackedWatermarkBufferFactory::waitForExpectedAccountBalanceWithTimeout(
+    std::chrono::milliseconds timeout) {
+  return expected_balances_met_.WaitForNotificationWithTimeout(absl::FromChrono(timeout));


I remember I explicitly choose to use Notification over mutex_.AwaitWithTimeout. It's been a while, but I think my original reasoning is that only the worker thread should do accumulations and notify the test thread when the condition is met rather than try to have the test thread access the account balances which might change in the worker thread.

For now Notification has what we need.

KBaichoo · 2021-05-19T19:59:01Z

test/integration/tracked_watermark_buffer.cc

+  }
+
+  removeDanglingAccounts();
+  if (account_infos_.size() < expected_balances_->num_accounts_) {


Done.

I think using the existing mechanism that most test do (if the responses are completed at the endpoint) is sufficient.

KBaichoo · 2021-05-19T20:28:21Z

test/integration/buffer_accounting_integration_test.cc

+  // Wait for all requests to buffered the response from upstream.
+  if (streamBufferAccounting()) {
+    EXPECT_TRUE(
+        buffer_factory_->waitForExpectedAccountBalanceWithTimeout(TestUtility::DefaultTimeout))


Not sure that sending requests should really be the job of the buffer_factory. That seems a bit strange.

test/integration/buffer_accounting_integration_test.cc

test/integration/tracked_watermark_buffer.h

test/integration/buffer_accounting_integration_test.cc

KBaichoo · 2021-05-20T00:11:58Z

/retest

repokitteh-read-only · 2021-05-20T00:12:01Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16218 (comment) was created by @KBaichoo.

see: more, trace.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente · 2021-05-20T21:55:30Z

test/integration/buffer_accounting_integration_test.cc

+  // Wait for all requests to buffered the response from upstream.
+  if (streamBufferAccounting()) {
+    EXPECT_TRUE(
+        buffer_factory_->waitForExpectedAccountBalanceWithTimeout(TestUtility::DefaultTimeout))


Yeah, I wasn't understanding why you saw the need to mix expectations, sending requests and waiting for expectations. Hopefully the info below will make things more clear:

We have ways to query the factory about the state of buffers created by it. For example, TrackedWatermarkBufferFactory::maxBufferSize

That method is implemented by storing some information about the buffer in an atlernate data structure kept by the factory which is updated from the check watermark overrides. It seems to me that your extensions to the tracked buffer factory to also gather information about accounts can make some basic assumptions about check watermark functions being called after changes to buffers which result in updates to the number of bytes tracked by the accounts.

Therefore, it is possible to keep a map from account to account size which is updated on each call to check watermark functions which could be used to get the expected size of each account under a lock. Given that map with a mirrored version of the account sizes, you can implement methods to get info about the size of accounts which are implemented using mechanisms similar to those used in the implementation of TrackedWatermarkBufferFactory::maxBufferSize and TrackedWatermarkBufferFactory::waitUntilTotalBufferedExceeds (via Mutex::AwaitWithTimeout, see https://github.com/envoyproxy/envoy/pull/14714/files#diff-9f19f8978f392143007cd837f851e2674b7b61227f71d6f907a0e60a72cd0b4b)

Using this technique would allow sending requests in the normal way and implement a version of wait for expectation that takes the expectations as function arguments.

antoniovicente · 2021-05-20T22:03:14Z

test/integration/tracked_watermark_buffer.cc

+    main_tid = server.api().threadFactory().currentThreadId();
+
+    slot->runOnAllThreads(
+        [&main_tid, &server, &func, this](OptRef<ThreadLocal::ThreadLocalObject>) {


nit: drop the & for main_tid so the tid is captured by value.

antoniovicente · 2021-05-20T22:06:35Z

test/integration/tracked_watermark_buffer.cc

+  ThreadLocal::TypedSlotPtr<> slot;
+  Envoy::Thread::ThreadId main_tid;
+
+  server.dispatcher().post([&] {


This seems more complicated than I expected. Wondering if there's a way to simplify by capturing dispatchers associated with threads where accounts are created.

antoniovicente · 2021-05-20T22:26:43Z

test/per_file_coverage.sh

 "source/common/thread:0.0" # Death tests don't report LCOV
 "source/common/matcher:93.3"
-"source/common/quic:87.8"
+"source/common/quic:87.7"


This change shouldn't be needed if you merge upstream/main again.

Sorry, didn't see the change (75aecf2) that got accepted yesterday. Needed this prior.

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente

Thanks for the implementing accounting for H2. Off to senior maintainers for a final review.

/assign-from @envoyproxy/senior-maintainers

repokitteh-read-only · 2021-05-21T15:52:50Z

@envoyproxy/senior-maintainers assignee is @lizan

🐱

Caused by: a #16218 (review) was submitted by @antoniovicente.

see: more, trace.

KBaichoo · 2021-05-21T17:30:00Z

/retest

repokitteh-read-only · 2021-05-21T17:30:05Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #16218 (comment) was created by @KBaichoo.

see: more, trace.

KBaichoo · 2021-05-25T20:18:32Z

friendly ping @lizan PTAL

…ry. (#17093) This PR tracks memory accounts using >1MB of allocated space, with feedback mechanisms based on credits and debits on accounts. It further creates the handle from which the BufferMemoryAccount can reset the stream, and has the WatermarkBufferFactory also produce the particular BufferMemoryAccountImpl used for tracking. Risk Level: Medium Testing: Unit and Integration test Docs Changes: NA Release Notes: NA -- not yet user facing Platform Specific Features: NA Runtime guard: Yes, envoy.test_only.per_stream_buffer_accounting from #16218 sufficient Related Issue #15791 Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Per-request buffer accounting is still a work in progress so behavior changes are protected by 'envoy.test_only.per_stream_buffer_accounting' which is disabled by default. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

…ry. (envoyproxy#17093) This PR tracks memory accounts using >1MB of allocated space, with feedback mechanisms based on credits and debits on accounts. It further creates the handle from which the BufferMemoryAccount can reset the stream, and has the WatermarkBufferFactory also produce the particular BufferMemoryAccountImpl used for tracking. Risk Level: Medium Testing: Unit and Integration test Docs Changes: NA Release Notes: NA -- not yet user facing Platform Specific Features: NA Runtime guard: Yes, envoy.test_only.per_stream_buffer_accounting from envoyproxy#16218 sufficient Related Issue envoyproxy#15791 Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Implemented H2 stream level buffer accounting.

8c99d4f

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot assigned antoniovicente Apr 28, 2021

antoniovicente reviewed Apr 29, 2021

View reviewed changes

Simplified stream API, H2 codec impl. Reverted setWatermark as change

44a5e05

was unnecessary. Fixed potential race condition in checking balances. Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo commented Apr 30, 2021

View reviewed changes

test/integration/tracked_watermark_buffer_test.cc Show resolved Hide resolved

KBaichoo added 4 commits April 30, 2021 20:55

Added test for Set and Wait for Expected Account Balance.

6956e95

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Fixing broken tests.

dbc66ad

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/main' into accounting-connect-…

65e8206

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Fixed broken extension test.

8abaa72

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo requested review from cpakulski and dio as code owners May 3, 2021 16:15

antoniovicente reviewed May 4, 2021

View reviewed changes

Moved location of method.

f7ca7dd

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo commented May 4, 2021

View reviewed changes

KBaichoo mentioned this pull request May 4, 2021

Cleanup: Remove setWatermarks(low,high). #16307

Merged

antoniovicente added the waiting label May 6, 2021

KBaichoo added 2 commits May 6, 2021 21:39

Merged upstream.

922a222

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Use Release Assert in test code with side effects (i.e. erase).

0295ac9

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label May 6, 2021

Minor Fixes.

1644e66

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed May 7, 2021

View reviewed changes

antoniovicente added the waiting label May 7, 2021

KBaichoo added 2 commits May 10, 2021 22:33

Added disabled by default test only runtime guard.

091dc3e

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/main' into accounting-connect-…

2d5fba4

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label May 10, 2021

KBaichoo changed the title ~~WIP: Implemented H2 stream level buffer accounting.~~ Implemented H2 stream level buffer accounting. May 10, 2021

Clangtidy.

3e5f947

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente reviewed May 19, 2021

View reviewed changes

KBaichoo added 2 commits May 19, 2021 20:40

Cleanup tests.

2e65c5d

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/main' into accounting-connect-…

f204c8a

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

KBaichoo commented May 19, 2021

View reviewed changes

antoniovicente reviewed May 19, 2021

View reviewed changes

test/integration/buffer_accounting_integration_test.cc Outdated Show resolved Hide resolved

test/integration/tracked_watermark_buffer.h Outdated Show resolved Hide resolved

test/integration/buffer_accounting_integration_test.cc Show resolved Hide resolved

antoniovicente added the waiting label May 19, 2021

Additional test infra changes.

c06b5d5

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

repokitteh-read-only bot removed the waiting label May 20, 2021

antoniovicente reviewed May 20, 2021

View reviewed changes

KBaichoo added 3 commits May 21, 2021 13:06

Nit fix.

7b32a8d

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Merge remote-tracking branch 'upstream/main' into accounting-connect-…

60ca164

…upstream Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

Coverage revert.

03e7a13

Signed-off-by: Kevin Baichoo <kbaichoo@google.com>

antoniovicente approved these changes May 21, 2021

View reviewed changes

repokitteh-read-only bot assigned lizan May 21, 2021

antoniovicente merged commit 5c51bf1 into envoyproxy:main May 27, 2021

KBaichoo mentioned this pull request Jun 22, 2021

Buffer: Implement tracking for BufferMemoryAccounts in WatermarkFactory. #17093

Merged

Conversation

KBaichoo commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

KBaichoo commented Apr 28, 2021

Uh oh!

antoniovicente left a comment

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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KBaichoo left a comment

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!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

KBaichoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

antoniovicente left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

KBaichoo commented May 10, 2021

Uh oh!

repokitteh-read-only bot commented May 17, 2021

KBaichoo commented Apr 28, 2021 •

edited

Loading