Add health checking from NGINX to individual endpoints. #146

chrismoos · 2017-01-20T05:26:34Z

Overview

This change adds support for the nginx_upstream_check_module and configures upstreams with health checks based on the readinessProbe in the service's pod(s).

It is configurable with ConfigMap (default disabled):

use-upstream-health-checks

If the upstream health checking is enabled there is also an additional endpoint at /upstream_status.

Notes

This was originally proposed in the old NGINX ingress controller @ kubernetes-retired/contrib#2072

This change depends on a new nginx-slim image that would target version 0.13. The PR for that is kubernetes-retired/contrib#2329

A request for this was also raised in #140.

k8s-reviewable · 2017-01-20T05:26:40Z

This change is

coveralls · 2017-01-20T05:43:21Z

Coverage decreased (-0.6%) to 42.723% when pulling 5a3a373 on chrismoos:nginx_health_check into fbcedc0 on kubernetes:master.

coveralls · 2017-02-01T05:39:17Z

Coverage decreased (-0.6%) to 43.645% when pulling 7d1eac3 on chrismoos:nginx_health_check into b9d272a on kubernetes:master.

chrismoos · 2017-02-09T04:27:56Z

@bprashanth I've added in the changes here to nginx that were made on kubernetes-retired/contrib#2329.

coveralls · 2017-02-09T04:52:55Z

Coverage decreased (-0.6%) to 43.777% when pulling 9867801 on chrismoos:nginx_health_check into 0b1fc38 on kubernetes:master.

cmluciano · 2017-02-09T19:54:54Z

Is there any sort of integration or unit tests that could be added here? Not sure if it is plugin specific.

bprashanth

Thanks for the pr, overall looks good, just a request for an example and a few minor code review comments

bprashanth · 2017-02-10T23:14:29Z

controllers/nginx/rootfs/etc/nginx/template/nginx.tmpl

@@ -365,6 +372,12 @@ http {
            access_log off;
            return 200;
        }
+
+        {{ if $cfg.UseUpstreamHealthChecks }}
+        location /upstream_status {


Please add an example, as in this pr: #235, there's already an examples/health-checks directory, just create a gce/ and nginx/ under that and add it under nginx/.

Please also mention in that example what this endpoint exposes.

bprashanth · 2017-02-10T23:16:04Z

core/pkg/ingress/controller/controller.go

@@ -302,6 +308,19 @@ func newIngressController(config *Configuration) *GenericController {
 		glog.Warning("Update of ingress status is disabled (flag --update-status=false was specified)")
 	}

+	ic.podLister.Indexer, ic.podController = cache.NewIndexerInformer(


Fyi this will increase the memory footprint of the controller in large clusters. Somethig to keep in mind, not much we can do about it at this point I guess. We can think about optimizing that when the time comes, however we might have to update examples that have request/limits set (unsure if there are any).

bprashanth · 2017-02-10T23:18:08Z

core/pkg/ingress/controller/controller.go

+	if err != nil {
+		return nil, fmt.Errorf("Failed to get pod listing: %v", err)
+	}
+	for _, pod := range pods {


I guess you want this to deterministically give you the same health check, even if there are 3 endpoint pods behind the service with all different readiness probes, we should sort based on creation timestamp and always take the first one.

bprashanth · 2017-02-10T23:21:15Z

core/pkg/ingress/controller/controller.go

+				if servicePort.Type == intstr.Int && int(port.ContainerPort) == servicePort.IntValue() ||
+					servicePort.Type == intstr.String && port.Name == servicePort.String() {
+					if container.ReadinessProbe != nil {
+						if container.ReadinessProbe.HTTPGet == nil || container.ReadinessProbe.HTTPGet.Scheme != "HTTP" {


Any reason to not support https health checks? (I'm fine with the HTTP caveat, just curious)

bprashanth · 2017-02-10T23:24:06Z

core/pkg/ingress/types.go

@@ -102,6 +102,16 @@ type BackendInfo struct {
 	Repository string `json:"repository"`
 }

+// UpstreamCheck is used to configure ingress health checks
+type UpstreamCheck struct {
+	HttpSend       string


please add a godoc comment for each public field, especially HttpSend, Fall and Rise which are non-obvious.
Also please s/Http/HTTP in conformance with https://github.com/golang/go/wiki/CodeReviewComments#initialisms

bprashanth · 2017-02-10T23:25:30Z

Also please add unittests

bprashanth · 2017-02-10T23:29:55Z

Ideally a unittest might seed the pod cache with some fake pods and check to see that the right health check is retrieved with int/named ports. Let me know if you need me to dig up example unittests where we do something similar.

@cmluciano integration tests are a little harder, we're working on something generally applicable cross kube-repo, but till then we mostly rely on e2es and unittests. The ingress e2es already point at an rc with a readiness probe, so we can probably modify that to fail if this feature doesn't work once the image is published: https://github.com/kubernetes/kubernetes/blob/master/test/e2e/testing-manifests/ingress/http/rc.yaml#L18

bprashanth · 2017-02-10T23:31:16Z

@cmluciano integration tests are a little harder

#5

abstrctn · 2017-03-14T16:13:58Z

Is this still under active development? Would be very useful for us, I'd be happy to lend a hand if it's helpful.

chrismoos · 2017-07-16T20:28:55Z

See #981, this is probably preferable to this as unfortunately this duplicates the readinessProbe checking already built into Kubernetes.

Add nginx 1.11.3 | Resolve merge conflicts by moving dependency updates to separate commit

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 20, 2017

chrismoos mentioned this pull request Feb 8, 2017

Add nginx_upstream_check_module. kubernetes-retired/contrib#2329

Closed

bprashanth assigned aledbf and bprashanth Feb 8, 2017

Add health checking from NGINX to individual endpoints.

9867801

chrismoos force-pushed the nginx_health_check branch from 7d1eac3 to 9867801 Compare February 9, 2017 04:26

bprashanth reviewed Feb 10, 2017

View reviewed changes

chrismoos closed this Jul 16, 2017

krunalhinguu pushed a commit to krunalhinguu/ingress-nginx that referenced this pull request Jan 6, 2025

Merge pull request kubernetes#146 from chiukapoor/nginx-1.11.3

d4f428d

Add nginx 1.11.3 | Resolve merge conflicts by moving dependency updates to separate commit

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add health checking from NGINX to individual endpoints. #146

Add health checking from NGINX to individual endpoints. #146

chrismoos commented Jan 20, 2017

k8s-reviewable commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Feb 1, 2017

chrismoos commented Feb 9, 2017

coveralls commented Feb 9, 2017

cmluciano commented Feb 9, 2017

bprashanth left a comment

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth Feb 10, 2017

bprashanth commented Feb 10, 2017

bprashanth commented Feb 10, 2017

bprashanth commented Feb 10, 2017

abstrctn commented Mar 14, 2017

chrismoos commented Jul 16, 2017

Add health checking from NGINX to individual endpoints. #146

Add health checking from NGINX to individual endpoints. #146

Conversation

chrismoos commented Jan 20, 2017

Overview

Notes

k8s-reviewable commented Jan 20, 2017

coveralls commented Jan 20, 2017

coveralls commented Feb 1, 2017

chrismoos commented Feb 9, 2017

coveralls commented Feb 9, 2017

cmluciano commented Feb 9, 2017

bprashanth left a comment

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth Feb 10, 2017

Choose a reason for hiding this comment

bprashanth commented Feb 10, 2017

bprashanth commented Feb 10, 2017

bprashanth commented Feb 10, 2017

abstrctn commented Mar 14, 2017

chrismoos commented Jul 16, 2017