Add an option to register services and their checks idempotently #4905

ShimmerGlass · 2018-11-06T14:13:36Z

Fixes point #1 of: #4903

When registering several times a service on a local Agent using
/v1/agent/service/register, if checks are removed, those checks are not
removed from the service.
This change adds the option to be idempotent when registering services
and makes sure only specified checks are registered for the service when specifying &idempotent=true

pierresouchay

LGTM, but a few changes:

handle properly '?idempotent' query param
add documentation

pierresouchay · 2018-11-06T15:01:06Z

agent/agent_endpoint.go

@@ -890,8 +890,15 @@ func (s *HTTPServer) AgentRegisterService(resp http.ResponseWriter, req *http.Re
 	}

 	// Add the service.
-	if err := s.agent.AddService(ns, chkTypes, true, token, ConfigSourceRemote); err != nil {
-		return nil, err
+	idempotent, _ := strconv.ParseBool(req.URL.Query().Get("idempotent"))


For the sake of consistency with other Consul APIs, you might check for parameter being present as true (similar to what is done in '&passing' or '&stale' for instance) (but handle false as you do still)

/agent/service/register => idempotent=false
/agent/service/register?idempotent => idempotent=true
/agent/service/register?idempotent=true => idempotent=true
/agent/service/register?idempotent=false => idempotent=false

pierresouchay

LGTM

ShimmerGlass · 2019-06-20T13:57:59Z

@i0rek done

hanshasselberg

Thanks for your work - I left a couple of comments, nothing major though! 🙏

agent/agent.go

agent/agent_endpoint.go

hanshasselberg · 2019-07-02T18:49:53Z

@Aestek I left a couple of comments earlier. just making sure you saw that.

pierresouchay · 2019-07-10T11:18:59Z

@Aestek Can you fix the remaining issues?

ShimmerGlass · 2019-07-10T12:26:16Z

@i0rek done (:

freddygv

Thanks @Aestek, just have a few comments inline.

agent/agent_endpoint.go

agent/agent_endpoint_test.go

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

ShimmerGlass · 2019-07-11T08:37:04Z

thx @freddygv, fixed except the last point.

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

ShimmerGlass · 2019-07-12T13:32:29Z

@freddygv / @banks Done !

freddygv

@Aestek I'm seeing a timeout in the following test when running it locally and in CI:
TestAgent_RegisterService_ReRegister_ReplaceExistingChecks

Can you please look into it? I ran it locally with:
go test -v -run TestAgent_RegisterService_ReRegister_ReplaceExistingChecks ./agent

Also, I found some outdated references to remove-old-checks, so I made some suggestions inline.

agent/agent_endpoint_test.go

agent/agent_endpoint.go

freddygv · 2019-08-13T17:57:42Z

Hi @pierresouchay and @Aestek, can you please take a look at the test mentioned above.

We can get this merged once that's fixed.

ShimmerGlass · 2019-08-14T15:39:00Z

@freddygv Done ! The test was indeed hanging, a rebase miskate. This is fixed now

freddygv

Thanks!

freddygv · 2019-08-26T17:41:59Z

@Aestek we had to stop merging into master a couple of weeks ago until the final v1.6.0 was released. Now there are a couple merge conflicts in this PR. Can you please resolve them? We would like to include this change in v1.6.1, which is coming very soon.

When registering several times a service on a local Agent using `/v1/agent/service/register`, if checks are removed, those checks are not removed from the service. This change adds the option to be idempotent when registering services and makes sure only specified checks are registered for the service.

ShimmerGlass · 2019-09-02T14:06:35Z

@freddygv done

ShimmerGlass · 2019-09-03T08:29:08Z

Thx !

…th central config enabled Also: * Finished threading replaceExistingChecks setting (from GH-4905) through service manager. * Respected the original configSource value that was used to register a service or a check when restoring persisted data. * Run several existing tests with and without central config enabled (not exhaustive yet). * Switch to ioutil.ReadFile for all types of agent persistence. Fixes #6323

jameshartig · 2019-09-13T21:59:16Z

It looks like the docs say ?replace-existing-checks=1 but the code has:

query.Get("replace-existing-checks") == "" || query.Get("replace-existing-checks") == "true"

Should the docs be updated to say that the value should be =true or should =1 be allowed?

Also, how might this be added to the api go client? It looks like Agent.ServiceRegister does:

r := a.c.newRequest("PUT", "/v1/agent/service/register")

Can this be an option that's passed in AgentServiceRegistration or does it need to be a query parameter?

banks · 2019-09-17T12:56:16Z

@fastest963 thanks, well spotted it does look like we should at least update the docs and add this to the api package. I'll create a new issue so we can track it and not loose this update in the sea of stuff.

…th central config enabled Also: * Finished threading replaceExistingChecks setting (from GH-4905) through service manager. * Respected the original configSource value that was used to register a service or a check when restoring persisted data. * Run several existing tests with and without central config enabled (not exhaustive yet). * Switch to ioutil.ReadFile for all types of agent persistence.

…th central config enabled (#6472) Also: * Finished threading replaceExistingChecks setting (from GH-4905) through service manager. * Respected the original configSource value that was used to register a service or a check when restoring persisted data. * Run several existing tests with and without central config enabled (not exhaustive yet). * Switch to ioutil.ReadFile for all types of agent persistence.

pierresouchay suggested changes Nov 6, 2018

View reviewed changes

ShimmerGlass force-pushed the service-idempotent-registration branch from e2f870e to 64d7748 Compare November 6, 2018 17:22

pierresouchay reviewed Nov 6, 2018

View reviewed changes

pierresouchay approved these changes Nov 6, 2018

View reviewed changes

pierresouchay mentioned this pull request Nov 13, 2018

Prevent status flap when re-registering a check #4904

Merged

hanshasselberg mentioned this pull request Jun 20, 2019

Service registration idempotence regarding HealthChecks #4903

Closed

ShimmerGlass force-pushed the service-idempotent-registration branch from 64d7748 to 985e731 Compare June 20, 2019 13:57

hanshasselberg requested changes Jun 24, 2019

View reviewed changes

freddygv self-assigned this Jul 3, 2019

ShimmerGlass force-pushed the service-idempotent-registration branch from 985e731 to f3b2db9 Compare July 10, 2019 12:25

freddygv suggested changes Jul 10, 2019

View reviewed changes

agent/agent_endpoint.go Outdated Show resolved Hide resolved

agent/agent_endpoint_test.go Outdated Show resolved Hide resolved

website/source/api/agent/service.html.md Outdated Show resolved Hide resolved

website/source/api/agent/service.html.md Show resolved Hide resolved

ShimmerGlass force-pushed the service-idempotent-registration branch from f3b2db9 to 700c4e8 Compare July 11, 2019 08:33

banks approved these changes Jul 12, 2019

View reviewed changes

website/source/api/agent/service.html.md Show resolved Hide resolved

ShimmerGlass force-pushed the service-idempotent-registration branch from 700c4e8 to b2a103b Compare July 12, 2019 13:32

freddygv suggested changes Jul 16, 2019

View reviewed changes

agent/agent_endpoint_test.go Outdated Show resolved Hide resolved

agent/agent_endpoint_test.go Outdated Show resolved Hide resolved

agent/agent_endpoint.go Outdated Show resolved Hide resolved

ShimmerGlass force-pushed the service-idempotent-registration branch from b2a103b to dd138aa Compare August 14, 2019 15:38

freddygv approved these changes Aug 14, 2019

View reviewed changes

hanshasselberg approved these changes Aug 27, 2019

View reviewed changes

hanshasselberg added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Aug 27, 2019

ShimmerGlass force-pushed the service-idempotent-registration branch from dd138aa to 080008d Compare September 2, 2019 14:06

freddygv merged commit 7c7b7f2 into hashicorp:master Sep 2, 2019

ShimmerGlass deleted the service-idempotent-registration branch September 3, 2019 08:29

rboyer mentioned this pull request Sep 10, 2019

agent: tolerate more failure scenarios during service registration with central config enabled #6472

Merged

banks mentioned this pull request Sep 17, 2019

api: replace-existing-checks param needs api package support #6496

Closed

hetianyi mentioned this pull request Feb 28, 2020

Re-register service in consul if the service get's de-registered spring-cloud/spring-cloud-consul#197

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add an option to register services and their checks idempotently #4905

Add an option to register services and their checks idempotently #4905

ShimmerGlass commented Nov 6, 2018

pierresouchay left a comment

pierresouchay Nov 6, 2018

pierresouchay left a comment

ShimmerGlass commented Jun 20, 2019

hanshasselberg left a comment

hanshasselberg commented Jul 2, 2019

pierresouchay commented Jul 10, 2019

ShimmerGlass commented Jul 10, 2019

freddygv left a comment

ShimmerGlass commented Jul 11, 2019

ShimmerGlass commented Jul 12, 2019

freddygv left a comment

freddygv commented Aug 13, 2019

ShimmerGlass commented Aug 14, 2019

freddygv left a comment

freddygv commented Aug 26, 2019

ShimmerGlass commented Sep 2, 2019

ShimmerGlass commented Sep 3, 2019

jameshartig commented Sep 13, 2019 •

edited

Loading

banks commented Sep 17, 2019

Add an option to register services and their checks idempotently #4905

Add an option to register services and their checks idempotently #4905

Conversation

ShimmerGlass commented Nov 6, 2018

pierresouchay left a comment

Choose a reason for hiding this comment

pierresouchay Nov 6, 2018

Choose a reason for hiding this comment

pierresouchay left a comment

Choose a reason for hiding this comment

ShimmerGlass commented Jun 20, 2019

hanshasselberg left a comment

Choose a reason for hiding this comment

hanshasselberg commented Jul 2, 2019

pierresouchay commented Jul 10, 2019

ShimmerGlass commented Jul 10, 2019

freddygv left a comment

Choose a reason for hiding this comment

ShimmerGlass commented Jul 11, 2019

ShimmerGlass commented Jul 12, 2019

freddygv left a comment

Choose a reason for hiding this comment

freddygv commented Aug 13, 2019

ShimmerGlass commented Aug 14, 2019

freddygv left a comment

Choose a reason for hiding this comment

freddygv commented Aug 26, 2019

ShimmerGlass commented Sep 2, 2019

ShimmerGlass commented Sep 3, 2019

jameshartig commented Sep 13, 2019 • edited Loading

banks commented Sep 17, 2019

jameshartig commented Sep 13, 2019 •

edited

Loading