upstream: load distribution in Total Panic mode by cpakulski · Pull Request #9343 · envoyproxy/envoy

cpakulski · 2019-12-13T19:57:34Z

Description: Changed how load is calculated when all priority levels are in panic mode. Each priority level receives percentage of the traffic related to the number of hosts in that priority regardless of the health status of the hosts. This smooths out how traffic is shifted when hosts become unhealthy. See #4685 for design proposal and discussion.

Risk Level: Medium

Testing: Unit tests.

Docs Changes: Yes - updated section about panic threshold.

Release Notes: N/A
Fixes #4685

… panic mode. Health status is ignored and each priority level takes load based on number of hosts in given priority level in relation to the number of all hosts in all priority levels. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp

Thanks for working on this! Left a few comments to get you started.

docs/root/intro/arch_overview/upstream/load_balancing/panic_threshold.rst

source/common/upstream/load_balancer_impl.cc

test/common/upstream/load_balancer_impl_test.cc

cpakulski · 2019-12-18T16:35:10Z

@snowp Thank you for code review. I am working on addressing your comments. There is one corner case which popped up during unit testing. This is when there is no heathy hosts across all priority levels. After my latest changes the algorithm just keeps running in total panic mode and distributes load to all priorities. However, there are some assumptions in the existing code that if there are no healthy hosts, chooseHost method should return nullptr. Interestingly, not all LBs have such assumptions. Ring LB has and its unit tests fail. Also aggregate cluster unit tests also assume that chooseHost returns nullptr. I think that TotalPanic mode should apply only when all levels are in panic mode and there is at least one healthy host in the cluster. I will change my code to work that way and will push code again.

snowp · 2019-12-20T11:48:27Z

That seems like an odd behavior, I would have expected panic mode to trigger even when there are no healthy hosts. Otherwise you might be saved by panic mode, only for your traffic to start failing once all hosts go unhealthy. @mattklein123 do you know the what the original intended behavior was around panic routing /w no healthy upstreams?

mattklein123 · 2019-12-20T16:14:31Z

That seems like an odd behavior, I would have expected panic mode to trigger even when there are no healthy hosts. Otherwise you might be saved by panic mode, only for your traffic to start failing once all hosts go unhealthy. @mattklein123 do you know the what the original intended behavior was around panic routing /w no healthy upstreams?

Definitely a bug. Please fix!

cpakulski · 2019-12-20T16:17:21Z

Thanks for clarification! Will adjust the code accordingly.

stale · 2019-12-27T16:28:56Z

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

cpakulski · 2020-01-02T14:59:13Z

wip: ping

Refactored unit tests to reflect load calculation in TotalPanic. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-06T23:18:25Z

/retest

repokitteh-read-only · 2020-01-06T23:18:30Z

🔨 rebuilding ci/circleci: api (failed build)

🐱

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

see: more, trace.

std::accumulate to calculate total number of hosts. Small fixes after code review. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-07T14:24:17Z

@snowp I addressed all your code review comments accept #9343 (comment). I have to check the code to understand how a backend node ends up in excludedHost list and if we should route the traffic there while in panic mode.

…xcluded hosts. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-10T13:44:43Z

@snowp I addressed the last issue #9343 (comment). excludedHosts array is filled in https://github.com/envoyproxy/envoy/blob/v1.12.2/source/common/upstream/upstream_impl.cc#L855-L872 and traffic is router to excludedHosts in TotalPanic state. The code is ready for another pass.

snowp

Thanks this looks pretty good, just a couple of comments

source/common/upstream/load_balancer_impl.cc

Added unit tests to make sure that excluded_hosts are not taken into account when calculating load in TotalPanic mode. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-15T23:23:00Z

/retest

repokitteh-read-only · 2020-01-15T23:23:04Z

🐴 hold your horses - no failures detected, yet.

🐱

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

see: more, trace.

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp

Thanks for iterating, this is getting close to ready I think

source/common/upstream/load_balancer_impl.cc

test/common/upstream/load_balancer_impl_test.cc

test/extensions/clusters/aggregate/cluster_test.cc

when calculating load in TotalPanic mode. Added unit tests for the case above. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-16T20:40:56Z

/retest

repokitteh-read-only · 2020-01-16T20:41:00Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp

Nice, I think this looks good modulo the one comment. @alyssawilk do you also want to take a look?

Can you bump the risk level of this change? Given how it touches core load balancing code I'd say its at least a medium risky change.

cpakulski · 2020-01-17T01:40:40Z

@snowp. The issue in that one comment was fixed in fe83128. Without that fix my unit tests were failing.

Bumped risk level to Medium.

Thanks for reviewing my changes.

alyssawilk

Looks good! Just a few nits from me

docs/root/intro/version_history.rst

alyssawilk · 2020-01-21T18:42:07Z

source/common/upstream/load_balancer_impl.cc

+  // This is corner case when panic is disabled and there is no hosts available.
+  // But the load must be distributed somewhere.
+  // This is just a placeholder, because no traffic will be directed to
+  // priority 0 anyways, because it is empty.


