cluster manager: use clusters() instead of get() for main thread cluster validation#14204
Conversation
This is a follow up to #13906. It replaces use of the thread local clusters with the main thread clusters() output for static route validation. This will enable further cleanups in the cluster manager code. Signed-off-by: Matt Klein <mklein@lyft.com>
|
This is not fully correct. Doing some more invasive work here to clean more stuff up. /wait |
Signed-off-by: Matt Klein <mklein@lyft.com>
…ead_local_clusters Signed-off-by: Matt Klein <mklein@lyft.com>
|
This is ready for review. It's mostly mechanical. I can split out the config_impl changes as that is the only real functional change if reviewers prefer. |
…ead_local_clusters Signed-off-by: Matt Klein <mklein@lyft.com>
|
/retest for OSX. I don't think clang-tidy is going to pass on this one. |
|
Retrying Azure Pipelines: |
snowp
left a comment
There was a problem hiding this comment.
LGTM!
Very happy to see get() renamed, it's definitely been a bit of a gotcha.
|
Was ClusterManager::clusters() always unsafe to call on any but the main thread? If not, this seems like a breaking API change that could have caused silent failures. |
Yes. This probably should have been better documented and ASSERTed. |
due to envoyproxy#14204, doing static cluster checks for AsyncClientFactory can only be done on the main thread. adds a ENVOY_BUG check to ensure that cluster checks are skipped when not running on the main thread. Signed-off-by: Michael Wozniak <wozz@koh.ms> Signed-off-by: wozz <wozz@users.noreply.github.com>
Due to #14204, doing static cluster checks for AsyncClientFactory can only be done on the main thread. Signed-off-by: Michael Wozniak <wozz@koh.ms>
Due to envoyproxy/envoy#14204, doing static cluster checks for AsyncClientFactory can only be done on the main thread. Signed-off-by: Michael Wozniak <wozz@koh.ms>
This is a follow up to #13906. It does a few different things:
validation as well as some other places that were using thread local clusters when they should have
been using main thread clusters during config ingestion.
have to explicitly opt in to what clusters they want. This makes tests more intentional and less magical,
but required a bunch of manual fixes. Overall this reduces quite a lot of test debt when using mocks.
This will enable further cleanups in the cluster manager
code.
Risk Level: Low
Testing: Existing tests
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A