cluster: unstuck cluster manager when update the initializing cluster by lambdai · Pull Request #13875 · envoyproxy/envoy

lambdai · 2020-11-03T05:58:59Z

Commit Message:
Remove existing initializing cluster when updating secondary clusters.

Return existing cluster in ClusterManagerImp::loadCluster() to delay
the destroy of existing cluster.
Remove the existing cluster from the secondary cluster list to accounting
the warming secondary cluster correctly.

Additional Description:
Risk Level: LOW
Testing: integration test
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
fix #13874

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-11-03T08:30:36Z

The new test AdsClusterV3Test.ClusterUpdateWhenWarming is failing on CI but I cannot reproduce locally

Is it another #11877 ? It seems CDS and EDS update on Envoy are concurrent executing

mattklein123 · 2020-11-03T20:26:22Z

The PR that is being reverted is causing production issues, so let's land that revert, and then work on re-applying with better tests? cc @Shikugawa

/wait

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-11-03T22:46:25Z

linux_x64 release test passed!

mattklein123 · 2020-11-03T23:50:33Z

Sorry is this still valid after reverting the other PR? Or should I wait until that issue is resolved?

/wait-any

lambdai · 2020-11-04T00:09:33Z

@mattklein123
There are two PR
#12783 and #13344

My fix is based on #12783 which is not revered.

BTW: I just realized #13344 is the only one reverted but I suspect the problem is introduced in #12783

lambdai · 2020-11-04T00:12:07Z

@mattklein123
There are two PR
#12783 and #13344

My fix is based on #12783 which is not revered.

BTW: I just realized #13344 is the only one reverted but I suspect the problem is introduced in #12783

Because the major risky change in #13344 is protected by the default off runtime key.

See https://github.com/envoyproxy/envoy/pull/13886/files#diff-c904f9b0d49cdd938ffaa952192372529415afa8602d7dc6acb904e61111d8a5L433-L438

lizan · 2020-11-04T00:35:12Z

Based on my conversation with @rgs1 the crash that they are seeing is likely due to #13344.

rgs1 · 2020-11-04T00:41:38Z

I can try this one tomorrow, we should spot the problem immediately.

rgs1 · 2020-11-04T00:42:04Z

I can try this one tomorrow, we should spot the problem immediately.

That is, on top of master with #13344 already reverted.

rgs1 · 2020-11-04T00:42:23Z

I can try this one tomorrow, we should spot the problem immediately.

That is, on top of master with #13344 already reverted.

Oh nvm, this is not for the crash.

source/common/upstream/cluster_manager_impl.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks in general this makes sense to me with a few small comments.

/wait

source/common/upstream/cluster_manager_impl.cc

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

…erfix Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Thanks LGTM pending potentially a better integration test for the primary case. Can you also merge main to pick up the CI fix?

/wait

mattklein123 · 2020-11-05T16:11:57Z

test/common/upstream/cluster_manager_impl_test.cc

+  // The override cluster is added. Manually drop the previous cluster. In production flow this is
+  // achieved by ClusterManagerImpl.
+  sds.reset();


Is it possible to add a cluster manager test of this case that uses "real" DNS clusters? I think the main sequence that can actually happen here is:

CDS adds DNS cluster

DNS cluster begins init

CDS updates DNS cluster?

I think you could also pretty easily test this in your integration test below by doing:

Have CDS deliver a DNS cluster configured with health checking.

DNS cluster won't initialize pending health checking.

Update DNS cluster

?

Thank you for the DNS with health check!

After a quick search I didn't find an existing example but this test case
TEST_F(ClusterManagerImplTest, RemoveWarmingCluster) seem putting a defaultStaticCluster("fake_cluster") in warm.

Writing a test

A slight difference is that DNS cluster is always depending on resolver.

I modified the cluster to STATIC type to make it immediate ready.

Thanks this all looks good, but can you add a real integration test for this in ADS integration test? I think it's really easy based on what you already have. Just use a static cluster with health checking, it will be stuck initializing, then you can update it?

/wait

I had the impression that static cluster with bad health check doesn't block initialization...
Yeah, adding integrate test to confirm initialization and the behavior health check behavior. Will udpate

Added.

Also confirmed that failed health check in static cluster also turn the warm cluster to ready.
And STRICT DNS resolve failure has the same scary side effect.

I end up not accept the tcp connection but not write back to hold the DNS request.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

test/common/grpc/grpc_client_integration.h

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai · 2020-11-07T00:05:26Z

test/integration/base_integration_test.cc

  }
 }

+AssertionResult compareSets(const std::set<std::string>& set1, const std::set<std::string>& set2,


Move this function up so that more functions defined in this file can call it.

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123

Awesome, thank you!

…envoyproxy#13875) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

* cluster manager: avoid immediate activation for dynamic inserted cluster when initialize (envoyproxy#12783) Signed-off-by: Shikugawa <rei@tetrate.io> Signed-off-by: Yuchen Dai <silentdai@gmail.com> * cluster: unstuck cluster manager when update the initializing cluster (envoyproxy#13875) Signed-off-by: Yuchen Dai <silentdai@gmail.com> Co-authored-by: Rei Shimizu <rei@tetrate.io>

lambdai added 2 commits November 3, 2020 05:42

dup counter

c867cb1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

decrease warming cluster counter

9d3849a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai changed the title ~~Warmingclusterfix~~ cluster: unstuck cluster manager when update the initializing cluster Nov 3, 2020

lambdai mentioned this pull request Nov 3, 2020

Cluster initialization issue. Envoy start could be blocked forever #13874

Closed

fix test

77c85c1

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

ggreenway assigned mattklein123 Nov 3, 2020

lizan self-assigned this Nov 3, 2020

repokitteh-read-only bot added the waiting label Nov 3, 2020

fix cluster warming and last warming

4dbde08

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 3, 2020

remove debug stub

c04f9da

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot added the waiting:any label Nov 3, 2020

repokitteh-read-only bot removed the waiting:any label Nov 4, 2020

rgs1 reviewed Nov 4, 2020

View reviewed changes

source/common/upstream/cluster_manager_impl.cc Outdated Show resolved Hide resolved

const

8b1f033

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Nov 4, 2020

Update proxy sha istio/istio#28500

Merged

mattklein123 requested changes Nov 4, 2020

View reviewed changes

source/common/upstream/cluster_manager_impl.cc Show resolved Hide resolved

source/common/upstream/cluster_manager_impl.cc Outdated Show resolved Hide resolved

source/common/upstream/cluster_manager_impl.cc Outdated Show resolved Hide resolved

repokitteh-read-only bot added the waiting label Nov 4, 2020

lambdai added 2 commits November 4, 2020 19:02

const

533fb44

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'me/warmingclusterfix' into warmingclust…

07a8ce0

…erfix Signed-off-by: Yuchen Dai <silentdai@gmail.com>

primary cluster dedup

07250bb

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 4, 2020

lambdai added 4 commits November 4, 2020 21:55

integration test cover primary cluster

125ea72

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

unit test for primary warming cluster

7dd7bf5

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

typo

395e525

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

allow apt-get update failure in azure pipeline per lizan

fa02576

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 requested changes Nov 5, 2020

View reviewed changes

repokitteh-read-only bot added the waiting label Nov 5, 2020

lambdai added 3 commits November 5, 2020 21:19

adding test

b2965c3

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

Merge remote-tracking branch 'origin/master' into warmingclusterfix

74e9a0d

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

test complete

7fb0b15

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 5, 2020

fix singlton check by executing VerifyAndClearExpectations, why?

43bb75d

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Nov 6, 2020

istio-proxy v1.8.0-alpha2 crashing on aggregate cluster istio/istio#28620

Closed

repokitteh-read-only bot added the waiting label Nov 6, 2020

lizan reviewed Nov 6, 2020

View reviewed changes

test/common/grpc/grpc_client_integration.h Outdated Show resolved Hide resolved

add integration test

7f991cd

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

repokitteh-read-only bot removed the waiting label Nov 7, 2020

revert grpc client integration

278b80a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai commented Nov 7, 2020

View reviewed changes

replace dns by tcp health check

9927f6a

Signed-off-by: Yuchen Dai <silentdai@gmail.com>

mattklein123 approved these changes Nov 9, 2020

View reviewed changes

mattklein123 merged commit 7a15e5d into envoyproxy:master Nov 9, 2020

lambdai deleted the warmingclusterfix branch November 9, 2020 20:25

lambdai added a commit to lambdai/envoy-dai that referenced this pull request Nov 10, 2020

cluster: unstuck cluster manager when update the initializing cluster (…

f5697ea

…envoyproxy#13875) Signed-off-by: Yuchen Dai <silentdai@gmail.com>

lambdai mentioned this pull request Nov 10, 2020

[1.8] fix cluster early warm up istio/envoy#285

Merged

Conversation

lambdai commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lambdai commented Nov 3, 2020

Uh oh!

mattklein123 commented Nov 3, 2020

Uh oh!

lambdai commented Nov 3, 2020

Uh oh!

mattklein123 commented Nov 3, 2020

Uh oh!

lambdai commented Nov 4, 2020

Uh oh!

lambdai commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Nov 4, 2020

Uh oh!

rgs1 commented Nov 4, 2020

Uh oh!

rgs1 commented Nov 4, 2020

Uh oh!

rgs1 commented Nov 4, 2020

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 5, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 6, 2020

Choose a reason for hiding this comment

Uh oh!

lambdai Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lambdai Nov 7, 2020

Choose a reason for hiding this comment

Uh oh!

mattklein123 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

lambdai commented Nov 3, 2020 •

edited

Loading

lambdai commented Nov 4, 2020 •

edited

Loading