Implement /v1/agent/health/service/<service name> endpoint #3551

kamaradclimber · 2017-10-07T16:21:49Z

This endpoint aggregate all checks related to on the agent
and return an appropriate http code + the string describing the worst
check.

This allows to cleanly expose service status to other component, hiding
complexity of multiple checks.
This is especially useful to use consul to feed a loadbalancer which
would deleguate healthchecking to consul agent.

Exposing this endpoint on the agent is necessary to avoid a hit on
consul servers and avoid decreasing resiliency (this endpoint will work
even if there is no consul leader in the cluster).

Fix #2488, relates to #802

Change-Id: Ib340c62bbbba46fd4256ed31474d8ffb1762d4df
Signed-off-by: Grégoire Seux g.seux@criteo.com

kamaradclimber · 2017-10-07T17:29:38Z

test in master branch do not pass at the moment, but I'll rebase when they will be green again

pierrecdn

As requested, mostly copied what has been said in criteo-forks@ea589ed.

pierrecdn · 2017-10-10T08:21:03Z

agent/agent_endpoint.go

+	}
+	if len(serviceChecks) == 0 {
+		resp.WriteHeader(http.StatusBadRequest)
+		fmt.Fprintf(resp, "Invalid serviceID %s", serviceID)


Bad request or Not found (404) in that case ?

404 is probably more relevant, thx

pierrecdn · 2017-10-10T08:23:29Z

agent/agent_endpoint.go

+	// TODO: should we filter using acls like in AgentChecks method?
+	serviceChecks := make(api.HealthChecks, 0)
+	for _, c := range checks {
+		if c.ServiceID == serviceID {


Usually these APIs are based on the service names.
It would be probably a good thing to also authorize calling the endpoint by passing the name. This would imply returning all the services matching the service name on the agent.

Unless we add another endpoint, I think it might be confusing for users to interpret parameter as service id or name depending on xxx (based on what btw?).

Any input from hashicorp member?

No, the endpoints could be different (/service vs /services for ex).
My point was to allow using svc names when using this API.

pierrecdn · 2017-10-10T08:27:26Z

agent/agent_endpoint.go

+	case api.HealthPassing:
+		resp.WriteHeader(http.StatusOK)
+	default:
+		resp.WriteHeader(http.StatusServiceUnavailable)


While the "unavailable" semantic is limited, HTTP-speaking, to "503", I would use the body to add details regarding what is currently failing.

What kind of information would you add? which healthcheck?

I would at least add the name of the failing healthcheck(s).

kamaradclimber · 2017-10-12T16:17:18Z

@slackpad would you have feedback regarding this PR?

kamaradclimber · 2017-10-29T10:23:50Z

@magiconair would you have feedback on that PR?

kamaradclimber · 2017-11-19T12:02:42Z

rebasing on latest commit on master passing the CI. I'm hoping this will allow to get some attention on this PR.

kamaradclimber · 2018-01-23T17:57:50Z

I've rebased and fixed conflicts. Is anyone interested by the feature? I'd gladly take feedback from hashicorp member.

kamaradclimber · 2018-02-11T08:56:30Z

After discussion with @pierrecdn, he will update this PR to take the service name as input (instead of service id). This will better fit the use case of loadbalancer (such as haproxy or F5) for which healthcheck endpoint is usually not configurable apart from the hostname/post couple.

pierrecdn · 2018-02-13T10:07:12Z

So as discussed, I'm adding these changes on top.

pierresouchay · 2018-03-27T09:59:07Z

Added more tests

We could also use this service to create aliases of services, see for instance https://groups.google.com/d/msg/consul-tool/iKJlYP-wGLw/HA4XcQERBQAJ

kamaradclimber · 2018-04-14T08:40:19Z

@preetapan would you have feedback on that PR?
We are using it to improve our LB stack to leverage consul health checks (instead of asking the loadbalancers to have their own).

pearkes · 2018-04-27T16:27:07Z

A couple things, sorry for the delay in feedback here @kamaradclimber.

Do you mind removing changes to the makefile?
This will need documentation (website/) given it is a new API endpoint
Importantly, we're having a bit of trouble getting to the bottom of your use case. We see some similar discussion in Local health endpoint #2488 in regards to ELBs, and think this looks like generally a reasonable change, but still don't see the specific use case. Can you explain in depth?

Thanks!

pierresouchay · 2018-04-27T17:15:53Z

Hello @pearkes ,

We basically want to use this in order to use Consul as a Cache for all healthChecks for a given service.
The idea is to have a service with 3 healthChecks:

one very fast and dumb on service port (currently used by our LBs)
a 2nd one very complex run every 10s, more complex, opening a database connection for instance
another more complex run every minute on admin port (detecting deadlocks for instance)

HeathChecks 2 and 3 are on another port than the main service, but the first healthcheck is dumb. What we want to do is to have a combinaison of all those heath checks, thus the loadbalancer will take better decision. Thus, this endpoint output the result of healthchecks[1,2,3] without calling those healthchecks (so, for instance does not open a new DB connection).

What it means is:

every LB can call very quickly this new endpoint and have a very good quality heathcheck
as soon as we add new heath checks, we do not call heavily those heath checks that are called ONLY by Consul, but the aggregated result is sent to LB
it also means we can use ONLY one healthcheck per service on our load balancers (many of them cannot have several healthchecks)

Another option is to be able to rename services as described in this thread: https://groups.google.com/d/msg/consul-tool/iKJlYP-wGLw/HA4XcQERBQAJ => it gives us the ability to easily rename service without duplicating everything

I'll try to create the website documentation as soon as possible

Kind regards

kadaan · 2018-05-01T06:37:25Z

@kamaradclimber I had the same needs as you and solved it via consulate. The main difference is Consulate is a sidecar that can be installed on the same box as consul-agent.

kamaradclimber · 2018-05-01T07:48:27Z

Thanks @kadaan. Indeed consulate solve the same need, with more configurability. However it requires one more agent running on all machines (or container depending on your model), to monitor, to update, to patch to tweak.
Given the simplicity of the proposed feature, I feel it makes sense to have it in consul itself.

Of course it depends on what consul maintainers think about this.

kadaan · 2018-05-01T07:57:24Z

@kamaradclimber Thanks! There are plusses and minuses both ways. For us it works well as all the concerns above are abstracted away via ansible. We use consul to verify consulate health, and prometheus scrape the metrics from consulate using consul discovery. That said, if it had already been part of consul, there is no way I would have spent time building it.

One other benefit of the sidecar approach for us is being able to get this built and out in a really short period of time. Trying to get even a "simple" feature merge into a open source project can take a long time. When things are baked into the core, they really need to be sure that the long term support costs are worth it, the feature is generic enough, etc.

So, if Hashicorp decides to take it as a core piece of the consul offering, I'll move to that as soon as I can upgrade.

On the other hand, if they don't take the change, I see Consulate growing in importance for our stack.

Like you we want to boil down all health checking to be backed by consul checks. That means the ELBs should verify consul health checks, ansible handlers should verify consul health checks, etc..

kamaradclimber · 2018-05-01T08:02:04Z

@pearkes, is the use case clearer?
We want to leverage consul as a source for our load balancer while avoiding to duplicate health checks definition.
Doing so allow users to understand only one service definition format (consul) and benefit from healthchecks in all systems built around it:

pseudo loadbalancing with dns (using consul dns api)
regular loadbalancers (in our case haproxy and F5)
client side loadbalancers (loadbalancer running in client processes or as client companions)

As @pierresouchay explained, with thispatch, consul has the ability to hide the complexity of healthchecks (definition, frequency) and expose a dead simple api for all consumers.

One addition benefit for users is they can describe very heavy healthchecks if necessary, which will be executed only by the consul agent (instead of having it executed by all loadbalancers instances).

In criteo, we've built a free loadbalancer for all our internal users based on consul+haproxy (next paris HUG will give some details I think) where all consul services benefit from a loadbalancer without any additional configuration required thanks to this patch.

As explained in the first commit of the PR, having such endpoint of the agent (instead of having it on the consul server) allows to have a very good resiliency. In a large cluster, loosing consul leadership during incident is nearly inevitable, you don't want your loadbalancing stack to die whne this happens.

pearkes · 2018-05-04T16:21:09Z

Thanks for all of the context @kamaradclimber and @pierrecdn -- we agree this seems like a good idea and we'll review in detail soon.

pierresouchay · 2018-06-05T22:04:58Z

@pearkes Any news or suggestions regarding this PR ?

Kind regards

mkeeler

@pierresouchay First off, sorry for the delay on this review. I have been heads down on an upcoming ACL overhaul.

In addition to the 3 minor comments I left, I would still like to see the API package updated to utilize the new endpoint in api/agent.go.

mkeeler · 2018-10-05T17:47:27Z

agent/agent_endpoint.go

+				fmt.Fprint(resp, status)
+				return nil, nil
+			}
+			resp.Header().Add("Content-Type", "application/json")


No need for this as its done in http.go

No, this is actually needed: since we return non HTTP 200 Error codes, If we don't do that, the Mime Type won't be properly sent (because HTTP Status Code has already been sent)

I did not find a way to both specify the mime-type, having errors properly specified (ie: having correct JSON format) and having the proper error code.

What do you suggest?

mkeeler · 2018-10-05T17:47:36Z

agent/agent_endpoint.go

+		fmt.Fprintf(resp, "ServiceId %s not found", serviceID)
+		return nil, nil
+	}
+	resp.Header().Add("Content-Type", "application/json")


mkeeler · 2018-10-05T17:48:11Z

agent/agent_endpoint.go

+		fmt.Fprint(resp, status)
+		return nil, nil
+	}
+	resp.Header().Add("Content-Type", "application/json")


