Buffer Factory: Configuration of the minimum account size to track. #17562

KBaichoo · 2021-08-02T14:15:44Z

We can now configure the minimum account size to track.

Signed-off-by: Kevin Baichoo [email protected]

Commit Message: Buffer Factory: Configuration of the minimum account size to track.
Additional Description:
Risk Level: low
Testing: unit test
Docs Changes: NA
Release Notes: NA -- the feature isn't entirely user-facing. The documentation and release note changes are already finished in a PR that builds off of this. See: https://github.com/KBaichoo/envoy/pull/111/files#diff-c69815663eb82a9e98598ce5dc2c55bc7a4b0b180dd7fe188ef1227bdab9160c
Related Issue #15791

account size to track. Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only · 2021-08-02T14:15:52Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17562 was opened by KBaichoo.

see: more, trace.

KBaichoo · 2021-08-02T14:18:03Z

/assign @antoniovicente

PTAL

Signed-off-by: Kevin Baichoo <[email protected]>

htuch

API review

htuch · 2021-08-03T04:06:42Z

api/envoy/config/bootstrap/v3/bootstrap.proto

@@ -219,6 +219,8 @@ message Bootstrap {
    (udpa.annotations.security).configure_for_untrusted_upstream = true
  ];

+  BufferFactoryConfig buffer_factory_config = 33;


Needs comment. Should this be placed in some other message like ResourceTrackingConfig for future proofing? Not sure what else we anticipate here, but not a big fan of tons of knobs on the top-level bootstrap.

