Adding e2e tests for validating cascading deletion of federation resources #36630

nikhiljindal · 2016-11-11T06:19:30Z

Adding e2e tests for code in #36330 and #36476.

Adding test cases to ensure that cascading deletion happens as expected.
Also adding code to ensure that all resources are cleaned up in AfterEach.

cc @kubernetes/sig-cluster-federation @caesarxuchao @madhusudancs

This change is

nikhiljindal · 2016-11-11T06:21:22Z

Adding to 1.5 milestone.
Test only changes should be fine.
cc @saad-ali as FYI.

nikhiljindal · 2016-11-11T06:21:50Z

@k8s-bot federation gce e2e test this

nikhiljindal · 2016-11-11T07:44:12Z

federation e2e failed to come up. It timed out waiting for load balancer status. Unrelated to this PR. Retrying.

@k8s-bot federation gce e2e test this

dims · 2016-11-11T12:16:54Z

looks like unrelated errors:

W1111 00:28:53.685] ssh: connect to host 107.178.220.197 port 22: Connection timed out
W1111 00:28:53.690] ERROR: (gcloud.compute.ssh) [/usr/bin/ssh] exited with return code [255]. See https://cloud.google.com/compute/docs/troubleshooting#ssherrors for troubleshooting hints.

dims · 2016-11-11T12:17:04Z

@k8s-bot federation gce e2e test this

nikhiljindal · 2016-11-11T19:55:50Z

@k8s-bot federation gce e2e test this

saad-ali · 2016-11-12T01:06:54Z

Adding to 1.5 milestone.
Test only changes should be fine.
cc @saad-ali as FYI.

Ack. Thanks.

dims · 2016-11-13T18:53:45Z

@k8s-bot federation gce e2e test this

dims · 2016-11-14T00:04:13Z

@nikhiljindal looks like the tests never ends. i see a started.json from 4 hours ago and it has not finished yet

http://gcsweb.k8s.io/gcs/kubernetes-jenkins/pr-logs/pull/36630/pull-kubernetes-federation-e2e-gce/22

nikhiljindal · 2016-11-15T22:47:48Z

haha I still see it as running :)
Will try again after rebasing

nikhiljindal · 2016-11-15T22:49:02Z

@k8s-bot federation gce e2e test this

k8s-ci-robot · 2016-11-15T22:59:48Z

Jenkins GCE etcd3 e2e failed for commit b3759b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 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-11-15T22:59:48Z

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

k8s-ci-robot · 2016-11-15T22:59:49Z

Jenkins GCE e2e failed for commit b3759b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm 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-11-15T23:00:19Z

Jenkins GCI GCE e2e failed for commit b3759b9. 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-11-15T23:00:19Z

Jenkins Kubemark GCE e2e failed for commit b3759b9. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark 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-11-15T23:01:17Z

Jenkins Federation GCE e2e failed for commit b3759b9. Full PR test history.

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

nikhiljindal · 2016-11-16T00:52:44Z

GCE e2e failed with jq: command not found which is unrelated.
Retrying.
@k8s-bot cvm gce e2e test this

Also verified that all federation e2es are passing by running all of them locally.

@mwielgus Can you please take a look

nikhiljindal · 2016-11-16T18:36:14Z

Adding @madhusudancs as second reviewer since @mwielgus seems busy this week.

madhusudancs

@nikhiljindal Mostly minor comments.

madhusudancs · 2016-11-16T21:00:26Z

test/e2e/federated-ingress.go

