upstream: gradually decrease outlier detector's ejection time multiplier when host stays healthy by cpakulski · Pull Request #14235 · envoyproxy/envoy

cpakulski · 2020-12-01T20:49:45Z

Commit Message:
gradually decrease outlier detector's ejection time multiplier when node stays healthy

Additional Description:
Each time a host is ejected by outlier detector, the ejection time is increased. That increased ejection time stays with the host and is never decreased. This PR adds logic to gradually decrease the ejection time when node stays healthy.

Risk Level: Low
Testing: Added unit test.
Docs Changes: Updated outlier detector section.
Release Notes: No
Platform Specific Features:
Fixes #6016

when a node stays healthy. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-12-02T03:32:28Z

/retest

repokitteh-read-only · 2020-12-02T03:32:33Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14235 (comment) was created by @cpakulski.

see: more, trace.

snowp

Thanks for working on this, a few comments to get you started. I imagine we want this to be guarded by a configuration flag as per the linked issue?

snowp · 2020-12-02T19:24:48Z

-   for longer and longer periods if they continue to fail.
+   multiplied by the number of times the host has been ejected in a row. This causes hosts to get ejected
+   for longer and longer periods if they continue to fail. When the host becomes healthy, the ejection time 
+   multiplier decreases with time. Eventually, if the host stays healthy for long time, 


how long? could need some more detail on how the multiplier decreases

Added details to documentation.

snowp · 2020-12-02T19:26:04Z

  ASSERT(!host_.lock()->healthFlagGet(Host::HealthFlag::FAILED_OUTLIER_CHECK));
  host_.lock()->healthFlagSet(Host::HealthFlag::FAILED_OUTLIER_CHECK);
  num_ejections_++;
+  eject_time_backoff_++;


Should this be capped at some max value? During prolonged outages this would likely get really high right?

Correct. Thanks for pointing this out. I added max_ejection_time config to cap it.

snowp · 2020-12-02T19:27:06Z


+// Test verifies that ejection time increases each time the node is ejected,
+// and decreases when node stays healthy.
+#define EJECT_PERIOD_TICK(x) x * 10000


Not sure what this adds, but in general prefer functions over macros for this kinda stuff

It is just a helper. Instead of moving time using 10000, 20000, 30000, ..... 300000, this allows to use 1, 2,3, ... 30.
Converted it to constexpr function.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only · 2020-12-08T22:00:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14235 was synchronize by cpakulski.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123

Thanks API LGTM with small comment.

/wait

mattklein123 · 2020-12-09T19:25:50Z

 // See the :ref:`architecture overview <arch_overview_outlier_detection>` for
 // more information on outlier detection.
-// [#next-free-field: 21]
+// [#next-free-field: 22]


This needs a release note. Technically this is a breaking behavior change but I agree it's for the best, but we should call it out carefully in the release note.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 · 2020-12-11T17:26:20Z

Needs main merge, thanks.

/wait

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123 · 2020-12-11T18:57:45Z

 * kill_request: enable a way to configure kill header name in KillRequest proto.
 * memory: enable new tcmalloc with restartable sequences for aarch64 builds.
 * mongo proxy metrics: swapped network connection remote and local closed counters previously set reversed (`cx_destroy_local_with_active_rq` and `cx_destroy_remote_with_active_rq`).
+* outlier detection: added :ref:`max_ejection_time <envoy_v3_api_field_config.cluster.v3.OutlierDetection.max_ejection_time>` to limit ejection time growth when node stays unhealthy for extended period of time.


Can you make it more clear that the old behavior was unlimited growth and the new default is 5 minutes, etc.? Thank you.

/wait

Expanded the release note.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

mattklein123

Thanks!

In tests, used chrono::seconds to move simulated time forward. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-12-15T22:45:57Z

/retest

repokitteh-read-only · 2020-12-15T22:46:02Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14235 (comment) was created by @cpakulski.

see: more, trace.

snowp

Thanks this is close to ready, just one comment

snowp · 2020-12-16T00:13:02Z

 };

+// Names used in runtime configuration.
+const std::string max_ejection_percent_runtime = "outlier_detection.max_ejection_percent";


I would use constexpr absl::string_view here and make all the names use ProperCase to reflect that they're string constants

Done. Thanks.

…iew. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2020-12-16T20:16:01Z

/retest

repokitteh-read-only · 2020-12-16T20:16:06Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14235 (comment) was created by @cpakulski.

see: more, trace.

snowp

Thanks!

hzxuzhonghu · 2021-02-03T02:10:17Z

max_ejection_time with default 30s may break compatibility

FYI istio/istio#30181

ramaraochavali · 2021-02-04T10:59:19Z

