Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CDS: remove warming cluster if CDS response desired #13997

Merged
merged 8 commits into from
Nov 13, 2020

Conversation

lambdai
Copy link
Contributor

@lambdai lambdai commented Nov 12, 2020

Commit Message:
Remove both the active cluster and warming cluster if CDS response told so.
Fixed both SotW and Delta.

Additional Description:
Risk Level: Mid since the bad behavior exist for long time.
Testing: Fixed unit tests and integration test. Added new integration test.
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #13994
[Optional Deprecated:]

@lambdai lambdai requested a review from htuch November 12, 2020 08:57
Signed-off-by: Yuchen Dai <[email protected]>
@mattklein123 mattklein123 self-assigned this Nov 12, 2020
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1",
{"warming_cluster_1"}, {"warming_cluster_1"}, {"cluster_0"}));

// Send the second warming cluster and remove the first cluster.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the core of this test case.

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks this makes sense to me, but I would kindly request a slightly more involved CM API change to accomplish this that will get the code into a better state. Thank you!

/wait

include/envoy/upstream/cluster_manager.h Outdated Show resolved Hide resolved
@lambdai
Copy link
Contributor Author

lambdai commented Nov 13, 2020

merged master to resolve conflict

Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks LGTM with small comment.

/wait

source/common/upstream/cds_api_impl.cc Outdated Show resolved Hide resolved
source/server/admin/clusters_handler.cc Show resolved Hide resolved
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mattklein123 mattklein123 merged commit e9d85da into envoyproxy:master Nov 13, 2020
mpuncel added a commit to mpuncel/envoy that referenced this pull request Nov 18, 2020
* master: (117 commits)
  vrp: allow supervisord to open its log file (envoyproxy#14066)
  [http1] fix H/1 response pipelining (envoyproxy#13983)
  wasm: make dependency clearer (envoyproxy#14062)
  docs: updating 100-continue docs (envoyproxy#14040)
  quiche: fix stream trailer decoding issue (envoyproxy#13871)
  tidy: use last_github_commit script instead of target branch (envoyproxy#14052)
  stats: use RE2 and a better pattern to accelerate a single stats tag-extraction RE (envoyproxy#8831)
  wasm: use static registration for runtimes (envoyproxy#14014)
  grpc-json-transcoder: Add support for configuring unescaping behavior (envoyproxy#14009)
  ci: fix CodeQL-build by removing deprecated set-env command (envoyproxy#14046)
  config: fix crash when type URL doesn't match proto. (envoyproxy#14031)
  Build: Propagate user-supplied tags to external headers library. (envoyproxy#14016)
  [test host utils] use make_shared to avoid memory leaks (envoyproxy#14042)
  jwt_authn: update to jwt_verify_lib with 1 minute clock skew (envoyproxy#13872)
  quiche: update QUICHE tar (envoyproxy#13949)
  sds: improve watched directory documentation. (envoyproxy#14029)
  log the internal error message from *SSL when the cert and private key doesn't match (envoyproxy#14023)
  wasm: fix CPE for Wasmtime. (envoyproxy#14024)
  docs: Bump sphinxext-rediraffe version (envoyproxy#13996)
  CDS: remove warming cluster if CDS response desired (envoyproxy#13997)
  ...
qqustc pushed a commit to qqustc/envoy that referenced this pull request Nov 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

warming cluster is not removed at new CDS response
2 participants