Speed up pkg/controller/volume/scheduling unit tests #98912

wzshiming · 2021-02-09T08:30:35Z

What type of PR is this?
/kind cleanup
/kind flake

What this PR does / why we need it:
109s -> 24s

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot · 2021-02-09T08:30:43Z

@wzshiming: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

wzshiming · 2021-02-09T08:31:18Z

go test -race -c ./pkg/controller/volume/scheduling && stress ./scheduling.test
5s: 0 runs so far, 0 failures
10s: 0 runs so far, 0 failures
15s: 0 runs so far, 0 failures
20s: 0 runs so far, 0 failures
25s: 0 runs so far, 0 failures
30s: 8 runs so far, 0 failures
35s: 8 runs so far, 0 failures
40s: 8 runs so far, 0 failures
45s: 8 runs so far, 0 failures
50s: 12 runs so far, 0 failures
55s: 14 runs so far, 0 failures
1m0s: 16 runs so far, 0 failures
1m5s: 16 runs so far, 0 failures
1m10s: 16 runs so far, 0 failures
1m15s: 20 runs so far, 0 failures
1m20s: 22 runs so far, 0 failures
1m25s: 24 runs so far, 0 failures
1m30s: 24 runs so far, 0 failures
1m35s: 24 runs so far, 0 failures
1m40s: 28 runs so far, 0 failures
1m45s: 29 runs so far, 0 failures
1m50s: 31 runs so far, 0 failures
1m55s: 32 runs so far, 0 failures
2m0s: 32 runs so far, 0 failures
2m5s: 34 runs so far, 0 failures
2m10s: 37 runs so far, 0 failures
2m15s: 38 runs so far, 0 failures
2m20s: 40 runs so far, 0 failures
2m25s: 40 runs so far, 0 failures
2m30s: 41 runs so far, 0 failures
2m35s: 46 runs so far, 0 failures
2m40s: 47 runs so far, 0 failures
2m45s: 48 runs so far, 0 failures
2m50s: 49 runs so far, 0 failures
2m55s: 53 runs so far, 0 failures
3m0s: 54 runs so far, 0 failures
3m5s: 55 runs so far, 0 failures
3m10s: 55 runs so far, 0 failures
3m15s: 57 runs so far, 0 failures
3m20s: 59 runs so far, 0 failures
3m25s: 62 runs so far, 0 failures
3m30s: 63 runs so far, 0 failures
3m35s: 63 runs so far, 0 failures
3m40s: 64 runs so far, 0 failures
3m45s: 66 runs so far, 0 failures
3m50s: 70 runs so far, 0 failures
3m55s: 71 runs so far, 0 failures
4m0s: 71 runs so far, 0 failures
4m5s: 72 runs so far, 0 failures
4m10s: 73 runs so far, 0 failures
4m15s: 78 runs so far, 0 failures
4m20s: 79 runs so far, 0 failures
4m25s: 79 runs so far, 0 failures
4m30s: 80 runs so far, 0 failures
4m35s: 82 runs so far, 0 failures
4m40s: 86 runs so far, 0 failures
4m45s: 87 runs so far, 0 failures
4m50s: 87 runs so far, 0 failures
4m55s: 89 runs so far, 0 failures
5m0s: 91 runs so far, 0 failures
5m5s: 93 runs so far, 0 failures
5m10s: 95 runs so far, 0 failures
5m15s: 95 runs so far, 0 failures
5m20s: 97 runs so far, 0 failures
5m25s: 98 runs so far, 0 failures
5m30s: 100 runs so far, 0 failures
5m35s: 102 runs so far, 0 failures
5m40s: 103 runs so far, 0 failures
5m45s: 104 runs so far, 0 failures
5m50s: 106 runs so far, 0 failures
5m55s: 108 runs so far, 0 failures

BenTheElder · 2021-02-09T09:01:08Z

/retest

wzshiming · 2021-02-09T09:59:19Z

/retest

mucahitkurt · 2021-02-09T18:56:16Z

pkg/controller/volume/scheduling/scheduler_binder_test.go

