Draft
Conversation
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “FairTokenBucket” implementation intended to fairly distribute tokens across tenants (weighted) and requests (evenly within a tenant) for the bandwidth_share HTTP filter area.
Changes:
- Add
FairTokenBucket::{Factory,Tenant,Request}implementation backed byTokenBucketImpl. - Add unit tests covering fairness, deletion behavior, and token “stacking”.
- Add Bazel BUILD targets for the new library/tests and update CODEOWNERS for the extension path.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.h |
Declares the FairTokenBucket types and synchronization strategy. |
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc |
Implements token distribution, queuing, and spill logic. |
source/extensions/filters/http/bandwidth_share/BUILD |
Adds the fair_token_bucket_impl_lib Bazel target. |
test/extensions/filters/http/bandwidth_share/fair_token_bucket_impl_test.cc |
Adds unit tests for fairness and queue/spill behaviors. |
test/extensions/filters/http/bandwidth_share/BUILD |
Adds the fair_token_bucket_impl_test Bazel target. |
CODEOWNERS |
Adds owners for */extensions/filters/http/bandwidth_share. |
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.h
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/bandwidth_share/fair_token_bucket_impl_test.cc
Outdated
Show resolved
Hide resolved
test/extensions/filters/http/bandwidth_share/fair_token_bucket_impl_test.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Outdated
Show resolved
Hide resolved
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc
Show resolved
Hide resolved
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Signed-off-by: Raven Black <ravenblack@dropbox.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a new “fair” token-bucket implementation intended to support bandwidth share behavior under overload, along with initial unit tests and documentation updates in the bandwidth limiting/share area.
Changes:
- Introduces
FairTokenBucketimplementation (Factory/Tenant/Request) for weighted token distribution when limiting. - Adds a unit test suite and Bazel targets for the new implementation.
- Updates docs and CODEOWNERS for the new bandwidth_share filter area.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.h |
Declares the fair token bucket classes and data structures. |
source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.cc |
Implements spill/queueing and token distribution logic. |
source/extensions/filters/http/bandwidth_share/BUILD |
Adds a cc_library target for the new implementation. |
test/extensions/filters/http/bandwidth_share/fair_token_bucket_impl_test.cc |
Adds unit tests for fair-share behavior. |
test/extensions/filters/http/bandwidth_share/BUILD |
Adds a test target for the new unit tests. |
docs/root/intro/arch_overview/other_features/bandwidth_limiting.rst |
Mentions fair-sharing alongside bandwidth limiting (adds cross-ref). |
docs/root/configuration/http/http_filters/bandwidth_share_filter.rst |
Adds filter documentation for bandwidth share. |
CODEOWNERS |
Adds owners for the bandwidth_share HTTP filter directory. |
Comment on lines
+39
to
+43
| // Should be impossible to fail to lock because deletion removes from the map immediately. | ||
| ASSERT(tenant); | ||
| Thread::LockGuard lock(mutex_); | ||
| tenant->weight_ = weight; | ||
| return tenant; |
| } | ||
|
|
||
| uint64_t Request::consume(uint64_t want_tokens, bool allow_partial) { | ||
| ASSERT(allow_partial, "not implemented with allow_partial=false, because not used"); |
Comment on lines
+108
to
+111
| bool spill(Factory& factory, uint64_t tokens) ABSL_EXCLUSIVE_LOCKS_REQUIRED(&Factory::mutex_) { | ||
| std::cerr << "tenants_.size()=" << tenants_.size() << ", want_tokens_=" << want_tokens_ | ||
| << std::endl; | ||
| if (tokens == want_tokens_) { |
Comment on lines
+120
to
+140
| uint64_t tokens_per_weight = tokens / total_weight_; | ||
| // Give the unweighted remainder to whoever is first in the queue and needs more. | ||
| uint64_t spare_tokens = tokens - tokens_per_weight * total_weight_; | ||
| auto partition_point = std::partition( | ||
| tenants_.begin(), tenants_.end(), | ||
| [&](TenantWithRequests& t) ABSL_EXCLUSIVE_LOCKS_REQUIRED(&Factory::mutex_) { | ||
| uint64_t to_consume = std::min(tokens_per_weight * t.tenant_->weight_, t.want_tokens_); | ||
| if (spare_tokens > 0 && t.want_tokens_ > to_consume) { | ||
| uint64_t also_consume = std::min(spare_tokens, t.want_tokens_ - to_consume); | ||
| spare_tokens -= also_consume; | ||
| to_consume += also_consume; | ||
| } | ||
| tokens -= to_consume; | ||
| if (!spillToTenant(t, to_consume)) { | ||
| total_weight_ -= t.tenant_->weight_; | ||
| return false; | ||
| } | ||
| return true; | ||
| }); | ||
| if (partition_point != tenants_.end()) { | ||
| tenants_.erase(partition_point, tenants_.end()); |
Comment on lines
+3
to
+4
| #include <memory> | ||
| #include <tuple> |
| globally at the listener level or at a more specific level (e.g.: the virtual host or route level). | ||
|
|
||
| Envoy supports local (non-distributed) bandwidth fair-sharing of HTTP requests and response, via the | ||
| :ref:`HTTP bandwidth fair share filter <config_http_filters_bandwidth_fair_share>`. This can be activated |
Comment on lines
+8
to
+12
| * :ref:`v3 API reference <envoy_v3_api_msg_extensions.filters.http.bandwidth_share.v3.BandwidthLimit>` | ||
|
|
||
| The HTTP Bandwidth share filter limits the size of data flow to the max bandwidth set in the ``limit_kbps`` | ||
| when the request's route, virtual host or filter chain has a | ||
| :ref:`bandwidth share configuration <envoy_v3_api_msg_extensions.filters.http.bandwidth_share.v3.BandwidthLimit>`. |
| data then the source of data will have ``readDisable(true)`` set as described in the :repo:`flow control doc<source/docs/flow_control.md>`. | ||
|
|
||
| When actively being limited, the filter splits the available bandwidth between active tenants by weight, and | ||
| between parallel requests for a single tenant evently. For example, with six active requests divided |
Comment on lines
+1
to
+7
| #include "source/extensions/filters/http/bandwidth_share/fair_token_bucket_impl.h" | ||
|
|
||
| #include <chrono> | ||
|
|
||
| #include "source/common/common/assert.h" | ||
| #include "source/common/common/lock_guard.h" | ||
|
|
|
|
||
| #include "test/test_common/simulated_time_system.h" | ||
| #include "test/test_common/utility.h" | ||
|
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]