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.

Sign up for GitHub

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

Jump to bottom

outlier_detection: add always_eject_one_host #34796

Merged
merged 6 commits into from
Jul 4, 2024
Merged

outlier_detection: add always_eject_one_host #34796

merged 6 commits into from
Jul 4, 2024

Conversation

Pawan-Bishnoi
Copy link
Contributor

Commit Message: Add a config option to allow ejecting one host regardless of max_ejection_percentage
Risk Level: low
Testing: added test
Docs Changes: updated proto comment
Release Notes: todo
Fixes #34666

@Pawan-Bishnoi
outlier_detection: add may_always_eject_one
9092493 
Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #34796 was opened by Pawan-Bishnoi.

see: more, trace.

@Pawan-Bishnoi
review comments
bffc01e 
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
cpakulski
cpakulski reviewed Jun 19, 2024
View reviewed changes
Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

Small comments about docs and code comments.

api/envoy/config/cluster/v3/outlier_detection.proto Outdated Show resolved Hide resolved
test/common/upstream/outlier_detection_impl_test.cc Outdated Show resolved Hide resolved
@Pawan-Bishnoi
review comments
faaca02 
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
@tyxia
Copy link
Member

tyxia commented Jun 21, 2024

/wait

@cpakulski , Looks like Pawan has updated the code. PTAL Thanks!

cpakulski
cpakulski suggested changes Jun 21, 2024
View reviewed changes
Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

Thanks for addressing previous the comments.

Sorry that I have not addressed it before, but am thinking that always_eject_one_host is a better name for the variable. WDYT?

api/envoy/config/cluster/v3/outlier_detection.proto Outdated Show resolved Hide resolved
@Pawan-Bishnoi
review comments - rename field
98fdb6e 
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
@cpakulski
Copy link
Contributor

/retest

cpakulski
cpakulski suggested changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

Please rename a method. The rest looks good. Thanks.

source/common/upstream/outlier_detection_impl.h Outdated Show resolved Hide resolved
@Pawan-Bishnoi
review comments: rename method
63681bc 
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
@cpakulski
Copy link
Contributor

/retest

@Pawan-Bishnoi
add release note and minor fixes
845998e 
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

🐱

Caused by: #34796 was synchronize by Pawan-Bishnoi.

see: more, trace.

@Pawan-Bishnoi Pawan-Bishnoi changed the title outlier_detection: add may_always_eject_one outlier_detection: add always_eject_one_host Jun 25, 2024
cpakulski
cpakulski approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@cpakulski cpakulski left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

API still needs to be approved. @abeyad

@Pawan-Bishnoi
Copy link
Contributor Author

@abeyad ptal when you can 😊

abeyad
abeyad approved these changes Jul 3, 2024
View reviewed changes
Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

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

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Jul 3, 2024
@htuch htuch merged commit 36531d9 into envoyproxy:main Jul 4, 2024
53 checks passed
@Pawan-Bishnoi Pawan-Bishnoi deleted the option_to_override_mep branch July 4, 2024 15:53
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@Pawan-Bishnoi @cainelli
outlier_detection: add always_eject_one_host (envoyproxy#34796)
512b6b9 
Add a config option to allow ejecting one host regardless of max_ejection_percentage

Risk Level: low
Testing: added test
Docs Changes: updated proto comment
Release Notes: todo
Fixes envoyproxy#34666

Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@Pawan-Bishnoi @cainelli
outlier_detection: add always_eject_one_host (envoyproxy#34796)
665babf 
Add a config option to allow ejecting one host regardless of max_ejection_percentage

Risk Level: low
Testing: added test
Docs Changes: updated proto comment
Release Notes: todo
Fixes envoyproxy#34666

Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@Pawan-Bishnoi @cainelli
outlier_detection: add always_eject_one_host (envoyproxy#34796)
4621d42 
Add a config option to allow ejecting one host regardless of max_ejection_percentage

Risk Level: low
Testing: added test
Docs Changes: updated proto comment
Release Notes: todo
Fixes envoyproxy#34666

Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
cainelli pushed a commit to cainelli/envoy that referenced this pull request Jul 5, 2024
@Pawan-Bishnoi @cainelli
outlier_detection: add always_eject_one_host (envoyproxy#34796)
7701f59 
Add a config option to allow ejecting one host regardless of max_ejection_percentage

Risk Level: low
Testing: added test
Docs Changes: updated proto comment
Release Notes: todo
Fixes envoyproxy#34666

Signed-off-by: Pawan Bishnoi <pawanbishnoi@outlook.com>
Signed-off-by: Pawan Kumar <pawanbishnoi@outlook.com>
Signed-off-by: Fernando Cainelli <fernando.cainelli-external@getyourguide.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abeyad abeyad abeyad approved these changes

@cpakulski cpakulski cpakulski approved these changes

Assignees

@abeyad abeyad

@cpakulski cpakulski

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Outlier_detection: config option - always_eject_atleast_one
5 participants
@Pawan-Bishnoi @tyxia @cpakulski @abeyad @htuch