Skip to content
This repository has been archived by the owner on Apr 17, 2019. It is now read-only.

Nginx Ingress Controller - Missing healthcheck params in upstream #818

Closed
nottix opened this issue Apr 20, 2016 · 11 comments · Fixed by #1002
Closed

Nginx Ingress Controller - Missing healthcheck params in upstream #818

nottix opened this issue Apr 20, 2016 · 11 comments · Fixed by #1002

Comments

@nottix
Copy link

nottix commented Apr 20, 2016

Hi,

With current version (0.5) nginx marks a proxied server with fail status when a single request fails.

I think that the ningx template needs the healthcheck parameters for upstream section, like this:

upstream backend {
server 10.1.0.102 max_fails=3 fail_timeout=30s;
}

@nottix nottix changed the title [Ingress nginx] Missing healthcheck params in upstream Nginx Ingress Controller - Missing healthcheck params in upstream Apr 20, 2016
@bprashanth
Copy link

Actually the ideal case would be to scrape your pod endpoints and grap the parameters from there. It's something I haven't got to yet. It makes sense to have one idiom for periodic health checking in kube (liveness/readiness), instead of one for your pods that the kubelets/kube-proxy understand, and another that is different for each cloudprovider.

If the pod endpoint doesn't have a health check the lb backend can just pick a reasonable default. The suggested default makes sense.

I'd entertain the "health check via annotation per ingress idea" as a last resort, I don't think it's necessary but if enough people want it we can do it as a short term thing. It shouldn't be too hard.

@bprashanth
Copy link

@aledbf fyi

@nottix
Copy link
Author

nottix commented Apr 20, 2016

So do you plan to add default max_fails and fail_timeout configurable via ConfigMap?

@aledbf
Copy link
Contributor

aledbf commented Apr 20, 2016

@nottix I will add upstream-probe-max-fails and upstream-probe-fail-timeout as a global option in the ConfigMap.
The default will be upstream-probe-max-fails=3 and upstream-probe-fail-timeout=10s

@bprashanth
Copy link

I was actually suggesting that you set
https://github.com/kubernetes/kubernetes/blob/master/pkg/api/types.go#L861
In the readiness probe:
https://github.com/kubernetes/kubernetes/blob/master/pkg/api/types.go#L933

so the entier system has a consolidated view of "health"

@nottix
Copy link
Author

nottix commented Apr 20, 2016

Thank you @aledbf.

@bprashanth i'll add readiness probe to my pods, but the current nginx controller skips these values, is it right?

@aledbf
Copy link
Contributor

aledbf commented Apr 20, 2016

but the current nginx controller skips these values,

no, it just uses the defaults from nginx (max_fails=1 fail_timeout=10s). To respect the settings from the probes the default values should be max_fails=0 fail_timeout=0. I need to test this against pod with and without probes to verify the behavior.

@bprashanth
Copy link

Yeah currently you can get it to do what we're thinking by telling nginx to not health check at all, and waiting for Kube to remove the endpoints when they fail their own readiness.

Down the line it would be slick if the nginx controller just scraped these params from pod spec and update its own health checks. The eventual goal is one cross platform HC instead of N per cp/ingress controller etc.

@nottix
Copy link
Author

nottix commented Apr 20, 2016

But how can i tell to nginx to not health check my pod currently (0.5 version)?
Setting max_fails=0 fail_timeout=0 on upstream->server or setting readiness probe on pod?

Thank you guys.

@aledbf
Copy link
Contributor

aledbf commented Apr 20, 2016

@nottix if you need this now you need to build a custom version changing the go template

@nottix
Copy link
Author

nottix commented Apr 20, 2016

@aledbf ok, thank you. I'll change the go template for now, waiting for this improvements.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants