Make zone load balancing logic depend on local_cluster host distribution. #174

RomanDzhabarov · 2016-10-26T20:27:39Z

Documentation around runtime control/stats/general overview are going to be on a separate commit.

I was running simulation tests from load_balancer_simulation_test.cc, results look very good.

mattklein123 · 2016-10-27T16:24:27Z

source/common/upstream/load_balancer_impl.cc


-  // If number of hosts in a local zone big enough then route all requests to the same zone.
-  if (local_zone_healthy_hosts.size() * number_of_zones >= host_set_.healthyHosts().size()) {
+  std::vector<uint64_t> local_percentage =


Use a stack allocated array with the right size, rework the functions above to take the array and populate it if you want to do that. You can't allocate things like vectors in this code path.

Or just do the pre-calculation work now. I would actually prefer you just do that.

i made stack alloc for now, i want make sure things works good in prod

mattklein123 · 2016-10-27T16:26:43Z

source/common/upstream/load_balancer_impl.cc

+  // At this point we should route cross zone as we cannot route to the local zone.
+  stats_.zone_routing_no_sampled_.inc();
+
+  std::vector<uint64_t> capacity_left;


stack allocated array

mattklein123 · 2016-10-27T16:27:49Z

source/common/upstream/load_balancer_impl.cc

+  stats_.zone_routing_no_sampled_.inc();
+
+  std::vector<uint64_t> capacity_left;
+  // Local zone does not have additional capacity (we already routed what we could), but


More comments. Don't understand what is happening here.

…ts." This reverts commit ab35059. Conflicts: test/common/upstream/load_balancer_simulation_test.cc

ccaraman · 2016-10-31T18:26:41Z

source/common/upstream/load_balancer_impl.cc

+  if (total_hosts != 0) {
+    size_t i = 0;
+    for (const auto& zone_hosts : hosts_per_zone) {
+      ret[i++] = 10000ULL * zone_hosts.size() / total_hosts;


Please add comment explaining the 10,000ULL.

yep, good point, will add more details into load_balancer_impl.h. Done.

ccaraman · 2016-10-31T18:47:53Z

source/common/upstream/load_balancer_impl.cc

+  // Now we need to figure out how much traffic we can route cross zone and to which exact zone
+  // we should route. Percentage of requests routed cross zone to a specific zone should be
+  // proportional to the residual capacity upstream zone has.
+  //


nit: Needed?

mattklein123 · 2016-10-31T19:41:52Z

include/envoy/upstream/upstream.h

@@ -192,10 +192,12 @@ class HostSet {
  COUNTER(update_success)                                                                          \
  COUNTER(update_failure)                                                                          \
  COUNTER(zone_cluster_too_small)                                                                  \
-  COUNTER(zone_over_percentage)                                                                    \
+  COUNTER(zone_routing_all_directly)                                                               \


nit: can you prefix all of the lb ones with "lb_" and then reorder this list.

yep, will do for consistency with panic threshold stats

mattklein123 · 2016-10-31T19:46:23Z

source/common/upstream/load_balancer_impl.cc

-    stats_.zone_over_percentage_.inc();
-    return local_zone_healthy_hosts;
+  uint64_t local_percentage[number_of_zones];
+  calculateZonePercentage(local_host_set_->healthyHostsPerZone(), local_percentage);


ASSERT here also that local_host_set_->healthyHostsPerZone().size() == host_set_.healthyHostsPerZone().size()

mattklein123 · 2016-10-31T19:55:16Z

source/common/upstream/load_balancer_impl.h

+   * In this case we'll route requests to hosts no matter if they are healthy or not.
+   */
+  bool isGlobalPanic(const HostSet& host_set);
+  const std::vector<HostPtr>& tryChooseLocalZoneHosts();


nit: add comment or newline before this line

put comments.

mattklein123 · 2016-10-31T19:56:12Z

source/common/upstream/load_balancer_impl.cc

+  // Local zone (index 0) does not have residual capacity as we have routed all we could.
+  residual_capacity[0] = 0;
+  for (size_t i = 1; i < number_of_zones; ++i) {
+    // Only route to the zones that have additional capacity.


still having trouble quickly understanding this logic. More comments, and potentially an example.

more comments added

mattklein123 · 2016-10-31T19:56:57Z

source/common/upstream/load_balancer_impl.cc

-  // Not zone routed requests will be distributed between all hosts and hence
-  // we need to route only fraction of req_percent_to_route to the local zone.
-  double actual_routing_ratio = (ratio_to_route - zone_host_ratio) / (1 - zone_host_ratio);
+  // Random sampling to select specific zone for cross zone traffic based on the additional


more comments. What type of number does residual_capacity[number_of_zones - 1] have in it. What range?

more comments added

mattklein123 · 2016-10-31T21:21:42Z

source/common/upstream/load_balancer_impl.cc

+  // sampled value is, we accumulate values in residual capacity. This is what it will look like:
+  // residual_capacity: 0 10000 15000
+  // Now to find zone to route (bucket) we could simply iterate over residual_capacity searching
+  // where


nit: can you fix line wrapping in this comment block. Keep to 100 col flow unless you need a new paragraph.

:( fix format broke this... fixing

mattklein123

lg after nit

* Add Dockerfile for the C++ SDK. Signed-off-by: John Plevyak <jplevyak@gmail.com>

…#114) (envoyproxy#174) Previously, the update callback was called only when the secret was received for the first time or when its value changed. This meant that if the same secret (e.g. trusted CA) was used in multiple resources, then resources using it but configured after the secret was already received, remained unconfigured until the secret's value changed. The missing callback should have resulted in transport factories stuck in the "not ready" state, however, because of an incorrect code, the available secret was processed like inlined validation context, and only rules from the "secret" part of the validation context were applied, leading to a complete bypass of rules from the "default" part. Signed-off-by: Piotr Sikora <piotrsikora@google.com> Co-authored-by: Oliver Liu <yonggangl@google.com> Co-authored-by: Oliver Liu <yonggangl@google.com>

…verprovisioning_factor zh-translation: /intro/arch_overview/upstream/load_balancing/overprovisioning.rst

RomanDzhabarov added 3 commits October 26, 2016 13:22

initial change to balancing logic.

db76822

more requests, fix number of hosts.

db1df90

Add more tests around load balancing.

c83b5af

mattklein123 reviewed Oct 27, 2016

View reviewed changes

RomanDzhabarov added 4 commits October 27, 2016 14:55

comments.

fb39d7a

Enable tests, it takes around 25sec to complete all those tests.

ab35059

make 3% as we do not run through that many requests.

c7710ed

Revert "Enable tests, it takes around 25sec to complete all those tes…

82f2696

…ts." This reverts commit ab35059. Conflicts: test/common/upstream/load_balancer_simulation_test.cc

ccaraman reviewed Oct 31, 2016

View reviewed changes

more comments on multiplier and method.

879445d

ccaraman reviewed Oct 31, 2016

View reviewed changes

fix wording.

c756172

mattklein123 reviewed Oct 31, 2016

View reviewed changes

RomanDzhabarov added 3 commits October 31, 2016 14:08

comments.

f431d66

sort.

193f215

fix_format.

e49231a

mattklein123 reviewed Oct 31, 2016

View reviewed changes

mattklein123 approved these changes Oct 31, 2016

View reviewed changes

line wrapping.

90067cf

RomanDzhabarov merged commit d4d2393 into master Oct 31, 2016

RomanDzhabarov deleted the use_local_cluster branch November 1, 2016 02:46

jplevyak referenced this pull request in jplevyak/envoy Sep 17, 2019

Wasm dockerfile (istio#174)

d647c8f

* Add Dockerfile for the C++ SDK. Signed-off-by: John Plevyak <jplevyak@gmail.com>

jplevyak referenced this pull request in istio/envoy Sep 17, 2019

Wasm dockerfile (#174) (#108)

f13bebe

* Add Dockerfile for the C++ SDK. Signed-off-by: John Plevyak <jplevyak@gmail.com>

PiotrSikora pushed a commit to PiotrSikora/envoy that referenced this pull request Oct 10, 2019

Wasm dockerfile (envoyproxy#174)

0e5c253

* Add Dockerfile for the C++ SDK. Signed-off-by: John Plevyak <jplevyak@gmail.com>

wolfguoliang pushed a commit to wolfguoliang/envoy that referenced this pull request Jan 23, 2021

Merge pull request envoyproxy#174 from webup/intro/arch/upstream/lb/o…

99d4988

…verprovisioning_factor zh-translation: /intro/arch_overview/upstream/load_balancing/overprovisioning.rst

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make zone load balancing logic depend on local_cluster host distribution. #174

Make zone load balancing logic depend on local_cluster host distribution. #174

RomanDzhabarov commented Oct 26, 2016

mattklein123 Oct 27, 2016

mattklein123 Oct 27, 2016

RomanDzhabarov Oct 27, 2016

mattklein123 Oct 27, 2016

RomanDzhabarov Oct 27, 2016

mattklein123 Oct 27, 2016

RomanDzhabarov Oct 27, 2016

ccaraman Oct 31, 2016

RomanDzhabarov Oct 31, 2016 •

edited

Loading

ccaraman Oct 31, 2016 •

edited

Loading

RomanDzhabarov Oct 31, 2016

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016 •

edited

Loading

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016

mattklein123 Oct 31, 2016

RomanDzhabarov Oct 31, 2016

mattklein123 left a comment

Make zone load balancing logic depend on local_cluster host distribution. #174

Make zone load balancing logic depend on local_cluster host distribution. #174

Conversation

RomanDzhabarov commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanDzhabarov Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

ccaraman Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RomanDzhabarov Oct 31, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattklein123 left a comment

Choose a reason for hiding this comment

RomanDzhabarov Oct 31, 2016 •

edited

Loading

ccaraman Oct 31, 2016 •

edited

Loading

RomanDzhabarov Oct 31, 2016 •

edited

Loading