Same reason. At the time of this patch as least, there were no way I could send any JSON data with non HTTP 200

pierresouchay · 2018-10-05T20:02:13Z

@mkeeler Sorry, I did not see your previous old comment regarding api/agent, trying to add this ASAP

pierresouchay · 2018-10-15T14:03:56Z

@mkeeler Are you happy with this latest patch?

banks · 2018-11-09T12:02:03Z

Thanks for pursuing this and rebasing to make it mergeable again, we are still interested!

Matt is away for a while on leave and we've juggling some other release work but it's still on the radar!

banks

Couple notes on bad merge and doc formatting. Will discuss as mentioned but as I saw this I thought I'd mention it now so it doesn't add extra time after!

banks · 2018-11-09T12:03:24Z

website/source/api/agent/service.html.md

+```
+=======
+
+>>>>>>> origin


Looks like bad merge junk

banks · 2018-11-09T12:05:34Z

website/source/api/agent/service.html.md

+`Accept` starting with `text/plain`.
+
+Those endpoints return the aggregated values of all healthchecks for the
+service and will return the corresponding HTTP codes:


nit: "for the service instance(s)"

banks · 2018-11-09T12:06:27Z

website/source/api/agent/service.html.md

+
+| Result | Meaning                                                |
+| ------ | ------------------------------------------------------ |
+| `200`  | All healthchecks of this service are passing           |


nit: suggest we drop "of this service" or change it to "of every matching service instance" which seems unnecessary.

banks · 2018-11-09T12:07:20Z

website/source/api/agent/service.html.md

+  the aggregated status of given service
+* create aliases for a given service (thus, the healthcheck of alias uses
+  http://localhost:8500/v1/agent/service/id/aliased_service_id healthcheck)
+


These docs and examples are really great thanks!

Can we split the endpoints to make the consistent with all the other endpoint docs though please?

For example if we have other endpoints like /catalog/service/:name and /catalog/connect/:name that are almost identical except for one semantic detail we just document one and then make a stub with links in the other etc: https://www.consul.io/api/catalog.html#list-nodes-for-connect-capable-service

I think that patter would work fine here although the response format is subtly different so maybe only the description park would be shared/linked between.

Once, split, can we follow the same format as all other endpoints. i.e.:

## Title Description of purpose [Extended notes] | Method | Path | Produces | | ------ | ---------------------------- | -------------------------- | | `GET` | `/path` | `application/json` | The table below shows this endpoint's support for [blocking queries](/api/index.html#blocking-queries), [consistency modes](/api/index.html#consistency-modes), [agent caching](/api/index.html#agent-caching), and [required ACLs](/api/index.html#acls). | Blocking Queries | Consistency Modes | Agent Caching | ACL Required | | ---------------- | ----------------- | ------------- | -------------- | | `NO` | `none` | `none` | `service:read` | ### Sample Request ```text $ curl \ http://127.0.0.1:8500/v1/agent/services

Sample Response

{ "redis": { "ID": "redis", "Service": "redis", "Tags": [], "Meta": { "redis_version": "4.0" }, "Port": 8000, "Address": "", "EnableTagOverride": false, "Weights": { "Passing": 10, "Warning": 1 } } }

I suggest the format variants you make the table like we did for metrics (in this same file!):

Thanks!

ShimmerGlass · 2018-11-09T15:38:08Z

@banks I've updated the docs and split the endpoints

pierresouchay · 2018-11-09T15:46:12Z

@Aestek Thank you

This endpoint aggregate all checks related to <service id> on the agent and return an appropriate http code + the string describing the worst check. This allows to cleanly expose service status to other component, hiding complexity of multiple checks. This is especially useful to use consul to feed a loadbalancer which would deleguate healthchecking to consul agent. Exposing this endpoint on the agent is necessary to avoid a hit on consul servers and avoid decreasing resiliency (this endpoint will work even if there is no consul leader in the cluster). Fix hashicorp#2488, relates to hashicorp#802 Change-Id: Ib340c62bbbba46fd4256ed31474d8ffb1762d4df Signed-off-by: Grégoire Seux <g.seux@criteo.com>

In order to make it work, had to perform changes in http to return a payload while not returning HTTP 200

ShimmerGlass · 2018-11-09T16:08:58Z

