[incubator/thumbor] add container probes

[hairmare]

what

• adds livenessProbe and readinessProbe
• name the container port "http" so it can be referenced by name

why

• make it easier to rollout changes to the thumbor install without downtime
• naming ports helps reference them in other places (like probes)
• Thumbor has some info on the /healthcheck endpoint in their docs and they recommend if for use behind a load balancer (and K8s Services are kinda loadbalancers).

[osterman]

Thanks @hairmare ! I'm going to give at @goruha a chance to review as well, but LGTM.

[alebabai]

@hairmare looks good!
but what do you think about following solution? It will add more flexibility and avoid spec. hardcoding.

{{- with .Values.probes }}
{{ toYaml . | indent 10 }}
{{- end }}

and values.yaml part can be like this:

probes
  livenessProbe:
     httpGet:
       path: /healthcheck
       port: http
    initialDelaySeconds: 120
    periodSeconds: 10
    timeoutSeconds: 5
    failureThreshold: 6
    successThreshold: 1
  readinessProbe:
     httpGet:
       path: /healthcheck
       port: http
    initialDelaySeconds: 30
    periodSeconds: 10
    timeoutSeconds: 5
    failureThreshold: 6
    successThreshold: 1

[hairmare]

@alebabai I was mostly looking at existing helm/charts charts for inspiration and found quite some that follow this explicit style of configuration. Sadly the helm best practices don't really go into any detail on this.

I did helm create with 2.10 and it currently creates the following probes in a new deployment template.

          livenessProbe:
            httpGet:
              path: /
              port: http
          readinessProbe:
            httpGet:
              path: /
              port: http

How about something like this:

        probes:
          {{- if .Values.livenessProbe.enabled }}
          livenessProbe:
            httpGet:
              path: /healthcheck
              port: http
{{- with .Values.livenessProbe.config }}
{{ toYaml . | indent 12 }}
{{- end }}
          {{- end }}

This way the default from K8s are used if no values are specified and the actual checks are still in the template in a fashion close to what is proposed by helm create.

I feel like moving the complete probes to values.yaml kind of defeats the purpose of having a helm template in the first place.

The individual enable flags also make it nice and easy to spin up a broken thubmor pod that I can exec into if I need to to further debug failing healthchecks.

I'm not sure about the .Values.livenessProbe.config in my example above since it doesn't feel very natural.

[osterman]

defeats the purpose of having a helm template in the first place
@hairmare understand your point. There's a fine (somewhat arbitrary) line drawn and no rule when to do it. I like to think of values.yaml as a declarative interface to deploying resources on kubernetes. For the current case-in-point, there's a 1-to-1 mapping between elements for the probes (with exception of enabled), thus explicitly passing them doesn't really busy us anything other than extra maintenance =)

The approach @alebabai suggests is pretty common in the official helm/charts. Here are some examples:

• https://github.com/helm/charts/blob/master/incubator/zookeeper/templates/statefulset.yaml#L64-L67
• https://github.com/helm/charts/blob/master/stable/wordpress/templates/deployment.yaml#L120

I don't have a strong feeling either way. @goruha mention to me via Slack he prefers @alebabai way too.

We are though leaning towards this approach and adopted it in our monochart as well:

https://github.com/cloudposse/charts/blob/master/incubator/monochart/templates/deployment.yaml#L60-L62

Our monochart represents our stab at creating a one-size-fits-all chart for deploying the most "common" types of apps.

I'm not sure about the .Values.livenessProbe.config in my example above since it doesn't feel very natural.

Yea, don't like adding the extra level of config.

[hairmare]

@osterman @alebabai I updated by PR to look like the examples linked by @osterman.

[osterman]

thanks @hairmare !

[hairmare]

Thanks for the feedback and merge!