Add a simple network prober to the activator. #3257

mattmoor · 2019-02-17T23:45:23Z

When the flag -enable-network-probing is passed the activator will replace its retring transport logic with a simple network probe based on #3256 with a similar number of retries to what the retrying transport was previously configured to use. Enabling this allows the GRPC test with streaming and cold-start fairly reliably on my cluster (and also with the GRPC ping sample in knative/docs, with my fixes).

This change also refactors the GRPC test into 4 tests, for each of the logical things tested, which will hopefully reduce the amount of time this adds to e2e dramatically when we switch to use t.Parallel() since it will parallelize the two times this waits for a scale-to-zero.

Fixes: #3239
Fixes: #2856

mattmoor · 2019-02-18T00:18:58Z

/test pull-knative-serving-unit-tests

tcnghia · 2019-02-18T00:59:29Z

/test pull-knative-serving-integration-tests

cmd/activator/main.go

pkg/activator/handler/handler.go

pkg/activator/handler/handler_test.go

test/e2e/grpc_test.go

vagababov · 2019-02-19T00:44:52Z

/lgtm
/hold
in case you want someone else to look as well.

tcnghia · 2019-02-19T04:55:12Z

If this works, let's check it in because I feel like this is part of #813 which is work we signed up for in 0.4.0.

Thanks for the fix!

tcnghia · 2019-02-19T04:55:44Z

/lgtm

mattmoor · 2019-02-19T05:04:06Z

@tcnghia I'm worried that this is potentially disruptive to a key path. After 0.4.0 cuts, I'll check this in enabled by default and we can discuss cherry picking it into 0.4.x at the WG meeting after it's baked at least a little in master.

knative-prow-robot · 2019-02-19T05:08:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mattmoor

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

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

mattmoor · 2019-02-19T16:31:54Z

