Fix bug with watch handlers not being closed upon consul reload #3186
Conversation
This patch fixes watch registration through the config file and a broken log line when the watch registration fails. Fixes #3177
…nit test. Fixes issue#3185
|
@slackpad I based this off @magiconair 's fix in #3181, since watch handlers need to be registered first for this bug to show up. You'll have to merge his fix first before this one, or you can get both in if you merge this PR. |
preetapan
changed the title
There was a problem hiding this comment.
One thing to fix and one general comment, otherwise LGTM. I'll merge this to include @magiconair's change.
| @@ -118,3 +118,7 @@ func (p *Plan) shouldStop() bool { | |||
| return false | |||
| } | |||
| } | |||
|
|
|||
| func (p *Plan) IsStopped() bool { | |||
| return p.stop | |||
There was a problem hiding this comment.
This looks like it needs to take the stopLock
| if err != nil { | ||
| errs = multierror.Append(errs, fmt.Errorf("Failed to determine HTTP address: %v", err)) | ||
| } | ||
|
|
||
| // Deregister the old watches | ||
| for _, wp := range a.config.WatchPlans { | ||
| for _, wp := range prevConfig.WatchPlans { |
There was a problem hiding this comment.
I think we might want to eventually keep these on the agent itself and call a.unloadWatches() here, so we don't need to pass the old config, but this is a more minimal change so probably better for Tuesday.
|
I think if we move the |
|
@magiconair yeah that works for me - it's really just keeping a slice on the agent. |
Prior to 0.8.4, when "consul reload" was executed, any existing watch handlers were stopped via the Stop method. When things moved around, this broke in a way that only the first "consul reload" would do the right thing. Subsequent "consul reloads" would never find the new watch handlers because the reference to agent.config is always the first one.
My fix is the smallest possible change that works, because I want to get this in for the Tuesday release. I am still in favor of separating watch behavior from watch configuration so that this sort of stuff doesn't have to hang off agent.Config.