DaemonSet e2e: Update image and rolling upgrade test timeout #72738

alexbrand · 2019-01-09T17:12:06Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
This PR removes an unnecessary sleep from the serve-hostname binary that is used in conformance tests.

The reason for this change is that the introduction of this 60 second sleep has negatively impacted the DaemonSet rolling upgrade conformance test.

Specifically, it prevents the serve-hostname pods from terminating gracefully, and thus each pod takes at least 30 seconds to terminate (due to the default 30 second termination grace period).

In large clusters, the conformance test fails because rolling all pods of the DaemonSet takes longer than timeout configured in the conformance test (5 minutes).

Which issue(s) this PR fixes:

Fixes #71666

Special notes for your reviewer:
I have updated all the references to this image that I could find using code search. Not sure if I missed any.

Does this PR introduce a user-facing change?:

NONE

alexbrand · 2019-01-09T17:12:56Z

/sig testing

ixdy · 2019-01-09T17:30:06Z

/assign @freehan

ixdy · 2019-01-09T17:31:39Z

hack/testdata/pod-with-large-name.yaml

@@ -8,4 +8,4 @@ metadata:
 spec:
  containers:
  - name: kubernetes-serve-hostname
-    image: gcr.io/kubernetes-e2e-test-images/serve-hostname-amd64:1.1
+    image: gcr.io/kubernetes-e2e-test-images/serve-hostname-amd64:1.3


putting the reference updates into a separate commit might make this easier to cherrypick to release branches.

Thanks @ixdy. Will move the updates to a separate commit.

freehan · 2019-01-10T00:23:24Z

Can you add a flag for this instead?

alexbrand · 2019-01-10T21:36:35Z

@freehan I think adding a flag is a possibility, but another potential route is just updating the DaemonSet test instead. I tagged you in the original issue as there is some more discussion happening there. Would love your insight and thoughts.

alexbrand · 2019-01-22T16:17:42Z

Updated the title to reflect the new approach for this fix. This is a more direct fix to the problem being observed in #71666.

Instead of changing the serve-hostname image which is depended on by other tests, we fix the DaemonSet test directly.

@BenTheElder, @ixdy, @freehan PTAL

ixdy · 2019-01-30T18:38:16Z

/approve

I'll let @freehan apply lgtm.

k8s-ci-robot · 2019-01-30T18:38:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alexbrand, ixdy

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~~ [ixdy]

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

janetkuo · 2019-02-20T21:46:02Z

test/e2e/apps/daemon_set.go

+		// Set the termination grace period to 1 second. This is necessary because the
+		// serve-hostname binary sleeps for 1 minute after receiving SIGTERM.
+		terminationPeriod := int64(1)
+		ds.Spec.Template.Spec.TerminationGracePeriodSeconds = &terminationPeriod


This fix should work, but DaemonSet test shouldn't be tied to the serve-hostname image. We can simply change the image we use in this test:

image := framework.ServeHostnameImage

to something else (can pick from this list), such as nginx.

We can also increase the timeout based on number of nodes, as this affects the number of replicas a DaemonSet creates.

That is a good point. Should we take that route instead? Will take a look at the list of images.

Yes, thanks for looking into this!

Should we update the image used throughout all DaemonSet tests, or just the one used during the DaemonSet update test?

Updating the image used throughout all DaemonSet tests (i.e. change image := framework.ServeHostnameImage) is fine. Which image we pick doesn't matter as long as it can be pulled and doesn't take too long to start/terminate.

We can scale update timeout based on # of nodes too, maybe something like 5 mins + 1 min * # of nodes.

Use Nginx as the DaemonSet image instead of the ServeHostname image. This was changed because the ServeHostname has a sleep after terminating which makes it incompatible with the DaemonSet Rolling Upgrade e2e test. In addition, make the DaemonSet Rolling Upgrade e2e test timeout a function of the number of nodes that make up the cluster. This is required because the more nodes there are, the longer the time it will take to complete a rolling upgrade. Signed-off-by: Alexander Brand <alexbrand09@gmail.com>

alexbrand · 2019-02-22T20:52:34Z

Updated the image, and made the timeout a function of the number of nodes in the cluster. PTAL.

janetkuo · 2019-02-26T22:28:23Z

/lgtm

BenTheElder · 2019-02-26T22:38:46Z

thank you!

janetkuo · 2019-02-27T00:37:13Z

We need to cherrypick this to older releases as well.

janetkuo · 2019-03-01T18:46:39Z

Sent cherrypick PRs:
#74820
#74822

…38-upstream-release-1.12 Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout

…38-upstream-release-1.13 Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout

k8s-ci-robot requested review from adohe-zz and bowei January 9, 2019 17:12

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 9, 2019

k8s-ci-robot assigned freehan Jan 9, 2019

ixdy reviewed Jan 9, 2019

View reviewed changes

alexbrand force-pushed the fix-serve-hostname branch from bc0c247 to 42dc6e6 Compare January 9, 2019 17:49

alexbrand mentioned this pull request Jan 10, 2019

Conformance test for DaemonSet RollingUpdate to rigid on timeout #71666

Closed

timothysc added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 10, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jan 10, 2019

alexbrand force-pushed the fix-serve-hostname branch from 42dc6e6 to 4646917 Compare January 22, 2019 16:04

k8s-ci-robot added sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Jan 22, 2019

alexbrand force-pushed the fix-serve-hostname branch from 4646917 to 24e0008 Compare January 22, 2019 16:10

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 22, 2019

alexbrand changed the title ~~Remove sleep from serve-hostname binary used in conformance tests~~ Set termination grace period to 1 second in DaemonSet update e2e test Jan 22, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 30, 2019

janetkuo self-assigned this Feb 20, 2019

janetkuo reviewed Feb 20, 2019

View reviewed changes

janetkuo added kind/failing-test Categorizes issue or PR as related to a consistently or frequently failing test. and removed sig/storage Categorizes an issue or PR as relevant to SIG Storage. labels Feb 21, 2019

alexbrand force-pushed the fix-serve-hostname branch from 24e0008 to d971597 Compare February 22, 2019 20:51

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 22, 2019

janetkuo approved these changes Feb 26, 2019

View reviewed changes

janetkuo changed the title ~~Set termination grace period to 1 second in DaemonSet update e2e test~~ DaemonSet e2e: Update image and rolling upgrade test timeout Feb 26, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 26, 2019

k8s-ci-robot merged commit 4505f12 into kubernetes:master Feb 27, 2019

This was referenced Mar 1, 2019

Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout #74820

Merged

Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout #74822

Merged

k8s-ci-robot added a commit that referenced this pull request Mar 6, 2019

Merge pull request #74820 from janetkuo/automated-cherry-pick-of-#727…

657a540

…38-upstream-release-1.12 Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout

k8s-ci-robot added a commit that referenced this pull request Mar 18, 2019

Merge pull request #74822 from janetkuo/automated-cherry-pick-of-#727…

06c4ecc

…38-upstream-release-1.13 Automated cherry pick of #72738: DaemonSet e2e: Update image and rolling upgrade test timeout

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DaemonSet e2e: Update image and rolling upgrade test timeout #72738

DaemonSet e2e: Update image and rolling upgrade test timeout #72738

alexbrand commented Jan 9, 2019

alexbrand commented Jan 9, 2019

ixdy commented Jan 9, 2019

ixdy Jan 9, 2019

alexbrand Jan 9, 2019

freehan commented Jan 10, 2019

alexbrand commented Jan 10, 2019

alexbrand commented Jan 22, 2019

ixdy commented Jan 30, 2019

k8s-ci-robot commented Jan 30, 2019

janetkuo Feb 20, 2019

alexbrand Feb 21, 2019

janetkuo Feb 21, 2019

alexbrand Feb 21, 2019

janetkuo Feb 21, 2019 •

edited

alexbrand commented Feb 22, 2019

janetkuo commented Feb 26, 2019

BenTheElder commented Feb 26, 2019

janetkuo commented Feb 27, 2019

janetkuo commented Mar 1, 2019

DaemonSet e2e: Update image and rolling upgrade test timeout #72738

DaemonSet e2e: Update image and rolling upgrade test timeout #72738

Conversation

alexbrand commented Jan 9, 2019

alexbrand commented Jan 9, 2019

ixdy commented Jan 9, 2019

ixdy Jan 9, 2019

Choose a reason for hiding this comment

alexbrand Jan 9, 2019

Choose a reason for hiding this comment

freehan commented Jan 10, 2019

alexbrand commented Jan 10, 2019

alexbrand commented Jan 22, 2019

ixdy commented Jan 30, 2019

k8s-ci-robot commented Jan 30, 2019

janetkuo Feb 20, 2019

Choose a reason for hiding this comment

alexbrand Feb 21, 2019

Choose a reason for hiding this comment

janetkuo Feb 21, 2019

Choose a reason for hiding this comment

alexbrand Feb 21, 2019

Choose a reason for hiding this comment

janetkuo Feb 21, 2019 • edited

Choose a reason for hiding this comment

alexbrand commented Feb 22, 2019

janetkuo commented Feb 26, 2019

BenTheElder commented Feb 26, 2019

janetkuo commented Feb 27, 2019

janetkuo commented Mar 1, 2019

janetkuo Feb 21, 2019 •

edited