outlier_detector: accept large base_ejection_time when max_ejection_time not specified by cpakulski · Pull Request #14962 · envoyproxy/envoy

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

Commit Message:
accept large base_ejection_time when max_ejection_time not specified

Additional Description:
PR #14235 broke compatibility with istio. Istio sets large base_ejection_time and configs where max_jection_time was not specified were rejected. That scenario happened during upgrade from istio 1.8 to 1.9. See #14235 (comment).

This PR changes the logic to following:

if both base_ejection_time and max_ejection_time are specified in the config, they are compared and config is rejected when max_ejection_time is smaller than base_ejection_time.
if max_ejection_time is missing in the config, the default value of 300s is applied. If base_ejection_time is larger than 300s, then max_ejection_time is initialized to base_ejection_time.

Risk Level: Low
Testing: Added unit test to simulate receiving config without max_ejection_time and with large base_ejection_time.
Docs Changes: No
Release Notes: No
Platform Specific Features:

…ecified and base_ejection_time is larger than max_ejection_time's default. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

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

@lizan Can we backport this to 1.17 once it is merged?

antoniovicente

Drive by review as I look at the status of PRs pending review.

antoniovicente · 2021-02-09T02:00:58Z

+    // max_ejection_time was not specified in the config. Apply the default or base_ejection_time
+    // whatever is larger.
+    max_ejection_time_ms_ = DEFAULT_MAX_EJECTION_TIME_MS;
+    max_ejection_time_ms_ = std::max(max_ejection_time_ms_, base_ejection_time_ms_);


max_ejection_time_ms_ = std::max(DEFAULT_MAX_EJECTION_TIME_MS, base_ejection_time_ms_);

antoniovicente · 2021-02-09T02:02:14Z

          PROTOBUF_GET_WRAPPED_OR_DEFAULT(config, enforcing_local_origin_success_rate,
-                                          DEFAULT_ENFORCING_LOCAL_ORIGIN_SUCCESS_RATE))),
-      max_ejection_time_ms_(static_cast<uint64_t>(
-          PROTOBUF_GET_MS_OR_DEFAULT(config, max_ejection_time, DEFAULT_MAX_EJECTION_TIME_MS))) {}


Alternate implementation:

PROTOBUF_GET_MS_OR_DEFAULT( config, max_ejection_time, std::max(DEFAULT_MAX_EJECTION_TIME_MS, base_ejection_time_ms_))

That is good idea. Implemented like that but the change required conversion of DEFAULT_MAX_EJECTION_TIME_MS from const to constexpr.

antoniovicente · 2021-02-09T02:09:30Z

-      max_ejection_time_ms_(static_cast<uint64_t>(
-          PROTOBUF_GET_MS_OR_DEFAULT(config, max_ejection_time, DEFAULT_MAX_EJECTION_TIME_MS))) {}
+                                          DEFAULT_ENFORCING_LOCAL_ORIGIN_SUCCESS_RATE))) {
+  // Check if max_ejection_time is specified.


cc @envoyproxy/api-shepherds

Please update API comments since default behavior after this change is more complex than currently documented. There were some comments in the original PR about the change being technically breaking behavior; was the bad interaction with configured base ejection time the concern or something else? https://github.com/envoyproxy/envoy/pull/14235/files#r539583484

Updated the description of the parameter in proto file. Normally outlier detector checks if max_ejection is larger than base_ejection time. If max_ is smaller, the config is rejected. The corner case was when max_ejection was not specified (legacy config was used from before the max_ejection was introduced) and base_ejection time was larger than max_ejection default. The config was rejected and broke upgrade path in istio.

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

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

repokitteh-read-only · 2021-02-09T16:39:40Z

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

🐱

Caused by: #14962 was synchronize by cpakulski.

see: more, trace.

cpakulski · 2021-02-09T19:12:43Z

/retest

repokitteh-read-only · 2021-02-09T19:12:47Z

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

🐱

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

see: more, trace.

cpakulski · 2021-02-16T19:10:46Z

@markdroth Would you mind approving api? Syntax was not changed - just updated the comment. Thanks!

snowp

Seems good to me, just one nit.

@envoyproxy/api-shepherds ptal

snowp · 2021-02-17T17:25:55Z

+  // for more information. If not specified, the default value or
+  // :ref:`base_ejection_time<envoy_api_field_config.cluster.v3.OutlierDetection.base_ejection_time>` value is applied, whatever is larger.
  // Defaults to 300000ms or 300s.


I would inline the default here instead of referring to the default on L154 and defining it on L156

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

lizan

/lgtm api

lizan · 2021-02-17T18:55:48Z

+  // for more information. If not specified, the default value or
+  // :ref:`base_ejection_time<envoy_api_field_config.cluster.v3.OutlierDetection.base_ejection_time>` value is applied, whatever is larger.
  // Defaults to 300000ms or 300s.


snowp

Thanks!

outlier_detector: accept large base_ejection_time when max_ejection_time not specified (envoyproxy#14962) Changed logic in config verification when max_ejection_time is not specified and base_ejection_time is larger than max_ejection_time's default. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

outlier_detector: accept large base_ejection_time when max_ejection_time not specified (#14962) Changed logic in config verification when max_ejection_time is not specified and base_ejection_time is larger than max_ejection_time's default. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Changed logic in config verification when max_ejection_time is not sp…

cb09432

…ecified and base_ejection_time is larger than max_ejection_time's default. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski mentioned this pull request Feb 5, 2021

upstream: gradually decrease outlier detector's ejection time multiplier when host stays healthy #14235

Merged

cpakulski marked this pull request as ready for review February 5, 2021 20:27

ggreenway assigned snowp Feb 5, 2021

ramaraochavali mentioned this pull request Feb 6, 2021

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

Closed

antoniovicente added the backport/review Request to backport to stable releases label Feb 9, 2021

antoniovicente reviewed Feb 9, 2021

View reviewed changes

antoniovicente requested a review from snowp February 9, 2021 02:10

cpakulski added 3 commits February 9, 2021 16:08

Changed initialization logic of max_ejection_time.

5fca258

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

Kick CI

5bbc17d

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

Kick CI

53f2c2a

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

repokitteh-read-only Bot added the api label Feb 9, 2021

repokitteh-read-only Bot assigned markdroth Feb 9, 2021

snowp suggested changes Feb 17, 2021

View reviewed changes

snowp added the waiting label Feb 17, 2021

Updated doc section in outlier detector proto file.

c6fcba2

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

lizan reviewed Feb 17, 2021

View reviewed changes

repokitteh-read-only Bot added api and removed api waiting labels Feb 17, 2021

cpakulski requested review from lizan and snowp February 17, 2021 22:04

snowp approved these changes Feb 22, 2021

View reviewed changes

snowp merged commit 12df7d0 into envoyproxy:main Feb 22, 2021

Shikugawa removed the backport/review Request to backport to stable releases label Feb 23, 2021

Shikugawa added the backport/approved Approved backports to stable releases label Feb 23, 2021

cpakulski deleted the od_max branch February 23, 2021 16:35

Conversation

cpakulski commented Feb 5, 2021

Uh oh!

ramaraochavali commented Feb 6, 2021

Uh oh!

antoniovicente 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!

repokitteh-read-only Bot commented Feb 9, 2021

Uh oh!

cpakulski commented Feb 9, 2021

Uh oh!

repokitteh-read-only Bot commented Feb 9, 2021

Uh oh!

cpakulski commented Feb 16, 2021

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!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants