GCE: Fix operation polling and error handling #64630

nicksardo · 2018-06-01T20:24:03Z

Cloud functions using the generated API are bursting operation GET calls because we don't wait a minimum amount of time.

Fixes #64712
Fixes #64858

Changes

operationPollInterval is now 1 second instead of 3 seconds.
operationPollRateLimiter is now configured with 5 QPS / 5 burst instead of 10 QPS / 10 burst.
gceRateLimiter is now configured with a MinimumRateLimiter to wait the above operationPollInterval duration before waiting on the token rate limiter.
Operations are now rate limited on the very first GET call.
Operations are polled until DONE or context times out (even if operations.get fails continuously).
Compute operations are checked for errors when they're recognized as DONE.
All "wrapper" funcs now generate a context with an hour timeout.

ingress-gce will need to update its vendor and utilize the MinimumRateLimiter as well. Since ingress creates rate limiters based off flags, we'll need to check the resource type and operation while parsing the flags and wrap the appropriate one.

Special notes for your reviewer:
/assign bowei
/cc bowei

Fix Example
Creating an external load balancer

without fix: https://pastebin.com/raw/NNkeNWS3
with fix: https://pastebin.com/raw/x2iMLW5S (a difference of about 200 GET calls)

Release note:

GCE: Fixes operation polling to adhere to the specified interval. Furthermore, operation errors are now returned instead of ignored.

verult · 2018-06-11T17:20:30Z

/cc

nicksardo · 2018-06-11T22:04:23Z

/cc @rramkumar1

rramkumar1 · 2018-06-12T16:35:06Z

pkg/cloudprovider/providers/gce/cloud/service.go

@@ -27,6 +27,10 @@ import (
 	ga "google.golang.org/api/compute/v1"
 )

