upstream: Fallback policy per selector support by nezdolik · Pull Request #7087 · envoyproxy/envoy

nezdolik · 2019-05-28T14:39:44Z

Signed-off-by: Kateryna Nezdolii nezdolik@spotify.com

Duplicate of #6124 but with completed development.

Description: This code change allows to redefine fallback policy per specific subset selector. Because of how existing LbSubsetMap trie data structure is organised (mapping subset key to values), is not possible to do lookups for fallback policy only based on subset keys (had to introduce additional trie that maps subset keys to keys and has fallback policy on leaf level). Additional LbSubsetSelectorFallbackPolicy enum required to correctly identify the case when fallback policy is not set for given selector (otherwise it would always default to NO_FALLBACK, breaking backwards compatibility, if field is not set we should use top level fallback policy instead).

Risk Level: Medium
Testing: Done
Docs Changes: Updated related docs
Release Notes:
Fixes #5130

htuch · 2019-05-28T15:38:33Z

Please merge master to pick up #7090

yanavlasov · 2019-05-29T01:40:47Z

Please add unit tests for the new functionality in the subset_lb_test.cc.
EDIT: sorry I see you have added it. Github decided it was too large and not rendered it.

dio · 2019-05-29T07:28:02Z

@nezdolik seems like you need to fix DCO. Please follow this: https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#fixing-dco

To avoid missing DCO in a pushed commit in the future seems like enabling the commit hook (./support/bootstrap) is not a bad idea https://github.com/envoyproxy/envoy/blob/master/CONTRIBUTING.md#dco-sign-your-work (it also helps to check the format)

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

yanavlasov · 2019-05-30T00:16:11Z

Please add user documentation for the new feature here https://github.com/envoyproxy/envoy/blob/master/docs/root/intro/arch_overview/load_balancing/subsets.rst

yanavlasov · 2019-05-30T02:23:50Z

 }

+void SubsetLoadBalancer::initSubsetSelectorMap() {
+  selectors_ = std::make_shared<SubsetSelectorMap>();


Since selectors_ is always initialized, I suggest moving its initialization to the constructor initializer list or better yet initialize it right in the header at the point of definition.

@yanavlasov cannot init selectors_ in the header, as it depends on field subset_selectors_ being initialised in constructor. Moved out this initialisation into separate method, as constructor body is already big and it could be hard to read. What do you think?

I was suggesting to move just the first line where the pointer is initialized to point to an empty SubsetSelectorMap object to the header. However the way you have is fine as well.

yanavlasov · 2019-05-30T02:42:20Z

  typedef std::unordered_map<HashedValue, LbSubsetEntryPtr> ValueSubsetMap;
  typedef std::unordered_map<std::string, ValueSubsetMap> LbSubsetMap;

+  struct SubsetSelectorMap {


I think it is fine as it is. However you could likely simplify this. The subset fallback policy is fully identified by the set of selector keys for this subset. You can concatenate the sorted selector keys into one string and use it as a key for finding the fallback policy. In this way you do not need to build a trie and can just use std::unordered_set<std::string, envoy::api::v2::Cluster::LbSubsetConfig::LbSubsetSelectorFallbackPolicy>

@yanavlasov agree that that way code would be simplified. Was thinking about tradeoff though, if to switch to plain map, on each lookupHosts call LB will have to perform concatenation of keys from match criteria to look it up in map. While first approach brings code complexity but is kind of precomputing things on LB object creation so that when lookupHosts called we just iterate over each match criteria key and look it up recursively in a trie. If it's not a big deal of match criteria keys concatenation (from memory usage perspective), then having plain map seems more beneficial.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2019-05-31T16:14:44Z

@yanavlasov addressed some of review comments (added testing for compound selector, switched to struct for SubsetSelector type, removed optional in struct), all others are in progress

htuch

API level comments.
/wait

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2019-06-06T14:48:45Z

/retest

repokitteh-read-only · 2019-06-06T14:48:50Z

🔨 rebuilding ci/circleci: coverage (failed build)

🐱

Caused by: a #7087 (comment) was created by @nezdolik.

see: more, trace.

rgs1 · 2019-06-06T19:51:15Z

@nezdolik should panic mode also be configurable per selector?

zuercher

Thanks! I had a question about how the map of subset selectors/fallback policies is constructed, but otherwise I think it looks pretty good.

zuercher · 2019-06-06T23:46:10Z

+    const auto& selector_keys = subset_selector->selector_keys_;
+    uint32_t pos = 0;
+    selectors = selectors_;
+    for (const auto& key : selector_keys) {


What do you think about skipping this loop if subset_selector->fallback_policy_ is undefined? As it stands, the default behavior will be to build up this data structure with all the subsets even if no subset has a custom fallback policy configured. If you skip adding the subsets with no custom fallback, then SubsetSelectorMap.fallback_policy_ doesn't need to be optional and you can get rid of that complicated ternary below.

agree, thanks for pointing out

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2019-06-11T15:44:50Z

@rgs1 possibly yes, with this change in case there is override per selector and no hosts are found, nullptr is returned even if there is panic mode enabled on top level config.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

rgs1 · 2019-06-11T20:21:56Z

+    HostPredicate predicate = [](const Host&) -> bool { return true; };
+    selector_fallback_subset_any_ = std::make_unique<LbSubsetEntry>();
+    selector_fallback_subset_any_->priority_subset_.reset(
+        new PrioritySubsetImpl(*this, predicate, locality_weight_aware_, scale_locality_weight_));


nit: use make_shared() for consistency?

rgs1 · 2019-06-11T20:22:07Z

+                                        default_subset_metadata_, std::placeholders::_1);
+    selector_fallback_subset_default_ = std::make_unique<LbSubsetEntry>();
+    selector_fallback_subset_default_->priority_subset_.reset(
+        new PrioritySubsetImpl(*this, predicate, locality_weight_aware_, scale_locality_weight_));


While reading envoyproxy#7087, I noticed two things about subset_lb.{cc,h}: * a few shared_ptrs are assigned via make_unique, which works but is wasteful [extra allocation] * reset(new ...) is used, which is unsafe & not consistent with the rest of the file Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

While reading #7087, I noticed two things about subset_lb.{cc,h}: * a few shared_ptrs are assigned via make_unique, which works but is wasteful [extra allocation] * reset(new ...) is used, which is unsafe & not consistent with the rest of the file Signed-off-by: Raul Gutierrez Segales <rgs@pinterest.com>

htuch

Approved from API and general Envoy style perspective, will defer to @zuercher to merge.

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2019-06-13T16:02:48Z

@rgs1 switched back to shared_ptr, ideally LbSubsetEntryPtr should be unique_ptr, not sure why it was declared shared in first place

zuercher · 2019-06-13T17:31:55Z

@nezdolik if you can get that merge conflict fixed we can merge this (and then @snowp can merge his subset lb change).

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik · 2019-06-13T22:12:55Z

@zuercher merge conflict should be fixed now

nezdolik force-pushed the subset-lb-opt branch from 319bc61 to 33b92c2 Compare May 28, 2019 15:34

nezdolik force-pushed the subset-lb-opt branch 2 times, most recently from 29d9cde to 5d33bfe Compare May 28, 2019 23:19

yanavlasov reviewed May 29, 2019

View reviewed changes

Comment thread include/envoy/upstream/load_balancer_type.h Outdated

yanavlasov reviewed May 29, 2019

View reviewed changes

Comment thread source/common/upstream/subset_lb.cc Outdated

upstream: Fallback policy per selector support

2b5abec

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik force-pushed the subset-lb-opt branch from 2cf7d88 to 2b5abec Compare May 29, 2019 08:44

Kateryna Nezdolii added 3 commits May 29, 2019 10:52

fix format

58e06c6

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Remove unnecessary import

98c8faf

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

fix format

7a6e3ca

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

jmarantz assigned yanavlasov May 29, 2019

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread include/envoy/upstream/load_balancer_type.h Outdated

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread source/common/upstream/subset_lb.h Outdated

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread source/common/upstream/subset_lb.cc Outdated

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread source/common/upstream/subset_lb.cc Outdated

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread source/common/upstream/subset_lb.h Outdated

yanavlasov reviewed May 30, 2019

View reviewed changes

Comment thread test/common/upstream/subset_lb_test.cc Outdated

Kateryna Nezdolii added 2 commits May 31, 2019 18:09

apply review comments

8b680be

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

apply review comments

e87d71d

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik requested a review from htuch as a code owner May 31, 2019 16:09

htuch suggested changes May 31, 2019

View reviewed changes

Comment thread api/envoy/api/v2/cds.proto Outdated

Comment thread api/envoy/api/v2/cds.proto Outdated

repokitteh-read-only Bot added the waiting label May 31, 2019

apply review comments

01ebd22

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

htuch requested a review from zuercher June 6, 2019 04:25

htuch assigned zuercher Jun 6, 2019

apply review comments

43abe04

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik dismissed yanavlasov’s stale review via 43abe04 June 6, 2019 12:57

repokitteh-read-only Bot removed the waiting label Jun 6, 2019

zuercher reviewed Jun 6, 2019

View reviewed changes

Kateryna Nezdolii added 2 commits June 11, 2019 15:52

Merge branch 'master' of https://github.com/envoyproxy/envoy

b91c820

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

apply review comments

2eeaee4

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

apply review comment

1ad0591

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

zuercher previously approved these changes Jun 11, 2019

View reviewed changes

rgs1 reviewed Jun 11, 2019

View reviewed changes

rgs1 mentioned this pull request Jun 11, 2019

Use std::make_shared() for consistency/safety #7237

Merged

htuch previously approved these changes Jun 13, 2019

View reviewed changes

Kateryna Nezdolii added 2 commits June 13, 2019 17:01

apply review comments

025a906

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

Merge branch 'master' of https://github.com/envoyproxy/envoy

298dff7

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

nezdolik dismissed stale reviews from htuch and zuercher via 298dff7 June 13, 2019 15:59

merge origin master

dba2deb

Signed-off-by: Kateryna Nezdolii <nezdolik@spotify.com>

zuercher approved these changes Jun 14, 2019

View reviewed changes

zuercher merged commit 1a60b34 into envoyproxy:master Jun 14, 2019

Conversation

nezdolik commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

htuch commented May 28, 2019

Uh oh!

Uh oh!

yanavlasov commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dio commented May 29, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yanavlasov commented May 30, 2019

Uh oh!

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nezdolik commented May 31, 2019

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nezdolik commented Jun 6, 2019

Uh oh!

repokitteh-read-only Bot commented Jun 6, 2019

Uh oh!

rgs1 commented Jun 6, 2019

Uh oh!

zuercher left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nezdolik commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

nezdolik commented Jun 13, 2019

Uh oh!

zuercher commented Jun 13, 2019

Uh oh!

nezdolik commented Jun 13, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

nezdolik commented May 28, 2019 •

edited

Loading

yanavlasov commented May 29, 2019 •

edited

Loading

dio commented May 29, 2019 •

edited

Loading

nezdolik commented Jun 11, 2019 •

edited

Loading