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

kubelet: Fix racy kubelet tests. #7980

Merged
merged 2 commits into from May 11, 2015

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented May 8, 2015

Add fakePodWorkders to run syncPod() in serial for testing.

Fix #7788

//cc @yujuhong @vmarmol @dchen1107

@yifan-gu yifan-gu force-pushed the fix_kubelet_tests branch 2 times, most recently from 8f1c8be to 0d7ea94 Compare May 8, 2015 18:51
@dchen1107 dchen1107 self-assigned this May 8, 2015
@yifan-gu yifan-gu force-pushed the fix_kubelet_tests branch 2 times, most recently from 40a9c7f to f2b5e4f Compare May 8, 2015 19:00
@yifan-gu yifan-gu changed the title kubelet/ Fix racy kubelet tests. kubelet: Fix racy kubelet tests. May 8, 2015
@yujuhong
Copy link
Contributor

yujuhong commented May 8, 2015

Thanks @yifan-gu for the PR! Looks good overall.

My only concern is that we would be testing the behaviors of the pod workers in kubelet_tests.go without using the actually pod workers. The fake pod_worker may eventually deviate from the actually implementation as time goes by. Ideally, we would want to move away from examining all docker calls in details in kubelet_test.go, but this may not be done for a while.

Maybe we can add one or two tests for the fake pod worker to verify that it's issuing the same docker calls as the actual pod worker? What do you think about it? We can do this in a followup PR, or if you are busy with other things, feel free to file an issue. I can grab it when I have time :)

@dchen1107
Copy link
Member

LGTM overall. I am ok to merge the pr with TODOs based on @yujuhong's comments. Thanks!

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 8, 2015

@yujuhong Yes I see your concern. I think we can add tests for the fake one to verify it invokes syncPodFn for the same number of times, and with the same args as the real one.
I can do that in this PR.

@yifan-gu
Copy link
Contributor Author

yifan-gu commented May 8, 2015

Tests added, PTAL, thanks! @yujuhong @dchen1107

@yujuhong
Copy link
Contributor

yujuhong commented May 8, 2015

LGTM. Thanks!

Add fakePodWorkders to run syncPod() in serial for testing.
@yujuhong
Copy link
Contributor

@yifan-gu, did you kick off shippable/travis build or are they just taking forever to finish?

@yifan-gu
Copy link
Contributor Author

@yujuhong NM, I just noticed there is a build failure by my fault, so I fixed it and rebased. Wait for travis now :)

@yujuhong
Copy link
Contributor

Flake in one of the shippable build, but the other build passed. Merging.

yujuhong added a commit that referenced this pull request May 11, 2015
@yujuhong yujuhong merged commit 8b3130b into kubernetes:master May 11, 2015
@yifan-gu
Copy link
Contributor Author

@yujuhong What flakes? It that related?

@yujuhong
Copy link
Contributor

Nope. It's the good old pod status not being reported in time bug #6651.

@yifan-gu yifan-gu deleted the fix_kubelet_tests branch May 11, 2015 23:15
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.

TestSyncPodsCreatesNetAndContainerPullsImage has non-deterministic order of docker` calls
4 participants