e2e.go: Add -deployment, add a kops deployment method #33518

zmerlynn · 2016-09-26T22:08:12Z

What this PR does / why we need it: Adds a kops deployment method to e2e.go, so we can add full e2e coverage for a kops based bringup.

Special notes for your reviewer: A timely review would be appreciated given the wide-ish touchpoints through the file. I just had a pretty bad rebase here.

Release note:

Adds the -deployment option to e2e.go, adds the ability to run e2e.go using a `kops` deployment.

This splits off all the bash stuff into an interface, and plumbs
through a separate interface to bring up a cluster using "kops"
instead. Right now it assumes kops == AWS.

This change is

zmerlynn · 2016-09-26T22:08:40Z

cc @kubernetes/test-infra-maintainers @kubernetes/sig-aws

fejta

Exciting! This looks great although I have concerns about the download bit.

fejta · 2016-09-27T00:32:32Z

hack/e2e.go

 	return finishRunning("up", exec.Command("./hack/e2e-internal/e2e-up.sh"))
 }

-// Is the e2e cluster up?
-func IsUp() bool {
+func (b bash) IsUp() bool {


Will you sync up with Joe who is switching this to error?

Done and rebased.

fejta · 2016-09-27T00:33:39Z

hack/e2e.go

+	if err != nil {
+		return fmt.Errorf("error creating deployer: %v", err)
+	}
+	defer deploy.Destroy()


Should we xmlWrap this?

Obviated, killed Destroy in the last rev after offline convo.

fejta · 2016-09-27T00:35:43Z

hack/e2e.go

+		var err error
+		binaryURL := os.Getenv("KOPS_BINARY_DIR_URL")
+		if binaryURL == "" {
+			binaryURL, err = download("https://storage.googleapis.com/kops-ci/bin/latest-ci.txt")


Where does this code live?

The repo is at https://github.com/kubernetes/kops

fejta · 2016-09-27T00:36:23Z

hack/e2e.go

+}
+
+func download(url string) (string, error) {
+	resp, err := http.Get(url)


How does this handle intermittent flakes?

Obviated by next rev.

fejta · 2016-09-27T00:38:43Z

hack/e2e.go

+		return nil, fmt.Errorf("KOPS_STATE_STORE must be set to a valid S3 path for kops deployment")
+	}
+	// Presume the kops binary was supplied to us.
+	binary := os.Getenv("KOPS_BINARY")


I would prefer we make this a flag. I would like us to move away from magical environment variables that are hard to discover.

What do you think about making the download and cleanup of the kops binary happen outside e2e.go? These two tasks do not seem germane to running tests.

Talked offline, and then I worked some more with this. I think I can do the download entirely in shell in .yaml pretty easily, so I'm not sure it's worth the separate binary. I reworked it to be all flags and take the necessary bits coming in, and I can iterate further from there.

fejta · 2016-09-27T00:40:23Z

PS: Please sync up with @spxtr about how to test this before committing it. Also note that this requires pushing a new kubekins-e2e image once it is committed.

justinsb

kops interaction LGTM!

justinsb · 2016-09-27T04:55:41Z

hack/e2e.go

+		var err error
+		binaryURL := os.Getenv("KOPS_BINARY_DIR_URL")
+		if binaryURL == "" {
+			binaryURL, err = download("https://storage.googleapis.com/kops-ci/bin/latest-ci.txt")


The repo is at https://github.com/kubernetes/kops

justinsb · 2016-09-27T04:58:17Z

hack/e2e.go

+			log.Printf("Can't get cluster size, sleeping: %v", err)
+			continue
+		}
+		if n < k.nodes {


The master is registered as a node, as is not included in --node-count, so I think we want nodes + 1 (at least until we start testing HA master :-) )

Yeah, I meant to fix that, oops. :)

justinsb · 2016-09-27T04:59:05Z

hack/e2e.go

+		// Assume that if we already have it, it's good.
+		return nil
+	}
+	if err := finishRunning("kops export", exec.Command(k.binary, "export", "kubecfg", k.cluster)); err != nil {


This is nice, although actually kops update will actually already have exported it for you - as you inevitably (?) want the kubecfg if you're dealing with the cluster...

That was actually so you could point it at an existing kops-cluster and just say -test and it would Just Work, which is way better than GCE.

Also, in case the kubecfg section is unclear, this is striving to ensure that if you point it at a specific cluster, it tests that cluster, hence the /tmp kubecfg that's basically isolated to the cluster in question. There's a longstanding badness in the GCE bash-isms where it's just implicitly adding it to the default kubecfg, which can get sliced if you run -up -test in parallel, e.g.

justinsb · 2016-09-27T05:00:12Z

hack/e2e.go

+func (k kops) Down() error {
+	// We do a "kops get" first so the exit status of "kops delete" is
+	// more sensical in the case of a non-existant cluster. ("kops
+	// delete" will exit with status 1 on a non-existant cluster)


Do you think we should treat that as not-an-error in kops? Or maybe a different exit code?

I'm not clear on that. I actually didn't feel like fighting the exit status. Maybe it should be a 0 exit code for delete, but a 1 exit code for get?

zmerlynn · 2016-09-27T16:34:34Z

@fejta: PTAL. Syncing with @spxtr now.

errordeveloper · 2016-09-27T17:06:19Z

This looks like a great start, I will need to spend some time to understand how this can be used with kubeadm.

zmerlynn · 2016-09-27T17:10:39Z

@errordeveloper: Yeah, I gave @mikedanese a heads up I was doing this, so it should be a reasonable start.

k8s-ci-robot · 2016-09-27T18:03:32Z

Jenkins verification failed for commit 33ff8c577a5651dafa55b6d21fae0af3bb3c19a9. Full PR test history.

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

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

errordeveloper · 2016-09-27T18:28:24Z

@zmerlynn thanks for that, so @mikedanese actually mentioned it to on @kubernetes/sig-cluster-lifecycle call today. I am now thinking about it. Until now, I was only thinking about sort of acceptance tests for kubeadm, where we have rpm/deb packages, dockerd and systemd all setup and run kubeadm, which is all much lower level then this, and we need to detect breakages at the level early, but we do need e2e test to run on top soon also.

zmerlynn · 2016-09-27T18:41:44Z

@errordeveloper: My goal here is actually to get kops basic deployment into submit queue / PR builder territory, so if someone totally trashes AWS for some obscure reason, it's really easy to see. See kubernetes/test-infra#681 for a proposed updown job, which is a copy of one I added for GKE that just runs a barebones deployment / test networking / down. I've found the networking conformance tests are an excellent very short sniff test for basic cluster operation in the past (like O(<1m) to run, so it makes a really good smoke).

luxas · 2016-09-27T18:44:29Z

+1

@errordeveloper I think those are two different kinds of layers you're mentioning.
Indeed, the cluster has to be deployed first (via kube-up, kops or kubeadm), but after that I think the e2e process should be pretty the same.

"Acceptance testing" for kubeadm (which is more lightweight) also sounds good to me indeed.
But we need a place (maybe test-infra) where we can put the few kubeadm commands needed for bootstrapping a cluster in an automated fashion in order to be able to test kubeadm properly.

zmerlynn · 2016-09-27T20:43:46Z

I confirmed manually from build logs that, even though some of the builds were failing, the builds were doing what we wanted. I just popped the manual testing commit.

k8s-ci-robot · 2016-09-27T21:01:25Z

Jenkins GCI GKE smoke e2e failed for commit ce28398. 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.

spxtr

Looks good overall. It might be worth making "kops-nodes" just "nodes" and including that behavior in the bash deployer, but that can be done in the future if we want.

spxtr · 2016-09-27T20:55:37Z

hack/e2e.go

@@ -157,6 +167,11 @@ func run() error {
 		}
 	}

+	deploy, err := getDeployer()


I would suggest moving this into main and passing the deployer to run.

spxtr · 2016-09-27T21:05:09Z

hack/e2e.go

+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()


I'm slightly worried about relying on the existence of a temp file after we've closed it, but it's probably fine.

The ioutil.TempFile semantics leave the file until you unlink it, so this should be fine.

Sounds good.

spxtr · 2016-09-27T21:14:46Z

hack/e2e.go

+	// TODO(zmerlynn): More cluster validation. This should perhaps be
+	// added to kops and not here, but this is a fine place to loop
+	// for now.
+	for stop := time.Now().Add(10 * time.Minute); time.Now().Before(stop); time.Sleep(30 * time.Second) {


This is pretty neat.

spxtr · 2016-09-27T21:16:36Z

hack/e2e.go

+
+func (k kops) ClusterSize() (int, error) {
+	if err := k.SetupKubecfg(); err != nil {
+		return -1, err


Any reason to return -1 over 0?

Yeah. If anyone is looking at anything but err, I want to throw it in their face. :)

spxtr · 2016-09-27T21:19:29Z

hack/e2e.go

+	if err := k.SetupKubecfg(); err != nil {
+		return -1, err
+	}
+	o, err := exec.Command("kubectl", "get", "nodes", "--no-headers").Output()


Seems like this is independent of the deployer.

I'm a little wary of parsing raw command output, but I think this is relatively innocuous.

I moved it out of the kops impl so that all it did was call the deployer SetupKubecfg

This splits off all the bash stuff into an interface, and plumbs through a separate interface to bring up a cluster using "kops" instead. Right now it assumes kops == AWS.

zmerlynn · 2016-09-27T21:49:09Z

@spxtr: PTAL. Also have no idea what's going on with @k8s-ci-robot right now, but the GKE builder is failing consistently.

spxtr · 2016-09-27T22:01:45Z

hack/e2e.go

+	if err != nil {
+		return nil, err
+	}
+	defer f.Close()


Sounds good.

k8s-ci-robot · 2016-09-27T22:28:38Z

Jenkins GCI GCE e2e failed for commit d905478. Full PR test history.

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

k8s-ci-robot · 2016-09-27T22:36:09Z

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

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

k8s-github-robot · 2016-09-27T22:41:28Z

Automatic merge from submit-queue

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn assigned fejta Sep 26, 2016

googlebot added the cla: yes label Sep 26, 2016

zmerlynn added area/platform/aws area/test labels Sep 26, 2016

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

zmerlynn force-pushed the e2e-kops-up-2 branch from 7b3088e to c21276b Compare September 26, 2016 22:34

zmerlynn added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Sep 26, 2016

zmerlynn force-pushed the e2e-kops-up-2 branch from c21276b to b4a3166 Compare September 26, 2016 23:52

fejta reviewed Sep 27, 2016

View reviewed changes

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2016

justinsb approved these changes Sep 27, 2016

View reviewed changes

zmerlynn force-pushed the e2e-kops-up-2 branch from b4a3166 to 8193aa1 Compare September 27, 2016 16:33

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 27, 2016

zmerlynn force-pushed the e2e-kops-up-2 branch from fe223e9 to 33ff8c5 Compare September 27, 2016 17:38

zmerlynn force-pushed the e2e-kops-up-2 branch from 33ff8c5 to 3f92d11 Compare September 27, 2016 18:11

zmerlynn added a commit to zmerlynn/test-infra that referenced this pull request Sep 27, 2016

kops: Add a kops-updown job

4ea5123

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn mentioned this pull request Sep 27, 2016

kops: Add a kops-updown job kubernetes/test-infra#681

Merged

zmerlynn force-pushed the e2e-kops-up-2 branch from 3f92d11 to ce28398 Compare September 27, 2016 20:42

zmerlynn assigned spxtr and unassigned fejta Sep 27, 2016

spxtr reviewed Sep 27, 2016

View reviewed changes

e2e.go: Add -deployment, add a kops deployment method

d905478

This splits off all the bash stuff into an interface, and plumbs through a separate interface to bring up a cluster using "kops" instead. Right now it assumes kops == AWS.

zmerlynn force-pushed the e2e-kops-up-2 branch from ce28398 to d905478 Compare September 27, 2016 21:48

spxtr approved these changes Sep 27, 2016

View reviewed changes

hack/e2e.go

if err != nil {

return nil, err

}

defer f.Close()

Copy link

Contributor

spxtr Sep 27, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good.

zmerlynn added lgtm "Looks good to me", indicates that a PR is ready to be merged. retest-not-required labels Sep 27, 2016

k8s-github-robot merged commit 1f33091 into kubernetes:master Sep 27, 2016

zmerlynn deleted the e2e-kops-up-2 branch September 27, 2016 22:41

zmerlynn added a commit to zmerlynn/test-infra that referenced this pull request Sep 28, 2016

kops: Add a kops-updown job

f561e5b

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn added a commit to zmerlynn/test-infra that referenced this pull request Sep 28, 2016

kops: Add a kops-updown job

76852d2

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn added a commit to zmerlynn/test-infra that referenced this pull request Sep 28, 2016

kops: Add a kops-updown job

ed84680

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

zmerlynn added a commit to zmerlynn/test-infra that referenced this pull request Oct 3, 2016

kops: Add a kops-updown job

9bc6f1c

This is dipping a toe in the water. Companion to kubernetes/kubernetes#33518, and I won't merge it until that's in and a new docker image is pushed.

e2e.go: Add -deployment, add a kops deployment method #33518

e2e.go: Add -deployment, add a kops deployment method #33518

Conversation

zmerlynn commented Sep 26, 2016 • edited by k8s-oncall Loading

zmerlynn commented Sep 26, 2016

fejta 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

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

fejta commented Sep 27, 2016

justinsb 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

justinsb Sep 27, 2016 • edited Loading

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 Sep 27, 2016

errordeveloper commented Sep 27, 2016

zmerlynn commented Sep 27, 2016

k8s-ci-robot commented Sep 27, 2016

errordeveloper commented Sep 27, 2016

zmerlynn commented Sep 27, 2016

luxas commented Sep 27, 2016

zmerlynn commented Sep 27, 2016

k8s-ci-robot commented Sep 27, 2016

spxtr 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

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 Sep 27, 2016

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 27, 2016

k8s-ci-robot commented Sep 27, 2016

k8s-github-robot commented Sep 27, 2016

zmerlynn commented Sep 26, 2016 •

edited by k8s-oncall

Loading

justinsb Sep 27, 2016 •

edited

Loading