-
Notifications
You must be signed in to change notification settings - Fork 2k
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
consul: periodically reconcile services/checks #4170
Conversation
887291d
to
4c01d3f
Compare
command/agent/consul/client.go
Outdated
@@ -36,6 +36,9 @@ const ( | |||
// defaultMaxRetryInterval is the default max retry interval. | |||
defaultMaxRetryInterval = 30 * time.Second | |||
|
|||
// defaultPeriodicInterval is the default periodic sync interval. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a bit more to this comment. "The periodic sync reconciles state between the desired services and checks and the actual registered checks. This is done at an interval, rather than being purely edge triggered, to handle the case that the Consul agent's state may change underneath us".
command/agent/consul/client.go
Outdated
|
||
// Reset timer to periodic interval to periodically | ||
// reconile with Consul | ||
retryTimer.Stop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should use if !retryTimer.Stop() {
@@ -345,6 +345,15 @@ INIT: | |||
c.logger.Printf("[INFO] consul.sync: successfully updated services in Consul") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be a DEBUG?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, github's default hiding makes it appear noisier than it really is. It only logs when failures > 0
which means it logs as an indication that Consul has recovered.
In a steady state it will not log. I kind of like it because otherwise you would only see Consul syncing failures in the logs and not a clear success message.
Periodically sync services and checks from Nomad to Consul. This is mostly useful when testing with the Consul dev agent which does not persist state across restarts. However, this is a reasonable safety measure to prevent skew between Consul's state and Nomad's services+checks. Also modernized the test suite a bit.
4c01d3f
to
972e861
Compare
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. |
Periodically sync services and checks from Nomad to Consul. This is
mostly useful when testing with the Consul dev agent which does not
persist state across restarts. However, this is a reasonable safety
measure to prevent skew between Consul's state and Nomad's
services+checks.
Also modernized the test suite a bit.