On-demand VHDS implementation #8617

dmitri-d · 2019-10-15T15:48:42Z

Signed-off-by: Dmitri Dolguikh ddolguik@redhat.com

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Implements on-demand resolution of VirtualHosts via VHDS
Risk Level:
Testing:
Docs Changes:
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

dmitri-d · 2019-10-15T15:53:09Z

I decided to open a new PR instead of re-opening the original one (can be found here: https://github.com/envoyproxy/envoy/pull/6552/files).

Ping @htuch, @fredlas.

mattklein123 · 2019-10-15T15:55:34Z

Drive by: please merge #7415 into this PR so we make sure we have docs for this feature. Thank you!

fredlas

Mainly reviewing the parts that touch code I'm familiar with. It doesn't look like #8478 should dramatically change anything out from under you, so that's good.

source/common/http/conn_manager_impl.h

source/common/config/delta_subscription_state.cc

dmitri-d · 2019-10-17T20:55:27Z

rebased
fixed several formatting issues
responded to feedback

dmitri-d · 2019-10-17T22:53:37Z

rebased

fredlas

Partial review because github has started not accepting my comments? Let's see what's going on with that...

include/envoy/http/filter.h

include/envoy/router/rds.h

include/envoy/http/filter.h

source/common/config/delta_subscription_impl.cc

source/common/config/delta_subscription_state.h

source/common/router/rds_impl.cc

fredlas · 2019-10-17T22:32:54Z

source/common/router/rds_impl.cc

+// response has been propagated to the worker thread that was the request origin.
+bool RdsRouteConfigProviderImpl::requestVirtualHostsUpdate(const std::string& for_domain,
+                                                           std::function<void()> cb) {
+  if (!config()->usesVhds()) {


This if seems a little suspect. It seems like it is checking for a not-really-error but not-really-right condition. The sort of thing where things can muddle along mostly working, and it's hard to track down what's going wrong. Going by the names of the functions involved, I think it might be more safe and solid to, rather than doing this if in here, instead guard all uses of requestVirtualHostsUpdate with it. I think that would also let you drop the bool return type. (If all of that is actually possible).

I pulled the check for whether requestVirtualHostsUpdate is available to the very top (see extensions/filters/http/on_demand/on_demand_update.

source/common/router/rds_impl.h

source/common/router/route_config_update_receiver_impl.cc

fredlas

ok, now I am done with this round of review.

source/common/router/route_config_update_receiver_impl.cc

test/common/router/vhds_test.cc

test/extensions/filters/http/router/config_test.cc

dmitri-d · 2019-10-19T01:51:28Z

rebased

mattklein123

I haven't actually reviewed this change but I'm marking this requested changes to make sure we get VHDS fully documented for whatever exists today in the code base. This should include:

Merging Vhds readme #7415 into this PR as proper RST docs with arch overview, etc.
Getting VHDS up to parity with the fixes that I did in docs: misc doc debt #8678.

Thank you!

fredlas · 2019-10-21T20:12:30Z

LGTM

dmitri-d · 2019-10-21T21:46:52Z

rebased

dmitri-d · 2019-10-21T21:49:06Z

@lambdai: the PR I was talking about during the last xDS call, could you take a look when you get a chance?

dmitri-d · 2019-10-21T22:13:00Z

fixed formatting.

htuch

Thanks @dmitri-d. This is epic. It's a pretty big PR, so I've started my review with a few comments. I think that this is going to need a fair bit of review to get through. Also, as a general principle, PRs to the Envoy code base should either be large and mechanical or small and complicated (or even simple :) ). Please consider what can be done to simplify or reduce the amount of change in this PR.

My main initial set of questions are around the additional plumbing added for aliases. I find it somewhat surprising, I had envisaged aliases being far simpler to handle.

Tagging @lambdai and @stevenzzzz for implications of the update callbacks to the ownership and lifecycle of route config provider resources.
/wait

CODEOWNERS

source/common/http/conn_manager_impl.h

source/common/config/delta_subscription_state.h

include/envoy/http/filter.h

source/common/config/delta_subscription_state.cc

lambdai

I don't understand the design of putting aliasResolution into mux and subscription.
Since it is not implemented yet: if you can extract it to anther PR, we can focus on the rds and ondemand vhds.

@htuch WDYT?

include/envoy/config/subscription.h

source/common/config/delta_subscription_state.cc

lambdai · 2019-10-23T00:16:10Z

source/common/config/new_grpc_mux_impl.h

@@ -37,6 +37,8 @@ class NewGrpcMuxImpl : public GrpcMux,
  void pause(const std::string& type_url) override;
  void resume(const std::string& type_url) override;
  bool paused(const std::string& type_url) const override;
+  void requestAliasesResolution(const std::string& type_url,


Is it reasonable to promote alias req/resp to first class in mux and subscription?

Removed the method (pls. see the conversation above re: delta_subscription_state's requested_aliases_ field).

lambdai

I need to digest this PR with design doc since ondemand involves close interaction with worker threads and main thread. Is the design doc named "on demand RDS"? I need to make sure I am reading the correct doc. Thanks!

lambdai · 2019-10-23T03:22:22Z

source/common/http/conn_manager_impl.h

+  class RouteConfigUpdateRequester {
+  public:
+    virtual ~RouteConfigUpdateRequester() = default;
+    virtual void requestRouteConfigUpdate(const HeaderString&, std::function<void()>) {


I think requestRouteConfigUpdate() will be PURE when the PR is merged

lambdai · 2019-10-23T03:31:13Z

source/common/router/rds_impl.cc

@@ -211,6 +222,37 @@ void RdsRouteConfigProviderImpl::onConfigUpdate() {
    prev_config->config_ = new_config;
    return previous;
  });
+
+  const auto aliases = config_update_info_->aliasesInLastVhdsUpdate();


There are way too many copies of aliases. IMHO this one should be shared_ptr and the below runOnAllThreads will capture the shared_ptr

a counter-point: I don't think it's ever going to be large number of aliases in the set at one time (I expect a couple at most). Only two copies are being made: one from the original protobuf, the other one is put in the lambda context. Do you think shared_ptr is still worth it considering small size of aliases being copied and the overhead of accessing a shared_ptr?

source/common/router/rds_impl.cc

source/common/http/conn_manager_impl.cc

lambdai · 2019-10-23T04:05:50Z

source/common/router/rds_impl.h

@@ -164,6 +169,15 @@ class RdsRouteConfigSubscription : Envoy::Config::SubscriptionCallbacks,

 using RdsRouteConfigSubscriptionSharedPtr = std::shared_ptr<RdsRouteConfigSubscription>;

+struct UpdateOnDemandCallback {
+  const std::set<std::string> aliases_;


The const give me the hint that the set should be a shared_ptr among all threads

source/common/router/rds_impl.h

source/common/router/route_config_update_receiver_impl.cc

stevenzzzz · 2019-10-23T13:28:23Z

@dmitri-d could you pls link the issue or the design doc ?

stevenzzzz

It'd be nice if we can split this PR into a control plane side PR + a data-plane side PR.
Please link a design doc or add some more doc about how on-demand rds works.
In my previous understanding, on-demand xDS is a xDS-horizontal framework change.

include/envoy/config/grpc_mux.h

include/envoy/router/rds.h

source/common/router/rds_impl.cc

source/extensions/filters/http/on_demand/on_demand_update.h

stevenzzzz · 2019-10-23T14:14:05Z

source/extensions/filters/http/on_demand/on_demand_update.cc

+}
+
+Http::FilterHeadersStatus OnDemandRouteUpdate::decodeHeaders(Http::HeaderMap&, bool) {
+  requestRouteConfigUpdate();


shall we also check if the host is already in the route-table and continue from there. IIUC If RDS request doesn't return, this essentially hangs processing of every requests?

The check is made inside the method: https://github.com/envoyproxy/envoy/pull/8617/files#diff-53e919b583d3290e6fddf324e7e69885R13

source/extensions/filters/http/on_demand/on_demand_update.cc

source/common/http/conn_manager_impl.h

source/common/http/conn_manager_impl.cc

stevenzzzz · 2019-10-23T14:49:36Z

source/common/router/rds_impl.cc

+                              aliases.end(), std::back_inserter(aliases_not_in_update));
+          if (aliases_not_in_update.empty()) {
+            callbacks.pop();
+            update_on_demand_callback.cb_();


I have some life-circle concern here, a stream may not live as long as a subscription, in which case this callback ends in unknown behavior.

Do you mean the gRPC stream carrying the subscription? If so, it is a safe assumption; the subscription is a shared owner of a GrpcMux, which is the owner of the GrpcStream.

no, I mean the per HTTP request ActiveStream.

I pause the filter chain until a response arrives and the callback resumes its execution. Wouldn't that mean that the ActiveConnection sticks around too?

No, I am saying a subscription may outlive an activeConnection. For example, with lambdai@'s recent change, a RDS subscription will outlive a listener. when that happens, the update_on_demand_callback.cb_() end in undefined behavior.

lambdai

Flush my thoughts

include/envoy/config/subscription.h

source/common/http/conn_manager_impl.h

lambdai · 2019-12-17T23:50:58Z

source/common/router/rds_impl.cc

+      ENVOY_LOG(
+          debug,
+          "rds: vhds configuration present/changed, (re)starting vhds: config_name={} hash={}",
+          route_config_name_, config_update_info_->configHash());
      maybeCreateInitManager(version_info, noop_init_manager, resume_rds);


A reminder of #9254

lambdai · 2020-01-08T22:34:40Z

source/common/router/route_config_update_receiver_impl.cc

  route_config_proto_ = rc;
  last_config_hash_ = new_hash;
+  const uint64_t new_vhds_config_hash = rc.has_vhds() ? MessageUtil::hash(rc.vhds()) : 0ul;
+  vhds_configuration_changed_ = new_vhds_config_hash != last_vhds_config_hash_;


Same as the other last_ series members. These members represents the ongoing update but is used by dump function. I am able to reasonable about the life time now

source/common/router/route_config_update_receiver_impl.cc

source/common/router/rds_impl.cc

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2020-01-09T00:24:52Z

added WatchMap::convertAliasWatchesToNameWatches tests

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

…started Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2020-01-09T19:06:46Z

ping @lambdai: fixed RouteConfigUpdatedCallback lifecycle issues that you pointed out

include/envoy/router/rds.h

docs/root/configuration/http/http_filters/on_demand_updates_filter.rst

dmitri-d · 2020-01-09T20:58:15Z

merged in latest changes

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

source/common/router/route_config_update_receiver_impl.cc

htuch · 2020-01-09T20:19:44Z

source/common/router/route_config_update_receiver_impl.cc

+void RouteConfigUpdateReceiverImpl::collectResourceIdsInUpdate(
+    const Protobuf::RepeatedPtrField<envoy::service::discovery::v3alpha::Resource>&
+        added_resources) {
+  resource_ids_in_last_update_.clear();


I'm not sure how actionable this is, but not a huge fan of building these mutable stateful objects, would have preferred to just deliver a LastUpdateInfo object along with the update to the receivers and then discarded. But, it's probably not a deal breaker.

Agreed, not optimal. There's a TODO to split the state here: https://github.com/envoyproxy/envoy/pull/8617/files/8e7b8f2f43bf00042d538501a7a076da76a42c6e#diff-dcbe1eb34bb1b29e349d7c87c0f20337R61

test/common/config/new_grpc_mux_impl_test.cc

htuch · 2020-01-09T21:10:30Z

LGTM modulo nits (and I'd like to verify the coverage report, I can't see it for some reason on CircleCI). Thanks for the fantastic work @dmitri-d!

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2020-01-09T21:19:28Z

responded to feedback from @htuch and @stevenzzzz.

source/common/router/rds_impl.cc

lambdai

The DS code path LGTM
I am not very familiar with the http filter operation.
Throw the ball to @htuch @stevenzzzz

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2020-01-09T23:48:39Z

fixed weird formatting in source/common/http/conn_manager_impl.h

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d · 2020-01-11T01:32:13Z

added on_demand filter tests

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch

Fantastic, this is really a huge contribution to xDS, our first on-demand API and a big enabler of scalability work. Thanks for all the work on review iterations as well.

Docs seem solid now.

dmitri-d · 2020-01-12T01:19:46Z

Thank you everyone for reviews and feedback!

dmitri-d requested review from alyssawilk, mattklein123 and snowp as code owners October 15, 2019 15:48

fredlas suggested changes Oct 15, 2019

View reviewed changes

source/common/http/conn_manager_impl.h Outdated Show resolved Hide resolved

source/common/config/delta_subscription_state.cc Outdated Show resolved Hide resolved

source/common/config/delta_subscription_state.cc Outdated Show resolved Hide resolved

jmarantz assigned fredlas Oct 16, 2019

dmitri-d closed this Oct 17, 2019

dmitri-d reopened this Oct 17, 2019

fredlas suggested changes Oct 17, 2019

View reviewed changes

mattklein123 mentioned this pull request Oct 19, 2019

Vhds readme #7415

Merged

mattklein123 previously requested changes Oct 19, 2019

View reviewed changes

htuch self-assigned this Oct 22, 2019

htuch suggested changes Oct 22, 2019

View reviewed changes

repokitteh-read-only bot added the waiting label Oct 22, 2019

htuch requested review from stevenzzzz and lambdai October 22, 2019 05:24

lambdai suggested changes Oct 23, 2019

View reviewed changes

stevenzzzz reviewed Oct 23, 2019

View reviewed changes

lambdai reviewed Jan 8, 2020

View reviewed changes

source/common/router/rds_impl.cc Outdated Show resolved Hide resolved

Added WatchMap::convertAliasWatchesToNameWatches tests

498f96d

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Dmitri Dolguikh added 3 commits January 9, 2020 10:53

Resolved two lifecycle-related issues aroun RouteConfigUpdatedCallback

eefb971

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Responded to feedback

8e7b8f2

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

Merge remote-tracking branch 'upstream/master' into vhds-on-demand-re…

75accac

…started Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

stevenzzzz reviewed Jan 9, 2020

View reviewed changes

include/envoy/router/rds.h Show resolved Hide resolved

docs/root/configuration/http/http_filters/on_demand_updates_filter.rst Show resolved Hide resolved

Responded to feedback

c6e346d

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch reviewed Jan 9, 2020

View reviewed changes

Reponded to feedback

bb5636a

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

lambdai reviewed Jan 9, 2020

View reviewed changes

source/common/router/rds_impl.cc Show resolved Hide resolved

lambdai previously approved these changes Jan 9, 2020

View reviewed changes

Fixed formatting

53293f8

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

dmitri-d dismissed lambdai’s stale review via 53293f8 January 9, 2020 23:47

Added on_demand filter tests

dd2d191

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

fixing formatting issues

927fd44

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>

htuch approved these changes Jan 11, 2020

View reviewed changes

repokitteh-read-only bot removed the api label Jan 11, 2020

htuch merged commit 8e2d909 into envoyproxy:master Jan 11, 2020

dmitri-d deleted the vhds-on-demand-restarted branch January 12, 2020 07:07

On-demand VHDS implementation #8617

On-demand VHDS implementation #8617

Conversation

dmitri-d commented Oct 15, 2019

dmitri-d commented Oct 15, 2019

mattklein123 commented Oct 15, 2019

fredlas left a comment

Choose a reason for hiding this comment

dmitri-d commented Oct 17, 2019

dmitri-d commented Oct 17, 2019

fredlas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredlas left a comment

Choose a reason for hiding this comment

dmitri-d commented Oct 19, 2019

mattklein123 left a comment

Choose a reason for hiding this comment

fredlas commented Oct 21, 2019

dmitri-d commented Oct 21, 2019

dmitri-d commented Oct 21, 2019

dmitri-d commented Oct 21, 2019

htuch left a comment • edited Loading

Choose a reason for hiding this comment

lambdai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lambdai left a 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

stevenzzzz commented Oct 23, 2019

stevenzzzz left a 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

lambdai left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Jan 9, 2020

dmitri-d commented Jan 9, 2020

dmitri-d commented Jan 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

htuch commented Jan 9, 2020

dmitri-d commented Jan 9, 2020

lambdai left a comment

Choose a reason for hiding this comment

dmitri-d commented Jan 9, 2020

dmitri-d commented Jan 11, 2020

htuch left a comment

Choose a reason for hiding this comment

dmitri-d commented Jan 12, 2020

htuch left a comment •

edited

Loading

lambdai left a comment •

edited

Loading