Generalize Node preparation for e2e and integration tests #35129

gmarek · 2016-10-19T15:41:43Z

This change is

k8s-ci-robot · 2016-10-19T18:38:32Z

Jenkins GCI GKE smoke e2e failed for commit d0f20fc. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

gmarek · 2016-10-20T06:58:54Z

@k8s-bot gci gke test this issue: #IGNORE
checking something

gmarek · 2016-10-20T07:08:31Z

cc @timothysc @jayunit100

wojtek-t · 2016-10-20T07:36:16Z

test/e2e/framework/util.go

+
+type E2ETestNodePreparer struct {
+	client                clientset.Interface
+	countToStrategy       map[int]testutils.PrepareNodeStrategy


Please add comments what these define.

wojtek-t · 2016-10-20T07:40:22Z

test/e2e/framework/util.go

+			}
+			for attempt := 0; attempt < UpdateRetries; attempt++ {
+				_, err = p.client.Core().Nodes().Patch(nodes.Items[index].Name, api.MergePatchType, []byte(patch))
+				if err != nil {


if err == nil || !apierrs.IsConflict(err) { break }

it's more readable.

wojtek-t · 2016-10-20T07:44:35Z

test/e2e/framework/util.go

+				if err != nil {
+					glog.Errorf("Skipping cleanup of Node: failed to get Node %v: %v", name, err)
+					encounteredError = true
+					continue nodeloop


I really don't like those "goto-like" instructions.

What I suggest instead is opaque this for attempt := ... into

func() { }

and then change those continue nodeLoop to returns.

wojtek-t · 2016-10-20T07:44:50Z

test/e2e/framework/util.go

+					continue nodeloop
+				}
+				_, err = p.client.Core().Nodes().Update(updatedNode)
+				if err != nil {


Change error handling to what I suggested above.

wojtek-t · 2016-10-20T07:46:02Z

test/integration/framework/perf_utils.go

+			GenerateName: p.nodeNamePrefix,
+		},
+		Spec: api.NodeSpec{
+			ExternalID: "foo",


What's that? And why this is equal for all nodes?

I have no idea what it is, but validation fails without it, even though in types.go it says that it's optional.

Can you please add a TODO to invistigate why this is always the same for all nodes?

wojtek-t · 2016-10-20T07:46:37Z

test/integration/framework/perf_utils.go

+	}
+	for i := 0; i < numNodes; i++ {
+		if _, err := p.client.Core().Nodes().Create(baseNode); err != nil {
+			panic("error creating node: " + err.Error())


Please return error instead of panic.

wojtek-t · 2016-10-20T07:47:32Z

test/integration/framework/perf_utils.go

+		}
+	}
+
+	nodes := e2eframework.GetReadySchedulableNodesOrDie(p.client)


Can we reuse the PrepareNodes function from test/e2e/framework? It seems pretty much the same.

There are small differences - e.g. in e2e one we do store map of strategy used to modify given Node, here we don't need to count/check number of Nodes. I can try to extract the common part.

wojtek-t · 2016-10-20T07:48:24Z

test/integration/scheduler_perf/scheduler_bench_test.go

+	)
+	err := nodePreparer.PrepareNodes()
+	if err != nil {
+		panic(err)


b.Fatalf(err)

wojtek-t · 2016-10-20T07:48:48Z

test/integration/scheduler_perf/scheduler_test.go

+		"scheduler-perf-",
+	)
+	err := nodePreparer.PrepareNodes()
+	if err != nil {


b.Fatalf(err)

gmarek · 2016-10-20T08:28:02Z

@wojtek-t PTAL

wojtek-t · 2016-10-20T08:53:52Z

test/e2e/framework/util.go

+		sum += k
+		for ; index < sum; index++ {
+			err := testutils.DoPrepareNode(p.client, &nodes.Items[index], strategy)
+			if err != nil {


if err := testutils.Do...; err != nil {

wojtek-t · 2016-10-20T08:54:21Z

test/e2e/framework/util.go

+		strategy, found := p.nodeToAppliedStrategy[name]
+		if found {
+			err = testutils.DoCleanupNode(p.client, name, strategy)
+			if err != nil {


if err := testutils.Do...; err != nil {

wojtek-t · 2016-10-20T08:55:44Z

test/e2e/framework/util.go

+			}
+		}
+	}
+	if encounteredError {


Maybe instead of having this a bool value, do:

var encouteredError error

and then when you set it to true set it to this error, and here just return this

wojtek-t · 2016-10-20T08:56:25Z

test/integration/framework/perf_utils.go

+			GenerateName: p.nodeNamePrefix,
+		},
+		Spec: api.NodeSpec{
+			ExternalID: "foo",


Can you please add a TODO to invistigate why this is always the same for all nodes?

wojtek-t · 2016-10-20T08:57:03Z

test/integration/framework/perf_utils.go

+		sum += k
+		for ; index < sum; index++ {
+			err := testutils.DoPrepareNode(p.client, &nodes.Items[index], strategy)
+			if err != nil {


if err := testutils...; err != nil

wojtek-t · 2016-10-20T08:57:19Z

test/integration/framework/perf_utils.go

+	nodes := e2eframework.GetReadySchedulableNodesOrDie(p.client)
+	for i := range nodes.Items {
+		err := p.client.Core().Nodes().Delete(nodes.Items[i].Name, &api.DeleteOptions{})
+		if err != nil {


if err := ... ; err != nil

wojtek-t · 2016-10-20T08:57:32Z

test/integration/scheduler_perf/scheduler_bench_test.go

+		map[int]testutils.PrepareNodeStrategy{numNodes: &testutils.TrivialNodePrepareStrategy{}},
+		"scheduler-perf-",
+	)
+	err := nodePreparer.PrepareNodes()


if err := ... ; err != nil

wojtek-t · 2016-10-20T08:57:39Z

test/integration/scheduler_perf/scheduler_test.go

+		map[int]testutils.PrepareNodeStrategy{numNodes: &testutils.TrivialNodePrepareStrategy{}},
+		"scheduler-perf-",
+	)
+	err := nodePreparer.PrepareNodes()


if err := ... ; err != nil

wojtek-t · 2016-10-20T08:59:39Z

test/utils/runners.go

+		return nil
+	}
+	for attempt := 0; attempt < retries; attempt++ {
+		if _, err = client.Core().Nodes().Patch(node.Name, api.MergePatchType, []byte(patch)); err != nil {


Why not just:

if err == nil || !apiserrs.IsConflict(err) { return err }

it's more readable

I want to print out the patch contents (it was useful for debugging some other stuff some time ago).

Got it. So maybe to at least avoid nested ifs:

if err == nil { return nil } else if !apierrs.IsConflict(err) { return ... }

wojtek-t · 2016-10-20T08:59:46Z

test/utils/runners.go

+			return nil
+		}
+		if _, err = client.Core().Nodes().Update(updatedNode); err != nil {
+			if !apierrs.IsConflict(err) {


Same as above.

gmarek · 2016-10-20T09:13:48Z

@wojtek-t PTAL

wojtek-t

Some minor comments.

Also please squash commits.

wojtek-t · 2016-10-20T09:22:02Z

test/utils/runners.go

+		return nil
+	}
+	for attempt := 0; attempt < retries; attempt++ {
+		if _, err = client.Core().Nodes().Patch(node.Name, api.MergePatchType, []byte(patch)); err != nil {


Got it. So maybe to at least avoid nested ifs:

if err == nil { return nil } else if !apierrs.IsConflict(err) { return ... }

wojtek-t · 2016-10-20T09:22:44Z

test/e2e/framework/util.go

+			}
+		}
+	}
+	if encounteredError != nil {


just
return encouteredError (you don't need this if)

gmarek · 2016-10-20T09:31:04Z

@wojtek-t squashed, PTAL.

wojtek-t · 2016-10-20T10:11:37Z

lgtm

k8s-ci-robot · 2016-10-20T15:44:57Z

Jenkins GKE smoke e2e failed for commit fbb3d6b. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

gmarek · 2016-10-20T19:12:19Z

@k8s-bot gke test this issue: #IGNORE
startup problem

k8s-github-robot · 2016-10-20T20:48:42Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2016-10-20T21:34:03Z

Automatic merge from submit-queue

Automatic merge from submit-queue Use TestNodePreparer in Density test Depends on kubernetes#35129

gmarek self-assigned this Oct 19, 2016

googlebot added the cla: yes label Oct 19, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Oct 19, 2016

gmarek force-pushed the generalize branch from 88f5ec3 to d0f20fc Compare October 19, 2016 18:05

gmarek added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Oct 20, 2016

gmarek changed the title ~~WIP: Generalize Node preparation for e2e and integration tests~~ Generalize Node preparation for e2e and integration tests Oct 20, 2016

gmarek assigned wojtek-t and unassigned gmarek Oct 20, 2016

wojtek-t reviewed Oct 20, 2016

View reviewed changes

gmarek force-pushed the generalize branch from d0f20fc to a6a87ad Compare October 20, 2016 08:27

wojtek-t reviewed Oct 20, 2016

View reviewed changes

gmarek force-pushed the generalize branch 2 times, most recently from 5021668 to 1ea32f2 Compare October 20, 2016 09:16

wojtek-t reviewed Oct 20, 2016

View reviewed changes

gmarek force-pushed the generalize branch from 1ea32f2 to 96dddca Compare October 20, 2016 09:30

gmarek mentioned this pull request Oct 20, 2016

Use TestNodePreparer in Density test #35184

Merged

Generalize Node preparation for e2e and integration tests

fbb3d6b

gmarek force-pushed the generalize branch from 96dddca to fbb3d6b Compare October 20, 2016 09:51

wojtek-t added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 20, 2016

k8s-github-robot merged commit 7a03564 into kubernetes:master Oct 20, 2016

sttts pushed a commit to sttts/kubernetes that referenced this pull request Oct 24, 2016

Merge pull request kubernetes#35184 from gmarek/density

40b66e1

Automatic merge from submit-queue Use TestNodePreparer in Density test Depends on kubernetes#35129

gmarek deleted the generalize branch February 21, 2017 14:18

Generalize Node preparation for e2e and integration tests #35129

Generalize Node preparation for e2e and integration tests #35129

Conversation

gmarek commented Oct 19, 2016 • edited by k8s-oncall Loading

k8s-ci-robot commented Oct 19, 2016

gmarek commented Oct 20, 2016

gmarek commented Oct 20, 2016

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmarek commented Oct 20, 2016

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

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

gmarek commented Oct 20, 2016

wojtek-t 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

gmarek commented Oct 20, 2016

wojtek-t commented Oct 20, 2016

k8s-ci-robot commented Oct 20, 2016

gmarek commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

k8s-github-robot commented Oct 20, 2016

gmarek commented Oct 19, 2016 •

edited by k8s-oncall

Loading