Skip to content
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

Support configuration reload #48

Merged
merged 5 commits into from
Dec 16, 2015
Merged

Conversation

tgross
Copy link
Contributor

@tgross tgross commented Dec 14, 2015

@misterbisson @justenwalker this PR supports reloading the configuration (ref #9). This is not a completed implementation, but I figure getting in front of you guys would be a good idea. There will also almost certainly be merge conflicts with #46 so I'll be sure to clean those up first.

We accept SIGHUP to reload the configuration. This stops all polling and forces all advertised services to be deregistered (and then reloaded) in the discovery service. We have to do this because otherwise we can't actually update service configurations in Consul.

Right now the fairly large hole in the implementation that I'm planning on taking on starting tomorrow morning is that we don't properly cover the preStop and postStop functions. We pass their config struct in main, which is outside the loop for getting the config. It's just a bit of refactoring so we can stick the config in at the appropriate moment. Done

Accept SIGHUP to reload the configuration. This stops all polling and
forces all advertised services to be deregistered (and then reloaded)
in the discovery service.
@misterbisson
Copy link
Contributor

@tgross looks solid to me

Passing around config as a context to functions would be the ideomatic way.
But we need to support configuration reload from signals and have that reload
effect function calls in the main goroutine. Wherever possible we should be
accessing via `getConfig` at the "top" of a goroutine and then use the config
as context for a function after that.
config.QuitChannels = quit

// gracefully clean up so that our docker logs aren't cluttered after an exit 0
// TODO: do we really need this?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this and it turns out the answer is no, so I cut it.

@tgross
Copy link
Contributor Author

tgross commented Dec 15, 2015

@misterbisson and @justenwalker I've got this wrapped up. I've added a global (protected by a mutex) that stores the configuration so that top-level functions can get the new config at the time they're called, rather than at the time their caller is called. I'm not super-wild about how whether we're calling getConfig or just taking a pointer to the config as context are dependent on where they're being called just in terms of consistency. But keeping the global state out of almost all the functions does make this a lot more testable (and prevents us from having to take the RLock constantly)... I'm definitely open to suggestion as far as improvements.

My integration test (which I definitely do want to get added to our automation at some point as per #42):

$ make run
...
$ docker exec -it example_nginx_1 /bin/bash
root@17f38ba6e2c6:/# sed -i 's/80/8080/' /opt/containerbuddy/nginx.json
root@17f38ba6e2c6:/# kill -HUP 1
root@17f38ba6e2c6:/# exit

And then it's easy to verify by watching what Consul is doing.

@misterbisson
Copy link
Contributor

🏡 🚶

tgross added a commit that referenced this pull request Dec 16, 2015
Support configuration reload
@tgross tgross merged commit f96f855 into TritonDataCenter:master Dec 16, 2015
@tgross tgross deleted the config_reload branch April 4, 2017 15:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants