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

test: Speed up K8sServicesTest #11550

Merged
merged 6 commits into from May 20, 2020
Merged

test: Speed up K8sServicesTest #11550

merged 6 commits into from May 20, 2020

Conversation

brb
Copy link
Member

@brb brb commented May 15, 2020 •
edited

This PR speeds up the test suite mainly by unrolling curl requests (to avoid excessive kubectl exec) and running them in parallel.

Reviewable per commit.

Partially_fix #11442

@brb brb added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/misc This PR makes changes that have no direct user impact. labels May 15, 2020
@brb brb requested a review from a team as a code owner May 15, 2020 14:16
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 May 15, 2020
@brb brb marked this pull request as draft May 15, 2020 14:16
@brb
Copy link
Member Author

brb commented May 15, 2020

retest-net-next

@coveralls
Copy link

coveralls commented May 15, 2020 •
edited

Coverage Status

Coverage decreased (-0.02%) to 37.005% when pulling fc6f9bb610ba8e34d1f45ecd94d5a6b2c9df345a on pr/brb/cleanup-svc-tests into 3cef02f on master.

@brb brb force-pushed the pr/brb/cleanup-svc-tests branch from 51d8118 to c260b30 Compare May 15, 2020 14:56
@brb
Copy link
Member Author

brb commented May 15, 2020

test-focus K8sService*

@brb brb force-pushed the pr/brb/cleanup-svc-tests branch from c260b30 to 848cf86 Compare May 15, 2020 16:21
@brb
Copy link
Member Author

brb commented May 15, 2020

test-focus K8sService*

@brb
Copy link
Member Author

brb commented May 15, 2020

retest-net-next

3 similar comments
@brb
Copy link
Member Author

brb commented May 15, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 17, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-net-next

@brb brb force-pushed the pr/brb/cleanup-svc-tests branch from 848cf86 to 9b08238 Compare May 18, 2020 08:19
@brb brb changed the title test: paralellize testNodePort test: speed up K8sServicesTest May 18, 2020
@brb brb changed the title test: speed up K8sServicesTest test: Speed up K8sServicesTest May 18, 2020
@brb brb marked this pull request as ready for review May 18, 2020 08:31
@brb brb added this to In Progress (Cilium) in CI Force via automation May 18, 2020
@brb
Copy link
Member Author

brb commented May 18, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-gke

@brb brb force-pushed the pr/brb/cleanup-svc-tests branch from 9b08238 to 6df9fb4 Compare May 18, 2020 09:07
@brb
Copy link
Member Author

brb commented May 18, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-gke

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-4.9

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-4.19

@brb
Copy link
Member Author

brb commented May 18, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-4.19

@brb
Copy link
Member Author

brb commented May 18, 2020

retest-runtime

@brb brb force-pushed the pr/brb/cleanup-svc-tests branch from fc6f9bb to 9c03240 Compare May 19, 2020 07:32
brb added 6 commits May 19, 2020 09:32
@brb
test: Get node names and ip addrs only once in K8sServices
36888fa 
They don't change during test runs.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Parallelize testNodePort
e2e9a5c 
Regroup the test cases and run them in parallel. On my machine the
invocation of testNodePort() went from ~2:50 min to ~1:15 min.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Unroll curl requests in K8sService suite
6dca4ce 
To avoid excessive "kubectl exec", do curl requests in batches.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Move failBind tests to separate test case
ff6451f 
It's enough to test it only once, thus the move.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Skip duplicate ClusterIP tests
88c3d2a 
Test ClusterIP from a pod netns and a hostnetns in testNodePort(), so
that some ClusterIP test cases can be skipped.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
test: Call GinkgoRecover() from testNodePort() goroutines
9c03240 
Otherwise, in a case of a failure, ginkgo will panic leaving a cryptic
message, from which it's harder to determine which test case has failed.

Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented May 19, 2020

test-me-please

@brb
Copy link
Member Author

brb commented May 19, 2020

retest-net-next

2 similar comments
@brb
Copy link
Member Author

brb commented May 19, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 19, 2020

retest-net-next

nebril
nebril requested changes May 19, 2020
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

Thanks for this change, couple comments inline, can't wait to merge it!

test/k8sT/Services.go
wg.Add(1)
go func(url string) {
defer wg.Done()
testCurlRequest(testDSClient, url)
Copy link
Member

Choose a reason for hiding this comment

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

I think you will need to increment ExpectWithOffset offset (first argument) in each helper test function called in goroutines so we get better context information about error if assertion fails.

test/k8sT/Services.go
}
for _, url := range testURLsFromHosts {
wg.Add(1)
go func(url string) {
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

test/k8sT/Services.go
for _, url := range testURLsFromOutside {
wg.Add(1)
go func(url string) {
defer wg.Done()
Copy link
Member

Choose a reason for hiding this comment

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

ditto

test/k8sT/Services.go
fromPod, url, i, count)
}
By("Making %d curl requests from pod (host netns) %s to %q", count, fromPod, url)
cmd := fmt.Sprintf(`/bin/sh -c 'for i in $(seq 1 %d); do %s; done'`, count,
Copy link
Member

Choose a reason for hiding this comment

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

Holy mother of nesting. But I guess there is no way around this (unless we want to also parallelize curl calls?).

test/k8sT/Services.go Show resolved Hide resolved
@brb
Copy link
Member Author

brb commented May 19, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 19, 2020

retest-4.9

1 similar comment
@brb
Copy link
Member Author

brb commented May 19, 2020

retest-4.9

@brb
Copy link
Member Author

brb commented May 19, 2020

retest-4.19

@brb
Copy link
Member Author

brb commented May 19, 2020

retest-net-next

2 similar comments
@brb
Copy link
Member Author

brb commented May 20, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 20, 2020

retest-net-next

@brb
Copy link
Member Author

brb commented May 20, 2020

retest-4.9

@brb
Copy link
Member Author

brb commented May 20, 2020

retest-4.19

@brb
Copy link
Member Author

brb commented May 20, 2020

test-runtime

@brb
Copy link
Member Author

brb commented May 20, 2020

@nebril Thanks for the review! Considering that all the tests have passed, can we merge this, and then I will address your comments in a follow-up PR?

@nebril
Copy link
Member

nebril commented May 20, 2020

Let's merge, @brb will make a followup PR with QoL fixes.

@nebril nebril merged commit 60ab759 into master May 20, 2020
1.8.0 automation moved this from In progress to Merged May 20, 2020
CI Force automation moved this from In Progress (Cilium) to Fixed / Done May 20, 2020
@nebril nebril deleted the pr/brb/cleanup-svc-tests branch May 20, 2020 17:20
@brb brb mentioned this pull request May 25, 2020
Merged
@pchaigno pchaigno mentioned this pull request May 29, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nebril nebril nebril requested changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
CI Force
  
Fixed / Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@brb @coveralls @nebril @pchaigno