Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consolidate queue-proxy probe handlers #5465

Closed
yanweiguo opened this issue Sep 9, 2019 · 5 comments

Comments

@yanweiguo
Copy link
Contributor

@yanweiguo yanweiguo commented Sep 9, 2019

In what area(s)?

/area API
/area networking
/kind proposal

Describe the feature

Have a simple and concentrated handler in queue-proxy to handle probe requests.

Currently we have 3 handlers in queue-proxy:
https://github.com/knative/serving/blob/master/cmd/queue/main.go#L158
https://github.com/knative/serving/blob/master/cmd/queue/main.go#L501
https://github.com/knative/serving/blob/master/cmd/queue/main.go#L447

They handle the following probe requests:

Probe Request Source Path Extra features comment
Activator probe requests :8012 With header K-Network-Probe=queue. Expected queue as response body. Probe requests from Activator before it proxies external requests
VirtualService/Gateway probe requests :8012 With header K-Network-Probe=probe and non-empty K-Network-Hash header This is used to detect which version of a VirtualService an Envoy Pod is currently serving. They are proxied from VirtualService to activator/queue-proxy.
Kubelet probe requests :8012 With non-empty K-Kubelet-Probe header or with header user-agent=kube-probe/* I don't think currently kubectl sends probe requests to this path. We can delete it. Correct me if I was wrong.
Queue-proxy Readiness Exec probe requests :8022/health   Probe requests triggered by Kubelet Readiness Exec probe.

Proposing the following changes:

  • Delete handler for Kubelet probe.
  • Change Queue-proxy Readiness Exec Exec probe to send GET requests to :8012 instead of :8022\health with header K-Network-Probe=queue.
  • Reduce to a single handler in queue-proxy which:
    • probe user-container if K-Network-Probe=queue and returns queue as the body. This is for activator probe and Exec probe.
    • returns 200 if K-Network-Probe=queue and non-empty K-Network-Hash. This is for VirtualService probe.

cc @JRBANCEL @vagababov @tcnghia @mattmoor @markusthoemmes

@markusthoemmes

This comment has been minimized.

Copy link
Contributor

@markusthoemmes markusthoemmes commented Sep 10, 2019

Sounds good, two comments:

  1. The kubelet probe requests path would be used (or would have been used 🤔 ) if the user specified a custom http probe. As we run that probe ourselves now, that might indeed be not needed anymore.
  2. Ditching /health sounds good, we just need to make sure that quick retry mechanisms stay intact. We talked about moving that to a common place anyway so it's a welcome change!

Thanks or going for it.

@JRBANCEL

This comment has been minimized.

Copy link
Contributor

@JRBANCEL JRBANCEL commented Sep 10, 2019

I don't know if you'll be able to coalesce the handlers to a single handler because some logic (Activator Probing & VirtualService/Gateway Probing) is used in both Activator and Queue Proxy, while some other logic (probing user-container) is not.

But you should be able to clean that up for sure.

@yanweiguo yanweiguo self-assigned this Sep 10, 2019
@yanweiguo

This comment has been minimized.

Copy link
Contributor Author

@yanweiguo yanweiguo commented Sep 10, 2019

Hi @joshrider @shashwathi, you implemented the Exec probe. WDYT here?

@joshrider

This comment has been minimized.

Copy link
Member

@joshrider joshrider commented Sep 11, 2019

Sounds good to me. Queue-proxy could definitely use some tidying up. IIRC @markusthoemmes suggested something similar when the ExecProbe stuff was initially being done.

The other suggestion that was sort of left as a TODO was the possibility of having the activator's probe succeed without consulting the user-container if the healthState was already healthy.

@yanweiguo

This comment has been minimized.

Copy link
Contributor Author

@yanweiguo yanweiguo commented Sep 30, 2019

The remain work for this issue is to coalesce the two handlers to a single handler in queue-proxy. As @JRBANCEL mentioned, one handler is shared by both queue-proxy and activator. It is not easy to do without introducing non-relative codes to activator. So I'll keep as they are currently.

@yanweiguo yanweiguo closed this Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.