fingerprint: add support for fingerprinting multiple Consul clusters #18392
Conversation
| @@ -88,46 +109,61 @@ func (f *ConsulFingerprint) Periodic() (bool, time.Duration) { | |||
| return true, 15 * time.Second | |||
There was a problem hiding this comment.
Note for reviewers: unlike #18253, the Consul fingerprint doesn't back off once we've got a successful fingerprint. We probably should do that here too, but I don't want to change the existing behavior in this PR.
There was a problem hiding this comment.
Is there a reason for this? I mean if we fingerprint successfully, we should back off.
There was a problem hiding this comment.
Is there a reason for this?
If I recall correctly, the original reasoning was that we would change the client's fingerprint if Consul became unavailable. We no longer do that as of 1.4.0 in order to avoid a storm of Node.Register RPCs and tons of load on the Nomad servers whenever someone's Consul cluster is flapping. This had been implicated in a few major customer incidents!
I mean if we fingerprint successfully, we should back off.
It should be safe for us to do so now. I just didn't want to include that in this PR because I want to change one thing at a time. I'll make a follow-up PR for that.
| const ( | ||
| vaultAvailable = "available" | ||
| vaultUnavailable = "unavailable" | ||
| ) |
There was a problem hiding this comment.
Note to reviewers: this was dead code after #18253 that I missed
tgross
requested review from
pkazmierczak,
lgfa29,
schmichael and
jrasell
and removed request for
schmichael
tgross
force-pushed
the
multiple-consul-cluster-fingerprint
branch
from
130eae1 to
1514151
Compare
| @@ -88,46 +109,61 @@ func (f *ConsulFingerprint) Periodic() (bool, time.Duration) { | |||
| return true, 15 * time.Second | |||
There was a problem hiding this comment.
Is there a reason for this? I mean if we fingerprint successfully, we should back off.
Add fingerprinting we'll need to accept multiple Consul clusters in upcoming Nomad Enterprise features. The fingerprinter will create a map of Consul clients by cluster name. In Nomad CE, all but the default cluster will be ignored and there will be no visible behavior change. Ref: hashicorp/team-nomad#404
tgross
force-pushed
the
multiple-consul-cluster-fingerprint
branch
from
1514151 to
33a2046
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. |
Add fingerprinting we'll need to accept multiple Consul clusters in upcoming Nomad Enterprise features. The fingerprinter will create a map of Consul clients by cluster name. In Nomad CE, all but the default cluster will be ignored and there will be no visible behavior change.
Ref: https://github.com/hashicorp/team-nomad/issues/404
Note: similar to #18253, I'll need to prep an ENT PR to go with this one