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 inconsistent route order test flake in TestParserSNI #4777

Closed
wants to merge 2 commits into from

Conversation

rainest
Copy link
Contributor

@rainest rainest commented Oct 5, 2023

What this PR does / why we need it:

Use a consistent order for the routes attached to a service in the parser. This makes tests that care about the route index not flaky.

Which issue this PR fixes:

TestParserSNI began failing recently, e.g. https:/Kong/kubernetes-ingress-controller/actions/runs/6407581302/job/17394681361

The origin of the flake isn't clear, but 23de4f5 seems the most likely candidate for introducing it based on date and affected code.

Special notes for your reviewer:

This is, oddly enough, the only parser test that uses multiple indices. We could alternately look up the routes by name for this test and not change anything in the translator. IDK how likely we'd be to add other affected tests in the future, or how much we'd care about the small bit of essentially wasted CPU time to avoid it.

PR Readiness Checklist:

Complete these before marking the PR as ready to review:

  • the CHANGELOG.md release notes have been updated to reflect any significant (and particularly user-facing) changes introduced by this PR only fixes tests for an unreleased change

Use a consistent order for the routes attached to a service in the
parser. This makes tests that care about the route index not flaky.
@rainest rainest requested a review from a team as a code owner October 5, 2023 20:39
@rainest rainest enabled auto-merge (squash) October 5, 2023 20:41
@codecov
Copy link

codecov bot commented Oct 5, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (27c51e5) 77.7% compared to head (e0a4a84) 77.7%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #4777   +/-   ##
=====================================
  Coverage   77.7%   77.7%           
=====================================
  Files        163     164    +1     
  Lines      18541   18561   +20     
=====================================
+ Hits       14409   14436   +27     
+ Misses      3322    3316    -6     
+ Partials     810     809    -1     
Files Coverage Δ
internal/dataplane/parser/translators/ingress.go 95.0% <100.0%> (+0.1%) ⬆️
internal/util/stringheap.go 100.0% <100.0%> (ø)

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pull-request-size pull-request-size bot added size/M and removed size/S labels Oct 6, 2023
@rainest rainest force-pushed the fix/parser-test-route-order branch from 9f4beaa to b6e6b7b Compare October 6, 2023 21:55
@rainest rainest requested a review from pmalek October 6, 2023 21:55
@rainest rainest force-pushed the fix/parser-test-route-order branch from b6e6b7b to e0a4a84 Compare October 6, 2023 22:43
Copy link
Contributor

@randmonkey randmonkey left a comment

Choose a reason for hiding this comment

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

IMO it would be better to modify the unit tests (like finding the target route using lo.ContainsBy) instead of changing the parser to fix the order of routes if we do not have other reasons to have a fixed route order.

@pmalek
Copy link
Member

pmalek commented Oct 9, 2023

I have created #4781 which changes the tests instead of the code which creates the service routes.

One thing though, which is on my mind, is that perhaps we should actually do it as proposed in here because that might reduce sending unnecessary configuration change requests to Gateway, right? If we change it in tests then more requests could be sent because the order of service routes could change from parser run to parser run even though the underlying objects didn't change.

Comment on lines +148 to +149
// the order of routes ultimately doesn't matter, but using a consistent order simplifies unit testing,
// as the output only has array indices and cannot be accessed by name
Copy link
Member

Choose a reason for hiding this comment

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

That comment might not be 100% accurate because the order ultimately does matter when we want to figure out if the config is different than the one were going to overwrite (in Admin API) right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe sorting is handled in another layer - deckgen.ToDeckContent sorts all the entities:

I think it's okay to stick to that and leave the parser to return non-deterministically ordered slices.

Copy link
Member

Choose a reason for hiding this comment

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

Cool. In that case we could add a comment mentioning this explicitly. And just leave it as it. Would this mean that we can move forward with #4781 ?

package util

// StringHeap is an array of strings that implements container/heap.Interface.
type StringHeap []string
Copy link
Member

Choose a reason for hiding this comment

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

Would it be difficult to make the heap contain the ingressTranslationMeta (just as in the map) to prevent having both map and heap inside the ingressTranslationIndex?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried as much and got sad discovering that this didn't really slot neatly into the simple data structure :) We have to modify the entries as we go to add new paths, and we find the correct entry to modify based on the key, so we basically end up with some combination of a map and a heap to manage that, sooooo...

@pmalek pmalek changed the title Fix inconsistent route order test flake Fix inconsistent route order test flake in TestParserSNI Oct 9, 2023
@rainest rainest closed this Oct 9, 2023
auto-merge was automatically disabled October 9, 2023 23:10

Pull request was closed

@rainest rainest deleted the fix/parser-test-route-order branch October 9, 2023 23:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants