add StatefulSet MinReadySeconds e2e test #104078

atiratree · 2021-08-02T17:02:11Z

What type of PR is this?

What this PR does / why we need it:

2nd part of #103073 adding additional e2e test:

MinReadySeconds should update availableReplicas accordingly when enabled

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

[KEP]: https://github.com/kubernetes/enhancements/pull/2607
[Other doc]: https://github.com/kubernetes/website/pull/27683

k8s-ci-robot · 2021-08-02T17:02:19Z

Hi @atiratree. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

atiratree · 2021-08-02T17:03:00Z

@ravisantoshgudimetla @soltysh

ravisantoshgudimetla

Thank you for the PR @atiratree. Please look at my comments.

ravisantoshgudimetla · 2021-08-02T17:05:31Z

test/e2e/apps/statefulset.go

+		setHTTPProbe(ss)
+		ss, err := c.AppsV1().StatefulSets(ns).Create(context.TODO(), ss, metav1.CreateOptions{})
+		framework.ExpectNoError(err)
+		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 0)


Why this 0? Do you want to show that the replicas won't be available immediately? If yes, this could be racy if reaching this statement would take more than 10 seconds.

I did not want to put a longer time so the test can complete quickly.

There are two ways to solve this.

make MinReadySeconds a bit higher - eg 30s? (test takes longer)

remove the 0 check

@ravisantoshgudimetla and yes I want to check the replicas are not available immediately

Kube as a whole is eventually consistent. So, better to make the value here longer like 30 seconds as you mentioned and wait till 5 or 10 seconds and make sure that the AvailableReplicas is not updated. Even this may be racy somtimes. I'd prefer having -ve tests in integration of units by faking clock.

for now I put 30 there with the additional wait (thanks for the suggestion). I expect it not be that racy since we are just waiting for the controller to pick up the resource and set 0 (so waiting time should be our poll 10s)

We can revisit the test if it is failing(flaking).

I have similar concerns here as Ravi, this might be flaking when STS controller immediately creates the first replica and you'll never meet 0 status condition.

This is not about the creation of a replica, but about the availability and since we set MinReadySeconds to 30 we should observe 0 for the first 30 seconds. WaitForStatusAvailableReplicas is waiting only 0-10s to observe the 0 to prevent the race

ravisantoshgudimetla · 2021-08-02T17:07:55Z

test/e2e/apps/statefulset.go

@@ -1154,6 +1154,42 @@ var _ = SIGDescribe("StatefulSet", func() {
 		framework.ExpectNoError(err)
 		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 1)
 	})
+
+	ginkgo.It("MinReadySeconds should update availableReplicas accordingly when enabled [Feature:StatefulSetMinReadySeconds] [alpha]", func() {


AvailableReplicas should depend on the MinReadySeconds value specified?

test/e2e/apps/statefulset.go

ravisantoshgudimetla · 2021-08-02T17:10:08Z

test/e2e/apps/statefulset.go

+
+		ginkgo.By("check Available Replicas are shown in status")
+		out, err := framework.RunKubectl(ns, "get", "statefulset", ss.Name, "-o=yaml")
+		framework.ExpectNoError(err)


Please add a comment.

isn't the .By enough? What else do you have in mind?

Ya, By is good enough, I usually add a comment to separate code for different tests.

ravisantoshgudimetla

Just some nits but looks good for me.

@soltysh can you review as well?

pacoxu · 2021-09-03T03:52:49Z

/kind cleanup

ravisantoshgudimetla

@soltysh - Can I get a review of this PR now that

#104045 is getting merged.

ravisantoshgudimetla · 2021-09-13T17:07:19Z

test/e2e/apps/statefulset.go

+		setHTTPProbe(ss)
+		ss, err := c.AppsV1().StatefulSets(ns).Create(context.TODO(), ss, metav1.CreateOptions{})
+		framework.ExpectNoError(err)
+		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 0)


We can revisit the test if it is failing(flaking).

ravisantoshgudimetla · 2021-09-13T17:15:29Z

/retest

soltysh

A few nits, but mostly looks good.
/ok-to-test
/priority backlog
/traige accepted
/approve
/assign @ravisantoshgudimetla
for final lgtm

soltysh · 2021-09-15T11:05:59Z

test/e2e/apps/statefulset.go

@@ -1154,6 +1154,50 @@ var _ = SIGDescribe("StatefulSet", func() {
 		framework.ExpectNoError(err)
 		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 1)
 	})
+
+	ginkgo.It("AvailableReplicas should get updated accordingly when MinReadySeconds is enabled [Feature:StatefulSetMinReadySeconds] [alpha]", func() {


Nit: this is beta now that we merged #104045

thanks, fixed

soltysh · 2021-09-15T11:07:56Z

test/e2e/apps/statefulset.go

+		setHTTPProbe(ss)
+		ss, err := c.AppsV1().StatefulSets(ns).Create(context.TODO(), ss, metav1.CreateOptions{})
+		framework.ExpectNoError(err)
+		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 0)


I have similar concerns here as Ravi, this might be flaking when STS controller immediately creates the first replica and you'll never meet 0 status condition.

soltysh · 2021-09-15T11:08:26Z

test/e2e/apps/statefulset.go

+		framework.ExpectNoError(err)
+		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 0)
+		// let's check that the availableReplicas have still not updated
+		time.Sleep(5 * time.Second)


I'd prefer not to use time.Sleep but rather add those extra 5s in the next WaitFor..., if needed.

Although looking at WaitForStatusAvailableReplicas it waits up to 10mins, so you can just drop that bit.

as mentioned in #104078 (comment) we might observe 0 AvailableReplicas immediately and this ensures that we wait at least 5 seconds to check it still stays 0.

The next WaitForStatusAvailableReplicas check for Availability. This one checks for UnAvailability

Oh, I see what you mean, I missed the initial MinReadySeconds being set, I assumed it's not.

soltysh

/lgtm

soltysh · 2021-09-15T11:44:34Z

test/e2e/apps/statefulset.go

+		framework.ExpectNoError(err)
+		e2estatefulset.WaitForStatusAvailableReplicas(c, ss, 0)
+		// let's check that the availableReplicas have still not updated
+		time.Sleep(5 * time.Second)


Oh, I see what you mean, I missed the initial MinReadySeconds being set, I assumed it's not.

k8s-ci-robot · 2021-09-15T11:45:15Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: atiratree, soltysh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~test/e2e/apps/OWNERS~~ [soltysh]
~~test/e2e/framework/OWNERS~~ [soltysh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

soltysh · 2021-09-15T11:46:16Z

/triage accepted

ravisantoshgudimetla

Verfication failed, other than that LGTM.

ravisantoshgudimetla · 2021-09-15T13:50:37Z

/retest

k8s-ci-robot added the area/test label Aug 2, 2021

k8s-ci-robot requested review from foxish and krmayankk August 2, 2021 17:02

ravisantoshgudimetla reviewed Aug 2, 2021

View reviewed changes

atiratree force-pushed the e2e.minreadyseconds branch from f749b15 to 59e674f Compare August 2, 2021 17:22

ravisantoshgudimetla reviewed Aug 16, 2021

View reviewed changes

atiratree force-pushed the e2e.minreadyseconds branch from 59e674f to 5172c2b Compare August 19, 2021 13:50

k8s-ci-robot added the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Aug 19, 2021

atiratree force-pushed the e2e.minreadyseconds branch from 5172c2b to c2a91d9 Compare August 19, 2021 14:15

k8s-ci-robot added kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Sep 3, 2021

ravisantoshgudimetla reviewed Sep 13, 2021

View reviewed changes

soltysh reviewed Sep 15, 2021

View reviewed changes

k8s-ci-robot assigned ravisantoshgudimetla Sep 15, 2021

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Sep 15, 2021

add StatefulSet MinReadySeconds e2e test

2cad64d

atiratree force-pushed the e2e.minreadyseconds branch from 600b9ce to 2cad64d Compare September 15, 2021 11:33

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

soltysh approved these changes Sep 15, 2021

View reviewed changes

k8s-ci-robot assigned soltysh Sep 15, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2021

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 15, 2021

ravisantoshgudimetla reviewed Sep 15, 2021

View reviewed changes

k8s-ci-robot merged commit 07a4ae1 into kubernetes:master Sep 15, 2021

k8s-ci-robot added this to the v1.23 milestone Sep 15, 2021

atiratree mentioned this pull request Oct 4, 2021

REQUEST: New membership for atiratree kubernetes/org#3026

Closed

7 tasks

add StatefulSet MinReadySeconds e2e test #104078

add StatefulSet MinReadySeconds e2e test #104078

Conversation

atiratree commented Aug 2, 2021

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

k8s-ci-robot commented Aug 2, 2021

atiratree commented Aug 2, 2021

ravisantoshgudimetla 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

ravisantoshgudimetla Aug 16, 2021 • edited

Choose a reason for hiding this comment

atiratree Aug 19, 2021 • 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

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

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

pacoxu commented Sep 3, 2021

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Sep 13, 2021

soltysh 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

soltysh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Sep 15, 2021

soltysh commented Sep 15, 2021

ravisantoshgudimetla left a comment

Choose a reason for hiding this comment

ravisantoshgudimetla commented Sep 15, 2021

ravisantoshgudimetla Aug 16, 2021 •

edited

atiratree Aug 19, 2021 •

edited