Switch WaitForCertificate to informers to avoid broken watches #73030

tnozicka · 2019-01-17T17:39:03Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
ListWatchUntil can't handle closed watches and is being replaced by UntilWithSync based on informers.

Special notes for your reviewer:
Split from #50102

Replaces #73027 since this was already done in #50102 and we don't need to write it all over again.

Requires:

Make UntilWithSync wait for integrated informers to stop #73080

Release note:

NONE

/cc @wojtek-t @liggitt @tedyu

k8s-ci-robot · 2019-01-17T17:39:05Z

@tnozicka: GitHub didn't allow me to request PR reviews from the following users: tedyu.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

What type of PR is this?
/kind bug

What this PR does / why we need it:
ListWatchUntil can't handle closed watches and is being replaced by UntilWithSync based on informers.

Special notes for your reviewer:
Split from #50102

Replaces #73027 since this was already done in #50102 and we don't need to write it all over again.

Release note:
NONE
/cc @wojtek-t @liggitt @tedyu

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.

liggitt · 2019-01-17T18:06:54Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go

@@ -417,30 +416,6 @@ func TestRotateCertCreateCSRError(t *testing.T) {
 	}
 }

-func TestRotateCertWaitingForResultError(t *testing.T) {


is there a different watch error we could/should trigger to exercise this case, or is timeout the only error that can occur now?

I think informers just go on in parallel no matter what happens

wouldn't setting the timeout short let us exercise this case?

tnozicka · 2019-01-17T18:24:27Z

I'll have to fix the simple fake added 2 months ago here https://github.com/kubernetes/kubernetes/blame/fde87329cbfbd08c6cdf3b6b8dd354ee8e10a858/cmd/kubelet/app/server_bootstrap_test.go#L263 as the strict path matching won't work otherwise

tnozicka · 2019-01-18T16:27:51Z

requires #73080 first
/hold

tnozicka · 2019-01-22T08:04:03Z

rebased to pick #73080
/hold cancel

tnozicka · 2019-01-22T10:18:58Z

/retest

wojtek-t

LGTM - modulo some minor comment.

wojtek-t · 2019-01-22T11:53:16Z

cmd/kubelet/app/server_bootstrap_test.go

@@ -254,7 +255,11 @@ func (s *csrSimulator) ServeHTTP(w http.ResponseWriter, req *http.Request) {
 	defer s.lock.Unlock()
 	t := s.t

-	t.Logf("Request %s %s %s", req.Method, req.URL, req.UserAgent())
+	// filter out timeouts as csrSimulator don't support them
+	req.URL.RawQuery = regexp.MustCompile("&timeout=[0-9smh]*").ReplaceAllLiteralString(req.URL.RawQuery, "")


The * at the end means that we can probably filter out some other parameters.
Maybe that's fine, but if so this requires updating the comment.

the * applies only to the set it is following so it shouldn't filter out other params - https://play.golang.org/p/eOMx58K6U2x

suggest

q := req.URL.Query() q.Del("timeout") q.Del("timeoutSeconds") req.URL.RawQuery = q.Encode()

tnozicka · 2019-01-22T16:21:01Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

-	}); err != nil {
+	// Wait for the certificate to be signed. This interface and internal timout
+	// is a remainder after the old design using raw watch wrapped with backoff.
+	timeout := 15 * time.Minute


hope that seems like a reasonable timeout - there was 1m + the backoff
this whole func is wrapped in backoff and poolInfinite in other place where it accounts for failures. It would be nice to plumb it through but that's not in the scope of this PR.

previously, the backoff was a private package var we manipulated in tests... can we do the same with the timeout, to restore TestRotateCertWaitingForResultError?

(previously, the backoff took 4 * 1m + ~8 minutes + watch durations, so 15 minutes approximates current delay and seems ok)

tnozicka · 2019-01-22T19:12:39Z

/retest

wojtek-t · 2019-01-23T09:11:13Z

This looks reasonable to me now, but I will let @liggitt to make a final look.
/assign @liggitt

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

liggitt · 2019-01-23T14:22:58Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager_test.go

@@ -417,30 +416,6 @@ func TestRotateCertCreateCSRError(t *testing.T) {
 	}
 }