Yeah, this comment doesn't seem quite right. P[0] may have hosts (it's not necessarily empty). In theory it would just fail to accept queries right?

alyssawilk · 2020-01-21T18:48:00Z

source/common/upstream/load_balancer_impl.cc

+  // This is corner case when panic is disabled and there is no hosts available.
+  // But the load must be distributed somewhere.
+  // This is just a placeholder, because no traffic will be directed to
+  // priority 0 anyways, because it is empty.


Also I think there's a comment above recalculatePerPriorityState which I think may need updating.

source/common/upstream/load_balancer_impl.h

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski · 2020-01-23T00:13:00Z

/retest

repokitteh-read-only · 2020-01-23T00:13:04Z

🤷‍♀️ nothing to rebuild.

🐱

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

see: more, trace.

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

alyssawilk

@snowp any final comments from you?

snowp

This LGTM just seems to be an issue with the change log

docs/root/intro/version_history.rst

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp

Thanks!

mattklein123 assigned snowp Dec 13, 2019

snowp suggested changes Dec 15, 2019

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Dec 27, 2019

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Jan 2, 2020

cpakulski added 2 commits January 6, 2020 14:11

Small changes after code review.

659b260

Refactored unit tests to reflect load calculation in TotalPanic. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Merge branch 'master' into issue/4685

8c359cf

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Refactored LoadBalancerBase::recalculateLoadInTotalPanic method to use

9e0ff2a

std::accumulate to calculate total number of hosts. Small fixes after code review. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Adjusted load calculation algorithm in Total Panic mode NOT to skip e…

792bc88

…xcluded hosts. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp suggested changes Jan 13, 2020

View reviewed changes

cpakulski added 3 commits January 15, 2020 12:28

Small changes after code review.

fe83128

Added unit tests to make sure that excluded_hosts are not taken into account when calculating load in TotalPanic mode. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Merge branch 'master' into issue/4685

6884dff

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Renamed variable as per code review comment.

025147e

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Corrected spelling in user docs.

ab3d8f4

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp suggested changes Jan 16, 2020

View reviewed changes

cpakulski added 2 commits January 16, 2020 14:20

Added logic to add rounding error to the first non-empty priority

da18bcc

when calculating load in TotalPanic mode. Added unit tests for the case above. Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Merge branch 'master' into issue/4685

d46168d

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Corrected spelling mistake.

f776fb4

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

snowp suggested changes Jan 17, 2020

View reviewed changes

snowp assigned alyssawilk Jan 17, 2020

alyssawilk reviewed Jan 21, 2020

View reviewed changes

cpakulski added 3 commits January 22, 2020 12:56

Added comments as per code review.

781baaa

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Merge branch 'master' into issue/4685

bfedb3d

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Updated version document.

2f802ea

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski added 7 commits January 22, 2020 22:39

Kick CI

0aca253

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Kick CI

e151e24

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Kick CI

d511e43

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Kick CI

ff700e1

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Kick CI

8cf0c6c

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Kick CI

2b7d912

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

Merge branch 'master' into issue/4685

4d7b19f

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

alyssawilk previously approved these changes Jan 23, 2020

View reviewed changes

snowp suggested changes Jan 23, 2020

View reviewed changes

docs/root/intro/version_history.rst Outdated Show resolved Hide resolved

Corrected release notes (removed duplicated note).

48455cb

Signed-off-by: Christoph Pakulski <paker8848@gmail.com>

cpakulski dismissed alyssawilk’s stale review via 48455cb January 23, 2020 18:47

snowp approved these changes Jan 23, 2020

View reviewed changes

snowp merged commit 757e0fb into envoyproxy:master Jan 24, 2020

cpakulski deleted the issue/4685 branch January 24, 2020 01:38

Conversation

cpakulski commented Dec 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

snowp left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpakulski commented Dec 18, 2019

Uh oh!

snowp commented Dec 20, 2019

Uh oh!

mattklein123 commented Dec 20, 2019

Uh oh!

cpakulski commented Dec 20, 2019

Uh oh!

stale bot commented Dec 27, 2019

Uh oh!

cpakulski commented Jan 2, 2020

Uh oh!

cpakulski commented Jan 6, 2020

Uh oh!

repokitteh-read-only bot commented Jan 6, 2020

Uh oh!

cpakulski commented Jan 7, 2020

Uh oh!

cpakulski commented Jan 10, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpakulski commented Jan 15, 2020

Uh oh!

repokitteh-read-only bot commented Jan 15, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cpakulski commented Jan 16, 2020

Uh oh!

repokitteh-read-only bot commented Jan 16, 2020

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Jan 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alyssawilk Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

alyssawilk Jan 21, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cpakulski commented Jan 23, 2020

Uh oh!

repokitteh-read-only bot commented Jan 23, 2020

cpakulski commented Dec 13, 2019 •

edited

Loading

snowp left a comment •

edited

Loading

cpakulski commented Jan 17, 2020 •

edited

Loading