add tini to docker files #5000
add tini to docker files #5000
Conversation
There was a problem hiding this comment.
Thanks for contributing to skypilot @Ajay-Satish-01 ! Left some comments
|
/quicktest-core |
There was a problem hiding this comment.
LGTM, thanks @Ajay-Satish-01! BTW, when launching containers on VM and Kubernetes, the docker run and k8s pod manifest will override entrypoint:
skypilot/sky/provision/docker_utils.py
Line 120 in 052e0ea
Would you like to also change these in followup PRs?
|
For the follow-up PR, should I create a new preserve_entrypoint argument and include the command args only when we are not preserving the entry point? |
Should directly make the modification in this PR? It seems without changing the overwrite of entrypoint, this PR will not resolve #4750? |
|
@aylei can you please check if this is what you are expecting? |
| @@ -86,14 +86,26 @@ def docker_start_cmds( | |||
| container_name, | |||
| user_options, | |||
| docker_cmd, | |||
| preserve_entrypoint=False, | |||
There was a problem hiding this comment.
Thanks @Ajay-Satish-01 ! My question is how this flag is determined at caller, since user may provide their own image with no proper entrypoint
There was a problem hiding this comment.
Please check if this change is what you expect. Thanks
There was a problem hiding this comment.
Thanks for the update @Ajay-Satish-01 ! Left my thoughts for the entrypiont issue
| cmd = f'{self.docker_cmd} inspect --format="{{{{.Config.Entrypoint}}}}" {image}' | ||
| output = self._run(cmd, wait_for_docker_daemon=True) | ||
| # If output is [] or <nil>, the image has no entrypoint | ||
| return output != "[]" and output != "<nil>" |
There was a problem hiding this comment.
Even if the entrypoint exists, it might be inappropriate for sky container, e.g. most of application containers use the application binary as entrypoint.
Given this assumption, I think we can always use bash as the entrypiont, and exec tini to make tini the PID 1 process in the container if we found tini is available in the environment. I would like to follow this up and make commits to this PR later if you'd like!
There was a problem hiding this comment.
Sure. Thanks
Add tini to docker files #4750
Tested (run the relevant ones):
bash format.sh/smoke-test(CI) orpytest tests/test_smoke.py(local)/smoke-test -k test_name(CI) orpytest tests/test_smoke.py::test_name(local)/quicktest-core(CI) orpytest tests/smoke_tests/test_backward_compat.py(local)