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

Even Pods Spread - 6. Integration Test #80011

Merged
merged 1 commit into from Aug 2, 2019

Conversation

@Huang-Wei
Copy link
Member

commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

/retest

@Huang-Wei Huang-Wei referenced this pull request Jul 11, 2019

Open

Umbrella issue for Even Pods Spread [Alpha] #77284

6 of 9 tasks complete

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from f6d9c77 to 83b009f Jul 11, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 11, 2019

/retest

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2019

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.

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch 2 times, most recently from 4b89f08 to 95eba90 Jul 13, 2019

@ahg-g

This comment has been minimized.

Copy link
Member

commented Jul 29, 2019

Can you please rebase?

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from 95eba90 to b88af07 Jul 29, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

@ahg-g Rebase completed. PTAL.

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

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from b88af07 to c3c44d7 Jul 29, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XXL labels Jul 29, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

/retest

@@ -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 {

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 1, 2019

Author Member

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.

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

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

nit: add period at the end of the comment.

}

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@ahg-g
}
if err != nil {
// This could be a connection error so we want to retry.
return false, nil

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 1, 2019

Author Member

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

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 1, 2019

Author Member

Yes, sort of short-circuit.

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

nit: same as above, more specific naming

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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.

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

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

This comment has been minimized.

Copy link
@liu-cong

liu-cong Jul 29, 2019

Contributor

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

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 1, 2019

Author Member

Because node 0 has one pod already (L1001).

This comment has been minimized.

Copy link
@liu-cong

liu-cong Aug 1, 2019

Contributor

Thanks for explaining it to me.

@ahg-g
Copy link
Member

left a comment

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

}

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

This comment has been minimized.

Copy link
@ahg-g
pause := imageutils.GetPauseImageName()
tests := []struct {
name string
pod *v1.Pod

This comment has been minimized.

Copy link
@ahg-g
pod *v1.Pod
pods []*v1.Pod
fits bool
want []string // nodes expected to schedule onto

This comment has been minimized.

Copy link
@ahg-g

ahg-g Jul 31, 2019

Member

s/want/candidateNodes?

This comment has been minimized.

Copy link
@Huang-Wei

Huang-Wei Aug 1, 2019

Author Member

Done.

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from c3c44d7 to b9c760c Aug 1, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

[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:

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

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from b9c760c to 53ea857 Aug 1, 2019

@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

/lgtm

Thanks @Huang-Wei!

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 1, 2019

@Huang-Wei Huang-Wei force-pushed the Huang-Wei:eps-int-test branch from 53ea857 to caab8b7 Aug 1, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 1, 2019

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

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

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

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/hold cancel

@liu-cong

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

/lgtm

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 1, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/retest

@fejta-bot

This comment has been minimized.

Copy link

commented Aug 1, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/retest

1 similar comment
@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/retest

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

TestVolumeProvision keeps failing.

/retest

@Huang-Wei

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

/retest

@ahg-g

This comment has been minimized.

Copy link
Member

commented Aug 1, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 1, 2019

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

14 of 23 checks passed

pull-kubernetes-bazel-build Job triggered.
Details
pull-kubernetes-bazel-test Job triggered.
Details
pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-integration Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
pull-kubernetes-node-e2e Job triggered.
Details
pull-kubernetes-typecheck Job triggered.
Details
pull-kubernetes-verify Job triggered.
Details
cla/linuxfoundation Huang-Wei authorized
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

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

@Huang-Wei Huang-Wei deleted the Huang-Wei:eps-int-test branch Aug 2, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.