@@ -2002,7 +2012,7 @@ func TestBindPodVolumes(t *testing.T) {
 		claimsToProvision := []*v1.PersistentVolumeClaim{}
 		if !scenario.bindingsNil {
 			if scenario.binding != nil {
-				bindings = []*BindingInfo{scenario.binding}
+				bindings = []*BindingInfo{makeBinding(scenario.binding.pvc, scenario.binding.pv)}


Why do you call makeBinding twice? In scenario definitions and here.

If don't add this, it will fail when run stress.

5s: 0 runs so far, 0 failures 10s: 0 runs so far, 0 failures 15s: 0 runs so far, 0 failures 20s: 0 runs so far, 0 failures /var/folders/6v/7stmg2756wlfk9c_qnv1hnbm0000gn/T/go-stress-20210227T142520-109974642 ================== WARNING: DATA RACE Read at 0x00c000424cc8 by goroutine 223: k8s.io/kubernetes/pkg/controller/volume/scheduling.(*testEnv).assumeVolumes() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:401 +0x11a k8s.io/kubernetes/pkg/controller/volume/scheduling.TestBindPodVolumes.func6() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:2023 +0xbaa k8s.io/kubernetes/pkg/controller/volume/scheduling.TestBindPodVolumes.func7() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:2068 +0x101 testing.tRunner() /usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1123 +0x202 Previous write at 0x00c000424cc8 by goroutine 81: k8s.io/kubernetes/pkg/controller/volume/scheduling.(*volumeBinder).bindAPIUpdate() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder.go:507 +0x8c5 k8s.io/kubernetes/pkg/controller/volume/scheduling.(*volumeBinder).BindPodVolumes() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder.go:442 +0x376 k8s.io/kubernetes/pkg/controller/volume/scheduling.TestBindPodVolumes.func6() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:2054 +0x301 k8s.io/kubernetes/pkg/controller/volume/scheduling.TestBindPodVolumes.func7() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:2068 +0x101 testing.tRunner() /usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1123 +0x202 Goroutine 223 (running) created at: testing.(*T).Run() /usr/local/Cellar/go/1.15.6/libexec/src/testing/testing.go:1168 +0x5bb k8s.io/kubernetes/pkg/controller/volume/scheduling.TestBindPodVolumes() /Users/zsm/go/src/k8s.io/kubernetes/pkg/controller/volume/scheduling/scheduler_binder_test.go:2066 +0x19d3 testing.tRunner() /usr/local/Cellar/go/1.15.6/libexe … 25s: 3 runs so far, 1 failures (33.33%)

scenario.binding should be local to each test case. If not, that seems to indicate there is still some shared state somewhere.

msau42 · 2021-02-28T22:13:10Z

/lgtm
/approve

k8s-ci-robot · 2021-02-28T22:13:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, wzshiming

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controller/volume/scheduling/OWNERS~~ [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

msau42 · 2021-02-28T22:18:40Z

/hold

msau42 · 2021-02-28T22:20:55Z

pkg/controller/volume/scheduling/scheduler_binder_test.go

@@ -2002,7 +2012,7 @@ func TestBindPodVolumes(t *testing.T) {
 		claimsToProvision := []*v1.PersistentVolumeClaim{}
 		if !scenario.bindingsNil {
 			if scenario.binding != nil {
-				bindings = []*BindingInfo{scenario.binding}
+				bindings = []*BindingInfo{makeBinding(scenario.binding.pvc, scenario.binding.pv)}


scenario.binding should be local to each test case. If not, that seems to indicate there is still some shared state somewhere.

msau42 · 2021-02-28T22:23:59Z

pkg/controller/volume/scheduling/scheduler_binder_test.go

@@ -2053,7 +2063,10 @@ func TestBindPodVolumes(t *testing.T) {
 	}

 	for name, scenario := range scenarios {
-		t.Run(name, func(t *testing.T) { run(t, scenario) })
+		t.Run(name, func(t *testing.T) {
+			t.Parallel()


Do we need t.Parallel() here and at the top of the Test function? I see the other test cases don't have the call here, and most examples I see have the parallel call inside the t.Run()

Since this test function takes a minute here, add t.Parallel() to t.Run() to make all cases in the parallel.

Found that scenario is a shared state.

for name, scenario := range scenarios { scenario := scenario t.Run(name, func(t *testing.T) { t.Parallel() run(t, scenario) }) }

Thanks, Updated.

Great find! Thanks!

msau42 · 2021-03-01T19:02:15Z

/lgtm
/hold cancel

BenTheElder · 2021-03-01T22:07:23Z

Thank you!!

k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Feb 9, 2021

k8s-ci-robot requested review from cofyc and msau42 February 9, 2021 08:31

k8s-ci-robot added the sig/apps Categorizes an issue or PR as relevant to SIG Apps. label Feb 9, 2021

wzshiming mentioned this pull request Feb 9, 2021

Umbrella issue: slow/timing-out unit tests #98486

Closed

18 tasks

k8s-ci-robot removed the do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Feb 9, 2021

BenTheElder mentioned this pull request Feb 9, 2021

[Flaky] //pkg/kubelet TestSyncPodsDeletesWhenSourcesAreReadyPerQOS #98893

Closed

mucahitkurt reviewed Feb 9, 2021

View reviewed changes

k8s-ci-robot assigned msau42 Feb 28, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2021

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 28, 2021

msau42 reviewed Feb 28, 2021

View reviewed changes

Speed up pkg/controller/volume/scheduling unit tests

67e4ba0

wzshiming force-pushed the ut/speed-up-volume-scheduling branch from 5b2281d to 67e4ba0 Compare March 1, 2021 03:57

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2021

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Mar 1, 2021

k8s-ci-robot merged commit 1b88c2e into kubernetes:master Mar 1, 2021

k8s-ci-robot added this to the v1.21 milestone Mar 1, 2021

wzshiming deleted the ut/speed-up-volume-scheduling branch March 2, 2021 01:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up pkg/controller/volume/scheduling unit tests #98912

Speed up pkg/controller/volume/scheduling unit tests #98912

wzshiming commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

wzshiming commented Feb 9, 2021

BenTheElder commented Feb 9, 2021

wzshiming commented Feb 9, 2021

mucahitkurt Feb 9, 2021

wzshiming Feb 27, 2021

msau42 Feb 28, 2021

msau42 commented Feb 28, 2021

k8s-ci-robot commented Feb 28, 2021

msau42 commented Feb 28, 2021

msau42 Feb 28, 2021

msau42 Feb 28, 2021

wzshiming Mar 1, 2021 •

edited

wzshiming Mar 1, 2021 •

edited

wzshiming Mar 1, 2021

msau42 Mar 1, 2021

msau42 commented Mar 1, 2021

BenTheElder commented Mar 1, 2021

Speed up pkg/controller/volume/scheduling unit tests #98912

Speed up pkg/controller/volume/scheduling unit tests #98912

Conversation

wzshiming commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

wzshiming commented Feb 9, 2021

BenTheElder commented Feb 9, 2021

wzshiming commented Feb 9, 2021

mucahitkurt Feb 9, 2021

Choose a reason for hiding this comment

wzshiming Feb 27, 2021

Choose a reason for hiding this comment

msau42 Feb 28, 2021

Choose a reason for hiding this comment

msau42 commented Feb 28, 2021

k8s-ci-robot commented Feb 28, 2021

msau42 commented Feb 28, 2021

msau42 Feb 28, 2021

Choose a reason for hiding this comment

msau42 Feb 28, 2021

Choose a reason for hiding this comment

wzshiming Mar 1, 2021 • edited

Choose a reason for hiding this comment

wzshiming Mar 1, 2021 • edited

Choose a reason for hiding this comment

wzshiming Mar 1, 2021

Choose a reason for hiding this comment

msau42 Mar 1, 2021

Choose a reason for hiding this comment

msau42 commented Mar 1, 2021

BenTheElder commented Mar 1, 2021

wzshiming Mar 1, 2021 •

edited

wzshiming Mar 1, 2021 •

edited