@cpakulski Why is the default value enforced for "max_ejection_time" instead of retaining the old behaviour if "max_ejection_time" is not provided. I understand the intent of adding the max_ejection_time and it is a good change. However it breaks people (see istio/istio#30181) if base_ejection_time is set above 5m (the default value of max_ejection_time)

How about

Enforcing max_ejection_time only when it is explicitly provided or base_ejection_time < 5m (the default max_ejection_time if not provided) otherwise leave the current behaviour as is.

WDYT?
cc: @mattklein123 for your thoughts as well?

cpakulski · 2021-02-04T16:09:59Z

@ramaraochavali What is exactly the problem? Are the nodes ejected prematurely?

ramaraochavali · 2021-02-04T16:15:49Z

No. the problem is config is rejected by Envoy if there is an existing outlier detection setting with base_ejection_time > 5 m. Istio consumers can create such configuration and when they upgrade their config will get rejected.

cpakulski · 2021-02-04T16:28:36Z

Let me check, but it should not be rejected. The max ejection time should be capped at base_ejection_time and never increase (if base_ejection_time > max_ejection_time) or max_ejection_time is not specified.

ramaraochavali · 2021-02-05T03:49:02Z

@cpakulski More details, This is the error we are getting "outlier detector's max_ejection_time cannot be smaller than base_ejection_time" when it is rejected.

This validation is failing

envoy/source/common/upstream/outlier_detection_impl.cc

Line 286 in de8c505

    
           if (detector->config().maxEjectionTimeMs() < detector->config().baseEjectionTimeMs()) {

I think we should skip that validation if max_ejection_time is not set.
Since you get defaulting it here

envoy/source/common/upstream/outlier_detection_impl.cc

Line 254 in de8c505

    
           PROTOBUF_GET_MS_OR_DEFAULT(config, max_ejection_time, DEFAULT_MAX_EJECTION_TIME_MS))) {}

by the time it gets there it will always have value. Probably you should capture if it is set or not in a boolean and skip this

cpakulski · 2021-02-05T14:32:34Z

I am working on the fix and would like to keep the logic fairly clean. First I will add unit test which fails and then correct it.

cpakulski · 2021-02-05T17:38:56Z

The issue is fixed in #14962.

ramaraochavali · 2021-02-06T04:43:16Z

@cpakulski Thank you so much.

Gradually decrease outlier detector's ejection time multiplier

7a4c0ef

when a node stays healthy. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski marked this pull request as draft December 1, 2020 22:20

cpakulski marked this pull request as ready for review December 2, 2020 04:07

cpakulski changed the title ~~upstream: gradually decrease outlier detector's ejection time multiplier~~ upstream: gradually decrease outlier detector's ejection time multiplier when host stays healthy Dec 2, 2020

jmarantz assigned snowp Dec 2, 2020

snowp suggested changes Dec 2, 2020

View reviewed changes

snowp added the waiting label Dec 3, 2020

Introduced max_ejection_time to cap ejection time.

db064db

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot added api and removed waiting labels Dec 8, 2020

repokitteh-read-only Bot assigned mattklein123 Dec 8, 2020

Corrected spelling in docs.

a3fc752

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from snowp December 9, 2020 02:27

mattklein123 requested changes Dec 9, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Dec 9, 2020

Added release note.

737e3d2

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 11, 2020

repokitteh-read-only Bot added the waiting label Dec 11, 2020

Merge branch 'master' into issue/6016

293940b

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 11, 2020

mattklein123 reviewed Dec 11, 2020

View reviewed changes

repokitteh-read-only Bot added the waiting label Dec 11, 2020

Expanded release note.

9a4f078

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 11, 2020

mattklein123 previously approved these changes Dec 11, 2020

View reviewed changes

repokitteh-read-only Bot removed the api label Dec 11, 2020

snowp added the waiting label Dec 14, 2020

Replaced runtime config values with const strings.

f37d0d7

In tests, used chrono::seconds to move simulated time forward. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski dismissed mattklein123’s stale review via f37d0d7 December 15, 2020 17:51

repokitteh-read-only Bot added api and removed waiting labels Dec 15, 2020

snowp suggested changes Dec 16, 2020

View reviewed changes

snowp added the waiting label Dec 16, 2020

Converted outlier detector's runtime config strings to absl::string_v…

242fdc3

…iew. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

repokitteh-read-only Bot removed the waiting label Dec 16, 2020

cpakulski requested a review from snowp December 16, 2020 21:31

snowp approved these changes Dec 16, 2020

View reviewed changes

snowp merged commit 8b1569a into envoyproxy:master Dec 16, 2020

cpakulski deleted the issue/6016 branch December 16, 2020 22:41

ramaraochavali mentioned this pull request Feb 4, 2021

Envoy outlier detection max_ejection_time breaks backward compatibility istio/istio#30649

Closed

cpakulski mentioned this pull request Feb 5, 2021

outlier_detector: accept large base_ejection_time when max_ejection_time not specified #14962

Merged

cpakulski mentioned this pull request Feb 23, 2021

backport to 1.17: outlier_detector: accept large base_ejection_time when max_ejection_time not specified (#14962) #15155

Merged

lobkovilya mentioned this pull request May 4, 2022

OutlierDetection decrements ejectTimeBackoff too quickly #21142

Open

Conversation

cpakulski commented Dec 1, 2020

Uh oh!

cpakulski commented Dec 2, 2020

Uh oh!

repokitteh-read-only Bot commented Dec 2, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only Bot commented Dec 8, 2020

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Dec 11, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Dec 15, 2020

Uh oh!

repokitteh-read-only Bot commented Dec 15, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Dec 16, 2020

Uh oh!

repokitteh-read-only Bot commented Dec 16, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

hzxuzhonghu commented Feb 3, 2021

Uh oh!

ramaraochavali commented Feb 4, 2021

Uh oh!

cpakulski commented Feb 4, 2021

Uh oh!

ramaraochavali commented Feb 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpakulski commented Feb 4, 2021

Uh oh!

ramaraochavali commented Feb 5, 2021

Uh oh!

cpakulski commented Feb 5, 2021

Uh oh!

cpakulski commented Feb 5, 2021

Uh oh!

ramaraochavali commented Feb 6, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

ramaraochavali commented Feb 4, 2021 •

edited

Loading