Implementation of e2e test that re-sizes cluster #8243

jszczepkowski · 2015-05-14T10:23:03Z

Implementation of e2e test that re-sizes cluster; currently works on GCE.

gmarek · 2015-05-14T12:04:47Z

test/e2e/util.go

+// Context for checking pods responses by issuing GETs to them and verifying if the answer with pod name.
+type podResponseChecker struct {
+	c              *client.Client
+	ns             string


I don't like using abbreviations, especially one for namespace.

"ns" is a commonly used abbreviation for namespace in kubernetes, and, AFAK, golang standard in general encourages short variable names.

OK, I just wanted to mention that I don't like it:) It took me ~1sec to figure out what it stands for.

It took me a minute the first time I saw one too, but I think at this point ns is pretty common.

Yeah. If it's not only me maybe we should start using full 'namespace' for this...

I agree with @gmarek. Feel free to do a holistic cleanup of the e2e tests in this regard.

Now that I'm used to it I kind of like the brevity... but regardless, bikeshedding for a separate PR :-)

zmerlynn · 2015-05-14T16:00:02Z

I'm ... ugh. I wish I knew a better way to write this test, because the invasiveness of the variables at the top level, the grosness of shelling out to gcloud in the test - there's a lot that I see wrong and potentially very brittle here.

cc @cjcullen

roberthbailey · 2015-05-14T17:23:29Z

@mbforbes was looking into something similar and may have some suggestions.

mbforbes · 2015-05-14T18:18:35Z

cmd/e2e/e2e.go

@@ -55,6 +55,8 @@ func init() {
 	flag.StringVar(&cloudConfig.MasterName, "kube-master", "", "Name of the kubernetes master. Only required if provider is gce or gke")
 	flag.StringVar(&cloudConfig.ProjectID, "gce-project", "", "The GCE project being used, if applicable")
 	flag.StringVar(&cloudConfig.Zone, "gce-zone", "", "GCE zone being used, if applicable")
+	flag.StringVar(&cloudConfig.NodeInstanceGroup, "node-instance-group", "", "Name of the manage instance group for nodes. Valid only for gce and gke")


nit: s/manage/managed

Also note this is not valid for GKE

jszczepkowski · 2015-05-15T13:08:56Z

@mbforbes
All comments are applied, PTAL

gmarek · 2015-05-15T13:49:08Z

test/e2e/resize_nodes.go

+	}
+}
+
+func verifyPodsListining(c *client.Client, ns, name string, pods *api.PodList) {


verifyPodsResponding? By Listing I understand calling 'get pods' on API server.

same comment here—please change to return an error

Please either change to verifyPodsResponding as @gmarek suggested, or s/Listining/Listening

mbforbes · 2015-05-15T22:07:26Z

test/e2e/resize_nodes.go

+			Failf("Couldn't restore the original node instance group size: %s", err)
+		}
+		if err := waitForNodeInstanceGroupSize(testContext.CloudConfig.NumNodes); err != nil {
+			Failf("Couldn't restore the original node instance group size: %s", err)


nit: I think errors are typically logged with %v

mbforbes · 2015-05-15T22:24:08Z

OK, I think I've thrown enough comments at you for the time being :-) Please ping and I'll be happy to look again.

The one thing I wanted to ask explicitly—so it doesn't get lost in my syntactic comments—is whether the pods you're creating ("serve hostname") are guaranteed to run only one per node? I think that's what this test is intending, and what is implied, but I wanted to make sure that it's true. I think this can be done by ensuring that the port is locked so it'll conflict if two of the same pod try to get scheduled on a single node. Is the Ports: []api.ContainerPort{{ContainerPort: 9376}}, line in newReplicationController(...) doing that?

jszczepkowski · 2015-05-20T00:57:11Z

@mbforbes
All comments are applied now, PTAL.

Regarding distributing single pod per node: I achieve it by creating a service spanning all the pods (our scheduler distributes pods from the same service to different nodes).

mbforbes · 2015-05-21T02:14:24Z

test/e2e/resize_nodes.go

+		return -1, err
+	}
+	pattern := "currentSize: "
+	i := bytes.Index(output, []byte(pattern))


sorry if I missed this in the giant comment stream, but is there a reason not to use strings methods (https://golang.org/pkg/strings/) so you don't have to convert to []byte?

Sorry, I missed it. Done

mbforbes · 2015-05-21T02:37:33Z

Awesome, this is looking gorgeous; thanks so much for your patience @jszczepkowski.

I've left a handful more minor comments, but this LGTM after they're resolved.

mbforbes · 2015-05-21T18:13:10Z

hack/ginkgo-e2e.sh

 ginkgo_args=()
 if [[ ${GINKGO_PARALLEL} =~ ^[yY]$ ]]; then
  ginkgo_args+=("-p")
 fi

+# Use the kubectl binary from the same directory as the e2e binary.


Is this an intentionally added line? It looks like it might be a merge artifact?

It seems to be an artifact, removed.

jszczepkowski · 2015-05-22T06:43:00Z

@mbforbes PTAL

mbforbes · 2015-05-22T16:50:18Z

Looks good, thanks @jszczepkowski!

I'm still a little unclear on the pod rescheduling: because we decrease the number of nodes and ensure the pods are still running, it seems like the pods must not run with a hard requirement of 1/node. That means that the resize-up test could pass without any work being scheduled on the new node.

But regardless, it'd be good to get this test in--in the off-chance I'm not confused about this, we can always tweak it a bit later.

Implementation of e2e test that re-sizes cluster

googlebot added the cla: yes label May 14, 2015

jszczepkowski assigned gmarek May 14, 2015

jszczepkowski force-pushed the e2e-nodes branch from 7504422 to f9a8982 Compare May 14, 2015 11:43

gmarek reviewed May 14, 2015
View reviewed changes

jszczepkowski assigned ghost and unassigned gmarek May 14, 2015

mbforbes reviewed May 14, 2015
View reviewed changes

ghost assigned mbforbes and unassigned ghost May 14, 2015

gmarek reviewed May 15, 2015
View reviewed changes

mbforbes reviewed May 15, 2015
View reviewed changes

jszczepkowski force-pushed the e2e-nodes branch from a61690c to d34d6ee Compare May 20, 2015 00:52

jszczepkowski mentioned this pull request May 20, 2015

Write tests for dead node evacuation and rebooted node population #7051

Closed

mbforbes reviewed May 21, 2015
View reviewed changes

Implementation of e2e test that resizes cluster (works on GCE).

ef965a6

jszczepkowski force-pushed the e2e-nodes branch from d34d6ee to ef965a6 Compare May 22, 2015 06:38

mbforbes added a commit that referenced this pull request May 22, 2015

Merge pull request #8243 from jszczepkowski/e2e-nodes

14e47f2

Implementation of e2e test that re-sizes cluster

mbforbes merged commit 14e47f2 into kubernetes:master May 22, 2015

dchen1107 mentioned this pull request May 22, 2015

ResizeNode e2e test is flaky #8715

Closed

jszczepkowski mentioned this pull request Jul 23, 2015

Implement node autoscaler for GCE #11748

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation of e2e test that re-sizes cluster #8243

Implementation of e2e test that re-sizes cluster #8243

jszczepkowski commented May 14, 2015

gmarek May 14, 2015

jszczepkowski May 14, 2015

gmarek May 14, 2015

mbforbes May 14, 2015

gmarek May 15, 2015

ghost May 15, 2015

mbforbes May 15, 2015

zmerlynn commented May 14, 2015

roberthbailey commented May 14, 2015

mbforbes May 14, 2015

mbforbes May 14, 2015

jszczepkowski May 15, 2015

jszczepkowski commented May 15, 2015

gmarek May 15, 2015

mbforbes May 15, 2015

mbforbes May 15, 2015

jszczepkowski May 19, 2015

mbforbes May 15, 2015

jszczepkowski May 19, 2015

mbforbes commented May 15, 2015

jszczepkowski commented May 20, 2015

mbforbes May 21, 2015

jszczepkowski May 22, 2015

jszczepkowski May 22, 2015

mbforbes commented May 21, 2015

mbforbes May 21, 2015

jszczepkowski May 21, 2015

jszczepkowski commented May 22, 2015

mbforbes commented May 22, 2015

Implementation of e2e test that re-sizes cluster #8243

Implementation of e2e test that re-sizes cluster #8243

Conversation

jszczepkowski commented May 14, 2015

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

zmerlynn commented May 14, 2015

roberthbailey commented May 14, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszczepkowski commented May 15, 2015

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

mbforbes commented May 15, 2015

jszczepkowski commented May 20, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbforbes commented May 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jszczepkowski commented May 22, 2015

mbforbes commented May 22, 2015