Even Pods Spread - 6. Integration Test #80011

Huang-Wei · 2019-07-11T04:17:51Z

What type of PR is this?

/sig scheduling
/kind feature
/priority important-soon
/hold

/assign @bsalamat @ahg-g
/cc @krmayankk

What this PR does / why we need it:

This is the 6th PR of the "Even Pods Spread" KEP implementation. It focuses on integration tests.

Which issue(s) this PR fixes:

Part of #77284.

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

(will document all the changes in one place)

NONE

Huang-Wei · 2019-07-11T05:42:09Z

/retest

Huang-Wei · 2019-07-11T21:25:36Z

/retest

Huang-Wei · 2019-07-12T00:40:33Z

Need to revise ApplyFeaturegate() which is leaking changes to global state of pred/prio map. I will find time to resolve that and come back.

ahg-g · 2019-07-29T15:02:29Z

Can you please rebase?

Huang-Wei · 2019-07-29T17:48:43Z

@ahg-g Rebase completed. PTAL.

BTW: this PR is dependent on #80144 (I cherrypicked that PR here).

Huang-Wei · 2019-07-29T20:21:42Z

/retest

liu-cong · 2019-07-29T21:11:06Z

pkg/scheduler/testing/wrappers.go

@@ -152,6 +156,27 @@ func (p *PodWrapper) Namespace(s string) *PodWrapper {
 	return p
 }

