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

Resolve data race in setup syncer test #6282

Merged
merged 1 commit into from
Apr 8, 2022
Merged

Resolve data race in setup syncer test #6282

merged 1 commit into from
Apr 8, 2022

Conversation

sam-heilbron
Copy link
Contributor

@sam-heilbron sam-heilbron commented Apr 8, 2022 •
edited

Description

Resolve a data race that occurs in our setup syncer test

Context

I think this data race is due to our tests and not due to our implementation. Per kubernetes/kubernetes#89019 (comment), we do setup schemes at construction time, as we should. Moreover, this flake has only occurred in this specific test. If this were a bug in our Gloo code, I would have expected to see it fail in other tests.

Checklist:

  • I included a concise, user-facing changelog (for details, see https://github.com/solo-io/go-utils/tree/master/changelogutils) which references the issue that is resolved.
  • If I updated APIs (our protos) or helm values, I ran make -B install-go-tools generated-code to ensure there will be no code diff
  • I followed guidelines laid out in the Gloo Edge contribution guide
  • I opened a draft PR or added the work in progress label if my PR is not ready for review
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works

@solo-changelog-bot
Copy link

Issues linked to changelog:
#5860

@github-actions github-actions bot added the keep pr updated signals bulldozer to keep pr up to date with base branch label Apr 8, 2022
@sam-heilbron sam-heilbron marked this pull request as ready for review April 8, 2022 19:00
@github-actions
Copy link

github-actions bot commented Apr 8, 2022

Visit the preview URL for this PR (updated for commit cbaf4c2):

https://gloo-edge--pr6282-test-flakes-shared-i-qqzc1bva.web.app

(expires Fri, 15 Apr 2022 19:09:00 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

nfuden
nfuden approved these changes Apr 8, 2022
View reviewed changes
test/kube2e/gloo/setup_syncer_test.go
// SetupFunc is used to configure Gloo with appropriate configuration
// It is assumed to run once at construction time, and therefore it executes directives that
// are also assumed to only run at construction time.
// One of those, is the construction of schemes: https://github.com/kubernetes/kubernetes/pull/89019#issuecomment-600278461
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooh I remember seeing that and thinking it was just something we were doing in gloo.
Its great that you were able to identify where we actually call the scheme setup!

@soloio-bulldozer soloio-bulldozer bot merged commit 36a7fe0 into master Apr 8, 2022
@soloio-bulldozer soloio-bulldozer bot deleted the test-flakes/shared-informer-data-race branch April 8, 2022 19:42
sam-heilbron added a commit that referenced this pull request Apr 11, 2022
@sam-heilbron
Resolve data race in setup syncer test (#6282)
63af4e6
soloio-bulldozer bot added a commit that referenced this pull request Apr 11, 2022
@sam-heilbron @soloio-bulldozer
[1.11] Reduce test flakes (#6294)
e8f1b29 
* Reduce test flakes (#6211)

* First pass of cleanup

* more

* Fixup gateway e2e tests

* add changelog

* fix route options tests?

* cleanup comments in assertions

* Just after each to match just before

* re-add config

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve panic in gateway validation client (#6260)

* add test

* add fix for panicing test

* unfocus test

* add changelog

* goimports -w

* Update changelog/v1.12.0-beta2/robust-client-panic.yaml

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve consul plugin data race (#6271)

* use dependency injectio to handle fallback resolution

* add changelog

* fixup dns resolvers and test

* Resolve data race in setup syncer test (#6282)

* add changelogs

* delete duplicate changelogs

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
sam-heilbron added a commit that referenced this pull request Apr 11, 2022
@sam-heilbron @soloio-bulldozer
[1.11] Reduce test flakes (#6294)
44141e7 
* Reduce test flakes (#6211)

* First pass of cleanup

* more

* Fixup gateway e2e tests

* add changelog

* fix route options tests?

* cleanup comments in assertions

* Just after each to match just before

* re-add config

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve panic in gateway validation client (#6260)

* add test

* add fix for panicing test

* unfocus test

* add changelog

* goimports -w

* Update changelog/v1.12.0-beta2/robust-client-panic.yaml

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve consul plugin data race (#6271)

* use dependency injectio to handle fallback resolution

* add changelog

* fixup dns resolvers and test

* Resolve data race in setup syncer test (#6282)

* add changelogs

* delete duplicate changelogs

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
@sam-heilbron sam-heilbron mentioned this pull request Apr 11, 2022
Merged
8 tasks
sam-heilbron added a commit that referenced this pull request Apr 11, 2022
@sam-heilbron
Resolve data race in setup syncer test (#6282)
02f635c
soloio-bulldozer bot added a commit that referenced this pull request Apr 11, 2022
@sam-heilbron @soloio-bulldozer
[1.10] Reduce test flakes (#6297)
255b644 
* Reduce test flakes (#6211)

* First pass of cleanup

* more

* Fixup gateway e2e tests

* add changelog

* fix route options tests?

* cleanup comments in assertions

* Just after each to match just before

* re-add config

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve panic in gateway validation client (#6260)

* add test

* add fix for panicing test

* unfocus test

* add changelog

* goimports -w

* Update changelog/v1.12.0-beta2/robust-client-panic.yaml

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve consul plugin data race (#6271)

* use dependency injectio to handle fallback resolution

* add changelog

* fixup dns resolvers and test

* Resolve data race in setup syncer test (#6282)

* add changelogs

* delete duplicate changelogs

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
soloio-bulldozer bot added a commit that referenced this pull request Apr 12, 2022
@sam-heilbron @soloio-bulldozer
[1.9] Reduce test flakes (#6298)
2262a83 
* Reduce test flakes (#6211)

* First pass of cleanup

* more

* Fixup gateway e2e tests

* add changelog

* fix route options tests?

* cleanup comments in assertions

* Just after each to match just before

* re-add config

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve panic in gateway validation client (#6260)

* add test

* add fix for panicing test

* unfocus test

* add changelog

* goimports -w

* Update changelog/v1.12.0-beta2/robust-client-panic.yaml

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>

Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>

* Resolve consul plugin data race (#6271)

* use dependency injectio to handle fallback resolution

* add changelog

* fixup dns resolvers and test

* Resolve data race in setup syncer test (#6282)

* move changelog

* fixup registry

* de duplicate changelogs

* delete dynamic forward proxy test

* delete hybrid test

* undo kube2e changes, panic occurring with gomega

* update changelog

* undo delete

Co-authored-by: soloio-bulldozer[bot] <48420018+soloio-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: Kevin Dorosh <kevin.dorosh@solo.io>
@sam-heilbron sam-heilbron mentioned this pull request Sep 22, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nfuden nfuden nfuden approved these changes

Assignees
No one assigned
Labels
keep pr updated signals bulldozer to keep pr up to date with base branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@sam-heilbron @nfuden