Skip to content
This repository has been archived by the owner on Nov 30, 2021. It is now read-only.

fix(confd): services check for valid config before reloading #1112

Merged
merged 1 commit into from
Jul 15, 2014

Conversation

mboersma
Copy link
Member

@mboersma mboersma commented Jun 4, 2014

Adds a check_cmd to controller, database, and registry that will prevent their processes from reloading if configuration files are invalid. This is already in place for deis-router, logger does not use confd templating, and builder does not have a process that needs reloading. (Cache runs Redis in the foreground, and Redis ignores SIGHUP completely, so it's problematic.)

TESTING: Build controller, database, and registry. Test that make uninstall and make run work and that stopping and starting individual components does not cause others to fail with invalid configuration.

@mboersma mboersma added the bug label Jun 4, 2014
@mboersma mboersma self-assigned this Jun 4, 2014
@mboersma mboersma added this to the 0.9.1 milestone Jun 4, 2014
@carmstrong
Copy link
Contributor

Save yourself some time and don't forget to vagrant rsync ;)

@mboersma
Copy link
Member Author

mboersma commented Jun 4, 2014

vagrant rsync-auto FTW--you taught me that one.

@carmstrong
Copy link
Contributor

vagrant rsync-auto FTW--you taught me that one.

Yup! But it chokes hard when you swap out VMs underneath it.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling 5fffbe2 on check-configs into 7f0d1cb on master.

@bacongobbler
Copy link
Member

LGTM!

><> fleetctl stop deis-database
><> curl http://deis.local.deisapp.com/accounts/login/
<h1>Server Error (500)</h1>
><> fleetctl status deis-controller
● deis-controller.service - deis-controller
   Loaded: loaded (/run/fleet/units/deis-controller.service; linked-runtime)
   Active: active (running) since Wed 2014-06-04 12:28:47 UTC; 11min ago
  Process: 8849 ExecStartPre=/bin/sh -c docker inspect deis-controller >/dev/null && docker rm -f deis-controller || true (code=exited, status=0/SUCCESS)
  Process: 8838 ExecStartPre=/bin/sh -c docker history deis/controller >/dev/null || docker pull deis/controller:latest (code=exited, status=0/SUCCESS)
 Main PID: 8862 (docker)
   CGroup: /system.slice/deis-controller.service
           └─8862 /usr/bin/docker run --name deis-controller -p 8000:8000 -e PUBLISH=8000 -e HOST=172.17.8.100 --volumes-from=deis-logger deis/controller

Jun 04 12:40:09 deis-1 docker[8862]: self.connect()
Jun 04 12:40:09 deis-1 docker[8862]: File "/usr/local/lib/python2.7/dist-packages/django/db/backends/__init__.py", line 115, in connect
Jun 04 12:40:09 deis-1 docker[8862]: self.connection = self.get_new_connection(conn_params)
Jun 04 12:40:09 deis-1 docker[8862]: File "/usr/local/lib/python2.7/dist-packages/django/db/backends/postgresql_psycopg2/base.py", line 114, in get_new_connection
Jun 04 12:40:09 deis-1 docker[8862]: return Database.connect(**conn_params)
Jun 04 12:40:09 deis-1 docker[8862]: File "/usr/local/lib/python2.7/dist-packages/psycopg2/__init__.py", line 164, in connect
Jun 04 12:40:09 deis-1 docker[8862]: conn = _connect(dsn, connection_factory=connection_factory, async=async)
Jun 04 12:40:09 deis-1 docker[8862]: OperationalError: could not connect to server: Connection refused
Jun 04 12:40:09 deis-1 docker[8862]: Is the server running on host "172.17.8.100" and accepting
Jun 04 12:40:09 deis-1 docker[8862]: TCP/IP connections on port 5432?

Working as intended :D

@gabrtv
Copy link
Member

gabrtv commented Jul 14, 2014

@mboersma let's get this passing in Jenkins. This is a good fix.

@carmstrong
Copy link
Contributor

Code LGTM.

mboersma added a commit that referenced this pull request Jul 15, 2014
fix(confd): services check for valid config before reloading
@mboersma mboersma merged commit bb725fc into master Jul 15, 2014
@mboersma mboersma deleted the check-configs branch July 15, 2014 17:10
@johanneswuerbach
Copy link
Contributor

Isn't the purpose of the check_cmd to fail before the config is written and thats why you get {{ .src }} as argument?
https://github.com/kelseyhightower/confd/blob/master/docs/template-resources.md

I think this line https://github.com/deis/deis/pull/1112/files#diff-a7f2e8cb6e90716acc47faaeba542a1aR6 (an others referencing the already templated file), will result in a template written once with "<no value>" (as check_cmd checks before writing, but the file is fine) and then never written again, as the check_cmd will always exit with an error.

@mboersma
Copy link
Member Author

@johanneswuerbach, yep, this one slipped by review and QA I think. Luckily my goof seems to mean that we're always returning 0 from check_cmd, which is better than the alternative. I will come up with a fix for this ASAP.

@carmstrong
Copy link
Contributor

👍 Glad you had eyes on this, @johanneswuerbach!

@mboersma
Copy link
Member Author

@johanneswuerbach, I haven't forgotten this. I'm making amends in https://github.com/deis/deis/tree/fix-check-cmd. I'll ping you for a review when I make it PR. Thanks again!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants