From 253a2b7ab9d929a1761dfb3405d00158d05a364f Mon Sep 17 00:00:00 2001 From: Rama Date: Wed, 4 Jul 2018 18:18:17 +0530 Subject: [PATCH 1/5] jittered backoff implementation Signed-off-by: Rama --- source/common/common/BUILD | 1 + source/common/common/backoff_strategy.cc | 22 ++++++++ source/common/common/backoff_strategy.h | 36 +++++++++++++ source/common/router/BUILD | 1 + source/common/router/retry_state_impl.cc | 13 ++--- source/common/router/retry_state_impl.h | 4 +- test/common/common/BUILD | 1 + test/common/common/backoff_strategy_test.cc | 56 ++++++++++++++++++++- 8 files changed, 123 insertions(+), 11 deletions(-) diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 59d8827f8f6e7..f2811d7a546af 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -23,6 +23,7 @@ envoy_cc_library( deps = [ ":assert_lib", "//include/envoy/common:backoff_strategy_interface", + "//include/envoy/runtime:runtime_interface", ], ) diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index 8e345f17ac4e2..969a6bbd4326f 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -27,4 +27,26 @@ uint64_t ExponentialBackOffStrategy::computeNextInterval() { } return current_interval_; } + +JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, + Runtime::RandomGenerator& random) + : base_interval_(base_interval), random_(random) {} + +JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, + Runtime::RandomGenerator& random) + : base_interval_(base_interval), max_interval_(max_interval), random_(random) { + ASSERT(base_interval_ < max_interval_); +} + +uint64_t JitteredBackOffStrategy::nextBackOffMs() { return computeNextInterval(); } + +void JitteredBackOffStrategy::reset() { current_retry_ = 0; } + +uint64_t JitteredBackOffStrategy::computeNextInterval() { + current_retry_++; + uint32_t multiplier = (1 << current_retry_) - 1; + uint64_t new_interval = random_.random() % (base_interval_ * multiplier); + return (max_interval_ != 0 && new_interval > max_interval_) ? max_interval_ : new_interval; +} + } // namespace Envoy \ No newline at end of file diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h index 01c81d95d722d..cbd9bf4bc5ece 100644 --- a/source/common/common/backoff_strategy.h +++ b/source/common/common/backoff_strategy.h @@ -4,6 +4,7 @@ #include #include "envoy/common/backoff_strategy.h" +#include "envoy/runtime/runtime.h" #include "common/common/assert.h" @@ -30,4 +31,39 @@ class ExponentialBackOffStrategy : public BackOffStrategy { const double multiplier_; uint64_t current_interval_; }; + +/** + * Implementation of BackOffStrategy that uses a fully jittered exponential backoff algorithm. + */ +class JitteredBackOffStrategy : public BackOffStrategy { + +public: + /** + * Use this constructor if max_interval need not be enforced. + * @param base_interval the base_interval to be used for next backoff computation. + * @param random the random generator + */ + JitteredBackOffStrategy(uint64_t base_interval, Runtime::RandomGenerator& random); + + /** + * Use this constructor if max_interval need to be enforced. + * @param base_interval the base_interval to be used for next backoff computation. + * @param max_interval if the computed next backoff is more than this, this will be returned. + * @param random the random generator + */ + JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, + Runtime::RandomGenerator& random); + + // BackOffStrategy methods + uint64_t nextBackOffMs() override; + void reset() override; + +private: + uint64_t computeNextInterval(); + + const uint64_t base_interval_; + const uint64_t max_interval_{}; + uint32_t current_retry_{}; + Runtime::RandomGenerator& random_; +}; } // namespace Envoy diff --git a/source/common/router/BUILD b/source/common/router/BUILD index 1960b0c117c6f..d4dfd9712910f 100644 --- a/source/common/router/BUILD +++ b/source/common/router/BUILD @@ -123,6 +123,7 @@ envoy_cc_library( "//include/envoy/runtime:runtime_interface", "//include/envoy/upstream:upstream_interface", "//source/common/common:assert_lib", + "//source/common/common:backoff_lib", "//source/common/common:utility_lib", "//source/common/grpc:common_lib", "//source/common/http:codes_lib", diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index dccd5b123940e..957597e5d72d4 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -71,23 +71,20 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap& // Merge in the route policy. retry_on_ |= route_policy.retryOn(); retries_remaining_ = std::max(retries_remaining_, route_policy.numRetries()); + + backoff_strategy_ptr_ = std::make_unique( + runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25), random_); } RetryStateImpl::~RetryStateImpl() { resetRetry(); } void RetryStateImpl::enableBackoffTimer() { - // TODO(ramaraochavali): Implement JitteredExponentialBackOff and refactor this. - // We use a fully jittered exponential backoff algorithm. - current_retry_++; - uint32_t multiplier = (1 << current_retry_) - 1; - uint64_t base = runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25); - uint64_t timeout = random_.random() % (base * multiplier); - if (!retry_timer_) { retry_timer_ = dispatcher_.createTimer([this]() -> void { callback_(); }); } - retry_timer_->enableTimer(std::chrono::milliseconds(timeout)); + // We use a fully jittered exponential backoff algorithm. + retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_ptr_->nextBackOffMs())); } uint32_t RetryStateImpl::parseRetryOn(absl::string_view config) { diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 8f01320b83a3a..366b0d8a3bd40 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -10,6 +10,8 @@ #include "envoy/runtime/runtime.h" #include "envoy/upstream/upstream.h" +#include "common/common/backoff_strategy.h" + #include "absl/strings/string_view.h" #include "absl/types/optional.h" @@ -55,10 +57,10 @@ class RetryStateImpl : public RetryState { Event::Dispatcher& dispatcher_; uint32_t retry_on_{}; uint32_t retries_remaining_{1}; - uint32_t current_retry_{}; DoRetryCallback callback_; Event::TimerPtr retry_timer_; Upstream::ResourcePriority priority_; + BackOffStrategyPtr backoff_strategy_ptr_; }; } // namespace Router diff --git a/test/common/common/BUILD b/test/common/common/BUILD index a2705917f19c0..c7f435490b253 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -15,6 +15,7 @@ envoy_cc_test( srcs = ["backoff_strategy_test.cc"], deps = [ "//source/common/common:backoff_lib", + "//test/mocks/runtime:runtime_mocks", ], ) diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc index 88576b979f08d..3549ba956c905 100644 --- a/test/common/common/backoff_strategy_test.cc +++ b/test/common/common/backoff_strategy_test.cc @@ -1,7 +1,12 @@ #include "common/common/backoff_strategy.h" +#include "test/mocks/runtime/mocks.h" + #include "gtest/gtest.h" +using testing::NiceMock; +using testing::Return; + namespace Envoy { TEST(BackOffStrategyTest, ExponentialBackOffBasicTest) { @@ -32,7 +37,7 @@ TEST(BackOffStrategyTest, ExponentialBackOffMaxIntervalReached) { EXPECT_EQ(100, exponential_back_off.nextBackOffMs()); // Should return Max here } -TEST(BackOffStrategyTest, ExponentialBackOfReset) { +TEST(BackOffStrategyTest, ExponentialBackOffReset) { ExponentialBackOffStrategy exponential_back_off(10, 100, 2); EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); @@ -41,7 +46,7 @@ TEST(BackOffStrategyTest, ExponentialBackOfReset) { EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); // Should start from start } -TEST(BackOffStrategyTest, ExponentialBackOfResetAfterMaxReached) { +TEST(BackOffStrategyTest, ExponentialBackOffResetAfterMaxReached) { ExponentialBackOffStrategy exponential_back_off(10, 100, 2); EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); @@ -54,4 +59,51 @@ TEST(BackOffStrategyTest, ExponentialBackOfResetAfterMaxReached) { EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); // Should start from start } +TEST(BackOffStrategyTest, JitteredBackOffBasicFlow) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(27)); + + JitteredBackOffStrategy jittered_back_off(25, random); + EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); +} + +TEST(BackOffStrategyTest, JitteredBackOffBasicReset) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(27)); + + JitteredBackOffStrategy jittered_back_off(25, random); + EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); + + jittered_back_off.reset(); + EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); // Should start from start +} + +TEST(BackOffStrategyTest, JitteredBackOffWithMaxInterval) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(1024)); + + JitteredBackOffStrategy jittered_back_off(5, 15, random); + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); +} + +TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(1024)); + + JitteredBackOffStrategy jittered_back_off(5, 15, random); + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); // Should return Max here + + jittered_back_off.reset(); + EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); // Should start from start +} + } // namespace Envoy From 74fcf7127cff886d1446594b7e4309a21ba8ac0f Mon Sep 17 00:00:00 2001 From: Rama Date: Tue, 10 Jul 2018 21:16:57 +0530 Subject: [PATCH 2/5] make jitter backoff as the default Signed-off-by: Rama --- source/common/common/backoff_strategy.cc | 47 +++----------- source/common/common/backoff_strategy.h | 33 +--------- source/common/config/grpc_mux_impl.cc | 8 +-- source/common/config/grpc_mux_impl.h | 3 +- source/common/config/grpc_subscription_impl.h | 4 +- source/common/config/subscription_factory.h | 4 +- source/common/router/retry_state_impl.cc | 5 +- .../common/upstream/cluster_manager_impl.cc | 3 +- test/common/common/backoff_strategy_test.cc | 65 ++++--------------- test/common/config/BUILD | 1 + test/common/config/grpc_mux_impl_test.cc | 5 +- .../config/grpc_subscription_impl_test.cc | 6 ++ .../config/grpc_subscription_test_harness.h | 3 +- .../config/subscription_factory_test.cc | 1 + 14 files changed, 51 insertions(+), 137 deletions(-) diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index 969a6bbd4326f..23a6af61ae7ce 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -2,51 +2,22 @@ namespace Envoy { -ExponentialBackOffStrategy::ExponentialBackOffStrategy(uint64_t initial_interval, - uint64_t max_interval, double multiplier) - : initial_interval_(initial_interval), max_interval_(max_interval), multiplier_(multiplier), - current_interval_(0) { - ASSERT(multiplier_ > 1.0); - ASSERT(initial_interval_ <= max_interval_); - ASSERT(initial_interval_ * multiplier_ <= max_interval_); -} - -uint64_t ExponentialBackOffStrategy::nextBackOffMs() { return computeNextInterval(); } - -void ExponentialBackOffStrategy::reset() { current_interval_ = 0; } - -uint64_t ExponentialBackOffStrategy::computeNextInterval() { - if (current_interval_ == 0) { - current_interval_ = initial_interval_; - } else if (current_interval_ >= max_interval_) { - current_interval_ = max_interval_; - } else { - uint64_t new_interval = current_interval_; - new_interval = ceil(new_interval * multiplier_); - current_interval_ = new_interval > max_interval_ ? max_interval_ : new_interval; - } - return current_interval_; -} - -JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, - Runtime::RandomGenerator& random) - : base_interval_(base_interval), random_(random) {} - JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_t max_interval, Runtime::RandomGenerator& random) : base_interval_(base_interval), max_interval_(max_interval), random_(random) { - ASSERT(base_interval_ < max_interval_); + ASSERT(base_interval_ <= max_interval_); } -uint64_t JitteredBackOffStrategy::nextBackOffMs() { return computeNextInterval(); } - -void JitteredBackOffStrategy::reset() { current_retry_ = 0; } - -uint64_t JitteredBackOffStrategy::computeNextInterval() { +uint64_t JitteredBackOffStrategy::nextBackOffMs() { current_retry_++; uint32_t multiplier = (1 << current_retry_) - 1; - uint64_t new_interval = random_.random() % (base_interval_ * multiplier); - return (max_interval_ != 0 && new_interval > max_interval_) ? max_interval_ : new_interval; + if (multiplier == 0) { + current_retry_ = 1; + multiplier = (1 << current_retry_) - 1; + } + return std::min(random_.random() % (base_interval_ * multiplier), max_interval_); } +void JitteredBackOffStrategy::reset() { current_retry_ = 0; } + } // namespace Envoy \ No newline at end of file diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h index cbd9bf4bc5ece..ed116c1797361 100644 --- a/source/common/common/backoff_strategy.h +++ b/source/common/common/backoff_strategy.h @@ -10,41 +10,12 @@ namespace Envoy { -/** - * Implementation of BackOffStrategy that increases the back off period for each retry attempt. When - * the interval has reached the max interval, it is no longer increased. - */ -class ExponentialBackOffStrategy : public BackOffStrategy { - -public: - ExponentialBackOffStrategy(uint64_t initial_interval, uint64_t max_interval, double multiplier); - - // BackOffStrategy methods - uint64_t nextBackOffMs() override; - void reset() override; - -private: - uint64_t computeNextInterval(); - - const uint64_t initial_interval_; - const uint64_t max_interval_; - const double multiplier_; - uint64_t current_interval_; -}; - /** * Implementation of BackOffStrategy that uses a fully jittered exponential backoff algorithm. */ class JitteredBackOffStrategy : public BackOffStrategy { public: - /** - * Use this constructor if max_interval need not be enforced. - * @param base_interval the base_interval to be used for next backoff computation. - * @param random the random generator - */ - JitteredBackOffStrategy(uint64_t base_interval, Runtime::RandomGenerator& random); - /** * Use this constructor if max_interval need to be enforced. * @param base_interval the base_interval to be used for next backoff computation. @@ -59,11 +30,9 @@ class JitteredBackOffStrategy : public BackOffStrategy { void reset() override; private: - uint64_t computeNextInterval(); - const uint64_t base_interval_; const uint64_t max_interval_{}; - uint32_t current_retry_{}; + uint64_t current_retry_{}; Runtime::RandomGenerator& random_; }; } // namespace Envoy diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 2b04e476adb8a..d3ee78af94f2e 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -12,12 +12,12 @@ namespace Config { GrpcMuxImpl::GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, - MonotonicTimeSource& time_source) + Runtime::RandomGenerator& random, MonotonicTimeSource& time_source) : node_(node), async_client_(std::move(async_client)), service_method_(service_method), - time_source_(time_source) { + random_(random), time_source_(time_source) { retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); - backoff_strategy_ptr_ = std::make_unique( - RETRY_INITIAL_DELAY_MS, RETRY_MAX_DELAY_MS, MULTIPLIER); + backoff_strategy_ptr_ = std::make_unique(RETRY_INITIAL_DELAY_MS, + RETRY_MAX_DELAY_MS, random_); } GrpcMuxImpl::~GrpcMuxImpl() { diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index ca4fdad8922b0..51f8392d2b518 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -26,6 +26,7 @@ class GrpcMuxImpl : public GrpcMux, public: GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, Event::Dispatcher& dispatcher, const Protobuf::MethodDescriptor& service_method, + Runtime::RandomGenerator& random, MonotonicTimeSource& time_source = ProdMonotonicTimeSource::instance_); ~GrpcMuxImpl(); @@ -45,7 +46,6 @@ class GrpcMuxImpl : public GrpcMux, // TODO(htuch): Make this configurable or some static. const uint32_t RETRY_INITIAL_DELAY_MS = 500; const uint32_t RETRY_MAX_DELAY_MS = 30000; // Do not cross more than 30s - const double MULTIPLIER = 2; private: void setRetryTimer(); @@ -103,6 +103,7 @@ class GrpcMuxImpl : public GrpcMux, // Envoy's dependendency ordering. std::list subscriptions_; Event::TimerPtr retry_timer_; + Runtime::RandomGenerator& random_; MonotonicTimeSource& time_source_; BackOffStrategyPtr backoff_strategy_ptr_; }; diff --git a/source/common/config/grpc_subscription_impl.h b/source/common/config/grpc_subscription_impl.h index 4b07f02239900..6117b06b7cd10 100644 --- a/source/common/config/grpc_subscription_impl.h +++ b/source/common/config/grpc_subscription_impl.h @@ -15,9 +15,9 @@ template class GrpcSubscriptionImpl : public Config::Subscription { public: GrpcSubscriptionImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClientPtr async_client, - Event::Dispatcher& dispatcher, + Event::Dispatcher& dispatcher, Runtime::RandomGenerator& random, const Protobuf::MethodDescriptor& service_method, SubscriptionStats stats) - : grpc_mux_(node, std::move(async_client), dispatcher, service_method), + : grpc_mux_(node, std::move(async_client), dispatcher, service_method, random), grpc_mux_subscription_(grpc_mux_, stats) {} // Config::Subscription diff --git a/source/common/config/subscription_factory.h b/source/common/config/subscription_factory.h index f80cc09219f62..2189e37f38ee3 100644 --- a/source/common/config/subscription_factory.h +++ b/source/common/config/subscription_factory.h @@ -68,8 +68,8 @@ class SubscriptionFactory { Config::Utility::factoryForGrpcApiConfigSource(cm.grpcAsyncClientManager(), config.api_config_source(), scope) ->create(), - dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), - stats)); + dispatcher, random, + *Protobuf::DescriptorPool::generated_pool()->FindMethodByName(grpc_method), stats)); break; } default: diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 957597e5d72d4..15acb0518270e 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -71,9 +71,8 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap& // Merge in the route policy. retry_on_ |= route_policy.retryOn(); retries_remaining_ = std::max(retries_remaining_, route_policy.numRetries()); - - backoff_strategy_ptr_ = std::make_unique( - runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25), random_); + uint32_t base = runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25); + backoff_strategy_ptr_ = std::make_unique(base, base * 10, random_); } RetryStateImpl::~RetryStateImpl() { resetRetry(); } diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 2c28735fee4b0..5c80706ed096c 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/source/common/upstream/cluster_manager_impl.cc @@ -214,7 +214,8 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots ->create(), main_thread_dispatcher, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( - "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"))); + "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), + random_)); } else { ads_mux_.reset(new Config::NullGrpcMuxImpl()); } diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc index 3549ba956c905..a1d9f0bfcda90 100644 --- a/test/common/common/backoff_strategy_test.cc +++ b/test/common/common/backoff_strategy_test.cc @@ -9,61 +9,11 @@ using testing::Return; namespace Envoy { -TEST(BackOffStrategyTest, ExponentialBackOffBasicTest) { - ExponentialBackOffStrategy exponential_back_off(10, 100, 2); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(40, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(80, exponential_back_off.nextBackOffMs()); -} - -TEST(BackOffStrategyTest, ExponentialBackOffFractionalMultiplier) { - ExponentialBackOffStrategy exponential_back_off(10, 50, 1.5); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(15, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(23, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(35, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(50, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(50, exponential_back_off.nextBackOffMs()); -} - -TEST(BackOffStrategyTest, ExponentialBackOffMaxIntervalReached) { - ExponentialBackOffStrategy exponential_back_off(10, 100, 2); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(40, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(80, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(100, exponential_back_off.nextBackOffMs()); // Should return Max here - EXPECT_EQ(100, exponential_back_off.nextBackOffMs()); // Should return Max here -} - -TEST(BackOffStrategyTest, ExponentialBackOffReset) { - ExponentialBackOffStrategy exponential_back_off(10, 100, 2); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); - - exponential_back_off.reset(); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); // Should start from start -} - -TEST(BackOffStrategyTest, ExponentialBackOffResetAfterMaxReached) { - ExponentialBackOffStrategy exponential_back_off(10, 100, 2); - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(20, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(40, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(80, exponential_back_off.nextBackOffMs()); - EXPECT_EQ(100, exponential_back_off.nextBackOffMs()); // Should return Max here - - exponential_back_off.reset(); - - EXPECT_EQ(10, exponential_back_off.nextBackOffMs()); // Should start from start -} - TEST(BackOffStrategyTest, JitteredBackOffBasicFlow) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(27)); - JitteredBackOffStrategy jittered_back_off(25, random); + JitteredBackOffStrategy jittered_back_off(25, 30, random); EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); } @@ -72,7 +22,7 @@ TEST(BackOffStrategyTest, JitteredBackOffBasicReset) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(27)); - JitteredBackOffStrategy jittered_back_off(25, random); + JitteredBackOffStrategy jittered_back_off(25, 30, random); EXPECT_EQ(2, jittered_back_off.nextBackOffMs()); EXPECT_EQ(27, jittered_back_off.nextBackOffMs()); @@ -106,4 +56,15 @@ TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); // Should start from start } +TEST(BackOffStrategyTest, JitteredBackOffLongerDuration) { + NiceMock random; + ON_CALL(random, random()).WillByDefault(Return(1024)); + + JitteredBackOffStrategy jittered_back_off(5, 250, random); + + for (size_t i = 0; i < 10000; i++) { + EXPECT_GE(jittered_back_off.nextBackOffMs(), 1); + } +} + } // namespace Envoy diff --git a/test/common/config/BUILD b/test/common/config/BUILD index eff9ccd98328e..555bc63f7ec76 100644 --- a/test/common/config/BUILD +++ b/test/common/config/BUILD @@ -47,6 +47,7 @@ envoy_cc_test( "//test/mocks/config:config_mocks", "//test/mocks/event:event_mocks", "//test/mocks/grpc:grpc_mocks", + "//test/mocks/runtime:runtime_mocks", "//test/test_common:logging_lib", "//test/test_common:utility_lib", "@envoy_api//envoy/api/v2:discovery_cc", diff --git a/test/common/config/grpc_mux_impl_test.cc b/test/common/config/grpc_mux_impl_test.cc index ed1606109df6d..d0a1be52d76e3 100644 --- a/test/common/config/grpc_mux_impl_test.cc +++ b/test/common/config/grpc_mux_impl_test.cc @@ -10,6 +10,7 @@ #include "test/mocks/config/mocks.h" #include "test/mocks/event/mocks.h" #include "test/mocks/grpc/mocks.h" +#include "test/mocks/runtime/mocks.h" #include "test/test_common/logging.h" #include "test/test_common/utility.h" @@ -44,7 +45,7 @@ class GrpcMuxImplTest : public testing::Test { dispatcher_, *Protobuf::DescriptorPool::generated_pool()->FindMethodByName( "envoy.service.discovery.v2.AggregatedDiscoveryService.StreamAggregatedResources"), - time_source_)); + random_, time_source_)); } void expectSendMessage(const std::string& type_url, @@ -72,6 +73,7 @@ class GrpcMuxImplTest : public testing::Test { envoy::api::v2::core::Node node_; NiceMock dispatcher_; + Runtime::MockRandomGenerator random_; Grpc::MockAsyncClient* async_client_; Event::MockTimer* timer_; Event::TimerCb timer_cb_; @@ -112,6 +114,7 @@ TEST_F(GrpcMuxImplTest, ResetStream) { expectSendMessage("baz", {"z"}, ""); grpc_mux_->start(); + EXPECT_CALL(random_, random()); EXPECT_CALL(*timer_, enableTimer(_)); grpc_mux_->onRemoteClose(Grpc::Status::GrpcStatus::Canceled, ""); EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); diff --git a/test/common/config/grpc_subscription_impl_test.cc b/test/common/config/grpc_subscription_impl_test.cc index 546dd5e67d209..fd0974a961f7a 100644 --- a/test/common/config/grpc_subscription_impl_test.cc +++ b/test/common/config/grpc_subscription_impl_test.cc @@ -14,15 +14,20 @@ class GrpcSubscriptionImplTest : public GrpcSubscriptionTestHarness, public test TEST_F(GrpcSubscriptionImplTest, StreamCreationFailure) { InSequence s; EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(nullptr)); + EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); + EXPECT_CALL(random_, random()); EXPECT_CALL(*timer_, enableTimer(_)); subscription_->start({"cluster0", "cluster1"}, callbacks_); + verifyStats(2, 0, 0, 1, 0); // Ensure this doesn't cause an issue by sending a request, since we don't // have a gRPC stream. subscription_->updateResources({"cluster2"}); + // Retry and succeed. EXPECT_CALL(*async_client_, start(_, _)).WillOnce(Return(&async_stream_)); + expectSendMessage({"cluster2"}, ""); timer_cb_(); verifyStats(3, 0, 0, 1, 0); @@ -35,6 +40,7 @@ TEST_F(GrpcSubscriptionImplTest, RemoteStreamClose) { Http::HeaderMapPtr trailers{new Http::TestHeaderMapImpl{}}; subscription_->grpcMux().onReceiveTrailingMetadata(std::move(trailers)); EXPECT_CALL(*timer_, enableTimer(_)); + EXPECT_CALL(random_, random()); subscription_->grpcMux().onRemoteClose(Grpc::Status::GrpcStatus::Canceled, ""); verifyStats(1, 0, 0, 0, 0); // Retry and succeed. diff --git a/test/common/config/grpc_subscription_test_harness.h b/test/common/config/grpc_subscription_test_harness.h index c1fa1694fe5e3..06e22e0ef28a1 100644 --- a/test/common/config/grpc_subscription_test_harness.h +++ b/test/common/config/grpc_subscription_test_harness.h @@ -40,7 +40,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { })); subscription_.reset( new GrpcEdsSubscriptionImpl(node_, std::unique_ptr(async_client_), - dispatcher_, *method_descriptor_, stats_)); + dispatcher_, random_, *method_descriptor_, stats_)); } ~GrpcSubscriptionTestHarness() { EXPECT_CALL(async_stream_, sendMessage(_, false)); } @@ -129,6 +129,7 @@ class GrpcSubscriptionTestHarness : public SubscriptionTestHarness { Grpc::MockAsyncClient* async_client_; NiceMock cm_; Event::MockDispatcher dispatcher_; + Runtime::MockRandomGenerator random_; Event::MockTimer* timer_; Event::TimerCb timer_cb_; envoy::api::v2::core::Node node_; diff --git a/test/common/config/subscription_factory_test.cc b/test/common/config/subscription_factory_test.cc index 40139a53d3e7f..0dc79fc2b32e7 100644 --- a/test/common/config/subscription_factory_test.cc +++ b/test/common/config/subscription_factory_test.cc @@ -280,6 +280,7 @@ TEST_F(SubscriptionFactoryTest, GrpcSubscription) { })); return async_client_factory; })); + EXPECT_CALL(random_, random()); EXPECT_CALL(dispatcher_, createTimer_(_)); EXPECT_CALL(callbacks_, onConfigUpdateFailed(_)); subscriptionFromConfigSource(config)->start({"static_cluster"}, callbacks_); From 660a00ff5870aeafd4f212273bf1086b105c5b85 Mon Sep 17 00:00:00 2001 From: Rama Date: Tue, 10 Jul 2018 21:17:38 +0530 Subject: [PATCH 3/5] added comment Signed-off-by: Rama --- source/common/common/backoff_strategy.cc | 1 + 1 file changed, 1 insertion(+) diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index 23a6af61ae7ce..b9d0fbfbcdd6b 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -11,6 +11,7 @@ JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_ uint64_t JitteredBackOffStrategy::nextBackOffMs() { current_retry_++; uint32_t multiplier = (1 << current_retry_) - 1; + // for retries that take longer, multiplier may overflow and become zero. if (multiplier == 0) { current_retry_ = 1; multiplier = (1 << current_retry_) - 1; From 7abda1557f8b0767ff4b9950a1f43d67f31dc492 Mon Sep 17 00:00:00 2001 From: Rama Date: Tue, 10 Jul 2018 21:24:15 +0530 Subject: [PATCH 4/5] formatted Signed-off-by: Rama --- source/common/common/backoff_strategy.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index b9d0fbfbcdd6b..0aa15c5add47b 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -11,7 +11,7 @@ JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_ uint64_t JitteredBackOffStrategy::nextBackOffMs() { current_retry_++; uint32_t multiplier = (1 << current_retry_) - 1; - // for retries that take longer, multiplier may overflow and become zero. + // for retries that take longer, multiplier may overflow and become zero. if (multiplier == 0) { current_retry_ = 1; multiplier = (1 << current_retry_) - 1; From f07737b5dbb9a75ae3401cbd33db76920f5a13ee Mon Sep 17 00:00:00 2001 From: Rama Date: Wed, 11 Jul 2018 11:28:58 +0530 Subject: [PATCH 5/5] address review comments Signed-off-by: Rama --- source/common/common/backoff_strategy.cc | 14 ++++++------- source/common/common/backoff_strategy.h | 2 +- source/common/config/grpc_mux_impl.cc | 8 +++---- source/common/config/grpc_mux_impl.h | 2 +- source/common/router/retry_state_impl.cc | 7 ++++--- source/common/router/retry_state_impl.h | 2 +- test/common/common/backoff_strategy_test.cc | 23 ++++++--------------- 7 files changed, 23 insertions(+), 35 deletions(-) diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc index 0aa15c5add47b..d8a21be12c801 100644 --- a/source/common/common/backoff_strategy.cc +++ b/source/common/common/backoff_strategy.cc @@ -9,16 +9,14 @@ JitteredBackOffStrategy::JitteredBackOffStrategy(uint64_t base_interval, uint64_ } uint64_t JitteredBackOffStrategy::nextBackOffMs() { - current_retry_++; - uint32_t multiplier = (1 << current_retry_) - 1; - // for retries that take longer, multiplier may overflow and become zero. - if (multiplier == 0) { - current_retry_ = 1; - multiplier = (1 << current_retry_) - 1; + const uint64_t multiplier = (1 << current_retry_) - 1; + const uint64_t base_backoff = multiplier * base_interval_; + if (base_backoff <= max_interval_) { + current_retry_++; } - return std::min(random_.random() % (base_interval_ * multiplier), max_interval_); + return std::min(random_.random() % base_backoff, max_interval_); } -void JitteredBackOffStrategy::reset() { current_retry_ = 0; } +void JitteredBackOffStrategy::reset() { current_retry_ = 1; } } // namespace Envoy \ No newline at end of file diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h index ed116c1797361..ef55cf9060f6c 100644 --- a/source/common/common/backoff_strategy.h +++ b/source/common/common/backoff_strategy.h @@ -32,7 +32,7 @@ class JitteredBackOffStrategy : public BackOffStrategy { private: const uint64_t base_interval_; const uint64_t max_interval_{}; - uint64_t current_retry_{}; + uint64_t current_retry_{1}; Runtime::RandomGenerator& random_; }; } // namespace Envoy diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index d3ee78af94f2e..77b9fc84dc345 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -16,8 +16,8 @@ GrpcMuxImpl::GrpcMuxImpl(const envoy::api::v2::core::Node& node, Grpc::AsyncClie : node_(node), async_client_(std::move(async_client)), service_method_(service_method), random_(random), time_source_(time_source) { retry_timer_ = dispatcher.createTimer([this]() -> void { establishNewStream(); }); - backoff_strategy_ptr_ = std::make_unique(RETRY_INITIAL_DELAY_MS, - RETRY_MAX_DELAY_MS, random_); + backoff_strategy_ = std::make_unique(RETRY_INITIAL_DELAY_MS, + RETRY_MAX_DELAY_MS, random_); } GrpcMuxImpl::~GrpcMuxImpl() { @@ -31,7 +31,7 @@ GrpcMuxImpl::~GrpcMuxImpl() { void GrpcMuxImpl::start() { establishNewStream(); } void GrpcMuxImpl::setRetryTimer() { - retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_ptr_->nextBackOffMs())); + retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); } void GrpcMuxImpl::establishNewStream() { @@ -159,7 +159,7 @@ void GrpcMuxImpl::onReceiveInitialMetadata(Http::HeaderMapPtr&& metadata) { void GrpcMuxImpl::onReceiveMessage(std::unique_ptr&& message) { // Reset here so that it starts with fresh backoff interval on next disconnect. - backoff_strategy_ptr_->reset(); + backoff_strategy_->reset(); const std::string& type_url = message->type_url(); ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url, message->version_info()); diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 51f8392d2b518..5f2c99c2e6168 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -105,7 +105,7 @@ class GrpcMuxImpl : public GrpcMux, Event::TimerPtr retry_timer_; Runtime::RandomGenerator& random_; MonotonicTimeSource& time_source_; - BackOffStrategyPtr backoff_strategy_ptr_; + BackOffStrategyPtr backoff_strategy_; }; class NullGrpcMuxImpl : public GrpcMux { diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index 15acb0518270e..1aba35700e1aa 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -71,8 +71,9 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap& // Merge in the route policy. retry_on_ |= route_policy.retryOn(); retries_remaining_ = std::max(retries_remaining_, route_policy.numRetries()); - uint32_t base = runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25); - backoff_strategy_ptr_ = std::make_unique(base, base * 10, random_); + const uint32_t base = runtime_.snapshot().getInteger("upstream.base_retry_backoff_ms", 25); + // Cap the max interval to 10 times the base interval to ensure reasonable backoff intervals. + backoff_strategy_ = std::make_unique(base, base * 10, random_); } RetryStateImpl::~RetryStateImpl() { resetRetry(); } @@ -83,7 +84,7 @@ void RetryStateImpl::enableBackoffTimer() { } // We use a fully jittered exponential backoff algorithm. - retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_ptr_->nextBackOffMs())); + retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_->nextBackOffMs())); } uint32_t RetryStateImpl::parseRetryOn(absl::string_view config) { diff --git a/source/common/router/retry_state_impl.h b/source/common/router/retry_state_impl.h index 366b0d8a3bd40..71d7bf38cb5a4 100644 --- a/source/common/router/retry_state_impl.h +++ b/source/common/router/retry_state_impl.h @@ -60,7 +60,7 @@ class RetryStateImpl : public RetryState { DoRetryCallback callback_; Event::TimerPtr retry_timer_; Upstream::ResourcePriority priority_; - BackOffStrategyPtr backoff_strategy_ptr_; + BackOffStrategyPtr backoff_strategy_; }; } // namespace Router diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc index a1d9f0bfcda90..010768349983d 100644 --- a/test/common/common/backoff_strategy_test.cc +++ b/test/common/common/backoff_strategy_test.cc @@ -34,37 +34,26 @@ TEST(BackOffStrategyTest, JitteredBackOffWithMaxInterval) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(1024)); - JitteredBackOffStrategy jittered_back_off(5, 15, random); + JitteredBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); // Should return Max here - EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(49, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(94, jittered_back_off.nextBackOffMs()); + EXPECT_EQ(94, jittered_back_off.nextBackOffMs()); // Should return Max here } TEST(BackOffStrategyTest, JitteredBackOffWithMaxIntervalReset) { NiceMock random; ON_CALL(random, random()).WillByDefault(Return(1024)); - JitteredBackOffStrategy jittered_back_off(5, 15, random); + JitteredBackOffStrategy jittered_back_off(5, 100, random); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); EXPECT_EQ(9, jittered_back_off.nextBackOffMs()); - EXPECT_EQ(15, jittered_back_off.nextBackOffMs()); // Should return Max here + EXPECT_EQ(49, jittered_back_off.nextBackOffMs()); jittered_back_off.reset(); EXPECT_EQ(4, jittered_back_off.nextBackOffMs()); // Should start from start } - -TEST(BackOffStrategyTest, JitteredBackOffLongerDuration) { - NiceMock random; - ON_CALL(random, random()).WillByDefault(Return(1024)); - - JitteredBackOffStrategy jittered_back_off(5, 250, random); - - for (size_t i = 0; i < 10000; i++) { - EXPECT_GE(jittered_back_off.nextBackOffMs(), 1); - } -} - } // namespace Envoy