Test case for scalling down before scale up finished #34454

dshulyak · 2016-10-10T15:25:58Z

Verifies that current pet (one which was created last by scale up action)
will enter running before scale down will take place

related: #30082

This change is

dshulyak · 2016-10-10T15:27:35Z

Includes #32868

@bprashanth PTAL

erictune · 2016-10-10T16:33:28Z

@janetkuo PTAL

janetkuo · 2016-10-11T22:13:08Z

test/e2e/petset.go

+		It("scaling down before scale up is finished will wait until current pod will be running and ready", func() {
+			By("creating petset " + psName + " in namespace " + ns)
+			testProbe := &api.Probe{Handler: api.Handler{HTTPGet: &api.HTTPGetAction{Path: "/index.html", Port: intstr.IntOrString{IntVal: 80}}}}
+			ps := newPetSet(psName, ns, headlessSvcName, 1, nil, nil, labels, testProbe)


nit: group these input arguments of newPetSet to make it more clear

janetkuo · 2016-10-11T22:14:26Z

test/e2e/petset.go

+			ps := newPetSet(psName, ns, headlessSvcName, 1, nil, nil, labels, testProbe)
+			ps, err := c.Apps().PetSets(ns).Create(ps)
+			pst := &petSetTester{c: c}
+			Expect(err).NotTo(HaveOccurred())


move the error check to right after it's returned

janetkuo · 2016-10-11T22:17:10Z

test/e2e/petset.go

@@ -341,6 +341,41 @@ var _ = framework.KubeDescribe("PetSet [Slow] [Feature:PetSet]", func() {
 				previous = event.pod.Name
 			}
 		})
+
+		It("scaling down before scale up is finished will wait until current pod will be running and ready", func() {


suggest changing it to: "scaling down before scale up is finished should wait until current pod to be running and ready"

janetkuo · 2016-10-11T22:19:00Z

test/e2e/petset.go

+			pst.waitForRunningAndReady(1, ps)
+
+			By("verifying that scaling down before scale up is finished will wait until current pod will be running and ready")
+			// pet-2 should be the last pet scale up will create before will start scale down


rephrase the comment?

janetkuo · 2016-10-11T22:19:53Z

test/e2e/petset.go

+
+			By("verifying that scaling down before scale up is finished will wait until current pod will be running and ready")
+			// pet-2 should be the last pet scale up will create before will start scale down
+			lastPet := "pet-2"


consider renaming it to expectedLastPetName

janetkuo · 2016-10-11T22:24:29Z

test/e2e/petset.go

+			lastPet := "pet-2"
+			podTracker = []podEvent{}
+			pst.setHealthy(ps)
+			pst.update(ps.Namespace, ps.Name, func(ps *apps.PetSet) { ps.Spec.Replicas = 3 })


consider wrapping this to something like pst.scaleWithoutCheck (or better name, to differentiate from pst.scale)

janetkuo · 2016-10-11T22:38:14Z

test/e2e/petset.go

+			By("verifying that scaling down before scale up is finished will wait until current pod will be running and ready")
+			// pet-2 should be the last pet scale up will create before will start scale down
+			lastPet := "pet-2"
+			podTracker = []podEvent{}


podEventTracker

janetkuo · 2016-10-11T22:40:52Z

test/e2e/petset.go

+			pst.waitForRunningAndNotReady(2, ps)
+			pst.setHealthy(ps)
+			pst.update(ps.Namespace, ps.Name, func(ps *apps.PetSet) { ps.Spec.Replicas = 1 })
+			pst.restoreProbe(ps, testProbe)


I'd add more By steps in here, for example:

Scale up petset to 3

Before scaling up finishes, set 2nd pet to be not ready

Scale down petset to 1

Verify ...

janetkuo · 2016-10-12T00:19:07Z

test/e2e/petset.go

+				}
+				isReady := api.IsPodReady(event.pod)
+				if event.pod.Status.Phase != api.PodRunning || !isReady {
+					framework.Failf("%v expected to be running != %v and ready != %v", event.pod.Name, event.pod.Status.Phase, isReady)


since it's OR, it not necessary be "the running != %v and ready != %v"

janetkuo · 2016-10-12T00:20:58Z

test/e2e/petset.go

+			pst.update(ps.Namespace, ps.Name, func(ps *apps.PetSet) { ps.Spec.Replicas = 1 })
+			pst.restoreProbe(ps, testProbe)
+			for i := len(podTracker) - 1; i >= 0; i-- {
+				event := podTracker[i]


Could you explain more on what this loop is doing? Is it looking at the last (and only the last) event on pet-2 to see if the pod is ready? Do we need to wait before the check?

i wanted to verify that lastPet entered running before scale down started. Will rewrite with watcher

and it is in opposite direction because i wanted to find last event for this pod

bprashanth · 2016-10-12T00:24:25Z

@janetkuo thanks! let me know if you need help, otherwise I'll assume you got this

janetkuo · 2016-11-16T19:00:57Z

Ping @dshulyak for rebasing and addressing comments

dshulyak · 2016-11-18T16:25:54Z

@janetkuo fixed comments and rebased, please take a look when you will have time

janetkuo · 2016-11-18T22:39:25Z

test/e2e/petset.go

@@ -212,6 +212,64 @@ var _ = framework.KubeDescribe("StatefulSet [Slow] [Feature:PetSet]", func() {
 			pst.verifyPodAtIndex(updateIndex, ps, verify)
 		})

+		It("scaling down before scale up is finished should wait until current pod will be running and ready before it will be removed", func() {
+			By("creating petset " + psName + " in namespace " + ns)


stateful set

"Creating stateful set " + psName + " in namespace " + ns + ", and pausing scale operations after each pod"

janetkuo · 2016-11-18T22:43:09Z

test/e2e/petset.go

+
+			By("verifying that 2nd pod will not be removed if it is not healthy")
+			pst.confirmPetCount(2, ps, 10*time.Second)
+			expectedPodName := strings.Join([]string{ps.Name, "-1"}, "")


ps.Name + "-1" or fmt.Sprintf("%s-%d", ps.Name, 1) is more readable

janetkuo · 2016-11-18T22:50:00Z

test/e2e/petset.go

@@ -212,6 +212,64 @@ var _ = framework.KubeDescribe("StatefulSet [Slow] [Feature:PetSet]", func() {
 			pst.verifyPodAtIndex(updateIndex, ps, verify)
 		})

+		It("scaling down before scale up is finished should wait until current pod will be running and ready before it will be removed", func() {


"Scaling down before scaling up finishes should wait until current pod to be running"

Capitalize the first words of It and By steps

janetkuo · 2016-11-18T23:00:42Z

test/e2e/petset.go

+			pst.breakProbe(ps, testProbe)
+			pst.waitForRunningAndNotReady(2, ps)
+
+			By("scaling down to 1 replica and restoring health of 2nd pod")


"Continue scale operations after the 2nd pod, and scaling down to 1 replica"

janetkuo · 2016-11-18T23:04:19Z

test/e2e/petset.go

+			pst := &statefulSetTester{c: c}
+			pst.waitForRunningAndReady(1, ps)
+
+			By("scaling up stateful set " + psName + " to 3 replicas")


... and pausing after the 2nd pod

janetkuo · 2016-11-18T23:07:33Z

test/e2e/petset.go

+			pst.setHealthy(ps)
+			pst.updateReplicas(ps, 1)
+
+			By("verifying that 2nd pod will not be removed if it is not healthy")


"Verifying that the 2nd pod will not be removed if it's not running and ready"

janetkuo · 2016-11-18T23:10:22Z

test/e2e/petset.go

+			Expect(err).NotTo(HaveOccurred())
+			pst.restoreProbe(ps, testProbe)
+
+			By("verifying that 2nd pod will enter running and ready before it will be removed by scale down")


"Verifying the 2nd pod is not removed by scaling down until it becomes running and ready"

janetkuo · 2016-11-18T23:10:39Z

test/e2e/petset.go

+				},
+			))
+			Expect(err).NotTo(HaveOccurred())
+			pst.restoreProbe(ps, testProbe)


Move this down to the below By

janetkuo · 2016-11-18T23:15:08Z

Only nits, LGTM otherwise. This needs to wait for #32868

This change implements test cases to validate that: scale up will be done in order scale down in reverse order if any of the pets will be unhealthy, scale up/down will halt Change-Id: I4487a7c9823d94a2995bbb06accdfd8f3001354c

Verifies that current pet (one which was created last by scale up action) will enter running before scale down will take place Change-Id: Ib47644c22b3b097bc466f68c5cac46c78dd44552

dshulyak · 2016-11-21T10:44:30Z

@k8s-bot kops aws e2e test this

k8s-ci-robot · 2016-11-21T10:58:17Z

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

dshulyak · 2016-11-21T10:59:31Z

@k8s-bot gci gke e2e test this

k8s-ci-robot · 2016-11-21T11:07:09Z

Jenkins GKE smoke e2e failed for commit c09e969. 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-21T11:37:11Z

Jenkins kops AWS e2e failed for commit c09e969. Full PR test history.

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

dshulyak · 2016-11-21T12:16:44Z

@k8s-bot cvm gke e2e test this

dshulyak · 2016-11-21T14:24:29Z

@k8s-bot kops aws e2e test this

janetkuo

/lgtm

janetkuo · 2016-11-21T18:48:54Z

Adding do-not-merge since this needs to wait for #32868

janetkuo · 2016-11-21T19:42:34Z

#32868 is merged

k8s-github-robot · 2016-11-22T00:15:13Z

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

k8s-ci-robot · 2016-11-22T00:57:03Z

Jenkins Bazel Build failed for commit c09e969. Full PR test history.

The magic incantation to run this job again is @k8s-bot bazel 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-22T01:01:33Z

Automatic merge from submit-queue

googlebot added the cla: yes label Oct 10, 2016

k8s-github-robot assigned fejta Oct 10, 2016

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

janetkuo self-assigned this Oct 10, 2016

fejta removed their assignment Oct 10, 2016

janetkuo reviewed Oct 12, 2016

View reviewed changes

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

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

janetkuo added this to the v1.5 milestone Nov 14, 2016

janetkuo mentioned this pull request Nov 16, 2016

StatefulSet tests #36855

Closed

19 tasks

dshulyak force-pushed the current_enter_running branch from 8863691 to cf69b1e Compare November 18, 2016 16:16

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

janetkuo reviewed Nov 18, 2016

View reviewed changes

Validate pet set scale up/down order

36d1af6

This change implements test cases to validate that: scale up will be done in order scale down in reverse order if any of the pets will be unhealthy, scale up/down will halt Change-Id: I4487a7c9823d94a2995bbb06accdfd8f3001354c

dshulyak force-pushed the current_enter_running branch from cf69b1e to b46a36e Compare November 21, 2016 09:44

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

dshulyak force-pushed the current_enter_running branch from b46a36e to d9ea8c0 Compare November 21, 2016 09:45

Test case for scalling down before scale up finished

c09e969

Verifies that current pet (one which was created last by scale up action) will enter running before scale down will take place Change-Id: Ib47644c22b3b097bc466f68c5cac46c78dd44552

dshulyak force-pushed the current_enter_running branch from d9ea8c0 to c09e969 Compare November 21, 2016 09:46

janetkuo approved these changes Nov 21, 2016

View reviewed changes

janetkuo added the do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. label Nov 21, 2016

janetkuo added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Nov 21, 2016

k8s-github-robot merged commit 88a8094 into kubernetes:master Nov 22, 2016

Test case for scalling down before scale up finished #34454

Test case for scalling down before scale up finished #34454

Conversation

dshulyak commented Oct 10, 2016 • edited by k8s-oncall

dshulyak commented Oct 10, 2016

erictune commented Oct 10, 2016

janetkuo Oct 11, 2016 • edited

Choose a reason for hiding this comment

janetkuo Oct 11, 2016 • edited

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

janetkuo Oct 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janetkuo Oct 11, 2016 • edited

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

bprashanth commented Oct 12, 2016

janetkuo commented Nov 16, 2016 • edited

dshulyak commented Nov 18, 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

janetkuo commented Nov 18, 2016

dshulyak commented Nov 21, 2016

k8s-ci-robot commented Nov 21, 2016

dshulyak commented Nov 21, 2016

k8s-ci-robot commented Nov 21, 2016

k8s-ci-robot commented Nov 21, 2016

dshulyak commented Nov 21, 2016

dshulyak commented Nov 21, 2016

janetkuo left a comment

Choose a reason for hiding this comment

janetkuo commented Nov 21, 2016

janetkuo commented Nov 21, 2016

k8s-github-robot commented Nov 22, 2016

k8s-ci-robot commented Nov 22, 2016

k8s-github-robot commented Nov 22, 2016

dshulyak commented Oct 10, 2016 •

edited by k8s-oncall

janetkuo Oct 11, 2016 •

edited

janetkuo Oct 11, 2016 •

edited

janetkuo Oct 11, 2016 •

edited

janetkuo Oct 11, 2016 •

edited

janetkuo commented Nov 16, 2016 •

edited