+const (
+	maxOperationGetErrorStreak = 50


I would call this maxConsecutiveOperationGetErrors

rramkumar1 · 2018-06-12T16:35:18Z

pkg/cloudprovider/providers/gce/cloud/service.go

+// If an error occurs retrieving the operation, the loop will continue for `maxOpGetRetries` then
+// finally error. This is to prevent a transient error from bubbling up to controller-level logic.
+func (s *Service) pollOperation(ctx context.Context, op operation) error {
+	var errorStreak int


consecutiveErrors

rramkumar1 · 2018-06-12T22:04:59Z

/retest

k8s-ci-robot · 2018-06-13T17:22:07Z

@nicksardo: You must be a member of the kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/kind bug
/priority critical-urgent
/milestone v1.11
/sig gcp

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.

nicksardo · 2018-06-13T19:26:59Z

PTAL @bowei

bowei · 2018-06-14T17:49:21Z

pkg/cloudprovider/providers/gce/cloud/errors.go

+
+// Error returns a string representation including the HTTP Status code, GCE's error code
+// and a human readable message.
+func (e GCEOperationError) Error() string {


(e *GCEOperationError)

bowei · 2018-06-14T17:49:47Z

pkg/cloudprovider/providers/gce/cloud/errors.go

+}
+
+// Error returns a string representation including the last poll error encountered.
+func (e OperationPollingError) Error() string {


bowei · 2018-06-14T17:56:45Z

pkg/cloudprovider/providers/gce/cloud/ratelimit.go

+// acceptor is an object which blocks within Accept until a call is allowed to run.
+// Accept is a behavior of the flowcontrol.RateLimiter interface.
+type acceptor interface {
+	Accept()


bowei · 2018-06-14T17:56:51Z

pkg/cloudprovider/providers/gce/cloud/ratelimit.go

+
+// AcceptRateLimiter wraps an Acceptor with RateLimiter parameters.
+type AcceptRateLimiter struct {
+	Acceptor acceptor


bowei · 2018-06-14T17:57:11Z

pkg/cloudprovider/providers/gce/cloud/ratelimit.go

+// MinimumRateLimiter wraps a RateLimiter and will only call its Accept until the minimum
+// duration has been met or the context is cancelled.
+type MinimumRateLimiter struct {
+	RateLimiter RateLimiter


bowei · 2018-06-14T18:00:48Z

pkg/cloudprovider/providers/gce/cloud/service.go

+		// returning ctx.Err().
+		select {
+		case <-ctx.Done():
+			glog.V(5).Infof("op.pollOperation(%v, %v) not completed, poll count = %v, ctx.Err = %v", ctx, op, pollCount, ctx.Err())


use %d for ints

bowei · 2018-06-14T18:00:57Z

pkg/cloudprovider/providers/gce/cloud/service.go

-	glog.V(5).Infof("op.isDone(%v) complete; op = %v", ctx, op)
-	return nil
+
+	glog.V(5).Infof("op.isDone(%v) complete; op = %v, poll count = %v, op.err = %v", ctx, op, pollCount, op.error())


use %d for ints

bowei · 2018-06-14T18:01:20Z

pkg/cloudprovider/providers/gce/cloud/service_test.go

+)
+
+func TestPollOperation(t *testing.T) {
+	var attempts int


const totalAttempts = 11

bowei · 2018-06-14T18:01:33Z

pkg/cloudprovider/providers/gce/cloud/service_test.go

+func TestPollOperation(t *testing.T) {
+	var attempts int
+	fo := &fakeOperation{isDoneFunc: func(ctx context.Context) (bool, error) {
+		if attempts <= 10 {


< totalAttemps

bowei · 2018-06-14T18:01:49Z

pkg/cloudprovider/providers/gce/cloud/service_test.go

+		t.Errorf("pollOperation() = %v, want nil", err)
+	}
+	if attempts != 11 {
+		t.Errorf("`attempts` = %v, want 11", attempts)


want %d", ..., totalAttempts

bowei · 2018-06-14T18:22:39Z

/lgtm
/approve

k8s-ci-robot · 2018-06-14T18:22:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bowei, nicksardo

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:

~~pkg/cloudprovider/providers/gce/OWNERS~~ [bowei,nicksardo]

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

bowei · 2018-06-14T18:28:21Z

/milestone v1.11
/status approved-for-milestone

k8s-ci-robot · 2018-06-14T18:28:48Z

@bowei:

In response to this:

/woof

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.

k8s-github-robot · 2018-06-14T18:29:05Z

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

@bowei @nicksardo

Pull Request Labels

sig/gcp: Pull Request will be escalated to these SIGs if needed.
priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
kind/bug: Fixes a bug discovered during the current release.

Help

dims · 2018-06-14T19:24:42Z

/test pull-kubernetes-bazel-build

k8s-github-robot · 2018-06-14T21:10:36Z

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

k8s-github-robot · 2018-06-14T21:11:14Z

Automatic merge from submit-queue (batch tested with PRs 64272, 64630). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot · 2018-06-14T21:52:54Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-e2e-kops-aws	`787f3a6`	link	`/test pull-kubernetes-e2e-kops-aws`

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.

…630-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64630: Wait a minimum amount of time for polling operations Cherry pick of #64630 on release-1.10. #64630: Wait a minimum amount of time for polling operations

anfernee · 2018-06-21T01:00:06Z

pkg/cloudprovider/providers/gce/cloud/op.go

+		e := op.Error.Errors[0]
+		o.err = &GCEOperationError{HTTPStatusCode: op.HTTPStatusCode, Code: e.Code, Message: e.Message}
+	}
+	return true, nil


Lots of these code are very similar. actually you can embed a struct with an error method. When I wrote this, I realized that it also applies to String() and isDone(). feel free to ignore, totally optional. maybe i will submit a PR later.

k8s-ci-robot assigned bowei Jun 1, 2018

k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 1, 2018

k8s-ci-robot requested a review from bowei June 1, 2018 20:24

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 1, 2018

nicksardo force-pushed the fix-op-rate branch from 58d2146 to 15f82c9 Compare June 1, 2018 20:28

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 Jun 8, 2018

k8s-ci-robot requested a review from verult June 11, 2018 17:20

nicksardo force-pushed the fix-op-rate branch 5 times, most recently from 9210eaa to 72f68e2 Compare June 11, 2018 21:56

k8s-ci-robot requested a review from rramkumar1 June 11, 2018 22:04

nicksardo force-pushed the fix-op-rate branch from 72f68e2 to bee8fb7 Compare June 11, 2018 23:57

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 12, 2018

nicksardo force-pushed the fix-op-rate branch 2 times, most recently from f69e15c to 20b3bcc Compare June 12, 2018 16:14

rramkumar1 reviewed Jun 12, 2018

View reviewed changes

nicksardo force-pushed the fix-op-rate branch 2 times, most recently from 156bff2 to 891080a Compare June 12, 2018 16:42

nicksardo changed the title ~~GCE: Wait a minimum amount of time for polling operations~~ GCE: Fix operation polling and error handling Jun 12, 2018

nicksardo force-pushed the fix-op-rate branch 2 times, most recently from 3632b39 to 20e662b Compare June 12, 2018 16:52

k8s-ci-robot added kind/bug Categorizes issue or PR as related to a bug. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. sig/gcp labels Jun 13, 2018

bowei reviewed Jun 14, 2018

View reviewed changes

nicksardo force-pushed the fix-op-rate branch from b1618e3 to 1fb1824 Compare June 14, 2018 18:10

Nick Sardo added 2 commits June 14, 2018 11:20

Wait a minimum amount of time for polling operations

115ddc5

Use context with timeout instead of context.Background

787f3a6

nicksardo force-pushed the fix-op-rate branch from 1fb1824 to 787f3a6 Compare June 14, 2018 18:20

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

k8s-ci-robot added the status/approved-for-milestone label Jun 14, 2018

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

k8s-ci-robot added the ¯\_(ツ)_/¯ ¯\\\_(ツ)_/¯ label Jun 14, 2018

k8s-github-robot merged commit 0f87069 into kubernetes:master Jun 14, 2018

nicksardo mentioned this pull request Jun 14, 2018

Automated cherry pick of #64630: Wait a minimum amount of time for polling operations #65119

Merged

anfernee reviewed Jun 21, 2018

View reviewed changes

MrHohn mentioned this pull request Jun 21, 2018

[gce provider ] Wrapper for beta backend service create/update #64793

Merged

nicksardo mentioned this pull request Jul 19, 2018

GCE: Wrong HTTP status code returned in operation error #66399

Closed

RainbowMango mentioned this pull request Aug 22, 2019

Cleanup gce static analysis issues #81771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: Fix operation polling and error handling #64630

GCE: Fix operation polling and error handling #64630

nicksardo commented Jun 1, 2018 •

edited

Loading

verult commented Jun 11, 2018

nicksardo commented Jun 11, 2018

rramkumar1 Jun 12, 2018

nicksardo Jun 12, 2018

rramkumar1 Jun 12, 2018

rramkumar1 commented Jun 12, 2018

k8s-ci-robot commented Jun 13, 2018

nicksardo commented Jun 13, 2018 •

edited

Loading

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei Jun 14, 2018

bowei commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

bowei commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

dims commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018 •

edited

Loading

anfernee Jun 21, 2018

GCE: Fix operation polling and error handling #64630

GCE: Fix operation polling and error handling #64630

Conversation

nicksardo commented Jun 1, 2018 • edited Loading

verult commented Jun 11, 2018

nicksardo commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rramkumar1 commented Jun 12, 2018

k8s-ci-robot commented Jun 13, 2018

nicksardo commented Jun 13, 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

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

bowei commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

bowei commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

dims commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

k8s-github-robot commented Jun 14, 2018

k8s-ci-robot commented Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

nicksardo commented Jun 1, 2018 •

edited

Loading

nicksardo commented Jun 13, 2018 •

edited

Loading

k8s-ci-robot commented Jun 14, 2018 •

edited

Loading