Allow to kill new container that failed CHECKS#1160
Allow to kill new container that failed CHECKS#1160LTe wants to merge 1 commit intodokku:masterfrom LTe:kill-new-fix
Conversation
When the check does not pass there is no reason to close the container in the correct way. We can do this by using SIGKILL signal.
|
The checks failing only indicate that the service is not behaving as expected, it may still have things running that need to clean up before exiting. |
|
@3onyc So the current implementation is correct? |
|
@LTe In my opinion, yes. |
|
@3onyc This PR restores the previous functionality due to the assumption that the if the new container fails checks it's better to fail quickly as we know something went wrong. Can you provide more information as to why you think the current |
|
@3onyc bump |
|
Stop allows a container to properly clean up, CHECKS failing only indicates that something is wrong, and does not indicate the app failed to start completely. I can't currently think of a specific example, but giving containers a chance to clean up doesn't sound like a bad thing. |
|
I think that's fair. If we have background containers, they may work while the web version won't. Therefore, I'm 👎 on this PR. |
|
@LTe thanks for being patient with us :) |
|
+1 on the decision to keep the stop behavior. A rails example would be a job processor, such as sideqik. Upon crash, the OSS version of sidekiq can lose jobs that have been loaded into memory. So in a CHECKS failing container, the job processor could have started up and begun loading jobs, only to be forcibly terminated (crash) thus losing those jobs |
When the check does not pass there is no reason to close the container
in the correct way. We can do this by using SIGKILL signal.
Related: #1151