Fix old services not getting removed from consul on update #1668
Conversation
schmichael
force-pushed
the
b-fix-consul-updates
branch
from
55517ea
to
a63b2cc
Compare
| @@ -174,6 +174,8 @@ func NewSyncer(consulConfig *config.ConsulConfig, shutdownCh chan struct{}, logg | |||
| trackedChecks: make(map[consulCheckID]*consul.AgentCheckRegistration), | |||
| checkRunners: make(map[consulCheckID]*CheckRunner), | |||
| periodicCallbacks: make(map[string]types.PeriodicCallback), | |||
| // default noop implementation of addrFinder | |||
| addrFinder: func(string) (string, int) { return "", 0 }, | |||
There was a problem hiding this comment.
Not sure if this is a safe or wise thing to do.
schmichael
changed the title
Instead just remove all associated services on shutdown.
schmichael
changed the title
| } | ||
|
|
||
| found := 0 | ||
| for _, s := range cs.flattenedServices() { |
There was a problem hiding this comment.
Can we use reflect.DeepEquals here to check whether the agentServices are what we expect them to be? The assertion logic would be simpler.
There was a problem hiding this comment.
Hm, good call. Right now I'm just asserting the internal syncer state. I'll add comparisons of internal state against queryAgentServices too to make sure everything is consistent.
There was a problem hiding this comment.
Sadly queryAgent and fllatenedServices use different types, so I couldn't just DeepEquals.
Added the checks manually because they're important.
| @@ -355,7 +376,12 @@ func (c *Syncer) Shutdown() error { | |||
| } | |||
|
|
|||
| // De-register all the services from Consul | |||
| for serviceID := range c.trackedServices { | |||
| services, err := c.queryAgentServices() | |||
There was a problem hiding this comment.
I would change the comment above:
// De-register all services registered by this syncer from Consul
|
LGTM |
|
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 #1661
I'm not terribly happy with the thoroughness of my tests, so feel free to suggest improvements there.
Also includes a tiny unrelated change I made while meandering through the code base: