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

fix(kubernetes): switch k8s readiness check from exec to httpGet for ingress support #1171

Merged
merged 2 commits into from
Jan 31, 2019

Conversation

paulczar
Copy link
Contributor

services that have a healthcheck defined were getting an exec check
for the kubernetes readiness check and were running a wget command
against localhost to verify health. Kubernetes has the httpGet option
for readiness check that does this in a more k8s native way and doesn't
rely on running a command inside the pod.

This is especially important for people that want to put spinnaker
behind an ingress controller or istio as by default many ingress controllers
look for a http or tcp socket readiness check to be used by the cloud
loadbalancer to ensure the service is up.

From Ingress
documentation:

It’s also worth noting that even though health checks are not exposed directly through the ingress, there exist parallel concepts in Kubernetes such as readiness probes which allow you to achieve the same end result. Please review the controller specific docs to see how they handle health checks ( nginx, GCE).

The Google Ingress controller requires a http/tcp based readiness check
and the nginx Ingress controller can utilize them as well.

fixes spinnaker/spinnaker#3864

Signed-off-by: Paul Czarkowski username.taken@gmail.com

services that have a healthcheck defined were getting an `exec` check
for the kubernetes readiness check and were running a wget command
against localhost to verify health.  Kubernetes has the httpGet option
for readiness check that does this in a more k8s native way and doesn't
rely on running a command inside the pod.

This is especially important for people that want to put spinnaker
behind an ingress controller or istio as by default many ingress controllers
look for a http or tcp socket readiness check to be used by the cloud
loadbalancer to ensure the service is up.

From Ingress
[documentation](https://kubernetes.io/docs/concepts/services-networking/ingress/#loadbalancing):

```
It’s also worth noting that even though health checks are not exposed directly through the ingress, there exist parallel concepts in Kubernetes such as readiness probes which allow you to achieve the same end result. Please review the controller specific docs to see how they handle health checks ( nginx, GCE).

```

The Google Ingress controller requires a http/tcp based readiness check
and the nginx Ingress controller can utilize them as well.

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
@spinnakerbot
Copy link
Contributor

The following commits need their title changed:

  • 78b6ce7: switch k8s readiness check from exec to httpGet

Please format your commit title into the form:

<type>(<scope>): <subject>, e.g. fix(kubernetes): address NPE in status check

This allows us to easily generate changelogs & determine semantic version numbers when cutting releases. You can read more about commit conventions here.

@ajordens ajordens requested a review from lwander January 18, 2019 20:45
@paulczar paulczar changed the title switch k8s readiness check from exec to httpGet fix(kubernetes): switch k8s readiness check from exec to httpGet for ingress support Jan 18, 2019
@paulczar
Copy link
Contributor Author

Oh also an unavailable version of kubectl was being referenced in the dockerfiles so I had to update that to get a working kubectl in there to verify functionality.

@lwander
Copy link
Member

lwander commented Jan 23, 2019

Can you verify this works with the latest istio release? We started using exec as a workaround to their strange tls setup.

@paulczar
Copy link
Contributor Author

@lwander thanks for the heads up on istio. I've modified this to have a config setting to determine if the healthcheck should be an exec or not, with a default to exec. This way the current functionality with istio works, but is easily flagged to use httpGet instead. This should keep both use cases happy.

Copy link
Member

@lwander lwander left a comment

Choose a reason for hiding this comment

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

Awesome. I'll merge now & could you update the docs here to point out the new flag? https://www.spinnaker.io/reference/halyard/custom/#tweakable-service-settings

@lwander lwander merged commit baad218 into spinnaker:master Jan 31, 2019
paulczar added a commit to paulczar/spinnaker.github.io that referenced this pull request Feb 1, 2019
see spinnaker/halyard#1171

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
dorbin pushed a commit to spinnaker/spinnaker.github.io that referenced this pull request Apr 22, 2019
* update for new service setting `useExecHealthCheck`

see spinnaker/halyard#1171

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>

* Update custom.md

Signed-off-by: Paul Czarkowski <username.taken@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

halyard unable to do google ingress controller for spinnaker
3 participants