config: backoff strategy implementation #3758

ramaraochavali · 2018-06-29T05:56:01Z

Signed-off-by: Rama rama.rao@salesforce.com
Description:
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 <rama.rao@salesforce.com>

ramaraochavali · 2018-06-29T07:56:19Z

@htuch @mattklein123 can you PTAL?

htuch

Thanks @ramaraochavali for adding this!

htuch · 2018-06-29T19:11:56Z

source/common/common/backoff_strategy.cc

+  return current_interval_;
+}
+
+uint64_t ExponentialBackOffStrategy::multiplyInterval() {


Does this need to be a standalone method? Seems simple enough to inline. OTOH, if you want to later make this more sophisticated (see comment below), it should be standalone (just not named multiple..).

I will inline it for now and refactor it based on how random exponential is implemented in my follow-up PR

htuch · 2018-06-29T19:13:13Z

source/common/common/backoff_strategy.cc

+uint64_t ExponentialBackOffStrategy::multiplyInterval() {
+  uint64_t temp_interval = current_interval_;
+  temp_interval *= multiplier_;
+  return (temp_interval > max_interval_ ? max_interval_ : temp_interval);


Do we want any randomness in the backoff to desynchronize retries?

I will follow-up with a PR to introduce randomness based backoff. Then I will see if new class makes sense or refactor this. Till that time I will keep this as is.

htuch · 2018-06-29T19:14:01Z

source/common/common/backoff_strategy.cc

+                                                       const std::uint32_t multiplier)
+    : initial_interval_(initial_interval), max_interval_(max_interval), multiplier_(multiplier),
+      current_interval_(0) {
+  ASSERT(!(multiplier_ <= 0));


Should multiplier be a double? I.e. do we want to force the minimum backoff to be 2x?

We could change it to double. If we change it to double, I think minimum backoff can be 1.5 so I will change this like that. Is that fine?

htuch · 2018-06-29T19:14:56Z

source/common/common/backoff_strategy.h

+/**
+ * Generic interface for all backoff strategy implementations.
+ */
+class BackOffStrategy {


This can probably live in include/envoy since it is a pure interface.

htuch · 2018-06-29T19:15:10Z

source/common/common/backoff_strategy.h

+  /**
+   * Returns the next backoff interval.
+   */
+  virtual std::uint64_t nextBackOff() PURE;


Just uint64_t rather than std::uint64_t.

htuch · 2018-06-29T19:15:40Z

source/common/common/backoff_strategy.cc

+namespace Envoy {
+
+ExponentialBackOffStrategy::ExponentialBackOffStrategy(const std::uint64_t initial_interval,
+                                                       const std::uint64_t max_interval,


I think it would be good to remove the std:: prefix on all the numeric types in this PR.

htuch · 2018-06-29T19:18:24Z

source/common/config/grpc_mux_impl.h

@@ -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 = 5000;


Now that we have sensible backoff, I think you should shrink this down to something very small for the initial retry, e.g. 500ms. This will fix the problem that Istio are seeing (@costinm) in some cases where Pilot isn't ready. The exponential backoff will take us to a large number pretty quickly.

Yes. I will change

htuch · 2018-06-29T19:19:47Z

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.


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

+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...

@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?

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

htuch · 2018-06-29T19:20:30Z

source/common/config/grpc_mux_impl.h

@@ -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 = 5000;
+  const uint32_t RETRY_MAX_DELAY_MS = 120000; // Do not cross more than 2 mins


Seems too large IMHO for default, 2 minutes is a long time.. How about 30s?

I am fine. Just put some number so that we can discuss

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-06-30T05:50:55Z

@htuch addressed all the comments. PTAL.

ramaraochavali · 2018-07-03T03:41:53Z

@htuch when you get chance can you PTAL?

htuch

Looks good, thanks, a few more comments/questions.

htuch · 2018-07-03T16:00:38Z

source/common/common/backoff_strategy.cc

+                                                       const double multiplier)
+    : initial_interval_(initial_interval), max_interval_(max_interval), multiplier_(multiplier),
+      current_interval_(0) {
+  ASSERT(multiplier_ >= 1.5);


do you want it to be more than 2? I thought people can use 1.5 as multiplier as well. that is why I modified this.

I would suggest >= 1.0; 1.125 would be a valid value, for example.

ok. I think we discussed earlier that should be more than 1.0. So would validate against >1.0

htuch · 2018-07-03T16:01:04Z

source/common/common/backoff_strategy.cc

+
+namespace Envoy {
+
+ExponentialBackOffStrategy::ExponentialBackOffStrategy(const uint64_t initial_interval,


We generally don't make scalar args consts in Envoy (although it's not a terrible idea)..

htuch · 2018-07-03T16:02:50Z

test/common/common/backoff_strategy_test.cc

+}
+
+TEST(BackOffStrategyTest, ExponentialBackOfReset) {
+  ExponentialBackOffStrategy exponential_back_off(10, 100, 2);


Can you do some tests with a fractional backoff multiplier?

htuch · 2018-07-03T16:03:27Z

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<ExponentialBackOffStrategy>(


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

It did not actually.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-07-03T17:01:12Z

@htuch addressed the changes. PTAL..

htuch

Looks good modulo final comments.

htuch · 2018-07-03T18:20:53Z

source/common/common/backoff_strategy.h

+
+  uint64_t initial_interval_;
+  uint64_t max_interval_;
+  double multiplier_;


Nit: a bunch of these should be const as they are only set in the constructor.

htuch · 2018-07-03T18:21:32Z

include/envoy/common/backoff_strategy.h

+  virtual ~BackOffStrategy() {}
+
+  /**
+   * Returns the next backoff interval.


Please use Doxygen @return here. Also, please express the units.

htuch · 2018-07-03T18:21:51Z

include/envoy/common/backoff_strategy.h

+  /**
+   * Returns the next backoff interval.
+   */
+  virtual uint64_t nextBackOff() PURE;


Suggest renaming to nextBackoffMilliseconds or nextBackoffMs.

Signed-off-by: Rama <rama.rao@salesforce.com>

ramaraochavali · 2018-07-04T00:17:17Z

@htuch addressed. PTAL.

htuch

Rad, thanks!

ramaraochavali added 2 commits June 29, 2018 11:21

backoff strategy implementation

93007d2

Signed-off-by: Rama <rama.rao@salesforce.com>

formatted

d31d613

Signed-off-by: Rama <rama.rao@salesforce.com>

htuch self-assigned this Jun 29, 2018

htuch suggested changes Jun 29, 2018

View reviewed changes

ramaraochavali added 2 commits June 30, 2018 09:30

addressed review comments

9ebfd2a

Signed-off-by: Rama <rama.rao@salesforce.com>

inline multiply method

3b8ba1a

Signed-off-by: Rama <rama.rao@salesforce.com>

htuch suggested changes Jul 3, 2018

View reviewed changes

addressed comments

f2919b4

Signed-off-by: Rama <rama.rao@salesforce.com>

htuch suggested changes Jul 3, 2018

View reviewed changes

ramaraochavali added 2 commits July 4, 2018 05:42

minor comments

f8319f2

Signed-off-by: Rama <rama.rao@salesforce.com>

compilation issue

6f2d3e9

Signed-off-by: Rama <rama.rao@salesforce.com>

htuch approved these changes Jul 4, 2018

View reviewed changes

htuch merged commit 07b13c0 into envoyproxy:master Jul 4, 2018

ramaraochavali deleted the feature/ads_backoff branch July 4, 2018 04:12

ramaraochavali mentioned this pull request Jul 4, 2018

common: jittered backoff implementation #3791

Merged


		namespace Envoy {

		ExponentialBackOffStrategy::ExponentialBackOffStrategy(const uint64_t initial_interval,

config: backoff strategy implementation #3758

config: backoff strategy implementation #3758

Conversation

ramaraochavali commented Jun 29, 2018

ramaraochavali commented Jun 29, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Jun 30, 2018

ramaraochavali commented Jul 3, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Jul 3, 2018

htuch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ramaraochavali commented Jul 4, 2018

htuch left a comment

Choose a reason for hiding this comment