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.

Sign up for GitHub

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

Jump to bottom

hds: add support for delta updates in specifier #13067

Merged
merged 57 commits into from
Oct 14, 2020
Merged

hds: add support for delta updates in specifier #13067

merged 57 commits into from
Oct 14, 2020

Conversation

drewsortega
Copy link
Contributor

@drewsortega drewsortega commented Sep 11, 2020 •
edited
Loading

Commit Message: hds: add support for delta updates in specifier
Additional Description:
Implement incremental comparison of changes to the HealthCheckSpecifier used to configure the health discovery service. Previously all HDS Clusters would be reconstructed even if the same HealthCheckSpecifier was received, or if there was only some small change. This is unusable for the use case of a timed HealthCheckSpecifier update, where the HealthCheckSpecifier is update more often than a health check. Since all health checks were previously being reconstructed on each call, their results were inconsistent.

This implementation allows for the re-usage of objects that do not change between calls, Specifically to an HdsCluster, to a Host/Endpoint, and to a Health Checker.
Risk Level: Low
Testing: All previous tests pass. Added unit tests that test for:

  • No changes in the HealthCheckSpecifier should not attempt to update.
  • Changes to only some of the overall HdsClusters should not reconstruct or update all of them.
  • Changes to some endpoints/hosts should not update or reconstruct all of them.
  • Changes to some health checkers should not update or reconstruct all of them.

Docs Changes: N/A
Release Notes: Included

@drewsortega
added hash check to specifier, started with cluster
3607b52 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
formatting fixes
abadb7e 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
start setup for incremental changes to hds clusters
3d58e70 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
Kick CI
0f55f8b 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
move cluster config building into its own function
abff735 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add init check, break add and start HCs
a69ec54 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
set up for updating healthcheckers and hosts per cluster
9ded2b3 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add comment pseudocode
082fd05 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
update TODO comments
f32ac32 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
hash endpoints and health check lists to stop not needed updates
20f069f 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
finish health_checkers_ incremental updates
d18e03d 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
remove cluster vector clear, use std::move instead
a339771 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
finish first implementation of dynamic endpoint/host updates
28bc269 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
make maps not pointers, improve host map key
1d2d07a 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
remove return from updateHdsCluster and rename function
a456e75 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
unit test: repeated specifier
89d7043 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
Merge branch 'master' into hds_partial_updates
635c220 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
unit test: change around Clusters, add/remove with reuse
7993f9f 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
formatting fixes
cdc019e 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
fix: use correct cluster name map, hash correct endpoints list
0f2fafc 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
priority_set_ bug fix, pass endpoints test
37a9bdf 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
fix hcs using wrong hash, finish health checker update unit test
63d8aa7 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add better Endpoint hashing for map
c66236a 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
formatting fix
14011cd 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add more code comments for clarity
9a04a72 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
Copy link
Contributor Author

@htuch

@drewsortega
add alternative update function for PrioritySetImpl
2bd69ea 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add missing proto dependency for locality_endpoint
6d2440c 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
fix missing file content
2108707 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
intg test: remove cluster, add endpoint, reuse endpoint
79200e7 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
remove complexity of updating info_ object
c6b69fa 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
minor comment changes
187d103 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
test optimizations
e39cc33 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
comment fixes.
470be7b 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
Copy link
Contributor Author

FYI, new integration test was ran with flakiness verification, running 1000 times. No failures.

@drewsortega
Merge branch 'master' into hds_partial_updates
45b2340 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
style fixes, add TODO comment.
b6bc0ed 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@drewsortega
add more TODO comments.
0430665 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
@stale
Copy link

stale bot commented Oct 4, 2020

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Oct 4, 2020
@htuch
Copy link
Member

htuch commented Oct 5, 2020

@drewsortega huh, I think this was ready to merge when you left, not sure why I didn't merge it. Could you merge master?

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Oct 5, 2020
@drewsortega
Copy link
Contributor Author

Sure, I can get master merged ASAP.

@htuch
Copy link
Member

htuch commented Oct 12, 2020

@drewsortega any luck? :) I'm happy to do a new PR and do the fixups, but can also wait a bit, up to you.

@drewsortega
Merge remote-tracking branch 'upstream/master' into hds_partial_updates
50bca5d 
Signed-off-by: Drew S. Ortega <DrewSOrtega@pm.me>
@drewsortega
remove random from new unit test
380c1ca 
Signed-off-by: Drew S. Ortega <DrewSOrtega@pm.me>
@drewsortega
Copy link
Contributor Author

@htuch apologies for the delay, had to clean up some changes due to 'random' being removed. I've committed the completed merge, I'll keep watch for any odd CI failures if they happen.

@drewsortega
fix version history merge problems
b39a2f6 
Signed-off-by: Drew S. Ortega <DrewSOrtega@pm.me>
@drewsortega
apply format fix patch from CI
ddef6e5 
Signed-off-by: Drew S. Ortega <DrewSOrtega@pm.me>
@drewsortega
fix version history alphabetical order
e0fb96a 
Signed-off-by: Drew S. Ortega <DrewSOrtega@pm.me>
@drewsortega drewsortega requested a review from htuch October 14, 2020 20:05
@drewsortega
Copy link
Contributor Author

@htuch Merge seems to have gone well, I'll pass this one back to you!

htuch
htuch approved these changes Oct 14, 2020
View reviewed changes
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks! Appreciate all the work you did over the summer on this and the followup.

@htuch htuch merged commit 1ca5312 into envoyproxy:master Oct 14, 2020
@xdzhai
Copy link
Contributor

xdzhai commented Oct 14, 2020

Thank you both for getting it done!

mpuncel added a commit to mpuncel/envoy that referenced this pull request Oct 15, 2020
@mpuncel
Merge branch 'master' into mpuncel/secret-hc-sequence
76014af 
* master:
  ci: use multiple stage (envoyproxy#13557)
  tls: update BoringSSL to 2192bbc8 (4240). (envoyproxy#13567)
  fix macos v8 build (envoyproxy#13572)
  Fixed Health Check Fuzz corpus syntax (envoyproxy#13576)
  ci: Remove shellcheck diff (envoyproxy#13560)
  ci: Increate brew retry interval (envoyproxy#13565)
  dependencies: fix some of the fallout from Wasm merge. (envoyproxy#13569)
  hds: add support for delta updates in specifier (envoyproxy#13067)
  ci: workaround for actions/runner-images#1811 (envoyproxy#13577)
  ratelimit: be able to disable x-envoy-ratelimited response header sent (envoyproxy#13270)
  Update opencensus library (envoyproxy#13549)
  ci: use azp for api and go-control-plane sync (envoyproxy#13550)
  docs: Remove/make generic lyft references in docs (envoyproxy#13559)
  check_format: adding 2 more release note checks (envoyproxy#13444)
  [Wasm] Add cluster metadata fallback and upstream host metadata (envoyproxy#13477)
  [fuzz] Added validation for secrets (envoyproxy#13543)
  Add Platform Specific Feature guidance to PR template (envoyproxy#13547)

Signed-off-by: Michael Puncel <mpuncel@squareup.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xdzhai xdzhai xdzhai left review comments

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@drewsortega @htuch @xdzhai