Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions include/envoy/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,8 @@ envoy_cc_library(
name = "callback",
hdrs = ["callback.h"],
)

envoy_cc_library(
name = "backoff_strategy_interface",
hdrs = ["backoff_strategy.h"],
)
25 changes: 25 additions & 0 deletions include/envoy/common/backoff_strategy.h
Original file line number Diff line number Diff line change
@@ -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<BackOffStrategy> BackOffStrategyPtr;
} // namespace Envoy
10 changes: 10 additions & 0 deletions source/common/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
30 changes: 30 additions & 0 deletions source/common/common/backoff_strategy.cc
Original file line number Diff line number Diff line change
@@ -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
33 changes: 33 additions & 0 deletions source/common/common/backoff_strategy.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#pragma once

#include <cstdint>
#include <memory>

#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
1 change: 1 addition & 0 deletions source/common/config/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
7 changes: 6 additions & 1 deletion source/common/config/grpc_mux_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<ExponentialBackOffStrategy>(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this affect any existing tests? If not, how come?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It did not actually.

RETRY_INITIAL_DELAY_MS, RETRY_MAX_DELAY_MS, MULTIPLIER);
}

GrpcMuxImpl::~GrpcMuxImpl() {
Expand All @@ -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() {
Expand Down Expand Up @@ -156,6 +158,9 @@ void GrpcMuxImpl::onReceiveInitialMetadata(Http::HeaderMapPtr&& metadata) {
}

void GrpcMuxImpl::onReceiveMessage(std::unique_ptr<envoy::api::v2::DiscoveryResponse>&& 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) {
Expand Down
6 changes: 5 additions & 1 deletion source/common/config/grpc_mux_impl.h
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -101,6 +104,7 @@ class GrpcMuxImpl : public GrpcMux,
std::list<std::string> subscriptions_;
Event::TimerPtr retry_timer_;
MonotonicTimeSource& time_source_;
BackOffStrategyPtr backoff_strategy_ptr_;
};

class NullGrpcMuxImpl : public GrpcMux {
Expand Down
1 change: 1 addition & 0 deletions source/common/router/retry_state_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see, you have thought about jitter here..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, and, honestly, you should be using jittered backoff for the management connection also. I would not support any backoff policy that is not jittered...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mattklein123 i am ok with that. Let us review the PR for jittered backoff #3791 and I can move management connection to use this. @htuch thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think have jitter as default (and only built-in) scheme makes sense, best not to provide sharp objects that aren't needed.

// We use a fully jittered exponential backoff algorithm.
current_retry_++;
uint32_t multiplier = (1 << current_retry_) - 1;
Expand Down
8 changes: 8 additions & 0 deletions test/common/common/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -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"],
Expand Down
57 changes: 57 additions & 0 deletions test/common/common/backoff_strategy_test.cc
Original file line number Diff line number Diff line change
@@ -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