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

Prevent status flap when re-registering a check #4904

Merged

Conversation

ShimmerGlass
Copy link
Contributor

Fixes point #2 of: #4903

When registering a service each healthcheck status is saved and restored (https://github.com/hashicorp/consul/blob/master/agent/agent.go#L1914) to avoid unnecessary flaps in health state.
This change extends this feature to single check registration by moving this protection in AddCheck() so that both PUT /v1/agent/service/register and PUT /v1/agent/check/register behave in the same idempotent way.

Steps to reproduce

  1. Register a check :
curl -X PUT \
  http://127.0.0.1:8500/v1/agent/check/register \
  -H 'Content-Type: application/json' \
  -d '{
  "Name": "my_check",
  "ServiceID": "srv",
  "Interval": "10s",
  "Args": ["true"]
}'
  1. The check will initialize and change to passing
  2. Run the same request again
  3. The check status will quickly go from critical to passing (the delay for this transission is determined by https://github.com/hashicorp/consul/blob/master/agent/checks/check.go#L95)

When registering a service each healthcheck status is saved and restored
to avoid unnecessary flaps in health state.
This change extends this feature to single check registration, so that
both `PUT /v1/agent/service/register` and `PUT /v1/agent/check/register`
behave in the same indempotent way.
Copy link
Contributor

@pierresouchay pierresouchay left a comment

Choose a reason for hiding this comment

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

LGTM

@pierresouchay
Copy link
Contributor

@banks while #4905 is harder to merge, this one is simple enough and just improve the situation by not resetting the state and avoid unnecessary flap of services, would be a nice improvement IMHO

@banks banks added this to the Upcoming milestone Nov 13, 2018
@banks
Copy link
Member

banks commented Nov 13, 2018

Thanks @Aestek @pierresouchay, I've added it to the list to consider first glance seems good.

To make sure I understand, The code that is removed here already means that PUT /v1/agent/service/register behaves correctly right? It's only the PUT /v1/agent/check/register that exhibits the flap bug?

@ShimmerGlass
Copy link
Contributor Author

@banks yes, /v1/agent/service/register was already protected against flaps, this change extends the protection to /v1/agent/check/register

Copy link
Member

@mkeeler mkeeler left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@mkeeler mkeeler merged commit 8709213 into hashicorp:master Jan 7, 2019
@pierresouchay
Copy link
Contributor

@mkeeler thank you

@ShimmerGlass ShimmerGlass deleted the check-idempotent-registration branch January 8, 2019 09:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants