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 pod controller flaky unit test #192

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

LionelJouin
Copy link
Collaborator

What this PR does / why we need it:

client-go recommends to call WaitForCacheSync after calling Start on the SharedInformerFactory.
https://pkg.go.dev/k8s.io/[email protected]/informers#SharedInformerFactory

This fixes the flaky unit test for the pod controller.

ginkgo could be updated to its version 2 (github.com/onsi/ginkgo/v2)

Which issue(s) this PR fixes *
/

Special notes for your reviewer (optional):

I am not so familiar writing controller only with client-go, usually I use controller-runtime. So I don't really know what is done behind WaitForCacheSync except from what I could read in the documentation.

From what I observed, sometimes, handlePodUpdate is not called with the latest pod changes.

I tried to repeat the tests 100 times with this fix (using ginkgo --repeat=100 -timeout=240s ./pkg/controller/...), it never failed.

client-go recommends to call WaitForCacheSync after calling Start on the
SharedInformerFactory.
https://pkg.go.dev/k8s.io/[email protected]/informers#SharedInformerFactory

This fixes the flaky unit test for the pod controller.

ginkgo could be updated to its version 2 (github.com/onsi/ginkgo/v2)
@LionelJouin LionelJouin requested a review from maiqueb as a code owner November 29, 2023 22:49
@maiqueb
Copy link
Collaborator

maiqueb commented Dec 14, 2023

I need to make room to take a look at this; I was unaware the unit tests flaked.

Please be patient ...

@LionelJouin
Copy link
Collaborator Author

Sure, no problem.
The latest commit on the main branch didn't pass the unit test because of this issue for example.

@maiqueb maiqueb mentioned this pull request Jan 8, 2024
@maiqueb
Copy link
Collaborator

maiqueb commented Jan 8, 2024

Please rebase @LionelJouin

EDIT: seems I could just rebase & merge via the web UI.

Thanks again for the fix.

Copy link
Collaborator

@maiqueb maiqueb left a comment

Choose a reason for hiding this comment

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

Thank you. Good catch.

@maiqueb maiqueb merged commit f26c83c into k8snetworkplumbingwg:main Jan 8, 2024
1 of 2 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.

2 participants