Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Mar 3, 2021

Conversation

cpakulski
Copy link
Contributor

Commit Message:
backport to 1.17:
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:

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 <[email protected]>
@repokitteh-read-only
Copy link

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: #15155 was opened by cpakulski.

see: more, trace.

@cpakulski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

@cpakulski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

@cpakulski cpakulski marked this pull request as ready for review February 24, 2021 18:08
@cpakulski
Copy link
Contributor Author

/retest

@repokitteh-read-only
Copy link

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

🐱

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

see: more, trace.

@Shikugawa Shikugawa self-assigned this Feb 26, 2021
@Shikugawa
Copy link
Member

Could you approve this API change? @markdroth

@markdroth
Copy link
Contributor

/lgtm api

@cpakulski cpakulski requested a review from snowp February 26, 2021 18:24
@Shikugawa Shikugawa merged commit 3fce6a9 into envoyproxy:release/v1.17 Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants