Skip to content

runtime: add new runtime guard for previous least request lb patch#30768

Closed
wbpcode wants to merge 1 commit intoenvoyproxy:mainfrom
wbpcode:dev-new-runtime-guard
Closed

runtime: add new runtime guard for previous least request lb patch#30768
wbpcode wants to merge 1 commit intoenvoyproxy:mainfrom
wbpcode:dev-new-runtime-guard

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Nov 8, 2023

Commit Message:
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #30768 was opened by wbpcode.

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Nov 8, 2023

cc @kyessenov

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Nov 8, 2023

Hah, forgot to check the ci after one busy day, sorry for that. Will check it tomorrow morning.

// Do full scan if it's required explicitly or the number of choices is equal to or larger than
// the hosts size.
if ((hosts_to_use.size() <= choice_count_) || enable_full_scan_) {
if ((full_scan_if_host_num_less_than_choice_num_ && hosts_to_use.size() <= choice_count_) ||
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Is there a reported issue for this, and what is the reason? Thanks!

Copy link
Copy Markdown
Contributor

@kyessenov kyessenov Nov 8, 2023

Choose a reason for hiding this comment

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

Issue for public reference: Istio disables cluster stats so it causes this PR to change load balancing to always pick the first host. Due to the DNS traffic concentration, there's some unexpected DNS failure as a result. We were not able to get Istio CI to deploy #29873, see istio/istio#47694.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think we can revert that PR temporarily until we figure out a new API for that. (Today is the last day that could do a break change to the API)
cc @kyessenov

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Yeap, let's do that, and then open it again with a safe default (e.g. by putting && instead of || when the flag is enabled).

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.

4 participants