Don't retry POST requests in activator

[mdemirhan]

POST requests should not be retried in activator. Instead, activator should use health check signals to wait for the deployment to be fully resolved and working and then do a single POST request.

[sukhil-suresh]

/assign @pivotal-sukhil-suresh

[sukhil-suresh]

@mdemirhan - checking if this issue is blocked by #1241, which is still marked open?

[mdemirhan]

I don't think it should be blocked on that work. We should make this work for the default port for now. When (and if) we fix #1241, we should do it in a way that it enhances your changes in this work to support different ports. Let me know if that makes sense.

[sukhil-suresh]

@mdemirhan thanks for confirming that #1241 is not a blocker. I see your point; ideally, changes made for #1241 will be invisible to the changes (readiness probe calls) made for this issue. Happy to move ahead!

[sukhil-suresh]

Paired with @shashwathi on this yesterday and some thoughts...

@mdemirhan: we assume the intent is to avoid retries for ALL requests GET/POST (when waiting for forwarding to the revision pod). Basing this on your previous comment on a separate issue - #1292 (comment)

The activator/revision.go ActiveEndpoint already does a watch on the revision status and waits until the status is ready. And our understanding is that the activator/main.go retries are specifically for istio route rule update delay related problems. It would be nice if you could confirm.

Considering that the readiness/liveness probe is not mandated for user-container, it seems like the alternate is to use the queue-proxy readinessProbe. But, this /health endpoint only reports the readiness of the queue-proxy itself and does not necessarily reflect the user-container readiness.

[mdemirhan]

CC @tcnghia @akyyy @nikkithurmond @josephburnett

When the health status shows that the pod is healthy, we found that it is not immediately usable by the activator. The reason is, activator use service Ip to communicate with the revision and a pod being ready does not immediately translate into the service being ready to take traffic. In our experience, we found a delay of 3-4 seconds after the pod is ready, that the service can route traffic to the pod (most likely ip table configuration for the service ip to pod ip takes some time).

I like the idea of checking for queue-proxy's health if the user doesn't have a liveness/readiness probe. That would be helpful. Based on your suggestions, how does the following flow work:

• [Current behavior] Wait until the endpoint is ready
• [New behavior] Start checking the health probe using the service IP - if user is missing readiness/liveness, check queue proxy's readiness/liveness
• [Modified behavior] For both GET POST requests, make a single call and return that - no retries

I am partial on whether we should do a small number of retries for GET or just a single one. Let me know if this makes sense.

[sukhil-suresh]

@mdemirhan sorry about the delay in getting back. Yes, the [New behavior] that you have listed is doable and does make sense. Will proceed with this approach to replace the http retries.

Also, seems like there is a longer-term solution being considered in #1509. @shashwathi had suggested something similar last week - the idea of using TCP health checks using istio-proxy (the other container) on the revision pod. I had missed mentioning it in my previous comment

CC @dprotaso @tanzeeb @bsnchan

[mdemirhan]

#1509 or @shashwathi's suggestion is more of an addition to this, rather than a replacement. If the user specified a TCP or an HTTP health check in container spec, we should simply use that because that will be more reliable than a TCP health check to another container (in case the user code startup time is significant). However, if there is no health check endpoint specified in user-container's spec, doing a TCP probe to either queue proxy or istio proxy sounds like a good idea.

[sukhil-suresh]

Paired with @shashwathi on this.

@mdemirhan: Wanted to get your thoughts on some problems we encountered

Based on the K8s docs, the readinessProbe action can be defined by HTTPGet, TCPSocket, and Exec. For the initial iteration, the intent was to focus on the HTTPGet and TCPSocket only.

The good news is that we were able to successfully use the user-defined HTTPGet based readiness probe (instead of making multiple attempts at forwarding request to the revision pod).

However, we are running into problems with the user-defined TCPSocket based probes. TCPSocket based probes have only two values - host and port. The user is not allowed to specify port by the webhook validation. And for the host address, irrespective of whatever the user specifies, we are using the internal app service address.

So, at this point, we think the story may be blocked on issue #1241 at least for user-defined TCPSocket based probes.

Does it sound reasonable to submit a PR with support for user-defined HTTPGet probes and fall back on the default probe (queue-proxy readiness check)? In this case, if the user provides any non-HTTP probes, activator ignores it and uses default probe.

[mdemirhan]

Thanks for the detailed information Sukhil. That plan sounds good to me.

@tcnghia @akyyy @josephburnett FYI

[sukhil-suresh]

Summarizing discussion with @mdemirhan & @shashwathi :

We are trying to tackle the problem if user-defined readiness check probes are not provided and we want to use queue-proxy readiness check. However, the queue proxy exposes only port 8012 (RequestQueuePort) through service port mapping and so we are unable to target the queue proxy health handler from the activator.

Currently, the queue proxy /health endpoint is served on port 8022 (RequestQueueAdminPort). This port is not accessible through the service FQDN, so we are thinking of following options:

1. Queue proxy starts another health server on X port and expose this X port in service port mapping. In this option, the activator can use service http://service-FQDN:X/health to do default readiness check. This option also involves removing the /health endpoint from 8022 (RequestQueueAdminPort).

2. Expose 8022 (RequestQueueAdminPort) on queue proxy through service port mapping. One potential problem is /quitquitquit endpoint on this port is used as preStop hook so we are not sure about security implications of exposing this in service port mapping.

We are leaning towards option 1. What do you think? @tcnghia @josephburnett @ZhiminXiang

Feel free to correct us if we misunderstood the conversation @mdemirhan