From 07b13c0bdc17d435d238db9fb7b44ff7b9b84584 Mon Sep 17 00:00:00 2001 From: Rama Chavali Date: Wed, 4 Jul 2018 09:04:30 +0530 Subject: [PATCH] config: backoff strategy implementation (#3758) Implements an ExponentialBackOffStrategy that is used when Envoy retries management server connection. Risk Level: Low Testing: Added automated tests Docs Changes: N/A Release Notes: N/A Fixes #3737 Signed-off-by: Rama --- include/envoy/common/BUILD | 5 ++ include/envoy/common/backoff_strategy.h | 25 +++++++++ source/common/common/BUILD | 10 ++++ source/common/common/backoff_strategy.cc | 30 +++++++++++ source/common/common/backoff_strategy.h | 33 ++++++++++++ source/common/config/BUILD | 1 + source/common/config/grpc_mux_impl.cc | 7 ++- source/common/config/grpc_mux_impl.h | 6 ++- source/common/router/retry_state_impl.cc | 1 + test/common/common/BUILD | 8 +++ test/common/common/backoff_strategy_test.cc | 57 +++++++++++++++++++++ 11 files changed, 181 insertions(+), 2 deletions(-) create mode 100644 include/envoy/common/backoff_strategy.h create mode 100644 source/common/common/backoff_strategy.cc create mode 100644 source/common/common/backoff_strategy.h create mode 100644 test/common/common/backoff_strategy_test.cc diff --git a/include/envoy/common/BUILD b/include/envoy/common/BUILD index dee9f10b4479..507c15d284fe 100644 --- a/include/envoy/common/BUILD +++ b/include/envoy/common/BUILD @@ -37,3 +37,8 @@ envoy_cc_library( name = "callback", hdrs = ["callback.h"], ) + +envoy_cc_library( + name = "backoff_strategy_interface", + hdrs = ["backoff_strategy.h"], +) diff --git a/include/envoy/common/backoff_strategy.h b/include/envoy/common/backoff_strategy.h new file mode 100644 index 000000000000..63114e047fc2 --- /dev/null +++ b/include/envoy/common/backoff_strategy.h @@ -0,0 +1,25 @@ +#pragma once + +#include "envoy/common/pure.h" + +namespace Envoy { +/** + * Generic interface for all backoff strategy implementations. + */ +class BackOffStrategy { +public: + virtual ~BackOffStrategy() {} + + /** + * @return the next backoff interval in milli seconds. + */ + virtual uint64_t nextBackOffMs() PURE; + + /** + * Resets the intervals so that the back off intervals can start again. + */ + virtual void reset() PURE; +}; + +typedef std::unique_ptr BackOffStrategyPtr; +} // namespace Envoy \ No newline at end of file diff --git a/source/common/common/BUILD b/source/common/common/BUILD index 9d070f59939b..59d8827f8f6e 100644 --- a/source/common/common/BUILD +++ b/source/common/common/BUILD @@ -16,6 +16,16 @@ envoy_cc_library( deps = [":minimal_logger_lib"], ) +envoy_cc_library( + name = "backoff_lib", + srcs = ["backoff_strategy.cc"], + hdrs = ["backoff_strategy.h"], + deps = [ + ":assert_lib", + "//include/envoy/common:backoff_strategy_interface", + ], +) + envoy_cc_library( name = "base64_lib", srcs = ["base64.cc"], diff --git a/source/common/common/backoff_strategy.cc b/source/common/common/backoff_strategy.cc new file mode 100644 index 000000000000..8e345f17ac4e --- /dev/null +++ b/source/common/common/backoff_strategy.cc @@ -0,0 +1,30 @@ +#include "common/common/backoff_strategy.h" + +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_; +} +} // namespace Envoy \ No newline at end of file diff --git a/source/common/common/backoff_strategy.h b/source/common/common/backoff_strategy.h new file mode 100644 index 000000000000..01c81d95d722 --- /dev/null +++ b/source/common/common/backoff_strategy.h @@ -0,0 +1,33 @@ +#pragma once + +#include +#include + +#include "envoy/common/backoff_strategy.h" + +#include "common/common/assert.h" + +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_; +}; +} // namespace Envoy diff --git a/source/common/config/BUILD b/source/common/config/BUILD index 83067aa8806e..9c362e71e58d 100644 --- a/source/common/config/BUILD +++ b/source/common/config/BUILD @@ -142,6 +142,7 @@ envoy_cc_library( "//include/envoy/config:subscription_interface", "//include/envoy/grpc:async_client_interface", "//include/envoy/upstream:cluster_manager_interface", + "//source/common/common:backoff_lib", "//source/common/common:minimal_logger_lib", "//source/common/common:token_bucket_impl_lib", "//source/common/protobuf", diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index 0334ee09c6d3..2b04e476adb8 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/source/common/config/grpc_mux_impl.cc @@ -16,6 +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), 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); } GrpcMuxImpl::~GrpcMuxImpl() { @@ -29,7 +31,7 @@ GrpcMuxImpl::~GrpcMuxImpl() { void GrpcMuxImpl::start() { establishNewStream(); } void GrpcMuxImpl::setRetryTimer() { - retry_timer_->enableTimer(std::chrono::milliseconds(RETRY_DELAY_MS)); + retry_timer_->enableTimer(std::chrono::milliseconds(backoff_strategy_ptr_->nextBackOffMs())); } void GrpcMuxImpl::establishNewStream() { @@ -156,6 +158,9 @@ 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(); + const std::string& type_url = message->type_url(); ENVOY_LOG(debug, "Received gRPC message for {} at version {}", type_url, message->version_info()); if (api_state_.count(type_url) == 0) { diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 0eb1bfb90b15..ca4fdad8922b 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/source/common/config/grpc_mux_impl.h @@ -11,6 +11,7 @@ #include "envoy/grpc/status.h" #include "envoy/upstream/cluster_manager.h" +#include "common/common/backoff_strategy.h" #include "common/common/logger.h" namespace Envoy { @@ -42,7 +43,9 @@ class GrpcMuxImpl : public GrpcMux, void onRemoteClose(Grpc::Status::GrpcStatus status, const std::string& message) override; // TODO(htuch): Make this configurable or some static. - const uint32_t RETRY_DELAY_MS = 5000; + 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(); @@ -101,6 +104,7 @@ class GrpcMuxImpl : public GrpcMux, std::list subscriptions_; Event::TimerPtr retry_timer_; MonotonicTimeSource& time_source_; + BackOffStrategyPtr backoff_strategy_ptr_; }; class NullGrpcMuxImpl : public GrpcMux { diff --git a/source/common/router/retry_state_impl.cc b/source/common/router/retry_state_impl.cc index e925da9193f8..dccd5b123940 100644 --- a/source/common/router/retry_state_impl.cc +++ b/source/common/router/retry_state_impl.cc @@ -76,6 +76,7 @@ RetryStateImpl::RetryStateImpl(const RetryPolicy& route_policy, Http::HeaderMap& 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; diff --git a/test/common/common/BUILD b/test/common/common/BUILD index 82b9818292ee..a2705917f19c 100644 --- a/test/common/common/BUILD +++ b/test/common/common/BUILD @@ -10,6 +10,14 @@ load( envoy_package() +envoy_cc_test( + name = "backoff_strategy_test", + srcs = ["backoff_strategy_test.cc"], + deps = [ + "//source/common/common:backoff_lib", + ], +) + envoy_cc_test( name = "base64_test", srcs = ["base64_test.cc"], diff --git a/test/common/common/backoff_strategy_test.cc b/test/common/common/backoff_strategy_test.cc new file mode 100644 index 000000000000..88576b979f08 --- /dev/null +++ b/test/common/common/backoff_strategy_test.cc @@ -0,0 +1,57 @@ +#include "common/common/backoff_strategy.h" + +#include "gtest/gtest.h" + +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, ExponentialBackOfReset) { + 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, ExponentialBackOfResetAfterMaxReached) { + 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 +} + +} // namespace Envoy