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

use a separate snapshot cache for EDS #6250

Merged
merged 2 commits into from
Mar 7, 2024

Conversation

skriss
Copy link
Member

@skriss skriss commented Mar 6, 2024

Uses a separate go-control-plane snapshot
cache for EDS. This allows Endpoints updates
to be independent of other config updates,
minimizing the amount of unnecessary xDS
traffic that is generated. This is more consistent
with the legacy Contour xDS server's implementation.

Note, this PR sets the envoy/go-control-plane xDS server as the default for testing purposes, but that change will be reverted before merge.

@skriss skriss requested a review from a team as a code owner March 6, 2024 21:36
@skriss skriss requested review from tsaarni and sunjayBhatia and removed request for a team March 6, 2024 21:36
@sunjayBhatia sunjayBhatia requested review from a team, izturn and clayton-gonsalves and removed request for a team March 6, 2024 21:36
@skriss skriss added the release-note/small A small change that needs one line of explanation in the release notes. label Mar 6, 2024
Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Attention: Patch coverage is 8.92857% with 51 lines in your changes are missing coverage. Please review.

Project coverage is 81.19%. Comparing base (31ad9ba) to head (4326bd6).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6250      +/-   ##
==========================================
- Coverage   81.33%   81.19%   -0.15%     
==========================================
  Files         133      133              
  Lines       15775    15803      +28     
==========================================
  Hits        12831    12831              
- Misses       2650     2678      +28     
  Partials      294      294              
Files Coverage Δ
internal/protobuf/helpers.go 91.66% <100.00%> (-0.34%) ⬇️
cmd/contour/serve.go 22.66% <0.00%> (+0.13%) ⬆️
internal/xdscache/v3/endpointslicetranslator.go 79.12% <0.00%> (-0.29%) ⬇️
internal/xdscache/v3/endpointstranslator.go 86.89% <0.00%> (-0.86%) ⬇️
internal/xdscache/v3/snapshot.go 0.00% <0.00%> (ø)

log logrus.FieldLogger
resources map[envoy_resource_v3.Type]xdscache.ResourceCache
defaultCache envoy_cache_v3.SnapshotCache
edsCache envoy_cache_v3.SnapshotCache
Copy link
Member Author

@skriss skriss Mar 6, 2024

Choose a reason for hiding this comment

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

I switched to a SnapshotCache here instead of a LinearCache for EDS. It turns out the LinearCache doesn't behave as desired for us on control plane restarts. If Envoy already has config for a given resource at a particular version, then on a control plane restart, the version number of the resource in the cache will be reset to 1 (or close to 1), therefore will not be sent to Envoy since Envoy already has a "later" version of the resource. It will only be sent once the resource changes. However, this means that any changes that happened during the time of the restart may be ignored for a period of time.

With a SnapshotCache, any time the cached version differs from the last version Envoy saw, the resource will be re-sent, which better fits our use case given that versions are not durable across control plane restarts.

Copy link
Member

@sunjayBhatia sunjayBhatia left a comment

Choose a reason for hiding this comment

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

ah nice, yeah the LinearCache looks like it's version logic is not compatible with what we want, similar to why we switched from using a number version in this xDS server implementation (so that version 0 wouldn't become an issue on startup)

@skriss
Copy link
Member Author

skriss commented Mar 7, 2024

Running CI a couple more times, then will back out the change to the default xDS server for merge

- Triggers only EDS updates when endpoints change
- Does not trigger EDS updates when only non-endpoints change

Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
@skriss skriss merged commit a740314 into projectcontour:main Mar 7, 2024
25 of 26 checks passed
@skriss skriss deleted the pr-eds-snapshot-cache branch March 12, 2024 18:35
lubronzhan pushed a commit to lubronzhan/contour that referenced this pull request Mar 13, 2024
- Triggers only EDS updates when endpoints change
- Does not trigger EDS updates when only non-endpoints change

Updates projectcontour#2134.

Signed-off-by: Steve Kriss <stephen.kriss@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/small A small change that needs one line of explanation in the release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants