Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

consul - add readiness probe #56

Merged
merged 1 commit into from Sep 9, 2016

Conversation

lachie83
Copy link
Contributor

@lachie83 lachie83 commented Sep 1, 2016

Added readiness prod to deployment spec

@k8s-bot
Copy link

k8s-bot commented Sep 1, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

@viglesiasce
Copy link
Contributor

ok to test

@@ -157,6 +157,12 @@ spec:
- name: gossip-key
secret:
secretName: gossip-key
readinessProbe:
httpGet:
path: /v1/health/service/consul
Copy link
Contributor

Choose a reason for hiding this comment

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

Dumb question. Does this work with clustered consul? With the master when it was in boostrap mode, we had issues with the readinessProbe working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chrislovecnm - It should work and always return details of the servers in the cluster. I haven't had an issue where it hasn't worked using this chart. Have you? Is there another endpoint that would better server as readinessProbe?

Copy link
Contributor

Choose a reason for hiding this comment

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

yah we have had a problem with that endpoint, when using DNS names for discovery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay thanks. I'll take a look at polling another endpoint. Do you have any suggestions?

Copy link
Contributor

Choose a reason for hiding this comment

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

We did a netstat to see if Consul was listening to the port, but maybe it is the way we set it up. I don't personally like the netstat either. I will take it out for a test drive when I have a chance. But if it is working for you, we can iterate on it in incubator.

@lachie83
Copy link
Contributor Author

lachie83 commented Sep 8, 2016

I think this is GTG. LMK if anything else is outstanding.

@chrislovecnm
Copy link
Contributor

@viglesiasce LGTM ... How about u?

@viglesiasce
Copy link
Contributor

LGTM

@viglesiasce viglesiasce added the lgtm Indicates that a PR is ready to be merged. label Sep 9, 2016
@viglesiasce viglesiasce merged commit f13b90c into helm:master Sep 9, 2016
viglesiasce pushed a commit to viglesiasce/charts that referenced this pull request Oct 8, 2016
@lachie83 lachie83 deleted the fix-add-consul-healthchecks branch April 14, 2017 16:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants