upstream: update host's socket factory when metadata is updated. by cpakulski · Pull Request #16708 · envoyproxy/envoy

cpakulski · 2021-05-27T21:23:00Z

Commit Message:
Update host's socket factory when metadata is updated.

Additional Description:
During dynamic endpoint updates via EDS, BaseDynamicClusterImpl::updateDynamicHostList method
tries to minimize deletions/insertions. If only metadata was changed, endpoint is updated in-place by calling HostDescriptionImpl::metadata, which updates metadata but not fields dependent on metadata. In particular,
if metadata's transport socket matcher was changed, socket_factory was not updated.
Risk Level: Low
Testing: added unit test
Docs Changes: no
Release Notes: no
Platform Specific Features:
Fixes #16536

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2021-05-28T01:53:23Z

/retest

repokitteh-read-only · 2021-05-28T01:53:27Z

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

🐱

Caused by: a #16708 (comment) was created by @cpakulski.

see: more, trace.

lambdai · 2021-06-01T17:38:22Z

source/common/upstream/upstream_impl.h

  void metadata(MetadataConstSharedPtr new_metadata) override {
    absl::WriterMutexLock lock(&metadata_mutex_);
    metadata_ = new_metadata;
+    // Update data members dependent on metadata.


Could resolveTransportSocketFactory be moved out of the lock guard?
The referenced Metadata should be immutable and valid.

@lambdai I can add a scope around the lock to unlock it before calling resolveTransportSocketFactory. Why do you think it is necessary? Deadlock?

Sorry for the late reply.
It's not necessary but nice to have.
The only reason is performance. resolveTransportSocketFactory doesn't need the lock to protect

Take it or leave it. It's probably not a bottleneck.

auto factory = resolveTransportSocketFactory(address_, new_metadata.get());
{
absl::WriterMutexLock lock(&metadata_mutex_);
metadata_ = new_metadata;
socket_factory_ = factory;
}

Updated as you suggested. There is small performance improvement to resolve factory outside of mutex protected scope. Thanks!

…tected scope. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2021-06-16T20:04:59Z

/retest

repokitteh-read-only · 2021-06-16T20:05:03Z

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

🐱

Caused by: a #16708 (comment) was created by @cpakulski.

see: more, trace.

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2021-06-17T02:32:29Z

/retest

repokitteh-read-only · 2021-06-17T02:32:33Z

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

🐱

Caused by: a #16708 (comment) was created by @cpakulski.

see: more, trace.

lizan · 2021-06-18T21:48:35Z

source/common/upstream/upstream_impl.h

  HealthCheckHostMonitorPtr health_checker_;
  std::atomic<uint32_t> priority_;
-  Network::TransportSocketFactory& socket_factory_;
+  std::reference_wrapper<Network::TransportSocketFactory> socket_factory_;


This needs a ABSL_GUARDED_BY annotation as well? Should this use same mutex as metadata?

Annotation added .
Yeah, after checking the code layout I believe that it is possible that metadata update and creating a new upstream connection (which needs access to transport socket factory) to happen at the same time on different threads. To protect against race conditions, a mutex around socket_factory_ has been added.

It is possible that metadata_ is updated at the same time when socket_factory_ is used to create a new upstream connection. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski · 2021-06-21T23:47:05Z

/retest

repokitteh-read-only · 2021-06-21T23:47:08Z

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

🐱

Caused by: a #16708 (comment) was created by @cpakulski.

see: more, trace.

lizan

/assign-from @envoyproxy/maintainers
for non-tetrands review

repokitteh-read-only · 2021-06-28T17:28:23Z

@envoyproxy/maintainers assignee is @lizan

🐱

Caused by: a #16708 (review) was submitted by @lizan.

see: more, trace.

lizan · 2021-06-28T17:28:35Z

/assign-from @envoyproxy/maintainers

repokitteh-read-only · 2021-06-28T17:28:39Z

@envoyproxy/maintainers assignee is @lizan

🐱

Caused by: a #16708 (comment) was created by @lizan.

see: more, trace.

lizan · 2021-06-28T17:29:14Z

/assign-from @envoyproxy/maintainers

repokitteh-read-only · 2021-06-28T17:29:17Z

@envoyproxy/maintainers assignee is @jmarantz

🐱

Caused by: a #16708 (comment) was created by @lizan.

see: more, trace.

jmarantz

Some superficial comments here. I think someone that knows the semantics of this corner of the code also needs to review.

test/common/upstream/eds_test.cc

jmarantz · 2021-06-29T21:35:25Z

source/common/upstream/upstream_impl.h

+      metadata_ = new_metadata;
+    }
+    {
+      absl::WriterMutexLock lock(&socket_factory_mutex_);


is it semantically sound to have a separate lock for the socket factory? If there's a thread wakeup between these two locks will there be an inconsistent view of the data?

Simplified it and one mutex is used to protect metadata_ and fields dependent on it - in this case socket_factory_.

jmarantz · 2021-06-29T21:35:52Z

/wait

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

jmarantz

Thanks, looks much better.

This was pre-existing but most people I know including myself have seen worse performance from reader/writer locks vs just plain locks. This was pre-existing though.

cpakulski · 2021-07-01T13:27:53Z

@jmarantz Thanks for reviewing and explaining issue with locks.

jmarantz

@lizan do you want to to do a final senior maintainer pass?

* main: listener: match rebalancer to listener IP family type (envoyproxy#16914) jwt_authn: implementation of www-authenticate header (envoyproxy#16216) listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227) Small typo fix (envoyproxy#17247) Doc: Clarify request/response attributes are http-only (envoyproxy#17204) bazel/README.md: add aspell comment (envoyproxy#17072) docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225) remove the wrong comment on test (envoyproxy#17233) upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179) JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752) remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968) metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127) deps: update cel-cpp to 0.6.1 (envoyproxy#16293) Add ability to filter ConfigDump. (envoyproxy#16774) examples: fix Wasm example. (envoyproxy#17218) upstream: update host's socket factory when metadata is updated. (envoyproxy#16708) Signed-off-by: Garrett Bourg <bourg@squareup.com>

…oyproxy#16708) During dynamic endpoint updates via EDS, `BaseDynamicClusterImpl::updateDynamicHostList` method tries to minimize deletions/insertions. If only metadata was changed, endpoint is updated in-place by calling `HostDescriptionImpl::metadata`, which updates metadata but not fields dependent on metadata. In particular, if metadata's transport socket matcher was changed, socket_factory was not updated. Risk Level: Low Testing: added unit test Docs Changes: no Release Notes: no Platform Specific Features: Fixes envoyproxy#16536 Signed-off-by: Christoph Pakulski <christoph@tetrate.io> Signed-off-by: chris.xin <xinchuantao@qq.com>

…oyproxy#16708) During dynamic endpoint updates via EDS, `BaseDynamicClusterImpl::updateDynamicHostList` method tries to minimize deletions/insertions. If only metadata was changed, endpoint is updated in-place by calling `HostDescriptionImpl::metadata`, which updates metadata but not fields dependent on metadata. In particular, if metadata's transport socket matcher was changed, socket_factory was not updated. Risk Level: Low Testing: added unit test Docs Changes: no Release Notes: no Platform Specific Features: Fixes envoyproxy#16536 Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski added 3 commits May 27, 2021 21:09

Update host's socket factory when metadata is updated.

9074af6

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Added comment to test file.

5d58b92

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into issue/16536

1dabea0

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

phlax assigned lizan May 28, 2021

cpakulski marked this pull request as ready for review May 28, 2021 13:02

lizan mentioned this pull request Jun 1, 2021

transport_socket_matches doesn't update properly when endpoint metadata updates #16536

Closed

lambdai reviewed Jun 1, 2021

View reviewed changes

zuercher assigned lambdai Jun 7, 2021

Moved initialization of transport socket factory outside of mutex pro…

27cbdd6

…tected scope. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

Merge remote-tracking branch 'upstream/main' into issue/16536

a37c7d0

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

lizan reviewed Jun 18, 2021

View reviewed changes

Added mutex_ to protect update to HostDescription::socket_factory_.

79b4401

It is possible that metadata_ is updated at the same time when socket_factory_ is used to create a new upstream connection. Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski requested a review from lizan June 22, 2021 00:29

lizan previously approved these changes Jun 28, 2021

View reviewed changes

lizan removed their assignment Jun 28, 2021

repokitteh-read-only bot assigned jmarantz Jun 28, 2021

jmarantz reviewed Jun 29, 2021

View reviewed changes

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

Use single mutex for Host's metadata and socket_factory.

85c52c8

Signed-off-by: Christoph Pakulski <christoph@tetrate.io>

cpakulski dismissed lizan’s stale review via 85c52c8 June 30, 2021 21:53

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

cpakulski requested a review from jmarantz July 1, 2021 02:08

jmarantz approved these changes Jul 1, 2021

View reviewed changes

jmarantz assigned lizan Jul 1, 2021

jmarantz reviewed Jul 1, 2021

View reviewed changes

lizan approved these changes Jul 1, 2021

View reviewed changes

lizan merged commit 329b249 into envoyproxy:main Jul 1, 2021

Conversation

cpakulski commented May 27, 2021

Uh oh!

cpakulski commented May 28, 2021

Uh oh!

repokitteh-read-only bot commented May 28, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lambdai Jun 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

cpakulski commented Jun 16, 2021

Uh oh!

repokitteh-read-only bot commented Jun 16, 2021

Uh oh!

cpakulski commented Jun 17, 2021

Uh oh!

repokitteh-read-only bot commented Jun 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Jun 21, 2021

Uh oh!

repokitteh-read-only bot commented Jun 21, 2021

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jun 28, 2021

Uh oh!

lizan commented Jun 28, 2021

Uh oh!

repokitteh-read-only bot commented Jun 28, 2021

Uh oh!

lizan commented Jun 28, 2021

Uh oh!

repokitteh-read-only bot commented Jun 28, 2021

Uh oh!

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

jmarantz commented Jun 29, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

cpakulski commented Jul 1, 2021

Uh oh!

jmarantz left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lambdai Jun 15, 2021 •

edited

Loading