+// Container appends a container into PodSpec of the inner pod
+func (p *PodWrapper) Container(s string) *PodWrapper {


s -> image to avoid misunderstanding of s being the container name.

It's usually a pattern to make the variable short within a clear context. But if the context contains both namespace/name and image, yes, making the name more descriptive weighs simplicity.

liu-cong · 2019-07-29T21:11:19Z

pkg/scheduler/testing/wrappers.go

@@ -152,6 +156,27 @@ func (p *PodWrapper) Namespace(s string) *PodWrapper {
 	return p
 }

+// Container appends a container into PodSpec of the inner pod


nit: add period at the end of the comment.

liu-cong · 2019-07-29T21:13:11Z

pkg/scheduler/testing/wrappers.go

+}
+
+// Zero sets the TerminationGracePeriodSeconds of the inner pod to zero.
+func (p *PodWrapper) Zero() *PodWrapper {


I prefer to explicitly name it something like ZeroTerminationGracePeriod or NoTerminationGrace. Zero doesn't communicate clearly.

liu-cong · 2019-07-29T21:21:06Z

test/integration/scheduler/util.go

+		}
+		if err != nil {
+			// This could be a connection error so we want to retry.
+			return false, nil


Why checking errors.IsNotFound(err) and err != nil separately? The return values are identical.

Good catch. Only if err != nil should be kept. I updated all the occurrences.

liu-cong · 2019-07-29T21:23:51Z

test/integration/scheduler/util.go

+			// This could be a connection error so we want to retry.
+			return false, nil
+		}
+		if pod.Spec.NodeName == "" {


This seems unnecessary if the passed in nodeNames are not empty. But feel free to disregard if the purpose is to short-circuit.

Yes, sort of short-circuit.

liu-cong · 2019-07-29T22:00:20Z

test/integration/scheduler/predicates_test.go

+		name string
+		pod  *v1.Pod
+		pods []*v1.Pod
+		fits bool


nit: add a comment on what does fits mean. success is probably a better name.

liu-cong · 2019-07-29T22:05:29Z

test/integration/scheduler/priorities_test.go

+	pause := imageutils.GetPauseImageName()
+	tests := []struct {
+		name string
+		pod  *v1.Pod


nit: same as above, more specific naming

liu-cong · 2019-07-29T22:06:33Z

test/integration/scheduler/priorities_test.go

+		want []string // nodes expected to schedule onto
+	}{
+		// note: naming starts at index 0
+		// the symbol ~X~ means that node is infeasible


To make it easy to follow, why not use a list where list[i]=N means ith node has N pods on it. E.g., [3,1,1,1] means there are 3 pods on node 0 and node 0 is infeasible.

liu-cong · 2019-07-29T22:11:03Z

test/integration/scheduler/priorities_test.go

+		t.Run(tt.name, func(t *testing.T) {
+			allPods := append(tt.pods, tt.pod)
+			defer cleanupPods(cs, t, allPods)
+			for _, pod := range tt.pods {


The pod creation can go to a helper function that can be reused in predicates_test

liu-cong · 2019-07-29T22:23:04Z

test/integration/scheduler/predicates_test.go

+			want: []string{"node-1", "node-2", "node-3"},
+		},
+		{
+			name: "pod is required to be placed on zone0, so only node-1 fits",


Both node 0 and 1 are labeled "zone 0", so why does only node-1 fit?

Because node 0 has one pod already (L1001).

Thanks for explaining it to me.

ahg-g

minor stuff, looks great, I will lgtm once you address the other reviewer's comments.

ahg-g · 2019-07-31T01:16:52Z

pkg/scheduler/testing/wrappers.go

+}
+
+// Zero sets the TerminationGracePeriodSeconds of the inner pod to zero.
+func (p *PodWrapper) Zero() *PodWrapper {


ahg-g · 2019-07-31T01:22:35Z

test/integration/scheduler/predicates_test.go

+	pause := imageutils.GetPauseImageName()
+	tests := []struct {
+		name string
+		pod  *v1.Pod


ahg-g · 2019-07-31T01:25:41Z

test/integration/scheduler/predicates_test.go

+		pod  *v1.Pod
+		pods []*v1.Pod
+		fits bool
+		want []string // nodes expected to schedule onto


s/want/candidateNodes?

k8s-ci-robot · 2019-08-01T07:53:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Huang-Wei

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/scheduler/OWNERS~~ [Huang-Wei]
~~test/integration/scheduler/OWNERS~~ [Huang-Wei]

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

ahg-g · 2019-08-01T16:53:14Z

/lgtm

Thanks @Huang-Wei!

Huang-Wei · 2019-08-01T17:43:06Z

Thanks @ahg-g and @liu-cong for reviewing!

Just squashed the commits. Will ask for /lgtm later.

Huang-Wei · 2019-08-01T17:47:29Z

/hold cancel

liu-cong · 2019-08-01T17:53:30Z

/lgtm

k8s-ci-robot · 2019-08-01T17:53:37Z

@liu-cong: changing LGTM is restricted to collaborators

In response to this:

/lgtm

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.

Huang-Wei · 2019-08-01T18:18:43Z

/retest

fejta-bot · 2019-08-01T18:33:00Z

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

Huang-Wei · 2019-08-01T18:58:25Z

/retest

Huang-Wei · 2019-08-01T20:20:53Z

/retest

Huang-Wei · 2019-08-01T20:55:01Z

TestVolumeProvision keeps failing.

/retest

Huang-Wei · 2019-08-01T22:09:47Z

/retest

ahg-g · 2019-08-01T22:45:18Z

/lgtm

k8s-ci-robot · 2019-08-02T02:01:25Z

@Huang-Wei: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-gce-100-performance	`caab8b7`	link	`/test pull-kubernetes-e2e-gce-100-performance`
pull-kubernetes-e2e-gce	`caab8b7`	link	`/test pull-kubernetes-e2e-gce`
pull-kubernetes-kubemark-e2e-gce-big	`caab8b7`	link	`/test pull-kubernetes-kubemark-e2e-gce-big`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 11, 2019

k8s-ci-robot assigned ahg-g and bsalamat Jul 11, 2019

k8s-ci-robot requested a review from krmayankk July 11, 2019 04:17

Huang-Wei mentioned this pull request Jul 11, 2019

Umbrella issue for Even Pods Spread [Alpha] #77284

Closed

9 tasks

Huang-Wei force-pushed the eps-int-test branch from f6d9c77 to 83b009f Compare July 11, 2019 19:41

Huang-Wei force-pushed the eps-int-test branch 2 times, most recently from 4b89f08 to 95eba90 Compare July 14, 2019 17:49

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 18, 2019

Huang-Wei force-pushed the eps-int-test branch from 95eba90 to b88af07 Compare July 29, 2019 17:47

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 29, 2019

Huang-Wei force-pushed the eps-int-test branch from b88af07 to c3c44d7 Compare July 29, 2019 18:54

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 29, 2019

liu-cong reviewed Jul 29, 2019

View reviewed changes

ahg-g reviewed Jul 31, 2019

View reviewed changes

Huang-Wei force-pushed the eps-int-test branch from c3c44d7 to b9c760c Compare August 1, 2019 07:52

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 1, 2019

Huang-Wei force-pushed the eps-int-test branch from b9c760c to 53ea857 Compare August 1, 2019 08:10

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019

EvenPodsSpread: integration test

caab8b7

Huang-Wei force-pushed the eps-int-test branch from 53ea857 to caab8b7 Compare August 1, 2019 17:42

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

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 1, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 1, 2019

k8s-ci-robot merged commit e857ae0 into kubernetes:master Aug 2, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 2, 2019

Huang-Wei deleted the eps-int-test branch August 2, 2019 00:40

liu-cong mentioned this pull request Aug 21, 2019

REQUEST: New membership for liu-cong kubernetes/org#1127

Closed

6 tasks

Even Pods Spread - 6. Integration Test #80011

Even Pods Spread - 6. Integration Test #80011

Conversation

Huang-Wei commented Jul 11, 2019 • edited Loading

Huang-Wei commented Jul 11, 2019

Huang-Wei commented Jul 11, 2019

Huang-Wei commented Jul 12, 2019

ahg-g commented Jul 29, 2019

Huang-Wei commented Jul 29, 2019

Huang-Wei commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahg-g left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 1, 2019

ahg-g commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

liu-cong commented Aug 1, 2019

k8s-ci-robot commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

fejta-bot commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

Huang-Wei commented Aug 1, 2019

ahg-g commented Aug 1, 2019

k8s-ci-robot commented Aug 2, 2019

Huang-Wei commented Jul 11, 2019 •

edited

Loading