Replace tini in entrypoint.sh with catatonit#2968
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
🎉 Welcome to the Kubeflow Spark Operator! 🎉 Thanks for opening your first PR! We're happy to have you as part of our community 🚀 Here's what happens next:
Join the community:
Feel free to ask questions in the comments if you need any help or clarification! |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Replaces the tini init system with catatonit in both the Docker image and entrypoint script.
Changes:
- Install
catatonitinstead oftiniin the Dockerfile. - Update the entrypoint to invoke
catatonitinstead of/usr/bin/tini.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Dockerfile | Swap apt package tini for catatonit. |
| entrypoint.sh | Replace /usr/bin/tini -s -- with catatonit -- as PID 1 wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@vjanelle: changing LGTM is restricted to collaborators DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
|
||
| RUN apt-get update \ | ||
| && apt-get install -y tini \ | ||
| && apt-get install -y catatonit \ |
There was a problem hiding this comment.
can we consider adding --no-install-recommends to reduce image layer size: apt-get install -y --no-install-recommends catatonit ? To avoid pulling in unnecessary recommended packages. wdyt ?
There was a problem hiding this comment.
tini and catatonit are both small C programs that have the same posix headers as dependencies so switching out for catatonit won't be a big change in terms of adding new dependencies. That being said, catatonit is much larger since it is statically linked.
There are no recommended packages that would be installed for catatonit (or tini for that matter), so --no-install-recommends wouldn't make the size smaller than it would be.
`catatonit`'s use of signalfd is more performant than `tini`'s use of sigwait due to its event driven approach to receiving signals. `catatonit` also verifies the child is alive with after fork() whereas tini blindly trusts it worked. `catatonit` also closes all filedescriptors in the parent process, though entrypoint.sh does not currently open any fd's, having thise close be automatic in the future would be valuable in case any are opened in the entrypoint script in the future for whatever reason. Signed-off-by: Ali Maredia <amaredia@redhat.com>
alimaredia
force-pushed
the
replace-tini-with-catatonit
branch
from
22aa989 to
c2dbb0e
Compare
Purpose of this PR
Swap
tiniforcatatonitinentrypoint.sh. The following are some reasons whycatatonitshould be used instead oftini:catatonit's use of signalfd is more performant thantini's use of sigwait due to its event driven approach to receiving signals.catatonitalso verifies the child is alive with after fork() whereas tini blindly trusts it worked.catatonitalso closes all filedescriptors in the parent process, though entrypoint.sh does not currently open any fd's, having thise close be automatic in the future would be valuable in case any are opened in the entrypoint script in the future for whatever reason.Change Category
Rationale
Checklist
Additional Notes