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.

Sign up for GitHub

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

chart: add liveness/readiness probes to chartsvc deployment #645

Merged
merged 3 commits into from
Sep 18, 2018
Merged

chart: add liveness/readiness probes to chartsvc deployment #645

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
209f1e5
chart: add liveness/readiness probes to chartsvc deployment
Sep 14, 2018
5696b3c
Merge branch 'master' into add-chartsvc-probes
prydonius Sep 17, 2018
d8d58eb
Merge branch 'master' into add-chartsvc-probes
prydonius Sep 18, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion chart/kubeapps/Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
apiVersion: v1
name: kubeapps
version: 0.4.6
version: 0.4.7
appVersion: DEVEL
description: Kubeapps is a dashboard for your Kubernetes cluster that makes it easy to deploy and manage applications in your cluster using Helm
icon: https://raw.githubusercontent.com/kubeapps/kubeapps/master/docs/img/logo.png
Expand Down
4 changes: 4 additions & 0 deletions chart/kubeapps/templates/chartsvc-deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@ spec:
ports:
- name: http
containerPort: {{ .Values.chartsvc.service.port }}
livenessProbe:
{{ toYaml .Values.chartsvc.livenessProbe | indent 10 }}
readinessProbe:
{{ toYaml .Values.chartsvc.readinessProbe | indent 10 }}
resources:
{{ toYaml .Values.chartsvc.resources | indent 12 }}
{{- with .Values.chartsvc.nodeSelector }}
Expand Down
12 changes: 12 additions & 0 deletions chart/kubeapps/values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@ chartsvc:
# requests:
# cpu: 100m
# memory: 128Mi
livenessProbe:
httpGet:
path: /live
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this route (/live) based on any common practice? I've seen the proliferation of healthz instead

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you are just just using them.

Copy link
Contributor Author

@prydonius prydonius Sep 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@migmartri when I implemented this in chartsvc, I used the https://github.com/heptiolabs/healthcheck library that suggests using /live and /ready https://github.com/heptiolabs/healthcheck#usage. Specifically, the difference from using a single /healthz is having two separate endpoints that might have different implementations of the healthcheck (e.g. /ready might check that MongoDB is connectable, though not currently implemented).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh nice, I was not aware of that library, thanks for the info.

Specifically, the difference from using a single /healthz is having two separate endpoints that might have different implementations of the healthcheck (e.g. /ready might check that MongoDB is connectable, though not currently implemented).

Right, in the past I've been handling this by passing a liveness parameter to the /healthz endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@migmartri makes sense, yeah, the name of the endpoints doesn't matter much here I guess, although I agree that healthz is more common. /live and /ready are also possibly too specific to Kubernetes, although the concept does transfer across. Either way, probably not important enough to change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@migmartri makes sense, yeah, the name of the endpoints doesn't matter much here I guess, although I agree that healthz is more common. /live and /ready are also possibly too specific to Kubernetes, although the concept does transfer across. Either way, probably not important enough to change.

No it's not important, thanks for the info.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the implementation of those healthchecks and it seems that they do not do much apart of being mounted. So I think that this task is still valid.

port: 8080
initialDelaySeconds: 60
timeoutSeconds: 5
readinessProbe:
httpGet:
path: /ready
port: 8080
initialDelaySeconds: 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the default? Or you want to be explicit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oddly, the documentation doesn't specify what the default is: https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.11/#probe-v1-core. I just copied these from the other services that have healtchecks (dashboard and frontend). I think it's good to be explicit (especially given it's unclear what the default is), and I think that it's fine for readiness to not have any delay. The initialDelaySeconds for liveness might actually be too long for a simple service like this, but this could give it more time to attempt to connect to MongoDB.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a funny note. In their advanced example, they mount the readiness check as healthz :P https://godoc.org/github.com/heptiolabs/healthcheck

timeoutSeconds: 5
nodeSelector: {}
tolerations: []
affinity: {}
Expand Down