Skip to content

test: Fix possible race in waitForNPods helper function #10481

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
Merged
merged 3 commits into from
Mar 5, 2020
Merged

test: Fix possible race in waitForNPods helper function #10481

merged 3 commits into from
Mar 5, 2020

Conversation

brb
Copy link
Member

@brb brb commented Mar 5, 2020 •
edited
Loading

Previously, WaitforPods() could prematurely return when GetPods() returned no pods (e.g. WaitforPods() had been immediately called after sending a create pods request).

To avoid this, we should wait (by returning false) when the pod list is empty instead of setting the minRequired.

This makes the function to fail when a user intentionally set minRequired to 0 in WaitforNPodsRunning(), but I don't see any sane use case where setting minRequired to 0 would make sense.

Example of such race:

time="2020-02-29T13:05:44Z" level=debug msg="applying /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/test/k8sT/manifests/log-gatherer.yaml in namespace kube-system"
time="2020-02-29T13:05:44Z" level=debug msg="running command: kubectl apply --force=false -f /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/test/k8sT/manifests/log-gatherer.yaml -n kube-system"
cmd: "kubectl apply --force=false -f /home/jenkins/workspace/Cilium-PR-Ginkgo-Tests-Validated/k8s-1.11-gopath/src/github.com/cilium/cilium/test/k8sT/manifests/log-gatherer.yaml -n kube-system" exitCode: 0 duration: 92.720906ms stdout:
serviceaccount/cilium unchanged
daemonset.apps/log-gatherer unchanged

time="2020-02-29T13:05:44Z" level=debug msg="running command: kubectl -n kube-system get pods -l k8s-app=cilium-test-logs -o json"
cmd: "kubectl -n kube-system get pods -l k8s-app=cilium-test-logs -o json" exitCode: 0 duration: 43.672802ms stdout:
{
    "apiVersion": "v1",
    "items": [],
    "kind": "List",
    "metadata": {
        "resourceVersion": "",
        "selfLink": ""
    }
}

Fix #10277.

This change is Reviewable

@brb brb added pending-review kind/bug/CI This is a bug in the testing code. area/CI Continuous Integration testing issue or flake release-note/ci This PR makes changes to the CI. labels Mar 5, 2020
@brb brb requested a review from a team as a code owner March 5, 2020 11:14
@brb
Copy link
Member Author

brb commented Mar 5, 2020

test-me-please

@brb brb force-pushed the pr/brb/ci-fix-get-iface-flake branch from 93bce16 to d4c2122 Compare March 5, 2020 12:21
@brb
Copy link
Member Author

brb commented Mar 5, 2020

test-me-please

@coveralls
Copy link

coveralls commented Mar 5, 2020 •
edited
Loading

Coverage Status

Coverage increased (+0.01%) to 45.615% when pulling 44d19ed on pr/brb/ci-fix-get-iface-flake into 8ddc98e on master.

brb added 3 commits March 5, 2020 15:09
@brb
test: Do not wait for zgroup=bind pods
5d88b62 
They were removed in 7b6d34d ("CI: Switch CI FQDN tests to use
internet domains").

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Fix waiting for zgroup=testapp in Policies suite
9945d0b 
There is no namespace "foo".

Fixes: 31d16dc ("test: add e2e test for proxy-visibility")
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Fix possible race in waitForNPods helper function
44d19ed 
Previously, WaitforPods() could prematurely return when kub.GetPods()
returned no pods (e.g. WaitforPods() had been immediately called after
sending a create pods request).

To avoid this, we should wait (by returning false) when the pod list
is empty instead of setting the minRequired.

This makes the function to fail when a user intentionally set
minRequired to 0 in WaitforNPodsRunning(), but I don't see any sane use
case where setting minRequired to 0 would make sense.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb brb force-pushed the pr/brb/ci-fix-get-iface-flake branch from d4c2122 to 44d19ed Compare March 5, 2020 14:32
@brb
Copy link
Member Author

brb commented Mar 5, 2020

test-me-please

@aanm aanm merged commit 6abebd0 into master Mar 5, 2020
@aanm aanm deleted the pr/brb/ci-fix-get-iface-flake branch March 5, 2020 16:33
@vadorovsky vadorovsky mentioned this pull request Mar 6, 2020
Merged
@gandro gandro mentioned this pull request Mar 16, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/CI Continuous Integration testing issue or flake kind/bug/CI This is a bug in the testing code. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

CI: K8sPolicyTest Basic Test checks all kind of Kubernetes policies: Cilium cannot be installed: Failed to exec \"ip ...\" cmd on \"k8s1\" node
6 participants
@brb @coveralls @aanm @gandro @joestringer @tgraf