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

Set readiness probe port based on user's configuration #1241

Closed
ZhiminXiang opened this issue Jun 15, 2018 · 7 comments · Fixed by #2642
Closed

Set readiness probe port based on user's configuration #1241

ZhiminXiang opened this issue Jun 15, 2018 · 7 comments · Fixed by #2642
Assignees
Labels
area/networking kind/feature Well-understood/specified features, ready for coding.
Milestone

Comments

@ZhiminXiang
Copy link

/area networking
/kind dev

Expected Behavior

We should set readiness probe's port based on user's configuration.

Actual Behavior

Currently we always use RequestQueuePort as readiness probe port.

userContainer.ReadinessProbe.Handler.HTTPGet.Port = intstr.FromInt(queue.RequestQueuePort)

@google-prow-robot google-prow-robot added area/networking kind/feature Well-understood/specified features, ready for coding. labels Jun 15, 2018
@ZhiminXiang
Copy link
Author

@mdemirhan @tcnghia

@tcnghia
Copy link
Contributor

tcnghia commented Jun 18, 2018

The spec also mentions the kind of Readiness check if user don't provide HTTP health check (we'll resort to a TCP handshake).

@tcnghia
Copy link
Contributor

tcnghia commented Jun 28, 2018

According to spec, the user container always listen on :8080 right? This is hard-wired to queue-controller, and that in turn is exposed at RequestQueuePort.

By doing health check through RequestQueuePort we can make sure the health check is more end-to-end.

@tcnghia
Copy link
Contributor

tcnghia commented Jun 28, 2018

So I think there is no issue here, please reopen if there are other things that we need to address.

@tcnghia tcnghia closed this as completed Jun 28, 2018
@ZhiminXiang
Copy link
Author

ZhiminXiang commented Jun 28, 2018

I see. yea we should always send request to RequestQueuePort.

But when user defines their own port (e.g. 8042) for health check, should we redirect the health check request to 8042 in the queue proxy? Currently we always direct the request to 8080 in queue proxy.

@tcnghia

@ZhiminXiang ZhiminXiang reopened this Jun 28, 2018
@tcnghia
Copy link
Contributor

tcnghia commented Jun 28, 2018

I see, I think we can change to only use RequestQueuePort if the port is missing or same as the main port. Otherwise we leave it the same. Adding more proxies may over-complicate things with not a lot to gain.

@mattmoor
Copy link
Member

/assign @dgerd

mgencur pushed a commit to mgencur/serving-1 that referenced this issue Sep 12, 2022
* Update update-ci.sh

* Fix continuous name

* Use TLS in the same file

* Add 4.10 and drop 4.6, 4.7

Co-authored-by: Kenjiro Nakayama <nakayamakenjiro@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/networking kind/feature Well-understood/specified features, ready for coding.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants