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

dataclients/kubernetes: remove endpoint addresses cache #2929

Merged

Conversation

AlexanderYastrebov
Copy link
Member

@AlexanderYastrebov AlexanderYastrebov commented Feb 12, 2024

Returning cached endpoint addreses from exported GetEndpointAddresses makes it possible for caller to modify cached values.

Since the logic of obtaining endpoint addresses is lightweight this change simply drops caching altogether.

Follow up on #2917
See https://github.com/zalando/skipper/pull/2917/files#r1486405631

@AlexanderYastrebov AlexanderYastrebov added minor no risk changes, for example new filters bugfix Bug fixes and patches and removed minor no risk changes, for example new filters labels Feb 12, 2024
@AlexanderYastrebov AlexanderYastrebov force-pushed the dataclients/kubernetes/fix-mutable-addresses branch from a65ced9 to 738dd03 Compare February 12, 2024 16:06
Returning cached endpoint addreses from exported GetEndpointAddresses
makes it possible for caller to modify cached values.

Since the logic of obtaining endpoint addresses is lightweight
this change simply drops caching altogether.

Follow up on #2917

Signed-off-by: Alexander Yastrebov <alexander.yastrebov@zalando.de>
@AlexanderYastrebov AlexanderYastrebov force-pushed the dataclients/kubernetes/fix-mutable-addresses branch from 738dd03 to 37ac058 Compare February 12, 2024 16:21
@RomanZavodskikh
Copy link
Member

the logic of obtaining endpoint addresses is lightweight

BenchmarkCachedEndpoint seems to measure performance impact of removing cache, did you consider running it?
https://github.com/zalando/skipper/blob/v0.19.56/dataclients/kubernetes/clusterstate_test.go#L65

@AlexanderYastrebov
Copy link
Member Author

@RomanZavodskikh

It benchmarks cachedEndpoints. #2917 added separated cache for endpoint addresses, see #2917 (comment) and this change removes it.

@szuecs
Copy link
Member

szuecs commented Feb 12, 2024

Because we cache endpoints/endpointslices, the addresses cache does not really matter.

@szuecs
Copy link
Member

szuecs commented Feb 12, 2024

👍

1 similar comment
@AlexanderYastrebov
Copy link
Member Author

👍

@AlexanderYastrebov AlexanderYastrebov merged commit 500688f into master Feb 12, 2024
14 checks passed
@AlexanderYastrebov AlexanderYastrebov deleted the dataclients/kubernetes/fix-mutable-addresses branch February 12, 2024 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Bug fixes and patches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants