http: support resource removal for ecds by snowp · Pull Request #15368 · envoyproxy/envoy

snowp · 2021-03-08T19:23:52Z

This adds support for resource removal for ECDS, allowing it to work
with xDS TTLs. Upon expiry, the configuration is reset back to the null
state, which should result in Envoy reverting to the state it would find
itself in should the original xDS fetch time out.

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

Risk Level: Medium
Testing: Updated UTs
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a
Fixes #15336

This adds support for resource removal for ECDS, allowing it to work with xDS TTLs. Upon expiry, the configuration is reset back to the null state, which should result in Envoy reverting to the state it would find itself in should the original xDS fetch time out. Signed-off-by: Snow Pettersen <snowp@lyft.com>

lambdai · 2021-03-08T21:14:05Z

source/common/filter/http/filter_config_discovery_impl.cc

+void DynamicFilterConfigProviderImpl::onConfigRemoved(Config::ConfigAppliedCb cb) {
+  tls_.runOnAllThreads(
+      [cb](OptRef<ThreadLocalConfig> tls) {
+        tls->config_ = absl::nullopt;
+        if (cb) {
+          cb();
+        }
+      },
+      [this]() {
+        // This happens after all workers have discarded the previous config so it can be safely
+        // deleted on the main thread by an update with the new config.
+        this->current_config_ = absl::nullopt;
+      });
+}
+


I am not fully sure but I know some config has default config.
Maybe we should start with defining the exact remove semantic for such config?

CC @kyessenov

It should definitely revert back to the default config in that case. I had assumed that we fall back to when the provider returned empty, but it seems like it uses the standard update mechanism to fill in the default config, so seems like this PR needs some work

snowp · 2021-03-09T13:43:01Z

Going to wait for https://github.com/envoyproxy/envoy/pull/15371/files to land since there will be conflicts here

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

snowp · 2021-03-11T13:31:24Z

/retest

I think this can be reviewed now, I think the main change https://github.com/envoyproxy/envoy/pull/15371/files will require is clearing out all the other fields tracked from the last received response.

@kyessenov @htuch

repokitteh-read-only · 2021-03-11T13:31:28Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15368 (comment) was created by @snowp.

see: more, trace.

htuch · 2021-03-12T02:56:53Z

Would be great to have a first pass from @kyessenov if he has time.

kyessenov · 2021-03-12T19:15:40Z

source/common/filter/http/filter_config_discovery_impl.cc