+	IngressList, err := clientset.Extensions().Ingresses(nsName).List(v1.ListOptions{})
+	Expect(err).NotTo(HaveOccurred())
+	orphanDependents := false
+	for _, Ingress := range IngressList.Items {


Use DeleteCollection instead of looping through the items?

found there was an existing function here doing the same thing. Changed the code to reuse that

madhusudancs · 2016-11-16T21:00:57Z

test/e2e/federated-ingress.go

+// Verifies that ingresses are deleted from underlying clusters when orphan dependents is false
+// and they are not deleted when orphan dependents is true.
+func verifyCascadingDeletionForIngress(clientset *fedclientset.Clientset,
+	clusters map[string]*cluster, orphanDependents *bool, nsName string) {


Why this line break? We generally don't do it in Go.

old habit. Fixed

madhusudancs · 2016-11-16T21:01:25Z

test/e2e/federated-ingress.go

 /*
   equivalent returns true if the two ingress spec are equivalent.
 */
 func equivalentIngress(federatedIngress, clusterIngress v1beta1.Ingress) bool {
 	return reflect.DeepEqual(clusterIngress.Spec, federatedIngress.Spec)
 }

+// Verifies that ingresses are deleted from underlying clusters when orphan dependents is false


"verifyCascadingDeletionForIngress verifies that ..." is the recommended way, right?

Yes. We are not as strict about that for private functions. Fixed it here

madhusudancs · 2016-11-16T21:03:06Z

test/e2e/federated-ingress.go

+	ingressName := ingress.Name
+	// Check subclusters if the ingress was created there.
+	By(fmt.Sprintf("Waiting for ingress %s to be created in all underlying clusters", ingressName))
+	err := wait.Poll(5*time.Second, 2*time.Minute, func() (bool, error) {


Make 5*time.Second, 2*time.Minute package level constants?

removed this code to reuse an existing function

madhusudancs · 2016-11-16T21:05:25Z

test/e2e/federated-ingress.go

+	err := wait.Poll(5*time.Second, 2*time.Minute, func() (bool, error) {
+		for _, cluster := range clusters {
+			_, err := cluster.Extensions().Ingresses(nsName).Get(ingressName)
+			if err != nil {


I think this is somewhat more straightforward and easier to read/understand if it is written as:

if !errors.IsNotFound(err) { return false, err } if err != nil { return false, nil }

Problem with that is that !errors.IsNotFound(err) is true when err = nil. We do not want to return false in that case.
Changed it to:

if err != nil && errors.IsNotFound(err) { return false, nil } if err != nil { return false, err }

madhusudancs · 2016-11-16T22:22:19Z

test/e2e/federation-replicaset.go

+	errMessages := []string{}
+	for clusterName, clusterClientset := range clusters {
+		_, err := clusterClientset.Extensions().ReplicaSets(nsName).Get(replicaSetName)
+		if (orphanDependents == nil || *orphanDependents == true) && errors.IsNotFound(err) {


Same comment as above.

madhusudancs · 2016-11-16T22:22:37Z

test/e2e/federation-replicaset.go

+	By(fmt.Sprintf("Deleting replicaSet %s", replicaSetName))
+	deleteReplicaSetOrFail(clientset, nsName, replicaSetName, orphanDependents)
+
+	By(fmt.Sprintf("Verifying replicaSets %s in underlying clusters", replicaSetName))


ReplicaSets or replica sets. Here and everywhere else.

madhusudancs · 2016-11-16T22:22:43Z

test/e2e/federation-replicaset.go

+		}
+		return true, nil
+	})
+	framework.ExpectNoError(err, "Not all replicaSets created")


ReplicaSets or replica sets.

madhusudancs · 2016-11-16T22:23:09Z

test/e2e/federation-replicaset.go

+	replicaSet := createReplicaSetOrFail(clientset, nsName)
+	replicaSetName := replicaSet.Name
+	// Check subclusters if the replicaSet was created there.
+	By(fmt.Sprintf("Waiting for replicaSet %s to be created in all underlying clusters", replicaSetName))


ReplicaSets or replica sets.

fixed. here and at other places

madhusudancs · 2016-11-16T22:24:00Z

test/e2e/federation-replicaset.go

+	// Wait for the replicaSet to be deleted.
+	err = wait.Poll(5*time.Second, wait.ForeverTestTimeout, func() (bool, error) {
+		_, err := clientset.Extensions().ReplicaSets(nsName).Get(replicaSetName)
+		if err != nil && errors.IsNotFound(err) {


Same comment as above.

nikhiljindal · 2016-11-18T21:10:29Z

Thanks @madhusudancs Pushed a new commit with fixes.
PTAL

madhusudancs · 2016-11-18T22:21:48Z

test/e2e/federated-ingress.go

+	shouldExist := orphanDependents == nil || *orphanDependents == true
+	for clusterName, clusterClientset := range clusters {
+		_, err := clusterClientset.Extensions().Ingresses(nsName).Get(ingressName)
+		if shouldExist && errors.IsNotFound(err) {


I am not a big fan of having lots of logic in tests and I still think it should be split into 3 separate functions. But it is fine for now, I guess.

madhusudancs · 2016-11-18T22:34:36Z

LGTM!

nikhiljindal · 2016-11-18T22:44:40Z

Squashed commits.
Thanks @madhusudancs

k8s-ci-robot · 2016-11-18T23:36:58Z

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

k8s-github-robot · 2016-11-19T00:12:08Z

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

k8s-github-robot · 2016-11-19T01:03:27Z

Automatic merge from submit-queue

googlebot added the cla: yes label Nov 11, 2016

nikhiljindal added area/cluster-federation area/test release-note-none Denotes a PR that doesn't merit a release note. labels Nov 11, 2016

nikhiljindal added this to the v1.5 milestone Nov 11, 2016

k8s-github-robot assigned ixdy Nov 11, 2016

k8s-github-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Nov 11, 2016

nikhiljindal assigned mwielgus and unassigned ixdy Nov 11, 2016

nikhiljindal force-pushed the E2edsi branch from 8dba5be to 3e4b652 Compare November 11, 2016 06:27

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

nikhiljindal force-pushed the E2edsi branch from 3e4b652 to b3759b9 Compare November 15, 2016 22:48

nikhiljindal removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2016

nikhiljindal assigned madhusudancs Nov 16, 2016

madhusudancs reviewed Nov 16, 2016

View reviewed changes

madhusudancs reviewed Nov 18, 2016

View reviewed changes

madhusudancs added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2016

nikhiljindal added 2 commits November 18, 2016 14:44

Updating federation e2e tests to verify cascading deletion

14246a9

Automated bazel and owner changes

90645ce

nikhiljindal removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2016

nikhiljindal force-pushed the E2edsi branch from ee6b57d to 90645ce Compare November 18, 2016 22:44

nikhiljindal added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 18, 2016

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 18, 2016

k8s-github-robot merged commit c183664 into kubernetes:master Nov 19, 2016

nikhiljindal mentioned this pull request Nov 20, 2016

e2e Tests for Federated Daemonsets are not running #36665

Closed

ixdy mentioned this pull request Nov 21, 2016

increasing the timeout for federation e2e tests kubernetes/test-infra#1129

Merged

Adding e2e tests for validating cascading deletion of federation resources #36630

Adding e2e tests for validating cascading deletion of federation resources #36630

Conversation

nikhiljindal commented Nov 11, 2016 • edited Loading

nikhiljindal commented Nov 11, 2016

nikhiljindal commented Nov 11, 2016

nikhiljindal commented Nov 11, 2016

dims commented Nov 11, 2016

dims commented Nov 11, 2016

nikhiljindal commented Nov 11, 2016

saad-ali commented Nov 12, 2016

dims commented Nov 13, 2016

dims commented Nov 14, 2016

nikhiljindal commented Nov 15, 2016

nikhiljindal commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

k8s-ci-robot commented Nov 15, 2016

nikhiljindal commented Nov 16, 2016

nikhiljindal commented Nov 16, 2016

madhusudancs 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

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

nikhiljindal commented Nov 18, 2016

Choose a reason for hiding this comment

madhusudancs commented Nov 18, 2016

nikhiljindal commented Nov 18, 2016

k8s-ci-robot commented Nov 18, 2016

k8s-github-robot commented Nov 19, 2016

k8s-github-robot commented Nov 19, 2016

nikhiljindal commented Nov 11, 2016 •

edited

Loading