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.

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

Add livenessProbe to demo deployment #1605

Merged
merged 1 commit into from Aug 2, 2019

Conversation

@charlieegan3
Copy link
Contributor

charlieegan3 commented Aug 1, 2019

I expect many install OPA following this guide (as we did). Recent PRs have made steps to 'productionize' this (e.g. #1435)

We had an incident involving the controller where a stuck container was not restarted. We would have been helped if a liveness probe was configured. We copied the docs and this is our bad but we'd like to do our best to make sure others don't make the same mistake.

I figured it'd be ok to use the health endpoint here

We've made this change and it seems to be working ok for us.

@patrick-east

This comment has been minimized.

Copy link
Contributor

patrick-east commented Aug 1, 2019

Thanks for the patch! Totally agree that we should have these in the docs.

One thing i'm wondering is if, for the sake of consistency, we should have this match up with the default one on the official helm chart https://github.com/helm/charts/blob/master/stable/opa/values.yaml#L159-L172 ?

@charlieegan3

This comment has been minimized.

Copy link
Contributor Author

charlieegan3 commented Aug 2, 2019

@patrick-east Thanks, and no problem! 😁

I was torn between keeping the example concise and adding the check. Agree it is likely best to borrow from the chart here though. I've added this in 27f4e7b

Copy link
Contributor

patrick-east left a comment

LGTM.

If you can squash those two commits together we should be good to merge this

I expect many install OPA following this guide (as we did). Recent PRs
have made steps to 'productionize' this (e.g.
[#1435](#1435))

We had an incident involving the controller where a stuck container was
not restarted. We would have been helped if a liveness probe was
configured. We copied the docs and this is our bad but we'd like to do
our best to make sure others don't make the same mistake.

I figured it'd be ok to use the health endpoint
[here](https://github.com/open-policy-agent/opa/blob/master/docs/content/rest-api.md#health-api)

We've made this change and it seems to be working ok for us.

Signed-off-by: Charlie Egan <charlieegan3@users.noreply.github.com>
@charlieegan3

This comment has been minimized.

Copy link
Contributor Author

charlieegan3 commented Aug 2, 2019

👍 Should be good to go now. Thanks.

@patrick-east patrick-east merged commit 89eacac into open-policy-agent:master Aug 2, 2019
7 checks passed
7 checks passed
Header rules - openpolicyagent No header rules processed
Details
Pages changed - openpolicyagent All files already uploaded
Details
DCO DCO
Details
Mixed content - openpolicyagent No mixed content detected
Details
Redirect rules - openpolicyagent 57 redirect rules processed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/openpolicyagent/deploy-preview Deploy preview ready!
Details
@charlieegan3 charlieegan3 deleted the charlieegan3:add-liveness-to-demo-deployment branch Aug 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.