feat: add health probes to node & go service

[lance]

Until the next release of Quarkus in November, we'll need to have some special
checks so that we don't add these probes to Quarkus based functions. In the
meantime, we can at least set the liveness and readiness endpoints on the
Node.js/Go functions to /health/liveness and /health/readiness respectively.

Fixes: #169

Signed-off-by: Lance Ball lball@redhat.com

[matejvasek]

@lance I am getting port 0 instead 8080 in the resulting ksvc.

[matejvasek]

         livenessProbe:
           httpGet:
             path: /health/liveness
             port: 0
         name: user-container
         readinessProbe:
           httpGet:
             path: /health/readiness
             port: 0
           successThreshold: 1

[matejvasek]

I tired fix that and got:

Error: knative deployer failed to deploy the service: admission webhook "validation.webhook.serving.knative.dev" denied the request: validation failed: must not set the field(s): spec.template.spec.containers[0].livenessProbe.httpGet.port, spec.template.spec.containers[0].readinessProbe.httpGet.port

[matejvasek]

intstr.FromString(port) might be better.
But leads to validation error mentioned before.

[markusthoemmes]

You should be able to drop this completely. The "correct" port is inferred. The readiness endpoint has to be served on the "default" server though (the one listening on $PORT).

[matejvasek]

@lance Maybe the port shouldn't be set for ksvc. Maybe the knative operator sets it for pods automatically.

 420   │     livenessProbe:
 421   │       failureThreshold: 3
 422   │       httpGet:
 423   │         httpHeaders:
 424   │         - name: K-Kubelet-Probe
 425   │           value: queue
 426   │         path: /health/liveness
 427   │         port: 8012
 428   │         scheme: HTTP

 536   │     readinessProbe:
 537   │       exec:
 538   │         command:
 539   │         - /ko-app/queue
 540   │         - -probe-period
 541   │         - "0"
 542   │       failureThreshold: 3
 543   │       periodSeconds: 1
 544   │       successThreshold: 1
 545   │       timeoutSeconds: 10

Above is from pods generated for ksvc.

Looks like readiness probe is ignored and it uses something related to queue.
For liveness probe it takes path into account but port is autoset.

[matejvasek]

@rhuss could you pleas help us here, how are readiness/liveness probes supposed to work with knative service?

[lance]

@matejvasek your function was deployed when you first tried with the PR as written, and it came up? If so, then they work as expected, right? I tested on a local cluster and it was successful for me. When testing with Port removed, the service failed to come up.

[matejvasek]

@lance
I think it should be:

diff --git a/knative/deployer.go b/knative/deployer.go
index 5745ed0..760e51e 100644
--- a/knative/deployer.go
+++ b/knative/deployer.go
@@ -8,7 +8,6 @@ import (
        corev1 "k8s.io/api/core/v1"
        "k8s.io/apimachinery/pkg/api/errors"
        metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-       "k8s.io/apimachinery/pkg/util/intstr"
        servinglib "knative.dev/client/pkg/serving"
        "knative.dev/client/pkg/wait"
        servingv1 "knative.dev/serving/pkg/apis/serving/v1"
@@ -91,14 +90,11 @@ func (d *Deployer) Deploy(f faas.Function) (err error) {
        return nil
 }
 
-func probeFor(port, url string) *corev1.Probe {
+func probeFor(url string) *corev1.Probe {
        return &corev1.Probe{
                Handler: corev1.Handler{
                        HTTPGet: &corev1.HTTPGetAction{
                                Path: url,
-                               Port: intstr.IntOrString{
-                                       StrVal: port,
-                               },
                        },
                },
        }
@@ -115,8 +111,8 @@ func generateNewService(name, image string, healthChecks bool) *servingv1.Servic
        }
 
        if healthChecks {
-               containers[0].LivenessProbe = probeFor("8080", "/health/liveness")
-               containers[0].ReadinessProbe = probeFor("8080", "/health/readiness")
+               containers[0].LivenessProbe = probeFor("/health/liveness")
+               containers[0].ReadinessProbe = probeFor("/health/readiness")
        }
 
        return &v1.Service{

With that it works for me (service is up).

This:

Port: intstr.IntOrString{
    StrVal: port,
}

doesn't hurt, but it does nothing, because there is Type member it the struct and by default it's Int, so setting just StrVal doesn't have any effect.

I mean if anything it should be:

Port: intstr.IntOrString{
    Type:   intstr.String,
    StrVal: "8080",
},

[matejvasek]

When testing with Port removed, the service failed to come up.

that's odd, it works for me, I think removing it should have no effect.