upstream: introduce PriorityStateManager, refactor EDS by dio · Pull Request #3783 · envoyproxy/envoy

dio · 2018-07-03T13:03:03Z

Description:
This patch introduces PriorityStateManager to manage priorities/hosts/localities
as requirement prior updating the cluster's PrioritySet.
Since most of the codes are taken from eds.cc, this patch also refactors EDS codes.

This is part of the effort on adding load_assignment field to Cluster as signalled
in #3261.

Risk Level: Medium
Testing: bazel test //test/...
Docs Changes: N/A
Release Notes: N/A

Signed-off-by: Dhi Aurrahman dio@rockybars.com

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk

Cool! I think it'll be helpful to have some of this code complexity broken out. A couple of drive by comments from me before I vanish for the rest of the week :-)

alyssawilk · 2018-07-03T15:00:29Z

source/common/upstream/eds.cc

  }
+
+  auto& priority_state = priority_state_manager_->priority_state_;
+  priority_state.clear();


Isn't clearing and creating every time going to be more computationally expensive? It'd be good if we can do this refactor with minimal performance regression

I'm taking a ref and clearing it out. Previously, the priority_state is a local variable in this scope.

Does it makes sense to make PriorityStateManager a local variable in onConfigUpdate? It would mean reverting the Config::Utility::checkLocalInfo call, but other than that check, PSM isn't needed beyond the scope on onConfigUpdate.

alyssawilk · 2018-07-03T15:02:15Z

source/common/upstream/upstream_impl.h

+  // Returns the size of the current cluster priority state.
+  size_t size() const { return priority_state_.size(); }
+
+  PriorityState priority_state_;


I believe per our == google style this should be private

alyssawilk · 2018-07-03T15:04:16Z

source/common/upstream/upstream_impl.h

+
+/**
+ * Manages PriorityState of a cluster. PriorityState is a per-priority binding of a set of hosts
+ * with its corresponding locality weight map. This is not used at runtime, but useful to store


Sorry, can you clarify what you mean by this not being used at runtime?

Probably the wording is not right. I meant to say: it is configuration related only, not in data path.

alyssawilk · 2018-07-03T15:09:38Z

source/common/upstream/upstream_impl.cc

+  if (locality_lb_endpoint.has_locality() && locality_lb_endpoint.has_load_balancing_weight()) {
+    // TODO(dio): Add tests as per @alyssawilk's comment here
+    // https://github.com/envoyproxy/envoy/pull/3261/files#r188292098
+    priority_state_[priority].second[locality_lb_endpoint.locality()] =


Interestingly this looks like it's covered, but there's a few other spots missing. It'd be great to have unit tests as part of this refactor.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-09T15:45:37Z

@alyssawilk when you have time please do a more in-depth review 😄. Thank you!

alyssawilk

Sorry for the delay - I was out last week. With a more thorough inspection it looks really solid! Only two other nits from me :-)

alyssawilk · 2018-07-09T19:08:04Z

source/common/upstream/eds.cc

+                  health_status == envoy::api::v2::core::HealthStatus::DRAINING ||
+                  health_status == envoy::api::v2::core::HealthStatus::TIMEOUT
+              ? absl::optional<Host::HealthFlag>(Host::HealthFlag::FAILED_EDS_HEALTH)
+              : absl::nullopt);


Any reason to not do the health flag checks in the function?

alyssawilk · 2018-07-09T19:13:11Z

source/common/upstream/upstream_impl.cc

+}
+
+void PriorityStateManager::updateClusterPrioritySet(
+    const uint32_t priority, HostVectorSharedPtr current_hosts,


&& current_hosts if we're going to take ownership?

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-10T06:32:55Z

@alyssawilk @zuercher thanks for your time for reviewing this code! I think I have addressed your comments. When you have time please have a look. Thank you!

zuercher

Looks good to me. One small nit.

zuercher · 2018-07-10T16:20:39Z

source/common/upstream/upstream_impl.h

+                          Network::Address::InstanceConstSharedPtr address,
+                          const envoy::api::v2::endpoint::LocalityLbEndpoints& locality_lb_endpoint,
+                          const envoy::api::v2::endpoint::LbEndpoint& lb_endpoint,
+                          const Upstream::Host::HealthFlag health_checker_flag);


nit: Please add a note to the comment indicating when health_checker_flag is used.

Thanks, @zuercher. Added it here: 3d806d2.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2018-07-12T10:45:50Z

@alyssawilk @zuercher Please let me know if there is anything else that I should change. Thanks!

alyssawilk

Thanks both for the general code clean up and the refactor to make PriorityStateManager local - I think it made it an even cleaner PR.

* origin/master: config: making v2-config-only a boolean flag (envoyproxy#3847) lc trie: add exclusive flag. (envoyproxy#3825) upstream: introduce PriorityStateManager, refactor EDS (envoyproxy#3783) test: deflaking header_integration_test (envoyproxy#3849) http: new style WebSockets, where headers and data are processed by the filter chain. (envoyproxy#3776) common: minor doc updates (envoyproxy#3845) fix master build (envoyproxy#3844) logging: Requiring details for RELEASE_ASSERT (envoyproxy#3842) test: add test for consistency of RawStatData internal memory representation (envoyproxy#3843) common: jittered backoff implementation (envoyproxy#3791) format: run buildifier on .bzl files. (envoyproxy#3824) Support mutable metadata for endpoints (envoyproxy#3814) test: deflaking a test, improving debugability (envoyproxy#3829) Update ApiConfigSource docs with grpc_services only for GRPC configs (envoyproxy#3834) Add hard-coded /hot_restart_version test (envoyproxy#3832) healthchecks: Add interval_jitter_percent healthcheck option (envoyproxy#3816) Signed-off-by: Snow Pettersen <snowp@squareup.com>

akonradi · 2018-07-13T19:55:32Z

Opened #3856 to fix some off-the-end indexing in PriorityStateManager::updateClusterPrioritySet.

dio added 4 commits July 3, 2018 19:43

Add PriorityStateManager

be3cd0b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Refactor EDS to use PriorityStateManager

095393a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Use the ref to host set early

e94f2be

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove unused method

bd5cd8a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio mentioned this pull request Jul 3, 2018

upstream: add load_assigment field to Cluster #3261

Closed

alyssawilk reviewed Jul 3, 2018

View reviewed changes

dio added 3 commits July 3, 2018 23:26

Make it private

d58ed1c

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove comment

0af1828

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Remove health checker flag and add TODO

81c81a7

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

ggreenway assigned zuercher and alyssawilk Jul 3, 2018

dio mentioned this pull request Jul 4, 2018

Allow healthcheck on different port for non-EDS cluster types #439

Closed

dio added 3 commits July 4, 2018 10:00

Fix comments and add more ASSERTs

4601cd4

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix comment

55a8338

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Wrongly put some asserts

01dfffd

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk reviewed Jul 9, 2018

View reviewed changes

dio added 8 commits July 10, 2018 07:07

Reviewer comments

aedc2ed

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Preserve format

709fb6b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Use from arg

c9674ff

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Cleanup some superfluous newlines

0fc68dd

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Handle condition when LocalityWeightsMap is empty

d9d25d4

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Defer constructing PriorityStateManager

b6a0697

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

3664763

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Retry ipv6 failure

dd000d5

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

zuercher reviewed Jul 10, 2018

View reviewed changes

dio added 2 commits July 10, 2018 23:35

review: add comment to registerHostForPriority

3d806d2

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/master'

37003fe

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

alyssawilk approved these changes Jul 12, 2018

View reviewed changes

zuercher approved these changes Jul 12, 2018

View reviewed changes

alyssawilk merged commit 9de80f7 into envoyproxy:master Jul 12, 2018

ramaraochavali mentioned this pull request Dec 2, 2018

load balancer stat inconsistency #5168

Closed

Conversation

dio commented Jul 3, 2018

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

dio commented Jul 9, 2018

Uh oh!

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

dio commented Jul 10, 2018

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!

dio commented Jul 12, 2018

Uh oh!

alyssawilk left a comment

Choose a reason for hiding this comment

Uh oh!

akonradi commented Jul 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants