Consul Catalog Integration Fixes #5536
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
endocrimes
force-pushed
the
dani/consul
branch
3 times, most recently
from
54f0e0e
to
761314d
Compare
The current implementation of Service Registration uses a hash of the nomad-internal state of a service to register it with Consul, this means that any update to the service invalidates this name and we then deregister, and recreate the service in Consul. While this behaviour slightly simplifies reasoning about service registration, this becomes problematic when we add consul health checks to a service. When the service is re-registered, so are the checks, which default to failing for at least one check period. This commit migrates us to using a stable identifier based on the allocation, task, and service identifiers, and uses the difference between the remote and local state to decide when to push updates. It uses the existing hashing mechanic to decide when UpdateTask should regenerate service registrations for providing to Sync, but this should be removable as part of a future refactor. It additionally introduces the _nomad-check- prefix for check definitions, to allow for future allowing of consul features like maintenance mode.
This commit causes sync to skip deregistering checks that are not managed by nomad, such as service maintenance mode checks. This is handled in the same way as service registrations - by doing a Nomad specific prefix match.
endocrimes
force-pushed
the
dani/consul
branch
2 times, most recently
from
429d157
to
14c231a
Compare
|
Only travis job that's failing on push is the website and that seems unrelated. |
nickethier
approved these changes
May 8, 2019
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #4566
fixes #4537
The current implementation of Service Registration uses a hash of the nomad-internal state of a service to register it with Consul, this means that any update to the service invalidates this name and we then de-register, and recreate the service in Consul.
While this behavior slightly simplifies reasoning about service registration, this becomes problematic when we add consul health checks to a service. When the service is re-registered, so are the checks, which default to failing for at least one check period. This causes issues when a canary is promoted, as the old, stable allocs are immediately stopped, and the new ones updated, which causes a consul registration cycle and takes your service offline for one health check period.
This pull request introduces
v3of the Nomad-Consul integration style that moves back to having a stable identifier for a service, and registers updates based on changes to the internal struct state and remote state.Notes
There is currently a UX issue in Consul that means that when you update a Service's tags, its Checks will still return the ones they were registered against, however this does not effect the functionality of Consul or Nomad (Updating a Service definition via the API does not update Checks consul#5631).
There is an outstanding Nomad bug where the consul reconciler races against task restoration on a host, which means when restarting or updating a client, services will be removed from consul and re-registered, causing a 1-health-check-period status flap. This PR does not attempt to solve that bug as it will require a larger refactor of the client startup flow to be reliable, thus this PR does not attempt to handle older registrations outside of removing them as expected.