@markusthoemmes The probe endpoints are in 0.4 and unused. However, this would affect a conversation about whether to cherry-pick this (I added this to the #networking agenda on Thursday).

mattmoor

thanks, RFAL.

pkg/activator/handler/handler.go

mattmoor · 2019-02-19T17:08:40Z

/hold cancel

pkg/activator/handler/handler.go

vagababov · 2019-02-19T18:49:04Z

pkg/activator/handler/handler.go

+					http.CanonicalHeaderKey(network.ProbeHeaderName): []string{"true"},
+				},
+			}
+			settings := wait.Backoff{


I think this invalidates the comment above where maxRetries says that the sum will evaluate to 1 minute. My math shows you'll be getting 37.151 seconds (or 48397, depending on how wait internals are done), but still not a minute.

That comment was written for the other code path. We can clean that up when we remove the other path?

mattmoor · 2019-02-19T19:45:14Z

/test pull-knative-serving-integration-tests

vagababov

/lgtm

mattmoor · 2019-02-19T23:42:30Z

/test pull-knative-serving-integration-tests

vagababov · 2019-02-19T23:48:55Z

/lgtm

mattmoor · 2019-02-20T00:07:13Z

/test pull-knative-serving-integration-tests

greghaynes · 2019-02-20T01:25:10Z

pkg/activator/handler/handler.go

+					a.Logger.Errorw("Pod probe failed", zap.Error(err))
+					return false, nil
+				}
+				if probeResp.StatusCode != http.StatusOK {


Do we want to do similar to the retry roundtripper where we only continue on 503? I worry that retrying on any !ok response will surface e.g. 500 with a 1min delay on activation rather than instant fail.

I changed this to only continue on 503, and below to only declare success on http.StatusOK.

mattmoor · 2019-02-22T01:40:48Z

/test pull-knative-serving-upgrade-tests

pkg/activator/handler/handler.go

pkg/activator/handler/handler_test.go

pkg/activator/handler/handler.go

When the flag `-enable-network-probing` is passed (on by default) the activator will replace its retring transport logic with a simple network probe based on knative#3256 with a similar number of retries to what the retrying transport was previously configured to use. Enabling this allows the GRPC test with streaming and cold-start fairly reliably on my cluster (and also with the GRPC ping sample in knative/docs, with my fixes). This change also refactors the GRPC test into 4 tests, for each of the logical things tested, which will hopefully reduce the amount of time this adds to e2e dramatically when we switch to use `t.Parallel()` since it will parallelize the two times this waits for a scale-to-zero. Fixes: knative#3239 Fixes: knative#2856

k4leung4 · 2019-02-22T17:52:00Z

/lgtm

vagababov

/lgtm

pkg/activator/handler/handler.go

mattmoor · 2019-02-22T21:20:44Z

/retest

jonjohnsonjr · 2019-02-22T22:33:18Z

/retest

knative-prow-robot requested review from markusthoemmes, mdemirhan and srinivashegde86 February 17, 2019 23:45

mattmoor force-pushed the probe-based-activation branch from 334506c to fd054ea Compare February 18, 2019 00:20

vagababov reviewed Feb 18, 2019

View reviewed changes

mattmoor force-pushed the probe-based-activation branch 2 times, most recently from 3dec44f to d73c81f Compare February 19, 2019 00:29

knative-prow-robot assigned vagababov Feb 19, 2019

knative-prow-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Feb 19, 2019

mattmoor force-pushed the probe-based-activation branch from d73c81f to 7d95873 Compare February 19, 2019 01:46

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

mattmoor force-pushed the probe-based-activation branch from 7d95873 to d071d17 Compare February 19, 2019 04:17

knative-prow-robot assigned tcnghia Feb 19, 2019

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

mattmoor force-pushed the probe-based-activation branch from d071d17 to 4b868e7 Compare February 19, 2019 05:08

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

mattmoor force-pushed the probe-based-activation branch from 4b868e7 to f58c04c Compare February 19, 2019 05:25

mattmoor changed the title ~~[WIP] Add a simple network prober to the activator.~~ Add a simple network prober to the activator. Feb 19, 2019

knative-prow-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 19, 2019

mattmoor force-pushed the probe-based-activation branch from f58c04c to 0a6f316 Compare February 19, 2019 17:06

mattmoor commented Feb 19, 2019

View reviewed changes

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

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

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

knative-prow-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 19, 2019

vagababov suggested changes Feb 19, 2019

View reviewed changes

mattmoor force-pushed the probe-based-activation branch from 0a6f316 to dfb4c44 Compare February 19, 2019 19:13

vagababov reviewed Feb 19, 2019

View reviewed changes

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

greghaynes reviewed Feb 20, 2019

View reviewed changes

mattmoor force-pushed the probe-based-activation branch from dfb4c44 to dfcdcc8 Compare February 22, 2019 01:19

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

mattmoor force-pushed the probe-based-activation branch from dfcdcc8 to da4a5f3 Compare February 22, 2019 01:29

k4leung4 reviewed Feb 22, 2019

View reviewed changes

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

k4leung4 reviewed Feb 22, 2019

View reviewed changes

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

jonjohnsonjr reviewed Feb 22, 2019

View reviewed changes

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

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

mattmoor force-pushed the probe-based-activation branch from da4a5f3 to b5dbff6 Compare February 22, 2019 17:47

knative-prow-robot assigned k4leung4 Feb 22, 2019

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

vagababov reviewed Feb 22, 2019

View reviewed changes

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

knative-prow-robot merged commit 7fc1438 into knative:master Feb 22, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a simple network prober to the activator. #3257

Add a simple network prober to the activator. #3257

mattmoor commented Feb 17, 2019 •

edited

Loading

mattmoor commented Feb 18, 2019

tcnghia commented Feb 18, 2019

vagababov commented Feb 19, 2019

tcnghia commented Feb 19, 2019

tcnghia commented Feb 19, 2019

mattmoor commented Feb 19, 2019

knative-prow-robot commented Feb 19, 2019

mattmoor commented Feb 19, 2019

mattmoor left a comment

mattmoor commented Feb 19, 2019

vagababov Feb 19, 2019

mattmoor Feb 19, 2019

mattmoor commented Feb 19, 2019

vagababov left a comment •

edited

Loading

mattmoor commented Feb 19, 2019

vagababov commented Feb 19, 2019

mattmoor commented Feb 20, 2019

greghaynes Feb 20, 2019

mattmoor Feb 22, 2019

mattmoor commented Feb 22, 2019

k4leung4 commented Feb 22, 2019

vagababov left a comment

mattmoor commented Feb 22, 2019

jonjohnsonjr commented Feb 22, 2019

Add a simple network prober to the activator. #3257

Add a simple network prober to the activator. #3257

Conversation

mattmoor commented Feb 17, 2019 • edited Loading

mattmoor commented Feb 18, 2019

tcnghia commented Feb 18, 2019

vagababov commented Feb 19, 2019

tcnghia commented Feb 19, 2019

tcnghia commented Feb 19, 2019

mattmoor commented Feb 19, 2019

knative-prow-robot commented Feb 19, 2019

mattmoor commented Feb 19, 2019

mattmoor left a comment

Choose a reason for hiding this comment

mattmoor commented Feb 19, 2019

vagababov Feb 19, 2019

Choose a reason for hiding this comment

mattmoor Feb 19, 2019

Choose a reason for hiding this comment

mattmoor commented Feb 19, 2019

vagababov left a comment • edited Loading

Choose a reason for hiding this comment

mattmoor commented Feb 19, 2019

vagababov commented Feb 19, 2019

mattmoor commented Feb 20, 2019

greghaynes Feb 20, 2019

Choose a reason for hiding this comment

mattmoor Feb 22, 2019

Choose a reason for hiding this comment

mattmoor commented Feb 22, 2019

k4leung4 commented Feb 22, 2019

vagababov left a comment

Choose a reason for hiding this comment

mattmoor commented Feb 22, 2019

jonjohnsonjr commented Feb 22, 2019

mattmoor commented Feb 17, 2019 •

edited

Loading

vagababov left a comment •

edited

Loading