upstream: init host hc value based on hc value from other priorities #3959

snowp · 2018-07-26T04:41:09Z

Description:
In order to allow hosts to move from one priority to another, this
compares a "new" host against all known hosts in order to determine
whether we've already health checked this host successfully.
The idea is that if we already know that a host is healthy we should
treat it as such when it's added to a different priority. Without this, we'll
fail requests coming in between the EDS update and the moved hosts
being successfully health checked.

Signed-off-by: Snow Pettersen snowp@squareup.com

Risk Level: Low, only affects the initial health check value when hosts are added to a priority
Testing: Unit test
Docs Changes: N/A
Release Notes: N/A

Fixes #3930

In order to allow hosts to move from one priority to another, this compares a "new" host against all known hosts in order to determine whether we can avoid reseting the active health check value of the host. The idea is that if we already know that a host is healthy we should treat it as such when it's added to a different priority. Signed-off-by: Snow Pettersen <snowp@squareup.com>

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-07-26T13:24:13Z

Test failure seems to be due to a timeout in the websockets integration tests, which I doubt is related. Would it be possible to rerun the Ipv6 tests?

dio · 2018-07-26T13:31:18Z

git commit --allow-empty -m 'trigger build' -s; git push probably could help 😄.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Thanks for working on this.

mattklein123 · 2018-07-27T16:57:27Z

source/common/upstream/eds.cc

@@ -61,6 +61,15 @@ void EdsClusterImpl::onConfigUpdate(const ResourceVector& resources, const std::
    throw EnvoyException(fmt::format("Unexpected EDS cluster (expecting {}): {}", cluster_name_,
                                     cluster_load_assignment.cluster_name()));
  }
+
+  HostVector all_current_hosts;


Although I agree with the intention here, I'm concerned about the perf impact of this change, given that we are already having perf issues with this code which is starting to be addressed in #3941 by @rgs1.

I think we might need to holistically step back and try to figure out the right path forward for solving this issue while also keeping/improving perf. Concretely, I wonder if we should be keeping an "all hosts" unordered_map keyed by address in parallel to the per-priority vectors? This would allow us to quickly do checks for whether a host already exists in a different priority.

I will also review #3941 today and will probably have additional thoughts after I do that so will circle back then.

Yeah I thought about the map approach but I wasn't sure about the perf requirements of this code path so I wanted to float the naive solution first.

Thinking out loud a bit: if we kept such a map we could potentially simplify/speed up this code quite a bit by making addresses unique across priorities: instead of iterating over the current hosts we'd just check the map for an existing host with that address. This would make moving a host between priorities no longer require additional code and would eliminate the O(n) scan over existing addresses. A nice thing here is that we could reuse the Host from another priority when a host moves, saving us some heap allocations.

Alternatively if we want to maintain the current behavior of allowing duplicate addresses across priorities: we could maintain two maps std::unordered_map<AddressPtr, Host> and std::unordered_map<std::pair<AddressPtr, Integer>, Host> where one would let us look up hosts from any priority (for the "all hosts" check) and the other would let us look up hosts within the same priority. Essentially this transforms the code in my PR to two map lookups instead of two array scans per priority.

Unclear to me whether the host uniqueness only within priority invariant is intentional or just a result of the implementation, so not sure how important it is to maintain.

Unclear to me whether the host uniqueness only within priority invariant is intentional or just a result of the implementation, so not sure how important it is to maintain.

I actually don't know if this was intentional or not. I feel like it was but I'm not sure. IMO it would be great to simplify it if we can. @mpuncel @htuch @alyssawilk @zuercher do you remember the history here? I don't.

I don't have historical context on the code, but that plan sounds reasonable. I can't think of a reason you'd want multiple endpoints with the same address in the same cluster since there is proper support for load balancing weight.

stale · 2018-08-03T20:22:39Z

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!

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-08-06T19:14:30Z

Updated this to maintain a map of address -> host instead of constructing a list on each config update. PTAL

rgs1 · 2018-08-06T23:53:42Z

source/common/upstream/upstream_impl.cc

+      return updated_hosts.count(host->address()->asString()); 
+      }), current_hosts.end());
+
+  // Do a single pass over the current_hosts and remove any that were added to the map in the


copy/pasta?

rgs1 · 2018-08-07T00:04:14Z

source/common/upstream/eds.h

@@ -41,7 +41,7 @@ class EdsClusterImpl : public BaseDynamicClusterImpl,
                              LocalityWeightsMap& locality_weights_map,
                              LocalityWeightsMap& new_locality_weights_map,
                              PriorityStateManager& priority_state_manager,
-                              const HostVector& all_hosts);
+                              std::unordered_map<std::string, HostSharedPtr>& updated_hosts);


the new param name (updated_hosts) doesn't match the definition's

rgs1

@htuch @mattklein123 took the liberty of doing a quick pass, since everything related to health checks/eds/host updates performance is close to my heart.

rgs1 · 2018-08-07T00:15:51Z

source/common/upstream/eds.cc

+bool EdsClusterImpl::updateHostsPerLocality(
+    const uint32_t priority, const HostVector& new_hosts, LocalityWeightsMap& locality_weights_map,
+    LocalityWeightsMap& new_locality_weights_map, PriorityStateManager& priority_state_manager,
+    std::unordered_map<std::string, HostSharedPtr>& new_all_hosts) {


new_all_hosts is a bit confusing here, was this meant to be updated_hosts?

yeah names went through a few iterations, will change

rgs1 · 2018-08-07T00:21:35Z

source/common/upstream/eds.cc

-  if (updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed) ||
+
+  if (updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed,
+                            new_all_hosts, all_hosts_) ||


hmm why pass all_hosts_ as a param given it's a member of BaseDynamicClusterImpl (so updateDynamicHostList() has access to it already)?

good point! i'll update

rgs1 · 2018-08-07T00:22:08Z

source/common/upstream/eds.h

@@ -40,7 +40,8 @@ class EdsClusterImpl : public BaseDynamicClusterImpl,
  bool updateHostsPerLocality(const uint32_t priority, const HostVector& new_hosts,
                              LocalityWeightsMap& locality_weights_map,
                              LocalityWeightsMap& new_locality_weights_map,
-                              PriorityStateManager& priority_state_manager);
+                              PriorityStateManager& priority_state_manager,
+                              std::unordered_map<std::string, HostSharedPtr>& updated_hosts);


noted before, doesn't match the definition's param name.

rgs1 · 2018-08-07T00:22:55Z

source/common/upstream/upstream_impl.cc

-  std::unordered_set<std::string> host_addresses;
+
+  // Keep track of hosts we've seen during this pass so we can remove it from the
+  // list of current (i.e. existing hosts from this priority)


nit: most (all?) envoy comments are fully formed sentences (e.g.: finish them with a period, etc).

Signed-off-by: Snow Pettersen <snowp@squareup.com>

zuercher

Thanks for taking this on.

zuercher · 2018-08-07T16:27:56Z

source/common/upstream/upstream_impl.cc

+                                     }),
+                      current_hosts.end());
+
+  // Do a single pass over the current_hosts and remove any that were added to the map in the


I think this partial comment is spurious.

zuercher · 2018-08-07T16:30:11Z

source/common/upstream/upstream_impl.cc

  const bool dont_remove_healthy_hosts =
      health_checker_ != nullptr && !info()->drainConnectionsOnHostRemoval();
-  // If there are removed hosts, check to see if we should only delete if unhealthy.


Why remove this?

oops, i was supposed to remove the other comment you commented on

zuercher · 2018-08-07T16:31:32Z

source/common/upstream/upstream_impl.cc

        i = current_hosts.erase(i);
      } else {
        i++;
      }
    }
  }

+  // Add the remaining hosts into the updated list of all hosts
+  for (auto& host : final_hosts) {
+    updated_hosts[host->address()->asString()] = host;


It looks to me like every place where you put a host in final_hosts, you also add it to updated_hosts. It this loop necessary?

yeah you're right, i'll remove

zuercher · 2018-08-07T16:51:02Z

source/common/upstream/upstream_impl.cc

+  for (auto& host : final_hosts) {
+    updated_hosts[host->address()->asString()] = host;
+  }
+


There's a comment on line 952 (below) that I think needs to be updated. It currently says

// During the search we moved all of the hosts from hosts_ into final_hosts so just // move them back.

zuercher · 2018-08-07T16:55:53Z

source/common/upstream/upstream_impl.cc

@@ -1054,15 +1065,19 @@ void StrictDnsClusterImpl::ResolveTarget::startResolve() {
              parent_.info_, dns_address_, Network::Utility::getAddressWithPort(*address, port_),
              lb_endpoint_.metadata(), lb_endpoint_.load_balancing_weight().value(),
              locality_lb_endpoint_.locality(), lb_endpoint_.endpoint().health_check_config()));
+          updated_hosts[address->asString()] = new_hosts.back();


