-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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.
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
upstream: switch zone-aware routing to single level locality-aware. #1797
Conversation
We now do single level locality-aware routing, using the EDS supplied Locality consisting of (region, zone, sub_zone). This is only a simple extension and rename of zone aware routing, there is no support yet for multi-level locality routing (see envoyproxy#1796). Signed-off-by: Harvey Tuch <[email protected]>
include/envoy/upstream/upstream.h
Outdated
* @return hosts per zone, index 0 is dedicated to local zone hosts. | ||
* If there are no hosts in local zone for upstream cluster hostPerZone() will @return | ||
* @return hosts per locality, index 0 is dedicated to local locality hosts. | ||
* If there are no hosts in local locality for upstream cluster hosstPerLocality() will @return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo
@@ -13,7 +13,7 @@ | |||
namespace Envoy { | |||
namespace Upstream { | |||
|
|||
static const std::string RuntimeZoneEnabled = "upstream.zone_routing.enabled"; | |||
static const std::string RuntimezoneEnabled = "upstream.zone_routing.enabled"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why change case?
return; | ||
} | ||
|
||
size_t num_zones = host_set_.healthyHostsPerZone().size(); | ||
ASSERT(num_zones > 0); | ||
size_t num_localitys = host_set_.healthyHostsPerLocality().size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localities
// At this point it's guaranteed to be at least 2 zones. | ||
size_t number_of_zones = host_set_.healthyHostsPerZone().size(); | ||
// At this point it's guaranteed to be at least 2 localitys. | ||
size_t number_of_localitys = host_set_.healthyHostsPerLocality().size(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
localities
|
||
// This potentially can be optimized to be O(log(N)) where N is the number of zones. | ||
// This potentially can be optimized to be O(log(N)) where N is the number of localitys. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me guess: emacs query-replace ? /zone/locality/ ? ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, tree-wide search/replacing, will fix tomorrow, thanks for picking up these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few typos from mass search/replace :)
Signed-off-by: Harvey Tuch <[email protected]>
if (!local_info_.zoneName().empty()) { | ||
std::map<std::string, std::vector<HostSharedPtr>> hosts_per_zone; | ||
// If local locality is not defined then skip populating per locality hosts. | ||
const Locality local_locality(local_info_.node().locality()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: why copy here? Reference? Or does locality() return a copy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK I see this is a helper that unpacks the locality.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could do it more efficiently, with std::string&
in the std::tuple
, but I think this might be an issue with our multiple string
implementation today.
@@ -35,6 +35,20 @@ | |||
namespace Envoy { | |||
namespace Upstream { | |||
|
|||
// Wrapper around envoy::api::v2::Locality to make it easier to compare and construct literals (for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is used in not test code right? (Per my other comment). Change comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is used in both eds.cc
and eds_test.cc
. I'll update the comment.
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
Signed-off-by: Harvey Tuch <[email protected]>
* wire through route directive Signed-off-by: Kuat Yessenov <[email protected]> * fix format Signed-off-by: Kuat Yessenov <[email protected]> * propagate info Signed-off-by: Kuat Yessenov <[email protected]> * review Signed-off-by: Kuat Yessenov <[email protected]> * implement the API Signed-off-by: Kuat Yessenov <[email protected]> * manual test Signed-off-by: Kuat Yessenov <[email protected]> * review Signed-off-by: Kuat Yessenov <[email protected]> * fix upstream api change Signed-off-by: Kuat Yessenov <[email protected]> * add a test case Signed-off-by: Kuat Yessenov <[email protected]> * review, handle direct response headers Signed-off-by: Kuat Yessenov <[email protected]>
Description: Removes network-specific clusters and replaces with a filter that sets unique socket options based on current preferred network (via Reachability) status. Envoy internally creates a different socket pool for a unique set of socket options, so the effect of this is that we still use different sets of connections per interface, but no longer rely on a large set of clusters to do so. This PR also lays the groundwork for us to experiment with interface-bound sockets. Risk Level: High Testing: Existing unit and integration coverage Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>
Description: Removes network-specific clusters and replaces with a filter that sets unique socket options based on current preferred network (via Reachability) status. Envoy internally creates a different socket pool for a unique set of socket options, so the effect of this is that we still use different sets of connections per interface, but no longer rely on a large set of clusters to do so. This PR also lays the groundwork for us to experiment with interface-bound sockets. Risk Level: High Testing: Existing unit and integration coverage Signed-off-by: Mike Schore <[email protected]> Signed-off-by: JP Simard <[email protected]>
We now do single level locality-aware routing, using the EDS supplied Locality consisting of
(region, zone, sub_zone). This is only a simple extension and rename of zone aware routing, there is
no support yet for multi-level locality routing (see #1796).
Signed-off-by: Harvey Tuch [email protected]