+    }
+
+    last_config_hash_ = 0;
+  } else if (!added_resources.empty()) {


qq: Can resources be added and removed at the same time?

I think we'll never get both a removal and addition/update of the same resource and we only subscribe to one resource here so we shouldn't ever have to handle both at the same time. It would be nice to have this be better encoded in the API, but I'm not sure if there's a great way to handle this besides ASSERTs? cc @htuch

Yeah, I think the GrpcMux should ensure this property, but I can't say for certainty if it's true. We should make it part of the API IMHO, i.e. if the server does do a simultaneous add/remove, we don't expose any of the subscriptions to this. CC @dmitri-d @adisuissa

In the unified mux implementation there's a check for this: https://github.com/envoyproxy/envoy/pull/15473/files#diff-4228bc8f4213ff904d2a3584f69a2cf049180957f668d8bc0125d9bfa3379fa2R119. If failed, the DeltaDiscoveryResponse is discarded.

kyessenov · 2021-03-12T19:16:41Z

source/common/filter/http/filter_config_discovery_impl.h

  void onConfigUpdate(Envoy::Http::FilterFactoryCb config, const std::string&,
                      Config::ConfigAppliedCb cb) override;
+  void onConfigRemoved(Config::ConfigAppliedCb cb) override;
+  void setDefaultConfiguration(Envoy::Http::FilterFactoryCb config) override {


After the other PR, we could probably move this into the constructor?

kyessenov · 2021-03-12T19:22:14Z

test/integration/extension_discovery_integration_test.cc

+  {
+    // Reinstate the previous configuration.
+    sendXdsResponse("foo", "1", denyPrivateConfig(), true);
+    // Wait until the TTL removes the resource.


This comment sounds wrong.

kyessenov

Looks good to me, with some minor comments.

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

snowp · 2021-03-18T17:33:09Z

@kyessenov I merged in your change and I believe I've addressed the other comments, ptal

kyessenov · 2021-03-18T17:36:36Z

source/common/filter/http/filter_config_discovery_impl.cc


+namespace {
+void validateTypeUrlHelper(const std::string& type_url,
+                           absl::flat_hash_set<std::string> require_type_urls) {


nit: const flat_hash_set<string>&

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

htuch

Looks good! Just a few comments..
/wait

htuch · 2021-03-21T17:54:38Z

source/common/filter/http/filter_config_discovery_impl.cc

-        // This init target is used to activate the subscription but not wait
-        // for a response. It is used whenever a default config is provided to be
-        // used while waiting for a response.
+        // This init target is used to activate


Nit: Strange text reflow.

Can you fix this weird word wrap? LGTM and ready to ship once that is done, thanks!

Kept missing this for some reason! Will fix

source/common/filter/http/filter_config_discovery_impl.cc

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

htuch · 2021-03-23T01:38:21Z

source/common/filter/http/filter_config_discovery_impl.cc

+      std::make_shared<std::atomic<uint64_t>>(filter_config_providers.size());
+  for (auto* provider : filter_config_providers) {
+    update_cb(provider, [remaining_providers, done_cb] {
+      if (--(*remaining_providers) == 0) {


Even thought this int is atomic, can't this still race, e.g. imagine we have two threads A and B. They both see 2 remaining. A decrements, then B decrements, then A reads then B reads and they both see 0?

I think you need atomic compare/exchange, but even then, I rather we didn't review this kind of code more than once, so suggest factoring out to something in source/common/common, i.e. some sort of "apply to a container's elements in parallel and callback when done"..

Moved this to common/common and made it not thread safe since I changed it to be invoked as a completion callback for the runOnAllThreads call, which means it should get invoked on the main thread. I can see an argument for wanting to keep it thread safe just in case, but lmk what you think

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

htuch · 2021-03-25T03:25:15Z

source/common/common/containers.h

+
+/**
+ * Invokes a function for all elements in a container that accepts a completion callback which will
+ * be invoked asynchronously, invoking a final completion callback once all callbacks associated


.. and potentially cross thread? Or all on the calling thread..

htuch · 2021-03-25T03:30:08Z

source/common/common/containers.h

+
+  auto remaining_elements = std::make_shared<uint64_t>(container.size());
+  for (auto element : container) {
+    update_cb(element, [remaining_elements, done_cb] {


So, before we were doing N threads * M providers, but now we just do M providers. Presumably there was something hiding underneath the provider onConfigUpdate implying N concurrency. How does this now go away?

In general, this continuation passing style is really hard to read in C++ (I know it's common in some languages). @kyessenov is there a way to simplify the interfaces here to make them less hard to reason about in terms of concurrency?

Previously it would be decremented on the per thread callback, but I changed it to be done on the main thread completion callback instead. This eliminated the need to check the concurrency and also made the code no longer run on different threads, removing the need for atomics.

It might be possible to do something clever around RAII semantics and a shared pointer to avoid some of this complexity, let me try some stuff out.

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

might not actually simplify things as it adds some complexity around keeping a shared pointer around Signed-off-by: Snow Pettersen <snowp@lyft.com>

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

htuch

LGTM modulo comments, thanks for iterating!
/wait

htuch · 2021-03-29T21:56:13Z

source/common/common/containers.h

+ * of a container that may require asynchronous processing.
+ */
+template <class ContainerT, class UpdateCbT>
+void applyToAllWithCleanup(const ContainerT& container, UpdateCbT update_cb,


Can you document UpdateCbT expectation a bit more, or maybe add more structure around its type that isn't generic (since for example the cleanup arg is known)?

htuch · 2021-03-29T21:56:23Z

source/common/common/containers.h

+ * update callback to delay cleanup until some arbitrary time: the completion callback will be
+ * invoked once no more references to the provided shared pointer exists.
+ *
+ * This provides a thread safe way of tracking the completion of callbacks based on a the elements


Suggested change

* This provides a thread safe way of tracking the completion of callbacks based on a the elements

* This provides a thread safe way of tracking the completion of callbacks based on the elements

htuch · 2021-03-29T21:57:30Z

source/common/common/containers.h

+/**
+ * Invokes a function for all elements in a container, providing a cleanup function that will be
+ * executed after all the elements have been processed. The Cleanup object is provided to allow each
+ * update callback to delay cleanup until some arbitrary time: the completion callback will be


The docs on Cleanup probably belong inside the method. Probably can just talk about what done_cb does and the thread safety. It did require a couple of mins at staring at this to understand what is going on, but I think this is a pretty cool pattern.

I updated it a bit, lmk what you think. I think some mention of Cleanup is necessary since it's part of the function signature, but I moved most of it into the function body

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

htuch

LGTM, thanks!

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

snowp · 2021-04-02T17:06:39Z

Finally got CI green here, ptal

htuch

LGTM, thanks!

lambdai reviewed Mar 8, 2021

View reviewed changes

dio assigned lambdai Mar 9, 2021

add integration testing + default config handlign

3fa234a

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

snowp requested review from alyssawilk and mattklein123 as code owners March 9, 2021 17:56

Snow Pettersen added 5 commits March 9, 2021 19:04

use default for default integration test

0a0009c

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

Merge remote-tracking branch 'envoy/main' into ecds-ttl

1bd1bc7

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

ensure that we can reinstate the old config after ttl expiration

cf2296e

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

juggle types to reduce unimplemented functions

e074db2

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

fix test

bc8783d

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

htuch assigned kyessenov Mar 12, 2021

kyessenov reviewed Mar 12, 2021

View reviewed changes

Snow Pettersen added 4 commits March 17, 2021 17:30

Merge remote-tracking branch 'envoy/main' into ecds-ttl

7207f3a

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

format

bbfc9ac

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

fix tests + comments

c4f57fe

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

pass default configuration via ctor

2055a70

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

kyessenov reviewed Mar 18, 2021

View reviewed changes

const

d4ac9f0

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

snowp assigned htuch Mar 19, 2021

htuch suggested changes Mar 21, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 21, 2021

rework config_reload inc to not rely on concurrency

8a40eef

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

repokitteh-read-only bot removed the waiting label Mar 22, 2021

htuch reviewed Mar 23, 2021

View reviewed changes

Snow Pettersen added 4 commits March 23, 2021 19:13

extract helper to common/common

9a2cbb2

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

use shared pointer to avoid UB

51447bf

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

include memory

a2df2c1

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

tidy

fe386fc

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

htuch reviewed Mar 25, 2021

View reviewed changes

Snow Pettersen added 3 commits March 25, 2021 20:28

document lack of thread safety

e8af50d

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

attempt to simplify interface

a76358d

might not actually simplify things as it adds some complexity around keeping a shared pointer around Signed-off-by: Snow Pettersen <snowp@lyft.com>

format

2f231ee

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

htuch suggested changes Mar 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Mar 29, 2021

Snow Pettersen added 2 commits March 30, 2021 15:10

update comments

ed2c674

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

spelling

beec23f

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

repokitteh-read-only bot removed the waiting label Mar 30, 2021

Snow Pettersen added 2 commits March 31, 2021 15:04

fix text flow

ccb7653

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

Merge remote-tracking branch 'envoy/main' into ecds-ttl

8464866

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

htuch previously approved these changes Mar 31, 2021

View reviewed changes

Snow Pettersen added 2 commits March 31, 2021 22:15

Merge remote-tracking branch 'envoy/main' into ecds-ttl

bbc0102

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

add newline

050bc56

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

snowp dismissed htuch’s stale review via 050bc56 April 1, 2021 03:04

Snow Pettersen added 3 commits April 1, 2021 17:01

Merge remote-tracking branch 'envoy/main' into ecds-ttl

d858bb2

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

Merge remote-tracking branch 'envoy/main' into ecds-ttl

07cc744

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

fix bad merge

9420f67

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

htuch approved these changes Apr 2, 2021

View reviewed changes

htuch merged commit aea7de2 into envoyproxy:main Apr 2, 2021

	* This provides a thread safe way of tracking the completion of callbacks based on a the elements
	* This provides a thread safe way of tracking the completion of callbacks based on the elements

Conversation

snowp commented Mar 8, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp commented Mar 9, 2021

Uh oh!

snowp commented Mar 11, 2021

Uh oh!

repokitteh-read-only bot commented Mar 11, 2021

Uh oh!

htuch commented Mar 12, 2021

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!

kyessenov left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Mar 18, 2021

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!

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!

Uh oh!

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!

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!

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

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

snowp commented Apr 2, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment