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

rm explicitly after retrieving logs instead of with --rm #42

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

ScottFreeCode
Copy link
Contributor

@ScottFreeCode ScottFreeCode commented Feb 4, 2022

Fixes #36 by allowing a crashed container to stick around till the logs are captured and the container is destroyed in the stopping step.

Has two disadvantages:

  • There is an increased risk of orphaning the container if something catastrophic happens to Touchstone such that it can't run the container stop subroutine, since Docker no longer autoremoves it. EDIT: The increased risk is small, however. Prior to this change, if the container starts successfully and Touchstone dies and cannot signal it to stop, it will not stop either. This risk is only increased by no autoremove for crashed containers being left around if Touchstone also crashes, or if Touchstone crashes between stopping the container and (literally the next line) removing it. So, an existing risk we can't do anything more about, now also applies to a couple unlikely edge cases. Doesn't seem like a big deal the more I think about it.
  • The logs are not available until the service stops. This is the status quo currently, but Log services as they run (start capturing output immediately) #43 improves on this by beginning to capture the logs to the file immediately.

@ScottFreeCode
Copy link
Contributor Author

I tested the log capturing with a failure to become healthy on both PRs by adding the availability_endpoint parameter and adding an exception to the Python app/main.py and seeing that I got the logs when I ran Touchstone and it failed trying to wait for the service to become healthy.

@shanejansen shanejansen changed the base branch from master to 1.4.1 February 7, 2022 02:44
@shanejansen shanejansen merged commit 9646e0e into shanejansen:1.4.1 Feb 7, 2022
@ScottFreeCode ScottFreeCode deleted the fix-36/simple-rm branch February 7, 2022 11:19
@ScottFreeCode
Copy link
Contributor Author

Initially I cautioned:

There is an increased risk of orphaning the container if something catastrophic happens to Touchstone such that it can't run the container stop subroutine, since Docker no longer autoremoves it.

However…

The increased risk is small. Prior to this change, if the container starts successfully and Touchstone dies and cannot signal it to stop, it will not stop either. This risk is only increased by no autoremove for crashed containers being left around if Touchstone also crashes, or if Touchstone crashes between stopping the container and (literally the next line) removing it. So, an existing risk we can't do anything more about, now also applies to a couple unlikely edge cases. Doesn't seem like a big deal the more I think about it.

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.

Service logs do not cover immediate startup failures
2 participants