change ordering to allow prestart script to SIGHUP containerpilot

[wleese]

Due to ordering it is currently not possible to have the preStart script dynamically change containerpilot configuration.

Maybe this change has other consequences I cannot for see, but tests work and a kill -SIGHUP 1 from the preStart script now hits reloadConfig rather than causing containerpilot to exit.

[tgross]

Maybe this change has other consequences I cannot for see

Well in addition to what you're intending to fix, it'll fix a bug that we didn't realize we had -- if the preStart handler created zombies then they wouldn't be reaped because we hadn't set up the SIGCHLD signal handler yet!

The only potential source of trouble is what happens if we receive a SIGUSR1 or SIGTERM during the preStart. In this case we'll end up trying to terminate any polling functions, but they will be nil at that point in time. I think this is going to be ok but can we add a test that exercises that path to be sure?

[wleese]

In this case we'll end up trying to terminate any polling functions, but they will be nil at that point in time.

In the test I'm doing a SIGUSR1 from the prestart script, then using test-probe I send a SIGHUP and see if containerpilot still registers the test service. Lots of assumptions here regarding your comment and how the tests work.

[tgross]

This LGTM but a merge conflict popped up once #138 was merged. If we can get that resolved I think this is good-to-go.

[wleese]

Fixed the conflict but haven't tested the new stuff from #138 just yet. Will get to that tomorrow.

[wleese]

With #138 and attempting a HUP during onstart I get:
2016/05/03 03:02:40 Error initializing config: -config flag is required

Added a new test to check this path.

[wleese]

Interesting how a fix for the hup reload() now triggers: FAIL: test_sighup_deadlock

[misterbisson]

@wleese have you had any further insight into this one? Anything we can help with?

[wleese]

Priorities have shifted a bit, leaving no time for this PR the coming weeks.

On Thursday, 12 May 2016, Casey Bisson notifications@github.com wrote:

@wleese https://github.com/wleese have you had any further insight into
this one? Anything we can help with?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#140 (comment)

[tgross]

Closing in lieu of #165