(rewrote git history so its simpler to rebase on our side)

pierresouchay · 2018-11-22T18:16:34Z

@Peakles Possible to have a look, there were a kind of Consensus around this PR, and we spent quite some time rebasing it constantly for more than 1 year.

Furthermore, it does not seems that risky since it does not modify any existing endpoint

banks · 2018-11-28T11:50:56Z

Thanks for patience here folks. We do appreciate the effort, just as always a balancing act of spending time thinking through all the future implications and docs etc. and other work.

I'm going to tentatively tag this for 1.4.1. so it's on our list. That's not a definite promise it will get in if we have to release early etc. but hopefully it just needs us to recall the context and give a final pass.

mkeeler · 2019-01-04T16:17:15Z

@pierresouchay Now that I am back I am going to give this another look today. All your comments to my previous batch of requests sound good but its been a while so I am going to look it all over again.

pierresouchay · 2019-01-04T16:24:01Z

@mkeeler thank you so much!

mkeeler

Everything looks good. There is just the one question I have regarding usage of the CodeWithPayloadError struct.

Once that is worked out I see no barriers to getting this merged.

mkeeler · 2019-01-04T16:32:41Z

api/api.go

@@ -775,7 +787,9 @@ func (c *Client) query(endpoint string, out interface{}, q *QueryOptions) (*Quer
 	r.setQueryOptions(q)
 	rtt, resp, err := requireOK(c.doRequest(r))
 	if err != nil {
-		return nil, err
+		if _, ok := err.(CodeWithPayloadError); !ok {


Maybe I am just missing it but how can the CodeWithPayloadError ever be returned by requireOk/doRequest.

Also if this is the only place CodeWithPayloadError is used here and its not actually needed then the struct should be moved into the main agent package and out of the api package.

@mkeeler Yes, you are right (many refactoring were performed, it needed to be in api package in previous interations). Fixed in next patch

… anymore

mkeeler

Looks good to me. I think it will still require someone from consul-docs to review this too.

kamaradclimber · 2019-01-07T15:38:34Z

🎆

kamaradclimber force-pushed the agent_health_service branch 2 times, most recently from 8c15944 to 44f2488 Compare October 7, 2017 17:28

pierrecdn reviewed Oct 10, 2017

View reviewed changes

kamaradclimber force-pushed the agent_health_service branch 2 times, most recently from 3469195 to 97809cc Compare November 19, 2017 12:01

kamaradclimber force-pushed the agent_health_service branch 2 times, most recently from 9de4b16 to 58e43ee Compare January 23, 2018 17:56

pierrecdn force-pushed the agent_health_service branch 3 times, most recently from e40056c to c567c78 Compare February 13, 2018 19:20

kamaradclimber changed the title ~~Implement /v1/agent/health/service/<service id> endpoint~~ Implement /v1/agent/health/service/<service name> endpoint Mar 5, 2018

pierrecdn approved these changes Mar 27, 2018

View reviewed changes

pierresouchay mentioned this pull request Apr 30, 2018

Returning HTTP 429 for Warning Status kadaan/consulate#1

Closed

mkeeler requested changes Oct 5, 2018

View reviewed changes

ShimmerGlass force-pushed the agent_health_service branch from 211c21c to dd6f8dc Compare October 15, 2018 14:06

ShimmerGlass force-pushed the agent_health_service branch from 5595e69 to 5c7fd01 Compare November 9, 2018 10:11

banks added this to the Upcoming milestone Nov 9, 2018

banks requested changes Nov 9, 2018

View reviewed changes

pierresouchay and others added 4 commits November 9, 2018 17:02

Fixed unstable unit tests

3a99d79

Added api.agent for new endpoints

f60468a

In order to make it work, had to perform changes in http to return a payload while not returning HTTP 200

Add documentation for new /v1/agent/heath endpoints

c2b7743

ShimmerGlass force-pushed the agent_health_service branch from 74f2d93 to c2b7743 Compare November 9, 2018 16:07

banks modified the milestones: Upcoming, 1.4.1 Nov 28, 2018

mkeeler reviewed Jan 4, 2019

View reviewed changes

Moved CodeWithPayloadError to agent package since not required in api…

123f3b1

… anymore

kamaradclimber requested a review from a team as a code owner January 7, 2019 09:14

mkeeler approved these changes Jan 7, 2019

View reviewed changes

banks approved these changes Jan 7, 2019

View reviewed changes

mkeeler merged commit 4f62a3b into hashicorp:master Jan 7, 2019

pierresouchay mentioned this pull request May 20, 2020

BUGFIX #7374 When a service does not exists in an alias, consider it failing #7384

Merged

Implement /v1/agent/health/service/<service name> endpoint #3551

Implement /v1/agent/health/service/<service name> endpoint #3551

Conversation

kamaradclimber commented Oct 7, 2017

kamaradclimber commented Oct 7, 2017

pierrecdn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamaradclimber commented Oct 12, 2017

kamaradclimber commented Oct 29, 2017

kamaradclimber commented Nov 19, 2017

kamaradclimber commented Jan 23, 2018

kamaradclimber commented Feb 11, 2018

pierrecdn commented Feb 13, 2018

pierresouchay commented Mar 27, 2018

kamaradclimber commented Apr 14, 2018

pearkes commented Apr 27, 2018

pierresouchay commented Apr 27, 2018

kadaan commented May 1, 2018 • edited Loading

kamaradclimber commented May 1, 2018

kadaan commented May 1, 2018 • edited Loading

kamaradclimber commented May 1, 2018

pearkes commented May 4, 2018

pierresouchay commented Jun 5, 2018

mkeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierresouchay commented Oct 5, 2018

pierresouchay commented Oct 15, 2018

banks commented Nov 9, 2018

banks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sample Response

ShimmerGlass commented Nov 9, 2018

pierresouchay commented Nov 9, 2018

ShimmerGlass commented Nov 9, 2018

pierresouchay commented Nov 22, 2018

banks commented Nov 28, 2018

mkeeler commented Jan 4, 2019 • edited Loading

pierresouchay commented Jan 4, 2019

mkeeler left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeler left a comment

Choose a reason for hiding this comment

kamaradclimber commented Jan 7, 2019

kadaan commented May 1, 2018 •

edited

Loading

kadaan commented May 1, 2018 •

edited

Loading

mkeeler commented Jan 4, 2019 •

edited

Loading