Skip to content

Include jvmkill in Docker image #13736

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.

Sign up for GitHub

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

Merged
electrum merged 2 commits into from
Aug 19, 2022
Merged

Include jvmkill in Docker image #13736

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b1ef88
Use logical JDK tag for Docker image
electrum Aug 18, 2022
2978c43
Include jvmkill in Docker image
electrum Aug 18, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 13 additions & 1 deletion core/docker/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,18 @@
# See the License for the specific language governing permissions and
# limitations under the License.
#
FROM eclipse-temurin:17.0.4_8-jdk
FROM eclipse-temurin:17-jdk AS builder

RUN \
set -xeu && \
echo 'Acquire::Retries "3";' > /etc/apt/apt.conf.d/80-retries && \
echo 'Acquire::http::Timeout "15";' > /etc/apt/apt.conf.d/80-timeouts && \
apt-get update -q && \
apt-get install -y -q git gcc make && \
Copy link
Copy Markdown
Member

@martint martint Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much larger does this make the image? We shouldn't keep these tools beyond what's needed to build the image.

Copy link
Copy Markdown
Member Author

@electrum electrum Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a separate builder image. We just copy the (tiny) library into the final image.

git clone https://github.com/airlift/jvmkill /tmp/jvmkill && \
Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we pin this to some commit? I expect jvmkill to rarely change but still it would prevent accidentally breaking workflows here.

Copy link
Copy Markdown
Member Author

@electrum electrum Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jvmkill has been stable since its initial commit in 2015, so pointing to master seems good, since the only changes are bug fixes (like adding _exit() as an alternative way to die). I do like your idea of doing releases through GHA, then we could point to an exact release.

make -C /tmp/jvmkill

FROM eclipse-temurin:17-jdk

RUN \
set -xeu && \
Expand All @@ -30,6 +41,7 @@ ARG TRINO_VERSION
COPY trino-cli-${TRINO_VERSION}-executable.jar /usr/bin/trino
COPY --chown=trino:trino trino-server-${TRINO_VERSION} /usr/lib/trino
COPY --chown=trino:trino default/etc /etc/trino
COPY --chown=trino:trino --from=builder /tmp/jvmkill/libjvmkill.so /usr/lib/trino/bin

EXPOSE 8080
USER trino:trino
Expand Down
1 change: 1 addition & 0 deletions core/docker/default/etc/jvm.config
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
-server
-agentpath:/usr/lib/trino/bin/libjvmkill.so
Copy link
Copy Markdown
Member

@hashhar hashhar Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we compile from source I believe it would work on all 3 of x64, arm64 and ppcle64 arches?

Copy link
Copy Markdown
Member Author

@electrum electrum Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that’s why I did it this way. I wish there was an easier way to package this so we don’t need to build every time.

Copy link
Copy Markdown
Member

@nineinchnick nineinchnick Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add a workflow in the jvmkill repo that would create releases with binaries for different architectures.

Copy link
Copy Markdown
Member

@hashhar hashhar Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Existing is ok for me. Also serves as an example for others who want to add similar in their own docker images.

Copy link
Copy Markdown
Member Author

@electrum electrum Aug 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nineinchnick I like that idea. Then we could just download them from the GitHub releases URL?

-XX:InitialRAMPercentage=80
-XX:MaxRAMPercentage=80
-XX:G1HeapRegionSize=32M
Expand Down