htuch · 2021-08-03T04:06:55Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+// Configuration for the Buffer Factories that create Buffers and Accounts.
+message BufferFactoryConfig {
+  // The minimum account size at which Envoy starts tracking a stream.
+  // This *MUST* be a power of two.


Why not make it the exponent then?

Good idea, will go with this.

antoniovicente

Sorry for review delays. Main comment is how these new config fields would interact with overload manager actions.

antoniovicente · 2021-08-03T19:17:04Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+  // [account_tracking_threshold_bytes, 2 * account_tracking_threshold_bytes).
+  // With the 8th bucket tracking accounts
+  // >= 128 * account_tracking_threshold_bytes.
+  uint32 account_tracking_threshold_bytes = 1 [(validate.rules).uint32 = {gt: 0}];


Would it be possible to add a proto validator to verify it is a power of 2?

It's currently not supported by protogen validate, will go with exponent as htuch suggested.

antoniovicente · 2021-08-03T19:18:30Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+
+// Configuration for the Buffer Factories that create Buffers and Accounts.
+message BufferFactoryConfig {
+  // The minimum account size at which Envoy starts tracking a stream.


Worth providing config options for both the min and max thresholds?

I think min with 8 buckets based on power of two should be sufficient for now -- e.g. if first bucket's range is [Y, 2Y) the final bucket will be >= 128 * Y.

antoniovicente · 2021-08-03T19:31:20Z

api/envoy/config/bootstrap/v3/bootstrap.proto

@@ -642,3 +644,16 @@ message CustomInlineHeader {
  // The type of the header that is expected to be set as the inline header.
  InlineHeaderType inline_header_type = 2 [(validate.rules).enum = {defined_only: true}];
 }
+
+// Configuration for the Buffer Factories that create Buffers and Accounts.
+message BufferFactoryConfig {


Are there other places in the config where we should consider configuring this? HttpConnectionManager or listener may make sense. The issue is that each permutation of the config would require a separate set of tracker objects.

Also, what would the overload manager config for this functionality would look like? This new config field feels like typed config for the overload manager reset streams action.

There's one buffer factory per worker. I think configuring it on something dynamic e.g. listeners would add additional complexity.

I see the argument for this being a typed config for an overload manager reset stream action, would need to do a good amount of plumbing to expose the overload manager to the API object from which I can create the watermark factory. Of the 6 overload actions here: https://www.envoyproxy.io/docs/envoy/latest/configuration/operations/overload_manager/overload_manager#overload-actions only one uses additional typed_config (reduce_timeouts)

See: https://github.com/KBaichoo/envoy/pull/111/files#diff-c69815663eb82a9e98598ce5dc2c55bc7a4b0b180dd7fe188ef1227bdab9160cR170-R190 for how the overload manager could leverage this without additional typed config

I also see the benefit of tracking being independent e.g. stats monitoring the buckets

Looking into the feasibility of this in an overload_manager action:

api_impl creates watermark buffer factories through its creation of dispatchers. The watermark buffer factories should know the minimum account sizes to track at creation.

If we shift the knowledge of the minimum account size to an overload manager action, then the api_impl needs access to the overload manager in order to create dispatchers. But the overload manager needs the api object in its ctor.

Going down this route seems very convoluted given that dependence.

We could have instead the api_impl parse some of the overload manager config from the bootstrap to get the knowledge, but that also seems wrong since the module is leaking.

As such I'm leaning towards wrapping this in ResourceTrackingConfig or similar as htuch recommends to future proof it.

How about having these parameters are part of the toplevel OverloadManager proto since they are related to that functional area? The OverloadManager proto is already a parsed member in the bootstrap. The config option would go here:

envoy/api/envoy/config/overload/v3/overload.proto

Line 149 in 64d9e45

google.protobuf.Duration refresh_interval = 1;

Done, moved to top level of the Overload Manager config.

antoniovicente · 2021-08-03T20:07:46Z

source/common/buffer/watermark_buffer.cc

@@ -195,10 +215,11 @@ BufferMemoryAccountImpl::createAccount(WatermarkBufferFactory* factory,
 }

 int BufferMemoryAccountImpl::balanceToClassIndex() {
-  const uint64_t shifted_balance = buffer_memory_allocated_ >> 20; // shift by 1MB.
+  static uint32_t bitshift = factory_->bitshift();


Do you mean const?
Use of static here is sure to cause problems in tests as test cases beyond the first would effectively ignore config changes.

I prematurely optimized will make it const instead.

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo · 2021-08-05T13:51:25Z

/retest

repokitteh-read-only · 2021-08-05T13:51:29Z

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

🐱

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

see: more, trace.

htuch

/lgtm api

antoniovicente · 2021-08-09T17:37:59Z

source/common/buffer/watermark_buffer.h

@@ -206,6 +212,7 @@ class WatermarkBufferFactory : public WatermarkFactory {
  using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>,
                                                BufferMemoryAccountImpl::NUM_MEMORY_CLASSES_>;
  MemoryClassesToAccountsSet size_class_account_sets_;
+  uint32_t bitshift_;


A comment would be good.

antoniovicente · 2021-08-09T17:41:38Z

test/integration/tracked_watermark_buffer.cc

+    : WatermarkBufferFactory([min_tracking_bytes]() {
+        auto config = envoy::config::bootstrap::v3::ResourceTrackingConfig::BufferFactoryConfig();
+        if (min_tracking_bytes > 0) {
+          config.set_minimum_account_to_track_power_of_two(absl::bit_width(min_tracking_bytes));


Could you take minimum_account_to_track_power_of_two as a constructor argument instead of min_tracking_bytes?

antoniovicente · 2021-08-09T17:42:51Z

test/common/buffer/buffer_memory_account_test.cc

+
+TEST(WatermarkBufferFactoryTest, DefaultsToTrackingAccountsGreaterThanOrEqualTo256KB) {
+  TrackedWatermarkBufferFactory factory;
+  EXPECT_EQ(factory.bitshift(), 18);


It seems like the previous 2 test cases are testing the behavior of a test-only class.

I invoke the WatermarkBufferFactory ctor directly now.

antoniovicente · 2021-08-09T17:51:50Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+    // 2^(minimum_account_to_track_power_of_two + 1)) bytes.
+    // With the 8th bucket tracking accounts
+    // >= 128 * 2^minimum_account_to_track_power_of_two.
+    uint32 minimum_account_to_track_power_of_two = 1 [(validate.rules).uint32 = {lte: 63 gt: 0}];


Setting the exponent to 1 may not produce great results either since it would only track up to 128 bytes, don't know if it would be worth using a more reasonable floor like 1024 bytes.

Setting to 63 effectively disables all tracking since it would result in a bit shift that would 0 out all reasonable buffer byte sizes. I guess this is a reasonable max.

Floor is now 1024 bytes.

antoniovicente · 2021-08-09T17:53:08Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+    // [2^minimum_account_to_track_power_of_two,
+    // 2^(minimum_account_to_track_power_of_two + 1)) bytes.
+    // With the 8th bucket tracking accounts
+    // >= 128 * 2^minimum_account_to_track_power_of_two.


We should describe the default behavior. Another related question is what the default behavior should be: Some specific value or no resource tracking.

"No resource tracking" seems like the more reasonable default.

Done the default is to effectively disable tracking.

antoniovicente · 2021-08-09T17:57:45Z

api/envoy/config/bootstrap/v3/bootstrap.proto

@@ -642,3 +644,16 @@ message CustomInlineHeader {
  // The type of the header that is expected to be set as the inline header.
  InlineHeaderType inline_header_type = 2 [(validate.rules).enum = {defined_only: true}];
 }
+
+// Configuration for the Buffer Factories that create Buffers and Accounts.
+message BufferFactoryConfig {


How about having these parameters are part of the toplevel OverloadManager proto since they are related to that functional area? The OverloadManager proto is already a parsed member in the bootstrap. The config option would go here:

envoy/api/envoy/config/overload/v3/overload.proto

Line 149 in 64d9e45

google.protobuf.Duration refresh_interval = 1;

default behavior to not track. Signed-off-by: Kevin Baichoo <[email protected]>

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo

Thanks for the review @antoniovicente

KBaichoo · 2021-08-09T19:04:54Z

source/common/buffer/watermark_buffer.h

@@ -206,6 +212,7 @@ class WatermarkBufferFactory : public WatermarkFactory {
  using MemoryClassesToAccountsSet = std::array<absl::flat_hash_set<BufferMemoryAccountSharedPtr>,
                                                BufferMemoryAccountImpl::NUM_MEMORY_CLASSES_>;
  MemoryClassesToAccountsSet size_class_account_sets_;
+  uint32_t bitshift_;


KBaichoo · 2021-08-09T20:52:01Z

test/integration/tracked_watermark_buffer.cc

+    : WatermarkBufferFactory([min_tracking_bytes]() {
+        auto config = envoy::config::bootstrap::v3::ResourceTrackingConfig::BufferFactoryConfig();
+        if (min_tracking_bytes > 0) {
+          config.set_minimum_account_to_track_power_of_two(absl::bit_width(min_tracking_bytes));


KBaichoo · 2021-08-09T20:59:42Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+    // 2^(minimum_account_to_track_power_of_two + 1)) bytes.
+    // With the 8th bucket tracking accounts
+    // >= 128 * 2^minimum_account_to_track_power_of_two.
+    uint32 minimum_account_to_track_power_of_two = 1 [(validate.rules).uint32 = {lte: 63 gt: 0}];


Floor is now 1024 bytes.

KBaichoo · 2021-08-09T21:37:37Z

api/envoy/config/bootstrap/v3/bootstrap.proto

+    // [2^minimum_account_to_track_power_of_two,
+    // 2^(minimum_account_to_track_power_of_two + 1)) bytes.
+    // With the 8th bucket tracking accounts
+    // >= 128 * 2^minimum_account_to_track_power_of_two.


Done the default is to effectively disable tracking.

KBaichoo · 2021-08-09T21:48:12Z

test/common/buffer/buffer_memory_account_test.cc

+
+TEST(WatermarkBufferFactoryTest, DefaultsToTrackingAccountsGreaterThanOrEqualTo256KB) {
+  TrackedWatermarkBufferFactory factory;
+  EXPECT_EQ(factory.bitshift(), 18);


I invoke the WatermarkBufferFactory ctor directly now.

KBaichoo · 2021-08-09T22:20:28Z

api/envoy/config/bootstrap/v3/bootstrap.proto

@@ -642,3 +644,16 @@ message CustomInlineHeader {
  // The type of the header that is expected to be set as the inline header.
  InlineHeaderType inline_header_type = 2 [(validate.rules).enum = {defined_only: true}];
 }
+
+// Configuration for the Buffer Factories that create Buffers and Accounts.
+message BufferFactoryConfig {


Done, moved to top level of the Overload Manager config.

KBaichoo · 2021-08-10T00:00:38Z

/retest

repokitteh-read-only · 2021-08-10T00:00:41Z

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

🐱

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

see: more, trace.

yanavlasov

/wait

yanavlasov · 2021-08-11T14:49:43Z

api/envoy/config/overload/v3/overload.proto

+  // With the 8th bucket tracking accounts
+  // >= 128 * 2^minimum_account_to_track_power_of_two.
+  //
+  // Setting this to 63 effectively disables all tracking since the resulting


Shouldn't this be 64 - 8 = 56 ? Otherwise some higher order buckets will not be tracked.

sgtm will set the max to in pgv to 56 as that's the last valid config that'd use all 8 buckets (56-63). Don't expect that we'll have any buffers using 2^56 bytes in practice.

Omitting the field effectively disables it.

yanavlasov · 2021-08-11T15:00:33Z

envoy/api/api.h

+  /**
+   * @return the bootstrap Envoy started with.
+   */
+  virtual const envoy::config::bootstrap::v3::Bootstrap& bootstrap() const PURE;


I think dispatchers are always allocated through the Api interface. In this case you do not need this method. Just save the bootstrap in the ApiImpl and then pass buffer config to the dispatcher constructor.

Dispatchers are created outside of the Api interface in certain places.

Given that the Api has the information, and is already plumbed through the 3 ctors of the dispatcher, avoiding telescoping ctor by having the object with the information expose it seemed like the way to go.

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo

Thanks for the review @yanavlasov

KBaichoo · 2021-08-11T15:08:43Z

api/envoy/config/overload/v3/overload.proto

+  // With the 8th bucket tracking accounts
+  // >= 128 * 2^minimum_account_to_track_power_of_two.
+  //
+  // Setting this to 63 effectively disables all tracking since the resulting


sgtm will set the max to in pgv to 56 as that's the last valid config that'd use all 8 buckets (56-63). Don't expect that we'll have any buffers using 2^56 bytes in practice.

Omitting the field effectively disables it.

KBaichoo · 2021-08-11T15:35:36Z

envoy/api/api.h

+  /**
+   * @return the bootstrap Envoy started with.
+   */
+  virtual const envoy::config::bootstrap::v3::Bootstrap& bootstrap() const PURE;


Dispatchers are created outside of the Api interface in certain places.

Given that the Api has the information, and is already plumbed through the 3 ctors of the dispatcher, avoiding telescoping ctor by having the object with the information expose it seemed like the way to go.

antoniovicente

Thanks.

KBaichoo · 2021-08-11T20:09:04Z

@htuch for api-review round 2

htuch

/lgtm api

- refactoring `ProcessImpl`, creating a named constructor that allows us to create bootstrap initially and pass it to `Envoy::Api::Impl()` as required since envoyproxy/envoy#17562. Specifically: - Moving method `ProcessImpl::determineConcurrency()` out of the ProcessImpl class so that it can be used during its construction. - Moving code that extracts URIs from `process_impl.cc` into `process_bootstrap.cc`. - Adding a previously missing test case `CreatesBootstrapForH1RespectingPortInUri` into `process_bootstrap_test.cc`. - Removing a TODO that incorrectly indicated URI DNS resolution is optional. Envoy [requires](https://github.com/envoyproxy/envoy/blob/716ee8abc526d51f07ed6d3c2a5aa8a3b2481d9d/api/envoy/config/core/v3/address.proto#L64-L67) resolved IPs in the Bootstrap for cluster of type STATIC. - Creating a named constructor for `ProcessImpl` that creates the `Envoy::Api::Api` with an empty Bootstrap that is then replaced with the one generated. See an inline comment for explanation. - Moving callers onto the named constructor and making the original constructor of `ProcessImpl` private. - no changes to `.bazelrc`, `.bazelversion`, `run_envoy_docker.sh`. Signed-off-by: Jakub Sobon <[email protected]>

…nvoyproxy#17562) Risk Level: low Testing: unit test Docs Changes: NA Release Notes: NA -- the feature isn't entirely user-facing. The documentation and release note changes are already finished in a PR that builds off of this. See: https://github.com/KBaichoo/envoy/pull/111/files#diff-c69815663eb82a9e98598ce5dc2c55bc7a4b0b180dd7fe188ef1227bdab9160c Related Issue envoyproxy#15791 Signed-off-by: Kevin Baichoo <[email protected]>

Add configurability of BufferFactory. We can now configure the minimum

ec8b808

account size to track. Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only bot added the api label Aug 2, 2021

repokitteh-read-only bot assigned markdroth Aug 2, 2021

KBaichoo mentioned this pull request Aug 2, 2021

Buffer Factory: Add configurability of BufferFactory. KBaichoo/envoy#105

Closed

repokitteh-read-only bot assigned antoniovicente Aug 2, 2021

KBaichoo added 4 commits August 2, 2021 17:24

linux gcc error fix.

d6eedf2

Signed-off-by: Kevin Baichoo <[email protected]>

Merge remote-tracking branch 'upstream/main' into config-sizes

99581c2

Signed-off-by: Kevin Baichoo <[email protected]>

Fix merge snafu.

327f92e

Signed-off-by: Kevin Baichoo <[email protected]>

msvc does not support constexpr absl::bit_width.

64d9e45

Signed-off-by: Kevin Baichoo <[email protected]>

markdroth assigned htuch and unassigned markdroth Aug 2, 2021

htuch reviewed Aug 3, 2021

View reviewed changes

antoniovicente reviewed Aug 3, 2021

View reviewed changes

KBaichoo added 3 commits August 4, 2021 13:23

Merge remote-tracking branch 'upstream/main' into config-sizes

165f789

Signed-off-by: Kevin Baichoo <[email protected]>

API changes.

1fd0746

Signed-off-by: Kevin Baichoo <[email protected]>

format fix.

922c0b0

Signed-off-by: Kevin Baichoo <[email protected]>

htuch reviewed Aug 6, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 6, 2021

antoniovicente reviewed Aug 9, 2021

View reviewed changes

antoniovicente added the waiting label Aug 9, 2021

KBaichoo added 2 commits August 9, 2021 22:25

Move proto to overload manager top level config, cleanup tests, change

d676284

default behavior to not track. Signed-off-by: Kevin Baichoo <[email protected]>

Merge remote-tracking branch 'upstream/main' into config-sizes

a2fad3c

Signed-off-by: Kevin Baichoo <[email protected]>

repokitteh-read-only bot added api and removed waiting labels Aug 9, 2021

KBaichoo commented Aug 9, 2021

View reviewed changes

yanavlasov requested changes Aug 11, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 11, 2021

yanavlasov self-assigned this Aug 11, 2021

Update max value for minimum power of two to track.

cf59142

Signed-off-by: Kevin Baichoo <[email protected]>

KBaichoo commented Aug 11, 2021

View reviewed changes

repokitteh-read-only bot removed the waiting label Aug 11, 2021

antoniovicente approved these changes Aug 11, 2021

View reviewed changes

yanavlasov approved these changes Aug 11, 2021

View reviewed changes

htuch reviewed Aug 12, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 12, 2021

htuch merged commit b51f0ea into envoyproxy:main Aug 12, 2021

qqustc mentioned this pull request Aug 16, 2021

Update Envoy to 295aba4 (Aug 16 2021) envoyproxy/nighthawk#733

Closed

mum4k mentioned this pull request Aug 18, 2021

Update Envoy to 95038fe (Aug 12th 2021). envoyproxy/nighthawk#734

Merged

Buffer Factory: Configuration of the minimum account size to track. #17562

Buffer Factory: Configuration of the minimum account size to track. #17562

Conversation

KBaichoo commented Aug 2, 2021 • edited Loading

repokitteh-read-only bot commented Aug 2, 2021

KBaichoo commented Aug 2, 2021

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo Aug 3, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo commented Aug 5, 2021

repokitteh-read-only bot commented Aug 5, 2021

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo commented Aug 10, 2021

repokitteh-read-only bot commented Aug 10, 2021

yanavlasov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KBaichoo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antoniovicente left a comment

Choose a reason for hiding this comment

KBaichoo commented Aug 11, 2021

htuch left a comment

Choose a reason for hiding this comment

KBaichoo commented Aug 2, 2021 •

edited

Loading

KBaichoo Aug 3, 2021 •

edited

Loading