I think pre-emptively putting this host in updated_hosts will prevent the max host weight check from occurring in updateDynamicHostList. Why not just let that function do this work?

yeah i agree - i suspect this was left over from a previous iteration and is no longer needed.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp · 2018-08-09T19:48:44Z

This should be ready for another review

mattklein123

Thanks, I think this is a great improvement. Some small comments.

mattklein123 · 2018-08-11T16:48:04Z

source/common/upstream/upstream_impl.h

@@ -630,6 +632,7 @@ class StrictDnsClusterImpl : public BaseDynamicClusterImpl {
    uint32_t port_;
    Event::TimerPtr resolve_timer_;
    HostVector hosts_;
+    std::unordered_map<std::string, HostSharedPtr> all_hosts_;


Is this used?

mattklein123 · 2018-08-11T16:49:49Z

source/common/upstream/upstream_impl.cc

  if (!hosts_added.empty() || !current_hosts.empty()) {
    hosts_removed = std::move(current_hosts);
    current_hosts = std::move(final_hosts);
    return true;
  } else {
-    // During the search we moved all of the hosts from hosts_ into final_hosts so just
-    // move them back.
+    // During the search populated final_hosts with either from hosts_ or from all_hosts, so


This sentence is a little hard to read and I think the comment is important. Can you tighten?

mattklein123 · 2018-08-11T16:50:20Z

source/common/upstream/eds.cc

@@ -133,14 +138,17 @@ bool EdsClusterImpl::updateHostsPerLocality(const uint32_t priority, const HostV
  // out of the locality scheduler, we discover their new weights. We don't currently have a shared
  // object for locality weights that we can update here, we should add something like this to
  // improve performance and scalability of locality weight updates.
-  if (updateDynamicHostList(new_hosts, *current_hosts_copy, hosts_added, hosts_removed) ||
+


nit: del newline

mattklein123 · 2018-08-11T16:51:41Z

source/common/upstream/upstream_impl.h

@@ -503,6 +503,7 @@ class ClusterImplBase : public Cluster, protected Logger::Loggable<Logger::Id::u

 protected:
  PrioritySetImpl priority_set_;
+  std::unordered_map<std::string, Upstream::HostSharedPtr> all_hosts_;


Is this used in the base class? Should this be part of BaseDynamicClusterImpl ?

mattklein123 · 2018-08-11T16:53:48Z

source/common/upstream/upstream_impl.cc

+
+  // Keep track of hosts we've seen during this pass so we can remove it from the
+  // list of current (i.e. existing hosts from this priority) hosts.
+  std::unordered_set<std::string> seen_hosts(current_hosts.size());


This is related to the ongoing PR that @dio is working on where we are trying to make this code easier to understand. I actually think this PR is a huge improvement, but can you potentially either add more comments or think of different names for the variables? "seen_hosts" and "current_hosts" is still pretty confusing. In this code I would opt for very long and verbose names and/or lots of comments since it's so hard to understand.

Signed-off-by: Snow Pettersen <snowp@squareup.com>

rgs1

Did another pass

rgs1 · 2018-08-13T22:36:48Z

source/common/upstream/upstream_impl.cc

-      hosts_added.push_back(host);
-
-      // If we are depending on a health checker, we initialize to unhealthy.
+      // If we are depending on a health checker, determine what to initialize the new host


why is this comment changing?

+1 seems more clear before

rgs1 · 2018-08-13T22:43:44Z

source/common/upstream/upstream_impl.h

+   * @param hosts_added_to_priority will be populated with hosts added to the priority.
+   * @param hosts_removed_from_priority will be populated with hosts removed from the priority.
+   * @param updated_hosts is used to aggregate the new state of all hosts accross priority, and will
+   * be be updated with the hosts that remain in this priority after the update.


rgs1 · 2018-08-13T22:45:24Z

source/common/upstream/upstream_impl.h

+   * @param current_priority_hosts the full lists of hosts for the priority to be updated. The list
+   * will be modified to contain the updated list of hosts.
+   * @param hosts_added_to_priority will be populated with hosts added to the priority.
+   * @param hosts_removed_from_priority will be populated with hosts removed from the priority.


doesn't match the param name (hosts_removed)

mattklein123

Thanks, I think this is a big improvement. LGTM other than some nits. @zuercher can you take another pass through?

mattklein123 · 2018-08-13T23:09:49Z

source/common/upstream/upstream_impl.cc

-      hosts_added.push_back(host);
-
-      // If we are depending on a health checker, we initialize to unhealthy.
+      // If we are depending on a health checker, determine what to initialize the new host


+1 seems more clear before

Signed-off-by: Snow Pettersen <snowp@squareup.com>

zuercher

Looks good. One comment from me

zuercher · 2018-08-14T02:33:57Z

source/common/upstream/upstream_impl.h

+                             HostVector& hosts_removed_from_current_priority,
+                             std::unordered_map<std::string, HostSharedPtr>& updated_hosts);
+
+  std::unordered_map<std::string, Upstream::HostSharedPtr> all_hosts_;


Seems like all_hosts_ is only used by updateDynamicHostList. Perhaps it should be private with an explicit method that takes updated_hosts invoked after all the priorities have been updated? My reasoning is that it makes management of all_hosts_ explicitly the responsibility of BaseDynamicClusterImpl.

yeah i like that idea

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123

Nice!

zuercher

Thanks!

* origin/master: fix flaky RBAC integration test. (envoyproxy#4147) header_map: copy constructor for HeaderMapImpl. (envoyproxy#4129) test: moving websocket tests to using HTTP codec. (envoyproxy#4143) upstream: init host hc value based on hc value from other priorities (envoyproxy#3959) Signed-off-by: Snow Pettersen <snowp@squareup.com>

Snow Pettersen added 2 commits July 26, 2018 00:30

remove redundant comment

6c5d62a

Signed-off-by: Snow Pettersen <snowp@squareup.com>

snowp force-pushed the check-all-hosts branch from 3418e07 to 6c5d62a Compare July 26, 2018 04:48

kick build

58b293d

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 self-assigned this Jul 27, 2018

mattklein123 reviewed Jul 27, 2018

View reviewed changes

stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 3, 2018

Snow Pettersen added 2 commits August 3, 2018 17:04

mostly working

a302f4c

Signed-off-by: Snow Pettersen <snowp@squareup.com>

get all tests passing with map

d161f10

Signed-off-by: Snow Pettersen <snowp@squareup.com>

stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 3, 2018

formating

687e3ce

Signed-off-by: Snow Pettersen <snowp@squareup.com>

rgs1 reviewed Aug 6, 2018

View reviewed changes

rgs1 reviewed Aug 7, 2018

View reviewed changes

Snow Pettersen added 2 commits August 6, 2018 21:44

consistent naming, remove unecessary param, remove bad comment

945425c

Signed-off-by: Snow Pettersen <snowp@squareup.com>

formating

bee31ad

Signed-off-by: Snow Pettersen <snowp@squareup.com>

zuercher reviewed Aug 7, 2018

View reviewed changes

remove unnecessary code, update commetns

fcf5dc2

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 reviewed Aug 11, 2018

View reviewed changes

add comments, update names and other feedback

f0abdf6

Signed-off-by: Snow Pettersen <snowp@squareup.com>

rgs1 reviewed Aug 13, 2018

View reviewed changes

dio mentioned this pull request Aug 13, 2018

upstream: rebuild cluster when health check config is changed #4075

Merged

mattklein123 reviewed Aug 13, 2018

View reviewed changes

update comments from feedback

78242d9

Signed-off-by: Snow Pettersen <snowp@squareup.com>

zuercher reviewed Aug 14, 2018

View reviewed changes

encapsulate all_hosts_ to BaseDynamicClusterImpl

fc3bdee

Signed-off-by: Snow Pettersen <snowp@squareup.com>

mattklein123 approved these changes Aug 14, 2018

View reviewed changes

zuercher approved these changes Aug 14, 2018

View reviewed changes

mattklein123 merged commit da500d2 into envoyproxy:master Aug 14, 2018

bplotnick mentioned this pull request Aug 28, 2018

Endpoints in multiple priorities are treated independently #4280

Open

snowp mentioned this pull request Sep 20, 2018

Changing endpoint priority causes Envoy to lose track of the endpoint #4473

Closed

JonathanO mentioned this pull request Jan 2, 2021

upstream: Fix moving EDS hosts between priorities. #14483

Merged

upstream: init host hc value based on hc value from other priorities #3959

upstream: init host hc value based on hc value from other priorities #3959

Uh oh!

Conversation

snowp commented Jul 26, 2018

Uh oh!

snowp commented Jul 26, 2018

Uh oh!

dio commented Jul 26, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale bot commented Aug 3, 2018

Uh oh!

snowp commented Aug 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp commented Aug 9, 2018

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rgs1 left a comment

rgs1 left a comment •

edited

Loading