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

fix: implement comparable interface for ir.Xds to skip unnecessary updates #1795

Merged
merged 3 commits into from
Aug 17, 2023

Conversation

dboslee
Copy link
Contributor

@dboslee dboslee commented Aug 17, 2023

What this PR does / why we need it:
This change implements the comparable interface used by watchable to sort ir.Xds listeners when checking for differences. This reduces the number of xds updates that get pushed to envoy during re-syncs of k8s resources.

Which issue(s) this PR fixes:
fixes #1634

@dboslee dboslee requested a review from a team as a code owner August 17, 2023 17:46
Signed-off-by: David Boslee <david@goteleport.com>
@codecov
Copy link

codecov bot commented Aug 17, 2023

Codecov Report

Merging #1795 (a1bdeb2) into main (677cf03) will increase coverage by 0.05%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##             main    #1795      +/-   ##
==========================================
+ Coverage   64.95%   65.00%   +0.05%     
==========================================
  Files          84       84              
  Lines       12209    12230      +21     
==========================================
+ Hits         7930     7950      +20     
+ Misses       3773     3768       -5     
- Partials      506      512       +6     
Files Changed Coverage Δ
internal/ir/xds.go 72.56% <85.71%> (+0.89%) ⬆️

... and 3 files with indirect coverage changes

@arkodg
Copy link
Contributor

arkodg commented Aug 17, 2023

there's a lint error

Error: ST1016: methods on the same type should have the same receiver name (seen 1x "x1", 6x "x") (stylecheck)

probably can be recreated by running make lint locally

Signed-off-by: David Boslee <david@goteleport.com>
arkodg
arkodg previously approved these changes Aug 17, 2023
Copy link
Contributor

@arkodg arkodg 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 for adding this
would also be great to see if watchable is actually leveraging this Equal method with a test in
https://github.com/envoyproxy/gateway/blob/main/internal/message/watchutil_test.go

@dboslee
Copy link
Contributor Author

dboslee commented Aug 17, 2023

LGTM thanks for adding this would also be great to see if watchable is actually leveraging this Equal method with a test in https://github.com/envoyproxy/gateway/blob/main/internal/message/watchutil_test.go

Sure thing. I will add a test for this.

Signed-off-by: David Boslee <david@goteleport.com>
Copy link
Contributor

@arkodg arkodg left a comment

Choose a reason for hiding this comment

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

awesome, thanks !

@arkodg arkodg requested review from a team, kflynn, qicz and LanceEa and removed request for a team August 17, 2023 19:27
Copy link
Member

@AliceProxy AliceProxy left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for contributing 🚀

@AliceProxy AliceProxy merged commit 741e6b5 into envoyproxy:main Aug 17, 2023
18 checks passed
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.

watchable: skip an update if the elements in slice have not changed, but order has
3 participants