Fix error handling in gc e2e test #60671

jennybuckley · 2018-03-01T22:42:55Z

What this PR does / why we need it:
Error messages were not being surfaced in log of GC e2e test, part of fixing #60463

k8s-ci-robot · 2018-03-01T22:42:56Z

@jennybuckley: Adding do-not-merge/release-note-label-needed because the release note process has not been followed.

One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://git.k8s.io/community/contributors/devel/pull-requests.md#write-release-notes-if-needed.

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.

caesarxuchao · 2018-03-01T23:28:40Z

/lgtm
/approval

caesarxuchao · 2018-03-01T23:28:46Z

/release-note-none

lavalamp · 2018-03-01T23:37:38Z

If this weren't a test I'd ask for this to use the error list concept that (I think) the validation code defines (uses?) :)

jennybuckley · 2018-03-02T19:08:52Z

/kind bug
/priority important-soon
/sig api-machinery

liggitt · 2018-03-06T19:17:49Z

test/e2e/apimachinery/garbage_collector.go

@@ -610,19 +611,20 @@ var _ = SIGDescribe("Garbage collector", func() {
 			return verifyRemainingDeploymentsReplicaSetsPods(f, clientSet, deployment, 0, 1, 2)
 		})
 		if err != nil {
-			err = fmt.Errorf("Failed to wait to see if the garbage collecter mistakenly deletes the rs: %v", err)
+			errList := err


accumulating into list would probably make more sense than passing it back into fmt.Errorf

liggitt · 2018-03-06T19:17:59Z

test/e2e/apimachinery/garbage_collector.go

@@ -553,13 +553,14 @@ var _ = SIGDescribe("Garbage collector", func() {
 			return verifyRemainingDeploymentsReplicaSetsPods(f, clientSet, deployment, 0, 0, 0)
 		})
 		if err == wait.ErrWaitTimeout {
-			err = fmt.Errorf("Failed to wait for all rs to be garbage collected: %v", err)
+			errList := err


same here... accumulate?

jennybuckley · 2018-03-06T22:48:06Z

test/e2e/apimachinery/garbage_collector.go

-		err = wait.PollImmediate(5*time.Second, 2*time.Minute, func() (bool, error) {
-			return verifyRemainingDeploymentsReplicaSetsPods(f, clientSet, deployment, 0, 1, 2)
-		})
+		By("wait for 10s to see if the garbage collector mistakenly deletes the rs")


This is changed because it doesn't make sense to use PollImmediate to make sure something doesn't happen (at least the way it was being used here). If it hasn't happened by the first polling the PollImmediate will succeed and then what we are watching for may end up occurring later and we will miss it. How we were doing it, the test would only wait either ~0, or 5 seconds (depending on how long the deletion of the Deployment took, usually the 0s case), so this is actually an increase in wait time despite the log message diff indicating a decrease.

Good catch. Could you keep the code consistent with https://github.com/kubernetes/kubernetes/blame/6546b69964f96ff6dd7bfdd8e72f89f09bd3e305/test/e2e/apimachinery/garbage_collector.go#L447-L459?

jennybuckley · 2018-03-06T22:50:53Z

@liggitt I updated to use aggregated errors and also to fix two more places where errors could be missed and the test could pass when it should fail

jennybuckley · 2018-03-06T22:54:27Z

test/e2e/apimachinery/garbage_collector.go

@@ -552,15 +552,17 @@ var _ = SIGDescribe("Garbage collector", func() {
 		err = wait.PollImmediate(500*time.Millisecond, 1*time.Minute, func() (bool, error) {
 			return verifyRemainingDeploymentsReplicaSetsPods(f, clientSet, deployment, 0, 0, 0)
 		})
-		if err == wait.ErrWaitTimeout {
-			err = fmt.Errorf("Failed to wait for all rs to be garbage collected: %v", err)
+		if err != nil {


Fix for error swallowing missed by f08f504

liggitt · 2018-03-07T20:51:56Z

test/e2e/apimachinery/garbage_collector.go

-		})
-		if err != nil {
-			err = fmt.Errorf("Failed to wait to see if the garbage collecter mistakenly deletes the rs: %v", err)
+		By("wait for 30 seconds to see if the garbage collector mistakenly deletes the rs")


why poll at all? why not just sleep 30 seconds, then list and ensure length is 1? expecting to get a timeout error is confusing.

@liggitt
that is how I had it but changed it in favor of consistency with other similar code in this file
#60671 (comment)

Would this be better? jennybuckley@03b4b23#diff-4a7d8678f8505e86c1c6cc626cdc5eeeR613

@jennybuckley your code is better, let's change the rest of code to comply with yours.

jdumars · 2018-03-13T14:49:16Z

@sig-testing-approvers PTAL ASAP

liggitt · 2018-03-14T17:08:58Z

this is an improvement, but is it a release blocker?

jennybuckley · 2018-03-14T17:34:04Z

@liggitt no, I don't think it's a release blocker

liggitt · 2018-03-14T17:36:56Z

let's move it out of the milestone, then. we're down to the wire on 1.10 and only essential things should be in at this point

/milestone clear

jennybuckley · 2018-03-14T20:35:39Z

/retest

jennybuckley · 2018-06-01T18:24:43Z

/test pull-kubernetes-e2e-gce

jennybuckley · 2018-06-01T18:27:05Z

@liggitt @roycaihw
Maybe this would be useful for debugging the new GC flakes. it seemed less important once the gc flakes died down a while ago, but it looks like they are back.

roycaihw · 2018-06-01T21:00:41Z

test/e2e/apimachinery/garbage_collector.go

-			return false, nil
-		}); err != nil && err != wait.ErrWaitTimeout {
-			framework.Failf("%v", err)
+		time.Sleep(30 * time.Second)


nit: polling helps us fail earlier in case of error. but in reality we expect these tests to pass consistently, so there is no difference

This was a while ago, but I think the reason for that change was to keep consistency with the change I made to https://github.com/kubernetes/kubernetes/pull/60671/files#diff-4a7d8678f8505e86c1c6cc626cdc5eeeL608
In that case the way the polling was set up was completely broken, and it would always succeed within the first 0-5 seconds, even if the garbage collector mistakenly deleted the replica set.

Discussion about it here: #60671 (comment)

roycaihw · 2018-06-01T21:02:25Z

/lgtm

liggitt · 2018-06-01T23:23:10Z

/lgtm

k8s-ci-robot · 2018-06-01T23:23:17Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, jennybuckley, liggitt, roycaihw

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/OWNERS~~ [liggitt]

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

liggitt · 2018-06-01T23:23:43Z

/retest

liggitt · 2018-06-05T03:05:50Z

bringing in to help diagnose flakes in this test (c.f. https://storage.googleapis.com/k8s-gubernator/triage/index.html?ci=0&pr=1&sig=api-machinery&text=post%20mortem&job=pull-kubernetes-e2e-gce&test=Garbage%20collector)

/milestone v1.11

k8s-github-robot · 2018-06-05T03:06:25Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@caesarxuchao @jennybuckley @liggitt @roycaihw

Pull Request Labels

sig/api-machinery: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2018-06-05T03:59:54Z

/test all [submit-queue is verifying that this PR is safe to merge]

jennybuckley · 2018-06-05T04:42:19Z

/retest

liggitt · 2018-06-05T04:44:46Z

fyi, fix in progress for that flake in #64726

k8s-github-robot · 2018-06-05T05:11:12Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from fejta and janetkuo March 1, 2018 22:43

k8s-ci-robot assigned caesarxuchao Mar 1, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 1, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Mar 1, 2018

lavalamp added this to the v1.10 milestone Mar 1, 2018

lavalamp added the status/approved-for-milestone label Mar 1, 2018

k8s-github-robot added the milestone/incomplete-labels label Mar 2, 2018

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. labels Mar 2, 2018

k8s-github-robot removed the milestone/incomplete-labels label Mar 2, 2018

liggitt reviewed Mar 6, 2018

View reviewed changes

jennybuckley force-pushed the gc-test-error-message branch from 6dfc643 to af684f9 Compare March 6, 2018 22:44

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Mar 6, 2018

jennybuckley commented Mar 6, 2018

View reviewed changes

jennybuckley force-pushed the gc-test-error-message branch from ece5a70 to 218199c Compare March 7, 2018 18:59

liggitt reviewed Mar 7, 2018

View reviewed changes

Fix error handling in gc e2e test

5f518e6

jennybuckley force-pushed the gc-test-error-message branch from 218199c to 5f518e6 Compare March 14, 2018 17:29

k8s-ci-robot removed this from the v1.10 milestone Mar 14, 2018

fejta removed their request for review March 30, 2018 18:52

roycaihw reviewed Jun 1, 2018

View reviewed changes

k8s-ci-robot assigned roycaihw Jun 1, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 1, 2018

k8s-ci-robot assigned liggitt Jun 1, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 1, 2018

k8s-ci-robot added this to the v1.11 milestone Jun 5, 2018

k8s-github-robot merged commit 6c89575 into kubernetes:master Jun 5, 2018

Fix error handling in gc e2e test #60671

Fix error handling in gc e2e test #60671

Conversation

jennybuckley commented Mar 1, 2018

k8s-ci-robot commented Mar 1, 2018

caesarxuchao commented Mar 1, 2018

caesarxuchao commented Mar 1, 2018

lavalamp commented Mar 1, 2018

jennybuckley commented Mar 2, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley Mar 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jennybuckley commented Mar 6, 2018 • edited Loading

jennybuckley Mar 6, 2018 • 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

jdumars commented Mar 13, 2018

liggitt commented Mar 14, 2018

jennybuckley commented Mar 14, 2018

liggitt commented Mar 14, 2018

jennybuckley commented Mar 14, 2018

jennybuckley commented Jun 1, 2018

jennybuckley commented Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roycaihw commented Jun 1, 2018

liggitt commented Jun 1, 2018

k8s-ci-robot commented Jun 1, 2018

liggitt commented Jun 1, 2018

liggitt commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

jennybuckley commented Jun 5, 2018

liggitt commented Jun 5, 2018

k8s-github-robot commented Jun 5, 2018

jennybuckley Mar 6, 2018 •

edited

Loading

jennybuckley commented Mar 6, 2018 •

edited

Loading

jennybuckley Mar 6, 2018 •

edited

Loading

jennybuckley commented Jun 1, 2018 •

edited

Loading