search

RedpointArchive / phabricator

A Docker image that runs Phabricator, an open source software engineering tool
https://hub.docker.com/r/redpointgames/phabricator/
307 stars 98 forks source link

Don't restart run-ssh.sh when it cleanly exits. #67

Closed gomlgs closed 7 years ago

hach-que commented 7 years ago

If run-ssh.sh exits, it means the SSH server has stopped. It absolutely should restart the script to ensure SSHD starts again.

gomlgs commented 7 years ago

Right, I misread the loop at the bottom which waits for sshd to exit. My specific issue was that with keys not configured, the script would exit at the top (with code 0), but still be restarted.

I've changed the script to exit with code 1 if sshd dies.

gomlgs commented 7 years ago

June, would you like to take another look at this? Happy to discuss more.

hach-que commented 7 years ago

Why is startretries now set to 1? This means it will only restart once after failure which isn't what we want (we want it to try restarting forever until things work).

gomlgs commented 7 years ago

Done.

hach-que commented 7 years ago

In what cases will the script exit with anything other than exit code 0? I'm still not understanding the purpose of the change.

If the script always exits with exit code 1, and we always restart it on exit code 1, how is that different to what it does currently, where it always exits with exit code 0, and we always restart it regardless of the exit code? What behaviour actually changes?

gomlgs commented 7 years ago

The purpose of the change is to do the right thing irrespective of whether PHABRICATOR_HOST_KEYS_PATH is set or not.

At the top of the script (line 10), we currently exit with code 0 if the variable is not set. Having supervisord restart the script in this case is the wrong thing to do.

Near the bottom of the script (line 42), we exit with code 1 - In this case we do want the script to be run again. Similarly, the exit at the bottom (line 51) indicates that sshd quit (for some arbitrary reason), and again we do want the script to be run again.

hach-que commented 7 years ago

Oh, right! Sorry, GitHub wasn't showing me the whole file in the diff and I was getting confused. This looks good now.