Enable throttler in activator #3139

vvraskin · 2019-02-08T10:06:02Z

Proposed Changes

Enable throttler in activator/main.go and in pkg/activator/handler/handler.go.
Define endpoint observer and revision observers in the main method.
Return 503 in case of throttler overflow (should not happen unless the BreakersQueueDepth is reached), and 502 in case if the revision is not found (should not happen)
See the reduction of previously observed 503s - Buffer overload requests in Activator #1846 (comment)
Extend existing unit tests

Release Note

Throttle requests in activator in case of overflow.

knative-prow-robot

@vvraskin: 1 warning.

In response to this:

Fixes #2381

Proposed Changes

Enable throttler in activator/main.go and in pkg/activator/handler/handler.go.

Define endpoint observer and revision observers in the main method.

Return 503 in case of throttler overflow (should not happen unless the BreakersQueueDepth is reached), and 502 in case if the revision is not found (should not happen)

See the reduction of previously observed 503s - Buffer overload requests in Activator #1846 (comment)

Extend existing unit tests

Release Note
Throttle requests in activator in case of overflow.

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.

pkg/activator/throttler.go

markusthoemmes

Yay, thanks again for breaking the PRs into small pieces. I cannot stress enough how much that facilitated reviewing.

This looks good throughout, my comments are mostly refactors and suggestions. Haven't looked at the tests just yet.

cmd/activator/main.go

pkg/activator/handler/handler.go

vvraskin · 2019-02-08T13:42:25Z

Regarding the tests.
I actually see that they are bit mixed up with the current handler tests (since the throttling logic is clenched in the retrying handler at the moment). I'd like to refactor them in the upcoming PRs though. Where I'm planning to replace the revision polling with the Informer. This should allow to split the Throttling handler from the Retrying logic/Metric pushing handler. And it should be way easier to test the throttler without the overhead of the rest of the activator.

cmd/activator/main.go

pkg/activator/handler/handler.go

markusthoemmes

I'm fine with this as-is, thanks for making the quick changes @vvraskin

/approve

/cc @vagababov

Would you do me the honors and give this a second look? I'll leave to you to leave a LGTM. Thanks!

pkg/activator/handler/handler_test.go

vvraskin · 2019-02-10T20:10:42Z

The previous tests failed because of "Scale to 50" - will be rerun after the push. There was one failure of "BuildPipelineAndServe" which looked like an istio/envoy blip with "no healthy upstream".

mattmoor

Bunch of superficial comments, but one high-level one: we need to make sure we can horizontally scale the activator, and so we need to account for it's replication.

thanks for breaking this up.

cmd/activator/main.go

pkg/activator/handler/handler_test.go

pkg/activator/throttler_test.go

pkg/activator/handler/handler_test.go

pkg/activator/handler/handler.go

markusthoemmes

One last nit.

Do you plan on writing an E2E test, that asserts that the burst behavior is as we expect it?

cmd/activator/main.go

vvraskin · 2019-02-12T10:10:10Z

@markusthoemmes
Yeap, I'm planning to add an e2e tests after this one gets in.
On a side note, the latest unit tests/build failures due to a single left-over method, I cleaned it up.

@tcnghia could you please review/approve? I touched the reconciler files.
@mattmoor I moved the DeleteFunc to the Endpoints informer.

pkg/reconciler/filter.go

knative-metrics-robot · 2019-02-12T12:58:00Z

The following is the coverage report on pkg/.
Say /test pull-knative-serving-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/activator/throttler.go	98.0%	98.0%	0.0

vvraskin · 2019-02-12T13:25:51Z

The preUpgrade test seems to be flaky in the pipeline, and it passes locally, so
/test pull-knative-serving-upgrade-tests

…inor nits

…oring

…eFunc

vvraskin · 2019-02-12T16:09:06Z

Unrelated unit test failure,
/test pull-knative-serving-unit-tests

vagababov

/lgtm

markusthoemmes · 2019-02-12T16:30:23Z

/assign @mattmoor

For the leftover approval.

cmd/activator/main.go

mattmoor

/lgtm
/approve

knative-prow-robot · 2019-02-12T17:01:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: markusthoemmes, mattmoor, vvraskin

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/activator/OWNERS~~ [markusthoemmes,mattmoor]
~~pkg/activator/OWNERS~~ [markusthoemmes,mattmoor]
~~pkg/reconciler/OWNERS~~ [mattmoor]

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

knative-prow-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Feb 8, 2019

knative-prow-robot requested review from markusthoemmes and mdemirhan February 8, 2019 10:06

knative-prow-robot reviewed Feb 8, 2019

View reviewed changes

pkg/activator/throttler.go Outdated Show resolved Hide resolved

vvraskin mentioned this pull request Feb 8, 2019

Throttle requests in Activator in case of overflow #2653

Closed

markusthoemmes reviewed Feb 8, 2019

View reviewed changes

vvraskin force-pushed the enable_throttler branch from 5b58d2f to b93fce0 Compare February 8, 2019 13:33

markusthoemmes reviewed Feb 8, 2019

View reviewed changes

cmd/activator/main.go Show resolved Hide resolved

markusthoemmes reviewed Feb 8, 2019

View reviewed changes

pkg/activator/handler/handler.go Show resolved Hide resolved

markusthoemmes reviewed Feb 8, 2019

View reviewed changes

knative-prow-robot requested a review from vagababov February 8, 2019 14:52

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 8, 2019

markusthoemmes approved these changes Feb 8, 2019

View reviewed changes

vagababov reviewed Feb 8, 2019

View reviewed changes

pkg/activator/handler/handler_test.go Outdated Show resolved Hide resolved

pkg/activator/handler/handler_test.go Show resolved Hide resolved

pkg/activator/handler/handler_test.go Show resolved Hide resolved

vvraskin mentioned this pull request Feb 10, 2019

Activator should be sensitive to Revision concurrency. #3151

Closed

vvraskin force-pushed the enable_throttler branch from 1d837a3 to d696c6a Compare February 10, 2019 20:13

mattmoor reviewed Feb 10, 2019

View reviewed changes

knative-prow-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2019

This was referenced Feb 11, 2019

[WIP] Use informer to retrieve active endpoints in activator #3164

Closed

Consider the number of activators when throttling the requests #3165

Closed

Respond with 429 instead of 503 in case of overflow? #3166

Closed

markusthoemmes reviewed Feb 12, 2019

View reviewed changes

cmd/activator/main.go Show resolved Hide resolved

knative-prow-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2019

markusthoemmes reviewed Feb 12, 2019

View reviewed changes

pkg/reconciler/filter.go Outdated Show resolved Hide resolved

vvraskin added 13 commits February 12, 2019 16:55

Replace revision/endpoints getter with lister, move error handling, m…

86666ae

…inor nits

Update endpoints, restructure metric reporting

289bcbb

Further refactor proxyRequest

c655102

Print out an error in the handler

4a0dcf1

Add a comment regarding sleep, refactor test message

5950d44

Change vars to consts in the tests

1c8d774

Add filter for revision's endpoints, update indentation, minor refact…

94faa45

…oring

Use endpoinds observer handler to delete the breaker

2c16747

Sync informers before adding the Add handler

667908a

Generalize filter function

d278b0a

Use filter func for all metav1 objects

88c8177

Add tests for the new filter function, replace cast type in the Delet…

c3fe4c6

…eFunc

Unit tests for the delete opetations should be updated as well

9c7a884

vvraskin force-pushed the enable_throttler branch from ff1a6e4 to 9c7a884 Compare February 12, 2019 15:56

vagababov reviewed Feb 12, 2019

View reviewed changes

knative-prow-robot assigned vagababov Feb 12, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019

knative-prow-robot assigned mattmoor Feb 12, 2019

mattmoor reviewed Feb 12, 2019

View reviewed changes

cmd/activator/main.go Show resolved Hide resolved

Add a comment

fed9c0e

knative-prow-robot removed the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019

mattmoor reviewed Feb 12, 2019

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 12, 2019

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 12, 2019

mattmoor added this to the Serving 0.4 milestone Feb 12, 2019

knative-prow-robot merged commit 262cb03 into knative:master Feb 12, 2019

vvraskin deleted the enable_throttler branch February 13, 2019 13:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable throttler in activator #3139

Enable throttler in activator #3139

vvraskin commented Feb 8, 2019

knative-prow-robot left a comment

markusthoemmes left a comment

vvraskin commented Feb 8, 2019

markusthoemmes left a comment

vvraskin commented Feb 10, 2019

mattmoor left a comment

markusthoemmes left a comment

vvraskin commented Feb 12, 2019

knative-metrics-robot commented Feb 12, 2019

vvraskin commented Feb 12, 2019

vvraskin commented Feb 12, 2019

vagababov left a comment

markusthoemmes commented Feb 12, 2019

mattmoor left a comment

knative-prow-robot commented Feb 12, 2019

Enable throttler in activator #3139

Enable throttler in activator #3139

Conversation

vvraskin commented Feb 8, 2019

Proposed Changes

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

markusthoemmes left a comment

Choose a reason for hiding this comment

vvraskin commented Feb 8, 2019

markusthoemmes left a comment

Choose a reason for hiding this comment

vvraskin commented Feb 10, 2019

mattmoor left a comment

Choose a reason for hiding this comment

markusthoemmes left a comment

Choose a reason for hiding this comment

vvraskin commented Feb 12, 2019

knative-metrics-robot commented Feb 12, 2019

vvraskin commented Feb 12, 2019

vvraskin commented Feb 12, 2019

vagababov left a comment

Choose a reason for hiding this comment

markusthoemmes commented Feb 12, 2019

mattmoor left a comment

Choose a reason for hiding this comment

knative-prow-robot commented Feb 12, 2019