-
Notifications
You must be signed in to change notification settings - Fork 5.5k
retry extensions: implement "other priority" extension #4529
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
Changes from 4 commits
f91c5ce
f52c9dc
6b6c025
15967bc
2274a5a
158d084
f619696
16be1ce
60c4fa6
6d446f5
978f3f7
e5b3b36
48c0963
83166de
580cb5c
b8abcd3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load("//bazel:api_build_system.bzl", "api_proto_library_internal") | ||
|
|
||
| api_proto_library_internal( | ||
| name = "other_priority", | ||
| srcs = ["other_priority_config.proto"], | ||
| deps = [ | ||
| "//envoy/api/v2/core:base", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| syntax = "proto3"; | ||
|
|
||
| package envoy.config.retry.other_priority; | ||
|
|
||
| // A retry host selector that attempts to spread retries between priorities, even if certain | ||
| // priorities would not normally be attempted due to higher priorities being available. | ||
| message OtherPriorityConfig { | ||
| // How often the priority load should be updated based on previously attempted priorities. Useful | ||
| // to allow each priorities to receive more than one request before being excluded. | ||
| int32 update_frequency = 1; | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -70,10 +70,13 @@ class LoadBalancerBase : public LoadBalancer { | |
| // The priority-ordered set of hosts to use for load balancing. | ||
| const PrioritySet& priority_set_; | ||
|
|
||
| public: | ||
| // Called when a host set at the given priority level is updated. This updates | ||
| // per_priority_health_ for that priority level, and may update per_priority_load_ for all | ||
| // priority levels. | ||
| void recalculatePerPriorityState(uint32_t priority); | ||
| void static recalculatePerPriorityState(uint32_t priority, const PrioritySet& priority_set, | ||
| PriorityLoad& priority_load, | ||
| std::vector<uint32_t>& per_priority_health_); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this becomes a static function that doesn't access the members, I don't think the argument names want the _ suffix (and the comment should also probably be updated). |
||
|
|
||
| // The percentage load (0-100) for each priority level | ||
| std::vector<uint32_t> per_priority_load_; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you meant to make these vectors public?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, I'll fix |
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "well_known_names", | ||
| hdrs = ["well_known_names.h"], | ||
| deps = [ | ||
| "//source/common/singleton:const_singleton", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| licenses(["notice"]) # Apache 2 | ||
|
|
||
| load( | ||
| "//bazel:envoy_build_system.bzl", | ||
| "envoy_cc_library", | ||
| "envoy_package", | ||
| ) | ||
|
|
||
| envoy_package() | ||
|
|
||
| envoy_cc_library( | ||
| name = "other_priority_lib", | ||
| srcs = ["other_priority.cc"], | ||
| hdrs = ["other_priority.h"], | ||
| deps = [ | ||
| "//include/envoy/upstream:retry_interface", | ||
| "//source/common/upstream:load_balancer_lib", | ||
| ], | ||
| ) | ||
|
|
||
| envoy_cc_library( | ||
| name = "config", | ||
| srcs = ["config.cc"], | ||
| hdrs = ["config.h"], | ||
| deps = [ | ||
| ":other_priority_lib", | ||
| "//include/envoy/registry", | ||
| "//include/envoy/upstream:retry_interface", | ||
| "//source/common/protobuf", | ||
| "//source/extensions/retry/priority:well_known_names", | ||
| "@envoy_api//envoy/config/retry/other_priority:other_priority_cc", | ||
| ], | ||
| ) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,26 @@ | ||
| #include "extensions/retry/priority/other_priority/config.h" | ||
|
|
||
| #include "envoy/config/retry/other_priority/other_priority_config.pb.validate.h" | ||
| #include "envoy/registry/registry.h" | ||
| #include "envoy/upstream/retry.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Retry { | ||
| namespace Priority { | ||
|
|
||
| void OtherPriorityRetryPriorityFactory::createRetryPriority( | ||
| Upstream::RetryPriorityFactoryCallbacks& callbacks, const Protobuf::Message& config) { | ||
| callbacks.addRetryPriority(std::make_shared<OtherPriorityRetryPriority>( | ||
| MessageUtil::downcastAndValidate< | ||
| const envoy::config::retry::other_priority::OtherPriorityConfig&>(config) | ||
| .update_frequency())); | ||
| } | ||
|
|
||
| static Registry::RegisterFactory<OtherPriorityRetryPriorityFactory, Upstream::RetryPriorityFactory> | ||
| register_; | ||
|
|
||
| } // namespace Priority | ||
| } // namespace Retry | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| #pragma once | ||
|
|
||
| #include "envoy/upstream/retry.h" | ||
|
|
||
| #include "common/protobuf/protobuf.h" | ||
|
|
||
| #include "extensions/retry/priority/other_priority/other_priority.h" | ||
| #include "extensions/retry/priority/well_known_names.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Retry { | ||
| namespace Priority { | ||
|
|
||
| class OtherPriorityRetryPriorityFactory : public Upstream::RetryPriorityFactory { | ||
| public: | ||
| void createRetryPriority(Upstream::RetryPriorityFactoryCallbacks& callbacks, | ||
| const Protobuf::Message& config) override; | ||
|
|
||
| std::string name() const override { | ||
| return RetryPriorityValues::get().PreviousPrioritiesRetryPriority; | ||
| } | ||
|
|
||
| ProtobufTypes::MessagePtr createEmptyConfigProto() override { | ||
| return ProtobufTypes::MessagePtr(new ::Envoy::ProtobufWkt::Empty()); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace Priority | ||
| } // namespace Retry | ||
| } // namespace Extensions | ||
| } // namespace Envoy |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| #include "extensions/retry/priority/other_priority/other_priority.h" | ||
|
|
||
| namespace Envoy { | ||
| namespace Extensions { | ||
| namespace Retry { | ||
| namespace Priority { | ||
| const Upstream::PriorityLoad& OtherPriorityRetryPriority::determinePriorityLoad( | ||
| const Upstream::PrioritySet& priority_set, | ||
| const Upstream::PriorityLoad& original_priority_load) { | ||
| if (!priority_set_) { | ||
| priority_set_ = &priority_set; | ||
|
|
||
| excluded_priorities_.resize(original_priority_load.size()); | ||
| // Initialize our local priority_load_ and priority_health_, | ||
| // keeping them in sync with the member update cb. | ||
| for (auto& host_set : priority_set_->hostSetsPerPriority()) { | ||
| recalculatePerPriorityState(host_set->priority()); | ||
| } | ||
| callback_ = priority_set.addMemberUpdateCb([&, this](int priority, auto, auto) { | ||
| this->recalculatePerPriorityState(priority); | ||
| this->adjustForAttemptedPriorities(); | ||
| }); | ||
| } | ||
|
|
||
| // If we've not seen enough retries to modify the priority load, just | ||
| // return the original. | ||
| // If this retry should trigger an update, recalculate the priority load by excluding attempted | ||
| // priorities. | ||
| if (attempted_priorites_.size() < update_frequency_) { | ||
| return original_priority_load; | ||
| } else if (attempted_priorites_.size() % update_frequency_ == 0) { | ||
| for (auto priority : attempted_priorites_) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: |
||
| excluded_priorities_[priority] = true; | ||
| } | ||
|
|
||
| adjustForAttemptedPriorities(); | ||
| } | ||
|
|
||
| return per_priority_load_; | ||
| } | ||
|
|
||
| void OtherPriorityRetryPriority::adjustForAttemptedPriorities() { | ||
| // If all priorities are unhealthy to begin with, there's nothing to do. | ||
| if (!std::accumulate(per_priority_load_.begin(), per_priority_load_.end(), 0)) { | ||
| return; | ||
| } | ||
|
|
||
| // First we create an adjusted health view of the priorities, where attempted priorities are | ||
| // given a zero weight. | ||
| uint32_t total_health = 0; | ||
| std::vector<uint32_t> adjusted_per_priority_health; | ||
| adjusted_per_priority_health.resize(per_priority_health_.size()); | ||
| for (size_t i = 0; i < per_priority_health_.size(); ++i) { | ||
| if (!excluded_priorities_[i]) { | ||
| adjusted_per_priority_health[i] = per_priority_health_[i]; | ||
| total_health += per_priority_health_[i]; | ||
| } | ||
| } | ||
|
|
||
| total_health = std::min(total_health, 100u); | ||
|
|
||
| // If there are no healthy priorities left, we reset the attempted priorities and recurse. | ||
| // This allows us to fall back to the unmodified priority load when we run out of priorites | ||
| // instead of failing to route requests. | ||
| if (total_health == 0) { | ||
| for (size_t i = 0; i < excluded_priorities_.size(); ++i) { | ||
| excluded_priorities_[i] = false; | ||
| } | ||
| attempted_priorites_.clear(); | ||
|
|
||
| adjustForAttemptedPriorities(); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd prefer we avoid recursion in tricky code like this - I think it's too easy to have a refactor result in a loop. Given we;ve already verified that we aren't in the state where everything is unhealthy, what if we just factored out the prior section into a helper function and rerun the helper if we hit this case?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah that sounds better, I was being a bit too cute about it |
||
| return; | ||
| } | ||
|
|
||
| // We then adjust the load by rebalancing priorities with the adjusted health values. | ||
| Upstream::PriorityLoad per_priority_load; | ||
| per_priority_load.resize(per_priority_load_.size()); | ||
|
|
||
| size_t total_load = 100; | ||
| // The outer loop is used to eliminate rounding errors: any remaining load will be assigned to the | ||
| // first healthy priority. | ||
| while (total_load != 0) { | ||
| for (size_t i = 0; i < adjusted_per_priority_health.size(); ++i) { | ||
| // Now assign as much load as possible to the high priority levels and cease assigning load | ||
| // when total_load runs out. | ||
| auto delta = | ||
| std::min<uint32_t>(total_load, adjusted_per_priority_health[i] * 100 / total_health); | ||
| per_priority_load[i] += delta; | ||
| total_load -= delta; | ||
| } | ||
| } | ||
|
|
||
| per_priority_load_ = per_priority_load; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's unclear to me why this doesn't adjust the per_priority_load in place. If it can't be adjusted in place, this might be worth refactoring out so the call here looks more like:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it was a remnant of a previous iteration where I short circuiting during the update to fall back to the original values. I'll update it to update in place |
||
| } | ||
| } // namespace Priority | ||
| } // namespace Retry | ||
| } // namespace Extensions | ||
| } // namespace Envoy | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There might be some context I'm missing here but it's not obvious to me what the units of this frequency are.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The unit here is "number of attempts". I'll try to clarify the comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment definitely helps but reading it I misread and thought this was number_of_requests_using_default_priority. Can we extend it just a bit to say that the fourth and fifth will then use the priority load excluding priorities for the frirst 4 attempts?
We should also comment what happens when we run out of priority levels