Skip to content
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.

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

docker multistage build #276

Closed
wants to merge 2 commits into from
Closed

Conversation

oliverpool
Copy link

This PR make a multistage build, to only keep the compiled nginx in the final image.

[@kleisauke] Keep in mind that just copying the library dependencies and executables via a multi-stage build won't work with DNF (CentOS/RHEL/Fedora default package manager), so I'm a bit reluctant for such an approach.

Using ldd and tar, I managed to get it working fine.

I also took the liberty to update the docker/README.md file to document the availability of the image on the github registry.

Closes #204

@codecov
Copy link

codecov bot commented Mar 18, 2021

Codecov Report

Merging #276 (df80f1f) into 5.x (6862ddd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##               5.x      #276   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           45        45           
  Lines         1721      1721           
=========================================
  Hits          1721      1721           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6862ddd...df80f1f. Read the comment docs.

Copy link
Member

@kleisauke kleisauke left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! With that comment I meant to say that something like this:

FROM ghcr.io/weserv/images:5.x

# Update RPM packages
RUN dnf update -y

Will not update the dependencies, since DNF is not aware of the copied dependencies. Furthermore, because ldd looks only at the symbols table, it's unaware of libraries being loaded at runtime with dlopen (e.g., ImageMagick's dependencies in /usr/lib64/ImageMagick-x.y.z/modules-Q16/* are not copied along).

I think a better approach to reduce the image size would be to remove the -devel dependencies, the build directory, and the DNF cache after configuring and building the CMake project. For example, I saw that someone did this on one of the forks:
8349d64...palamccc:5.x

@oliverpool
Copy link
Author

Thank you for the detailed comment!

Will not update the dependencies, since DNF is not aware of the copied dependencies.

I think this is expected for most of the docker builds (since a dependency update will likely need a recompilation)

it's unaware of libraries being loaded at runtime with dlopen

Do you have an example of a API endpoint which needs such a library? (so that I can watch it fail and try some fixes)

@kleisauke
Copy link
Member

I think this is expected for most of the docker builds (since a dependency update will likely need a recompilation)

I'm not sure if I understand this. Since the majority of dependencies are ABI compatible you could just do this on a running container:

docker exec imagesweserv dnf update -y

This does not require recompilation of the entire Docker image nor of this project.

Do you have an example of a API endpoint which needs such a library? (so that I can watch it fail and try some fixes)

Try loading BMP or JPEG 2000 images (or any other libMagick image file types we "unofficially" support). After testing this, I noticed that this works without any problems since you are extending the builder image with this line:

FROM builder as runner

I think you meant do this instead:

Details
diff --git a/docker/Dockerfile b/docker/Dockerfile
index 1111111..2222222 100644
--- a/docker/Dockerfile
+++ b/docker/Dockerfile
@@ -46,10 +46,6 @@ RUN dnf install -y epel-release \
         pcre-devel \
         zlib-devel
 
-# Create nginx user and group
-RUN groupadd nginx \
-    && useradd -r -g nginx -s /sbin/nologin -c "Nginx web server" nginx
-
 # Copy the contents of this repository to the container
 COPY . /var/www/imagesweserv
 
@@ -84,11 +80,21 @@ RUN cmake3 .. \
 RUN ldd /usr/sbin/nginx | cut -d" " -f3 | xargs tar --dereference --absolute-names -cf libs.tar
 
 ######################
-FROM builder as runner
+FROM centos:8 as runner
+
+# Create nginx user and group
+RUN groupadd nginx \
+    && useradd -r -g nginx -s /sbin/nologin -c "Nginx web server" nginx
+
+# libjpeg-turbo is installed in a non-standard prefix
+ENV LD_LIBRARY_PATH=/opt/libjpeg-turbo/lib64
 
 # Copy nginx to the appropriate location
 COPY --from=builder /usr/sbin/nginx /usr/sbin/nginx
 
+# Copy the whole /etc/nginx directory
+COPY --from=builder /etc/nginx/ /etc/nginx/
+
 # Copy nginx configuration to the appropriate location
 COPY --from=builder /var/www/imagesweserv/ngx_conf/*.conf /etc/nginx/
 

i.e. start from a clean centos:8 base image instead of the recently built builder image.

Anyways, I think it's just easier to clean-up the "base" image instead of using a multi-stage image for the reasons given above. This also ensures that the Docker image is future-proof (libvips is considering using loadable modules for some dependencies in the near future) and easier to maintain.

@oliverpool
Copy link
Author

Anyways, I think it's just easier to clean-up the "base" image

You are probably right.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Publish Docker image on GitHub Container Registry
2 participants