-func TestRotateCertWaitingForResultError(t *testing.T) {


wouldn't setting the timeout short let us exercise this case?

liggitt · 2019-01-23T14:23:19Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

-	}); err != nil {
+	// Wait for the certificate to be signed. This interface and internal timout
+	// is a remainder after the old design using raw watch wrapped with backoff.
+	timeout := 15 * time.Minute


previously, the backoff was a private package var we manipulated in tests... can we do the same with the timeout, to restore TestRotateCertWaitingForResultError?

liggitt · 2019-01-23T14:24:39Z

staging/src/k8s.io/client-go/util/certificate/certificate_manager.go

-	}); err != nil {
+	// Wait for the certificate to be signed. This interface and internal timout
+	// is a remainder after the old design using raw watch wrapped with backoff.
+	timeout := 15 * time.Minute


(previously, the backoff took 4 * 1m + ~8 minutes + watch durations, so 15 minutes approximates current delay and seems ok)

tnozicka · 2019-02-06T19:11:20Z

@liggitt thanks, comments addressed

liggitt · 2019-02-06T22:46:15Z

/retest
/lgtm
/approve

k8s-ci-robot · 2019-02-06T22:46:46Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, tnozicka

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:

~~cmd/kubelet/OWNERS~~ [liggitt]
~~staging/src/k8s.io/client-go/util/certificate/OWNERS~~ [liggitt]

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

fejta-bot · 2019-02-07T04:49:11Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

tnozicka · 2019-02-07T07:28:32Z

/retest

k8s-ci-robot · 2019-02-07T07:53:07Z

@tnozicka: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	`29ba8b2`	link	`/test pull-kubernetes-local-e2e-containerized`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jan 17, 2019

k8s-ci-robot requested a review from liggitt January 17, 2019 17:39

k8s-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Jan 17, 2019

k8s-ci-robot requested a review from wojtek-t January 17, 2019 17:39

tnozicka mentioned this pull request Jan 17, 2019

Replace ListWatchUntil with UntilWithSync to avoid watch timeouts #73027

Closed

tnozicka force-pushed the fix-csr-list-watch branch from 1c8a8dd to 7b7406c Compare January 17, 2019 18:01

liggitt reviewed Jan 17, 2019

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 18, 2019

tnozicka force-pushed the fix-csr-list-watch branch from 7b7406c to 557b82c Compare January 18, 2019 16:34

tnozicka force-pushed the fix-csr-list-watch branch from 557b82c to 693eb08 Compare January 22, 2019 08:03

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 22, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 22, 2019

wojtek-t reviewed Jan 22, 2019

View reviewed changes

tnozicka force-pushed the fix-csr-list-watch branch from 693eb08 to 6924610 Compare January 22, 2019 14:06

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2019

tnozicka commented Jan 22, 2019

View reviewed changes

k8s-ci-robot assigned liggitt Jan 23, 2019

liggitt reviewed Jan 23, 2019

View reviewed changes

tnozicka force-pushed the fix-csr-list-watch branch from 6924610 to b51cb90 Compare February 6, 2019 19:09

tnozicka added 2 commits February 6, 2019 22:27

Switch WaitForCertificate to informers to avoid broken watches

a758872

Update bazel

29ba8b2

tnozicka force-pushed the fix-csr-list-watch branch from b51cb90 to 29ba8b2 Compare February 6, 2019 21:27

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

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

k8s-ci-robot merged commit 1b26097 into kubernetes:master Feb 7, 2019

tnozicka deleted the fix-csr-list-watch branch February 7, 2019 10:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch WaitForCertificate to informers to avoid broken watches #73030

Switch WaitForCertificate to informers to avoid broken watches #73030

tnozicka commented Jan 17, 2019 •

edited

k8s-ci-robot commented Jan 17, 2019

liggitt Jan 17, 2019

tnozicka Jan 17, 2019

liggitt Jan 23, 2019

tnozicka commented Jan 17, 2019

tnozicka commented Jan 18, 2019

tnozicka commented Jan 22, 2019

tnozicka commented Jan 22, 2019

wojtek-t left a comment

wojtek-t Jan 22, 2019

tnozicka Jan 22, 2019 •

edited

liggitt Jan 23, 2019

tnozicka Jan 22, 2019 •

edited

liggitt Jan 23, 2019

liggitt Jan 23, 2019

tnozicka commented Jan 22, 2019

wojtek-t commented Jan 23, 2019 •

edited

liggitt Jan 23, 2019

liggitt Jan 23, 2019

liggitt Jan 23, 2019

tnozicka commented Feb 6, 2019

liggitt commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

fejta-bot commented Feb 7, 2019

tnozicka commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019 •

edited

Switch WaitForCertificate to informers to avoid broken watches #73030

Switch WaitForCertificate to informers to avoid broken watches #73030

Conversation

tnozicka commented Jan 17, 2019 • edited

k8s-ci-robot commented Jan 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented Jan 17, 2019

tnozicka commented Jan 18, 2019

tnozicka commented Jan 22, 2019

tnozicka commented Jan 22, 2019

wojtek-t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka Jan 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka Jan 22, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented Jan 22, 2019

wojtek-t commented Jan 23, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnozicka commented Feb 6, 2019

liggitt commented Feb 6, 2019

k8s-ci-robot commented Feb 6, 2019

fejta-bot commented Feb 7, 2019

tnozicka commented Feb 7, 2019

k8s-ci-robot commented Feb 7, 2019 • edited

tnozicka commented Jan 17, 2019 •

edited

tnozicka Jan 22, 2019 •

edited

tnozicka Jan 22, 2019 •

edited

wojtek-t commented Jan 23, 2019 •

edited

k8s-ci-robot commented Feb 7, 2019 •

edited