Prepare queue healthHandler for optional aggressive retries #4649

joshrider · 2019-07-08T21:35:09Z

Co-authored-by: Shash Reddy [email protected]

Support #4014
One of many parts, spun out from #4600

Proposed Changes

Update queue health package to include TCP and HTTP retries and pave way for user-defined readinessProbes from the queue-proxy.
Following PRs will use this package.

Release Note

Co-authored-by: Shash Reddy <[email protected]>

googlebot · 2019-07-08T21:35:12Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

knative-prow-robot

@joshrider: 1 warning.

In response to this:

Co-authored-by: Shash Reddy [email protected]

Support #4014
One of many parts, spun out from #4600

Proposed Changes

Update queue health package to include TCP and HTTP retries and pave way for user-defined readinessProbes from the queue-proxy.

Following PRs will use this package.

Release Note

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/queue/health/probe.go

Co-authored-by: Shash Reddy <[email protected]>

cmd/queue/main.go

pkg/queue/health/health_state.go

pkg/queue/health/probe.go

vagababov · 2019-07-08T22:09:03Z

pkg/queue/health/probe.go

+		},
+		Timeout: config.Timeout,
+	}
+	url := fmt.Sprintf("%s://%s:%d%s", config.Scheme, config.Host, config.Port.IntValue(), config.Path)


Why not construct url.URL object and then use its String method?

If we build url.URL object then would we end up with something like below

url := url.URL{ Scheme: fmt.Sprintf("%s", config.Scheme), Host: fmt.Sprintf("%s:%d", config.Host, config.Port.IntValue()), Path: config.Path, }

Not sure if we gain anything by making building URL object.

Well:

Scheme can be just assigned (no need to sprintf)

Host is just config.Host + ":" + config.Port.String() (no need to sprintf)

you don't have to deal with validating slashes "/" (you don't currently, so in fact a wrong URL can be built)

Scheme can be just assigned (no need to sprintf)

config.Scheme is not string type so that needs to be converted.

I agree with the rest so I will update

URIScheme is string alias. So just string(config.Scheme) will do.

pkg/queue/health/probe.go

shashwathi · 2019-07-08T23:28:35Z

@vagababov : Addressed your comments. Ready for another review 👍

pkg/queue/health/probe.go

mattmoor · 2019-07-08T23:38:09Z

pkg/queue/health/probe.go

+	}
+
+	if !IsHTTPProbeReady(res) {
+		return errors.New("HTTP probe did not respond Ready")


Feels like this should include the status code 🤔

vagababov

Mostly cosmetic from me.

vagababov · 2019-07-08T23:46:35Z

pkg/queue/health/health_state.go

@@ -95,7 +97,7 @@ func (h *State) HealthHandler(prober func() bool) func(w http.ResponseWriter, r
 		}

 		switch {
-		case h.IsAlive():
+		case isNotAggressive && h.IsAlive():


I kind of envisioned the change to be !isAggressive here. But I guess this works also :)

will brainstorm a more descriptive name and update in future 🤔

vagababov · 2019-07-08T23:47:03Z

cmd/queue/main.go

@@ -249,7 +253,8 @@ func handler(reqChan chan queue.ReqEvent, breaker *queue.Breaker, handler http.H
 // Sets up /health and /wait-for-drain endpoints.
 func createAdminHandlers() *http.ServeMux {
 	mux := http.NewServeMux()
-	mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer))
+	// TODO(@joshrider): temporary change while waiting on other PRs to merge (See #4014)
+	mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer, true /*isNotAggressive is set to true to piggyback on previous success and not aggressively retry everytime*/))


Suggested change

mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer, true /*isNotAggressive is set to true to piggyback on previous success and not aggressively retry everytime*/))

mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer, true /*isNotAggressive*/))

vagababov · 2019-07-08T23:48:28Z

pkg/queue/health/probe.go

+		Timeout: config.Timeout,
+	}
+	url := url.URL{
+		Scheme: fmt.Sprintf("%s", config.Scheme),


Suggested change

Scheme: fmt.Sprintf("%s", config.Scheme),

Scheme: string(config.Scheme),

mattmoor

Couple comments still inline seem relevant

joshrider · 2019-07-09T13:53:10Z

On the case 👍

pkg/queue/health/probe.go

pkg/queue/health/probe_test.go

Co-authored-by: Shash Reddy <[email protected]>

knative-metrics-robot · 2019-07-09T15:01:52Z

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/queue/health/probe.go	100.0%	90.9%	-9.1

mattmoor · 2019-07-09T15:25:30Z

/lgtm
/approve

knative-prow-robot · 2019-07-09T15:25:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: joshrider, 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/queue/OWNERS~~ [mattmoor]
~~pkg/network/OWNERS~~ [mattmoor]
~~pkg/queue/OWNERS~~ [mattmoor]

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

googlebot · 2019-07-09T15:25:47Z

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

prepare queue healthHandler for optional aggressive retries knative#4014

be8f3fc

Co-authored-by: Shash Reddy <[email protected]>

googlebot added the cla: no Indicates the PR's author has not signed the CLA. label Jul 8, 2019

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

knative-prow-robot reviewed Jul 8, 2019

View reviewed changes

pkg/queue/health/probe.go Show resolved Hide resolved

knative-prow-robot requested review from mdemirhan and vagababov July 8, 2019 21:35

knative-prow-robot added the area/networking label Jul 8, 2019

add documentation for IsHTTPProbeReady

29ef695

Co-authored-by: Shash Reddy <[email protected]>

vagababov suggested changes Jul 8, 2019

View reviewed changes

shashwathi force-pushed the queue-health branch from b63ab10 to 3dc6fa9 Compare July 8, 2019 23:33

mattmoor reviewed Jul 8, 2019

View reviewed changes

vagababov reviewed Jul 8, 2019

View reviewed changes

Address comments

5969bf0

shashwathi force-pushed the queue-health branch from 3dc6fa9 to 5969bf0 Compare July 9, 2019 04:13

mattmoor reviewed Jul 9, 2019

View reviewed changes

add kubelet header to http probe, add status code to failed probe error

0503417

joshrider force-pushed the queue-health branch from d01c335 to 0503417 Compare July 9, 2019 13:59

mattmoor reviewed Jul 9, 2019

View reviewed changes

pkg/queue/health/probe.go Show resolved Hide resolved

pkg/queue/health/probe.go Outdated Show resolved Hide resolved

pkg/queue/health/probe_test.go Outdated Show resolved Hide resolved

use kubelet user-agent in http probe

c82f8a7

Co-authored-by: Shash Reddy <[email protected]>

knative-prow-robot assigned mattmoor Jul 9, 2019

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

mattmoor added cla: yes Indicates the PR's author has signed the CLA. and removed cla: no Indicates the PR's author has not signed the CLA. labels Jul 9, 2019

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

knative-prow-robot merged commit 82596a8 into knative:master Jul 9, 2019

joshrider mentioned this pull request Jul 9, 2019

Queue ReadinessProbe Kubelet Header e2e Test #4667

Merged

joshrider deleted the queue-health branch August 6, 2019 15:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prepare queue healthHandler for optional aggressive retries #4649

Prepare queue healthHandler for optional aggressive retries #4649

joshrider commented Jul 8, 2019

googlebot commented Jul 8, 2019

knative-prow-robot left a comment

vagababov Jul 8, 2019

shashwathi Jul 8, 2019 •

edited

Loading

vagababov Jul 8, 2019

shashwathi Jul 8, 2019

vagababov Jul 8, 2019

shashwathi commented Jul 8, 2019

mattmoor Jul 8, 2019

vagababov left a comment

vagababov Jul 8, 2019

joshrider Jul 9, 2019

vagababov Jul 8, 2019

vagababov Jul 8, 2019

mattmoor left a comment

joshrider commented Jul 9, 2019

knative-metrics-robot commented Jul 9, 2019

mattmoor commented Jul 9, 2019

knative-prow-robot commented Jul 9, 2019

googlebot commented Jul 9, 2019

	mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer, true /isNotAggressive is set to true to piggyback on previous success and not aggressively retry everytime/))
	mux.HandleFunc(requestQueueHealthPath, healthState.HealthHandler(probeUserContainer, true /isNotAggressive/))

	Scheme: fmt.Sprintf("%s", config.Scheme),
	Scheme: string(config.Scheme),

Prepare queue healthHandler for optional aggressive retries #4649

Prepare queue healthHandler for optional aggressive retries #4649

Conversation

joshrider commented Jul 8, 2019

Proposed Changes

googlebot commented Jul 8, 2019

knative-prow-robot left a comment

Choose a reason for hiding this comment

Proposed Changes

Choose a reason for hiding this comment

shashwathi Jul 8, 2019 • 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

shashwathi commented Jul 8, 2019

Choose a reason for hiding this comment

vagababov left a 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

mattmoor left a comment

Choose a reason for hiding this comment

joshrider commented Jul 9, 2019

knative-metrics-robot commented Jul 9, 2019

mattmoor commented Jul 9, 2019

knative-prow-robot commented Jul 9, 2019

googlebot commented Jul 9, 2019

shashwathi Jul 8, 2019 •

edited

Loading