[native] Optimize Dockerfile by duhow · Pull Request #25487 · prestodb/presto

duhow · 2025-07-03T09:10:38Z

Description

Changes in Dockerfile commands to optimize the container image.

Additionally, uses trap to forward the TERM signal (stop) to the application, instead of waiting until KILL.

Motivation and Context

💾 Saves ~500MB in Docker image, and makes updates even faster if reusing cached layers (eg. keep java version)

Impact

Pulling the image will be faster.

Test Plan

Contributor checklist

Please make sure your submission complies with our contributing guide, in particular code style and commit standards.
PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
Documented new properties (with its default value), SQL syntax, functions, or other functionality.
If release notes are required, they follow the release notes guidelines.
Adequate tests were added if applicable.
CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== RELEASE NOTES ==

General Changes
* Improve and optimize Docker image layers

linux-foundation-easycla · 2025-07-03T09:10:45Z

The committers listed above are authorized under a signed CLA.

✅ login: duhow / name: David Girón (e631b9f)

czentgr

Thanks for working on optimizing the image. I had some questions.

czentgr · 2025-07-03T15:14:55Z


 EXPOSE 8080

+VOLUME ["/var/lib/presto/data"]


What would be mounted here? What is in this directory provided by the host?
How would a user use now that this is mounted?

Based on the original Dockerfile which runs mkdir -p /var/lib/presto/data, I assume this is the default path for "persistent data of the application".

Refering to it as a VOLUME, it provides information to the image about the path that will be persisted by default, and when running a container, will create the volume in host by default.

Note that this will not mount the host directory, it will create a Docker volume matching that path inside the container.

https://docs.docker.com/build/building/best-practices/#volume

If this cause any problems, we can get rid of it, no problem.

Suggested change

VOLUME ["/var/lib/presto/data"]

Thanks, it should be ok persisting this. We should document this so a user can also mount this with the run command to a different place on the host.

czentgr · 2025-07-03T15:22:19Z

+trap 'kill -TERM $app 2>/dev/null' TERM

-$PRESTO_HOME/bin/launcher run
+$PRESTO_HOME/bin/launcher run &


Can you explain a bit more on the benefit of this change? This installs the forward for TERM which previously should also be seen if the container is stopped?
wait also is terminated with TERM so why wasn't this happening for presto_server previously. Looks like you said it was waiting for a KILL?

When you do docker stop, the container runtime by default will send a SIGTERM to the application.

In this case, the application is /bin/sh as we are running this entrypoint script, hence this script will receive the signal.

However, we want to send the signal to the application itself, otherwise it will keep running.

As the container runtime sees the container still running, after a timeout of 10 seconds, it will send a SIGKILL to force stop the application - this is why docker stop takes 10 seconds to stop the application.

Now, with this change, the script receives the signal (trap), and runs a function (send the signal to the running application), which in turn makes it shutdown.

Note, if there is another preferred signal to stop the application gracefully (eg. SIGINT, SIGUSR), we can change it.

Also note, since we are not doing that much operation in this entrypoint script, we could also remove it and call directly the program as ENTRYPOINT in Dockerfile, so that remains as main PID.
The program receives any ENV previously defined, unless you had any experience with this failing...?

@duhow Thanks for the explanation. So I was wondering if the 10s don't come from the config kShutdownOnsetSec. This is by default 10s. So even receiving the SIGTERM it will take 10s to actually shut down. basically, I think you are seeing expected behavior on docker stop with the main PID being presto_server.

The signal handler reacts on SIGINT and SIGTERM and stops it. The idea is to try and make sure that tasks complete.

So if the motivation was to reduce the docker stop time then the config is perhaps something to look at rather than changing the code here. Although the code change looks ok I'm not sure it does why you made the change originally.

The change in here is because the presto_server does not receive any SIGINT or SIGTERM signal, so it won't know it has to shutdown when doing docker stop.

This change however makes that the signal is passed to the application, which then should do whatever it needs before closing - any change in the application to shutdown properly (if needed) should be in another PR.

Understood. Actually, I was thinking of PrestoC++ but this is the python launcher for Presto Java which spawns the JVM.

unidevel · 2025-07-04T08:40:27Z

There is an issue when publishing the docker image, seems related to the mount, since we will build the images simultaneously =>

#12 [linux/arm64 stage-0 2/9] RUN --mount=type=cache,target=/var/cache/dnf     dnf install -y java-11-openjdk less procps python3     && ln -s $(which python3) /usr/bin/python     && mv /etc/yum/protected.d/systemd.conf /etc/yum/protected.d/systemd.conf.bak
#12 97.18 CentOS Stream 9 - Extras packages                12 kB/s |  19 kB     00:01    
#12 98.73 Last metadata expiration check: 0:00:01 ago on Fri Jul  4 08:35:21 2025.
#12 98.74 [Errno 2] No such file or directory: '/var/cache/dnf/metadata_lock.pid'
#12 ERROR: process "/bin/sh -c dnf install -y java-11-openjdk less procps python3     && ln -s $(which python3) /usr/bin/python     && mv /etc/yum/protected.d/systemd.conf /etc/yum/protected.d/systemd.conf.bak" did not complete successfully: exit code: 1

#10 [linux/ppc64le stage-0 2/9] RUN --mount=type=cache,target=/var/cache/dnf     dnf install -y java-11-openjdk less procps python3     && ln -s $(which python3) /usr/bin/python     && mv /etc/yum/protected.d/systemd.conf /etc/yum/protected.d/systemd.conf.bak
#10 97.32 (60/220): shared-mime-info-2.1-5.el9.ppc64le.rp 445 kB/s | 562 kB     00:01    
#10 97.62 (61/220): systemd-pam-252-53.el9.ppc64le.rpm    321 kB/s | 292 kB     00:00    
#10 98.07 (62/220): systemd-252-53.el9.ppc64le.rpm        3.0 MB/s | 4.1 MB     00:01    
#10 98.09 (63/220): systemd-rpm-macros-252-53.el9.noarch.  93 kB/s |  71 kB     00:00    
#10 98.88 (64/220): systemd-udev-252-53.el9.ppc64le.rpm   1.5 MB/s | 1.9 MB     00:01    
#10 CANCELED
------
 > [linux/arm64 stage-0 2/9] RUN --mount=type=cache,target=/var/cache/dnf     dnf install -y java-11-openjdk less procps python3     && ln -s $(which python3) /usr/bin/python     && mv /etc/yum/protected.d/systemd.conf /etc/yum/protected.d/systemd.conf.bak:
2.493 Waiting for process with pid 7 to finish.
30.96 CentOS Stream 9 - BaseOS                        8.0 MB/s | 9.9 MB     00:01    
45.43 CentOS Stream 9 - AppStream                      15 MB/s |  22 MB     00:01    
97.18 CentOS Stream 9 - Extras packages                12 kB/s |  19 kB     00:01    
98.73 Last metadata expiration check: 0:00:01 ago on Fri Jul  4 08:35:21 2025.
98.74 [Errno 2] No such file or directory: '/var/cache/dnf/metadata_lock.pid'

unidevel

LGTM, the cache mount seems great to accelerate the build time. Thanks for the optimization, please address @czentgr comments before merge.

czentgr

Please also squash the commits and change the commit message to have [native] as prefix because this PR only applies to Prestissimo.

czentgr · 2025-07-10T14:51:52Z


 EXPOSE 8080

+VOLUME ["/var/lib/presto/data"]


Thanks, it should be ok persisting this. We should document this so a user can also mount this with the run command to a different place on the host.

duhow · 2025-07-13T10:18:02Z

@czentgr done 👍🏻

czentgr · 2025-07-14T20:05:18Z

+trap 'kill -TERM $app 2>/dev/null' TERM

-$PRESTO_HOME/bin/launcher run
+$PRESTO_HOME/bin/launcher run &


Understood. Actually, I was thinking of PrestoC++ but this is the python launcher for Presto Java which spawns the JVM.

duhow requested review from a team, czentgr and unidevel as code owners July 3, 2025 09:10

czentgr reviewed Jul 3, 2025

View reviewed changes

duhow commented Jul 8, 2025

View reviewed changes

Comment thread docker/Dockerfile Outdated

unidevel approved these changes Jul 10, 2025

View reviewed changes

czentgr reviewed Jul 10, 2025

View reviewed changes

duhow changed the title ~~Optimize Dockerfile~~ [native] Optimize Dockerfile Jul 13, 2025

[native] optimize dockerfile

e631b9f

czentgr approved these changes Jul 14, 2025

View reviewed changes

czentgr merged commit aafe2e3 into prestodb:master Jul 14, 2025
108 checks passed

This was referenced Jul 24, 2025

Add release notes for 0.294 unix280/presto#39

Merged

Add release notes for 0.294 unix280/presto#40

Merged

prestodb-ci mentioned this pull request Jul 28, 2025

Add release notes for 0.294 #25633

Merged

6 tasks

Conversation

duhow commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

Release Notes

Uh oh!

linux-foundation-easycla Bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

czentgr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

unidevel commented Jul 4, 2025

Uh oh!

Uh oh!

unidevel left a comment

Choose a reason for hiding this comment

Uh oh!

czentgr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duhow commented Jul 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

duhow commented Jul 3, 2025 •

edited

Loading

linux-foundation-easycla Bot commented Jul 3, 2025 •

edited

Loading