Hash fields used in task service IDs #3632
Conversation
schmichael
force-pushed
the
b-3620-consul-tags
branch
from
40c64ed to
0e0af1c
Compare
schmichael
force-pushed
the
f-3380-custom-ports
branch
2 times, most recently
from
4c41676 to
c68ed09
Compare
schmichael
force-pushed
the
b-3620-consul-tags
branch
from
0e0af1c to
0967f46
Compare
|
What format have they changed from and to? |
| @@ -1,6 +1,9 @@ | |||
| ## 0.7.1 (Unreleased) | |||
|
|
|||
| __BACKWARDS INCOMPATIBILITIES:__ | |||
| * client: The format of service IDs in Consul has changed. If you rely upon | |||
| Nomad's service IDs (*not* service names; those are stable), you will need | |||
| to update your code. [GH-3632] | |||
There was a problem hiding this comment.
Extra space after period. We should say that relying on the ID is not advisable as it is liable to change
CHANGELOG.md
Outdated
| @@ -57,6 +60,7 @@ BUG FIXES: | |||
| explicitly [GH-3520] | |||
| * cli: Fix passing Consul address via flags [GH-3504] | |||
| * cli: Fix panic when running `keyring` commands [GH-3509] | |||
| * client: Fix advertising services with complex tags [GH-3632] | |||
There was a problem hiding this comment.
We should be more specific than complex. It is tags that require URL escaping
| // Unknown Nomad managed service; kill | ||
| if err := c.client.ServiceDeregister(id); err != nil { | ||
| if isOldNomadService(id) { | ||
| // Don't hard-fail on old entries. See #3620 |
There was a problem hiding this comment.
And this is how cruft is born 😢
There was a problem hiding this comment.
Yeah, I think the optimal solution is soft-failing on all deregistration calls, logging only the first failure per-id, and then logging if it ever succeeds... I guess that would only take a map of IDs to track... should I add that and drop isOldNomadService entirely?
There was a problem hiding this comment.
Yeah I wasn't asking for an action on this one. I think it is just unfortunate we will have to maintain this behavior for a while!
| // Make sure Port and Address are stable since | ||
| // PortLabel and AddressMode aren't included in the | ||
| // service ID. | ||
| if locals.Port == remotes.Port && locals.Address == remotes.Address { |
There was a problem hiding this comment.
Nice that it has become simpler 👍
command/agent/consul/client.go
Outdated
| parts[4] = service.Name | ||
| copy(parts[5:], service.Tags) | ||
| return strings.Join(parts, "-") | ||
| h := sha1.New() |
There was a problem hiding this comment.
Hmm why not keep the hash as a method on Service and have return nomadTaskPrefix + service.Hash()
There was a problem hiding this comment.
+1 on this. No reason to maintain all the old fields in the hash, all we care about is that this creates a unique id for the service.
The format is intentionally not documented outside of code because it is not something users should have to be aware of or rely on. That being said the old and new formats are here in the code: |
Fixes #3620 Previously we concatenated tags into task service IDs. This could break deregistration of tag names that contained double //s like some Fabio tags. This change breaks service ID backward compatibility so on upgrade all users services and checks will be removed and re-added with new IDs. This change has the side effect of including all service fields in the ID's hash, so we no longer have to track PortLabel and AddressMode changes independently.
schmichael
force-pushed
the
b-3620-consul-tags
branch
from
0967f46 to
9bb2581
Compare
|
Addressed issues and am merging into the parent branch |
|
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. |
Fixes #3620
Previously we concatenated tags into task service IDs. This could break
deregistration of tag names that contained double //s like some Fabio
tags.
This change breaks service ID backward compatibility so on upgrade all
users services and checks will be removed and re-added with new IDs.
This change has the side effect of including all service fields in the
ID's hash, so we no longer have to track PortLabel and AddressMode
changes independently.