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

Support Consul gRPC Health Checks #4251

Merged
merged 6 commits into from
May 4, 2018
Merged

Support Consul gRPC Health Checks #4251

merged 6 commits into from
May 4, 2018

Conversation

schmichael
Copy link
Member

@schmichael schmichael commented May 4, 2018

Add support for gRPC health checks. Tested against etcd and an altered greeter_server.

greeter_server.gz

patch

grpchealth.nomad

Bumped vendored Consul libraries to 1.0.7 which also required bumping Serf.

@schmichael schmichael force-pushed the f-grpc-checks branch 2 times, most recently from f3b985d to 698adc0 Compare May 4, 2018 00:03
@@ -2,355 +2,2064 @@
"comment": "",
"ignore": "test appengine",
"package": [
{"path":"context","revision":""},
Copy link
Member

Choose a reason for hiding this comment

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

vendorfmt?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oopsie. Thanks.

```

This check would translate to having a Consul check registration with the
[GRPC][consul_grpc] parameter similar to `10.0.3.1:4567/grpc.health.v1.Health`.
Copy link
Member

Choose a reason for hiding this comment

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

its not clear what "parameter similar to 10.0.3.1:4567/grpc.health.V1.health" means here

Copy link
Member Author

Choose a reason for hiding this comment

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

Consul only has a single grpc field that takes the full <host>:<port>/<endpoint> like a URL. Nomad can't do that because the host and port are determined at runtime.

So I'm trying to describe how Nomad's fields relate to the Consul field, but yeah... this is worded badly. Perhaps the fact that it's a single grpc field in Consul is an implementation detail I don't actually have to worry about comparing against?

🤔 I'll get this changed or remove it, but let me know if you have any suggestions!

Copy link
Member

@preetapan preetapan left a comment

Choose a reason for hiding this comment

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

Had one comment about clarifying a doc sentence.

Assuming the service's address is `10.0.3.1` and port is `4567`. See [Using
Driver Address Mode](#using-driver-address-mode) for details on address
selection.
In this example Consul would health check the `grpc.health.v1.Health` service
Copy link
Member Author

Choose a reason for hiding this comment

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

change service name

@schmichael schmichael merged commit bd4e761 into master May 4, 2018
@schmichael schmichael deleted the f-grpc-checks branch May 4, 2018 21:55
@github-actions
Copy link

github-actions bot commented Mar 2, 2023

I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants