Activator unit tests by tanzeeb · Pull Request #1689 · knative/serving

tanzeeb · 2018-07-25T18:05:40Z

Refactored retryRoundTripper and activationHandler into multiple
components to make testing simpler.

(Continuation of #1585)

tanzeeb · 2018-07-25T18:08:39Z

/assign @josephburnett @akyyy @mdemirhan

mdemirhan · 2018-07-25T19:04:06Z

+	}
+}
+
+func (h uploadHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {


Can you define the method on the pointer rather than the value type?
func (h *uploadHandler) ServeHTTP(...)

mdemirhan · 2018-07-25T19:04:40Z

+	}
+}
+
+func (fa fakeActivator) ActiveEndpoint(namespace, name string) (activator.Endpoint, activator.Status, error) {


Define the func on the pointer rather than the value type.

mdemirhan · 2018-07-25T19:07:31Z

+var baseTransport http.RoundTripper = roundTripperFunc(func(r *http.Request) (*http.Response, error) {
+	var transport http.RoundTripper = http.DefaultTransport
+	if r.ProtoMajor == 2 {
+		transport = h2cutil.DefaultTransport


My preference would be getting rid of this func and providing http1 and http2 transports as inputs to the rountripper. That way, we will get better coverage (i.e. the logic to select rounttripper is also covered) and also the tests wouldn't have to know the internals of rounttripper as much.

See: https://github.com/mdemirhan/serving/blob/2bf10f2fca8e56ea174b3259287f62bc2385f26c/pkg/activator/httptransport.go#L33 as an example.

And corresponding fake transport: https://github.com/mdemirhan/serving/blob/2bf10f2fca8e56ea174b3259287f62bc2385f26c/pkg/activator/httptransport_test.go#L38

And table tests:
https://github.com/mdemirhan/serving/blob/2bf10f2fca8e56ea174b3259287f62bc2385f26c/pkg/activator/httptransport_test.go#L69

It feels a little weird to have RetryRoundTripper be concerned with transport protocol selection. If there was better test coverage, would you be OK with keeping this logic separate?

Yes, that sounds good to me. Thanks!

mdemirhan · 2018-07-25T19:15:54Z

+		if resp.StatusCode == status {
+			resp.Body.Close()
+
+			return nil, fmt.Errorf("Filtering %d", status)


Converting an HTTP response to error doesn't seem right. This will definitely confuse people who are debugging this.

Instead of making this two roundtrippers that are chained together, can you simply get a func as an input: something like type ShouldRetryFunc func(*http.Response) (bool) and removing the statusFilterRoundTripper in favor of this func.

Yeah, that makes sense. I'll take a stab at this.

mdemirhan · 2018-07-25T19:18:50Z

+	}
+
+	// TODO: add metrics for number of tries and the response code.
+	if resp != nil {


Can you change this so that we also log error cases? It seems weird that we log happy path but not unhappy path. Something like:

if resp != nil { rrt.logger.Infof("Activation finished after %d attempt(s). Response code: %d", i, resp.StatusCode) } else { rrt.logger.Errorf("Activation failed after %d attempts. Last error: %v", i, err) }

mdemirhan · 2018-07-25T19:21:14Z

+		status    int
+		err       error
+	}{
+		{"filtered status", goodRT, 502, errors.New("Filtering 502")},


Test cases here seem to be too limited. Can we add tests to cover all the cases listed below?

https://github.com/mdemirhan/serving/blob/2bf10f2fca8e56ea174b3259287f62bc2385f26c/pkg/activator/httptransport_test.go#L81

Sure, I'll add test cases for http1/http2. I think everything else is covered in other, separate tests?

Actually there are a few more. List of test cases to add so far:

http1/http2

nil response body

Is the nil response body check actually necessary? From the source, Response Body is guaranteed to be non-nil:

https://github.com/golang/go/blob/master/src/net/http/response.go#L57-L62

google-prow-robot · 2018-08-01T23:32:13Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: tanzeeb
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: mattmoor

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

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

tanzeeb · 2018-08-02T19:03:20Z

Thanks for the feedback @mdemirhan. I've made some updates to the RetryRoundTripper and removed StatusFilterRoundTripper. The components have also been moved out of cmd/activator -- pkg/activator for ActivationHandler, and cmd/util for all others.

Refactored retryRoundTripper and activationHandler into multiple components to make testing simpler. Fixes knative#1231

…ipper

…dition as parameters

Co-authored-by: Nader Ziada <nziada@pivotal.io> Co-authored-by: David Protasowski <dprotasowski@pivotal.io>

knative-metrics-robot · 2018-08-09T17:57:18Z

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

File	Old Coverage	New Coverage
pkg/activator/util/retryer.go	Do not exist	100.0%
pkg/activator/util/rewinder.go	Do not exist	90.0%
pkg/activator/util/transports.go	Do not exist	100.0%
pkg/activator/handler/enforce_length_handler.go	Do not exist	100.0%
pkg/activator/handler/handler.go	Do not exist	100.0%
pkg/activator/handler/reporting_handler.go	Do not exist	100.0%

nader-ziada · 2018-08-09T18:25:05Z

@mdemirhan @josephburnett

Please take another look - just rebased this PR while @tanzeeb is out on PTO.

/cc @dprotaso

dprotaso

I helped wrap this up so I approve ;)

josephburnett · 2018-08-10T16:49:59Z

+
+// NewLinearRetryer will return a retryer that retries `action` up to
+// `maxRetries` times with `interval` delay between retries
+func NewLinearRetryer(interval time.Duration, maxRetries int) Retryer {


@markusthoemmes is adding exponential backoff in #1814 and I've suggested to @pivotal-sukhil-suresh to reuse that code in #1665 (review). This would be a good way to do that reuse, with an ExponentialRetrier.

So there are three changes in-flight in this space. I'm not sure what order they should land in. Any suggestions?

I'd suggest to land this one first. Then I can base my PR on top of it (I felt bad because I had no tests anyway) and then @pivotal-sukhil-suresh can base off of mine to gain the ability of having an exponential backoff. WDYT @josephburnett

That would be fine.

josephburnett

/lgtm
/approve

knative-prow-robot · 2018-08-10T17:08:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, josephburnett, tanzeeb

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~cmd/activator/OWNERS~~ [josephburnett]
~~cmd/queue/OWNERS~~ [josephburnett]
~~pkg/activator/OWNERS~~ [josephburnett]
~~pkg/h2c/OWNERS~~ [josephburnett]

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

josephburnett · 2018-08-10T17:09:03Z

@mdemirhan is on leave. It looks like all his review comments have been addressed.

nader-ziada · 2018-08-10T19:31:46Z

Thanks @josephburnett, will look at #1665 with @pivotal-sukhil-suresh after @markusthoemmes PR is merged in.

Was originally fixed in knative#1733, but accidentally reverted in knative#1689

google-prow-robot requested review from josephburnett and mattmoor July 25, 2018 18:05

google-prow-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 25, 2018

tanzeeb mentioned this pull request Jul 25, 2018

Add unit tests for activator #1585

Closed

google-prow-robot assigned akyyy, josephburnett and mdemirhan Jul 25, 2018

tanzeeb mentioned this pull request Jul 25, 2018

Add activator metrics and dashboard #1567

Closed

mdemirhan suggested changes Jul 25, 2018

View reviewed changes

This was referenced Jul 26, 2018

Add unit tests for cmd/activator/main.go. #1727

Closed

Add activator metrics and dashboard #1726

Merged

tanzeeb force-pushed the activator-unit-tests branch from 35294e6 to 70952ff Compare July 27, 2018 23:34

tanzeeb and others added 13 commits August 9, 2018 13:54

Add unit tests for activator

837cce5

Refactored retryRoundTripper and activationHandler into multiple components to make testing simpler. Fixes knative#1231

Use pointers in activator round trippers to reduce copying overhead

095944c

Add h2c.DefaultTransport, similar to http.DefaultTransport

7e434e9

Use pointer receivers for all handlers in cmd/activator

c423515

Log both happy and sad paths in the activator

7b8f356

Add explicit labels for table tests in activator

396d417

Add tests for http protocol selection in activator

9d65687

Separate retry logic from request logic in cmd/activator/retryRoundTr…

f41e5d4

…ipper

Refactor retryRoundTripper to take retry strategy and shouldRetry con…

dec60e7

…dition as parameters

Move non-activator specific components from cmd/activator to cmd/util

26dc0f8

Move activation handler into pkg/activator

503aea8

Add a more fluent interface to RetryRoundTripper

79fccdb

Rebase changes and further refactor stats reporting

41287ba

Co-authored-by: Nader Ziada <nziada@pivotal.io> Co-authored-by: David Protasowski <dprotasowski@pivotal.io>

nader-ziada force-pushed the activator-unit-tests branch from f57f317 to 41287ba Compare August 9, 2018 17:55

knative-prow-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Aug 9, 2018

knative-prow-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Aug 9, 2018

knative-prow-robot requested a review from dprotaso August 9, 2018 18:25

dprotaso approved these changes Aug 9, 2018

View reviewed changes

josephburnett reviewed Aug 10, 2018

View reviewed changes

josephburnett approved these changes Aug 10, 2018

View reviewed changes

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 10, 2018

knative-prow-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 10, 2018

josephburnett merged commit e3618fa into knative:master Aug 10, 2018

josephburnett mentioned this pull request Aug 10, 2018

Initial PR for avoiding http retries by activator #1665

Closed

tanzeeb added a commit to tanzeeb/serving that referenced this pull request Aug 15, 2018

Re-remove timeout hacks

9333b35

Was originally fixed in knative#1733, but accidentally reverted in knative#1689

tanzeeb mentioned this pull request Aug 15, 2018

Re-remove timeout hacks #1863

Closed

trisberg mentioned this pull request Aug 16, 2018

0->1 activation errors #1872

Closed

tcnghia mentioned this pull request Aug 17, 2018

Remove no longer needed workaround. #1887

Merged

Conversation

tanzeeb commented Jul 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tanzeeb commented Jul 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-prow-robot commented Aug 1, 2018

Uh oh!

tanzeeb commented Aug 2, 2018

Uh oh!

knative-metrics-robot commented Aug 9, 2018

Uh oh!

nader-ziada commented Aug 9, 2018

Uh oh!

dprotaso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

josephburnett left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Aug 10, 2018

Uh oh!

josephburnett commented Aug 10, 2018

Uh oh!

nader-ziada commented Aug 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

tanzeeb commented Jul 25, 2018 •

edited

Loading