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

Use Dockerfile to facilitate development #1384

Merged
merged 7 commits into from
Nov 10, 2020
Merged

Conversation

blootsvoets
Copy link
Contributor

Added a Dockerfile with supporting files directly to the WebAPI source repository. Adding the Dockerfile here facilitates development build deployments and deploying custom builds. Also, it allows separate Atlas and WebAPI developers to publish their own images (see OHDSI/Atlas#2080). For easiest integration, automated builds can be set up to create Docker images whenever a new release or development version is made.

This image is based on the https://github.com/OHDSI/Broadsea-WebTools image, excluding Atlas and using from-source compilation rather than getting the WebAPI from a released war file. The build itself uses multi-stage build. In the first part, the WAR file is built. The POM file is added before the source code, so that dependencies do not have to be re-downloaded on every code change, only on POM file changes. The second part uses only the WAR file of the first part and is functionally very similar to the broadsea-webtools image. Using a multi-stage build ensures that only the WAR file is added to the public image and all Maven build information is only available on the machine where the image is built.

This PR does not yet include docker build documentation. I'm unsure whether to put this in a wiki or in the README.

@anthonysena
Copy link
Collaborator

Tagging @leeevans to see if he can take a look at this to see how it might work with Broadsea.

@blootsvoets blootsvoets force-pushed the dockerfile branch 2 times, most recently from d0f5b24 to 1c8d6a7 Compare December 19, 2019 14:55
@blootsvoets blootsvoets changed the base branch from master to rc-2.7.5 December 19, 2019 14:56
@blootsvoets blootsvoets changed the base branch from rc-2.7.5 to master January 28, 2020 07:54
@blootsvoets
Copy link
Contributor Author

blootsvoets commented Jan 28, 2020

@leeevans I've rebased this PR to the latest master to facilitate the review process.

pom.xml Outdated Show resolved Hide resolved
@blootsvoets
Copy link
Contributor Author

Copying in @MaximMoinat to help this PR along.

@blootsvoets
Copy link
Contributor Author

I've updated the code to the latest master, and removed the commented tomcat exclusion. Could you please reconsider this PR?

@chgl
Copy link
Contributor

chgl commented Oct 2, 2020

Really interested in this container as well :)

I'm a big fan of distroless base images from both a size and security perspective. I've compared the size of the final image when using different base images in the final layer:

Image Size
openjdk:11 800MB
openjdk:11-jre 458MB
openjdk:11-jre-slim 377MB
gcr.io/distroless/java:11 370MB

Here's what the finaly layer looks like when using distroless. Note that we can't use ${JAVA_OPTS} and similar because shell expansion does not work as there's no shell installed.

# OHDSI WebAPI and ATLAS web application running as a Spring Boot application with Java 11
# Environment variables may be set using JAVA_TOOL_OPTIONS
FROM gcr.io/distroless/java:11

LABEL maintainer="Lee Evans - www.ltscomputingllc.com"

# set working directory to a fixed WebAPI directory
WORKDIR /var/lib/ohdsi/webapi

# deploy the just built OHDSI WebAPI war file
# copy resources in order of fewest changes to most changes.
# This way, the libraries step is not duplicated if the dependencies
# do not change.
COPY --from=builder /code/war/WEB-INF/lib*/* WEB-INF/lib/
COPY --from=builder /code/war/org org
COPY --from=builder /code/war/WEB-INF/classes WEB-INF/classes
COPY --from=builder /code/war/META-INF META-INF

EXPOSE 8080
USER 65532
ENTRYPOINT [ "java" ]
# Directly run the code as a WAR.
CMD [ "-cp", ".:WebAPI.jar:WEB-INF/lib/*.jar", "-Djava.security.egd=file:///dev/./urandom", "org.springframework.boot.loader.WarLauncher" ]

@blootsvoets
Copy link
Contributor Author

I like changing to the slim image, since it sacrifices almost no functionality we use for a footprint comparable to distroless to and fewer vulnerabilities. Distroless would be a good option for a production container-only deployment like Kubernetes.

  • fewer dependencies (and vulnerabilities)
  • fewer opportunities for gaining access to the container

However, having curl and environment variables, as well as the option to meaningfully exec into the container is quite useful, especially in hybrid deployments like with docker-compose. It makes it easier to configure the image at deploy time and to set up a health check.

If there is a large need for a distroless base, it may be useful to give users the option by providing multiple tag suffixes (-slim, -distroless). For future reference: by changing JAVA_OPTS to JAVA_TOOL_OPTIONS, it will apply to Java without shell as well.

@blootsvoets
Copy link
Contributor Author

Based on @chgl remarks, I've updated the base image to 11-jre-slim and changed the user to 101, matching the nginx unprivileged image.

Note: the latest code updates require the .git directory to be added to the docker build runtime, since maven now wants to determine the Git commit hash. If there is a more elegant way of doing this, I'd be happy to know.

@chgl
Copy link
Contributor

chgl commented Oct 9, 2020

Is there any way I can help move this forward? Having a Dockerfile in the repo will already allow us to use Docker Hub's GitHub integration to build a new image for every commit or tag. Setting it up should be painless but requires a GitHub token and a Docker Hub account (https://docs.docker.com/docker-hub/builds/). We might also provide the image via GitHub's own registry at some point.

@leeevans
Copy link
Contributor

leeevans commented Oct 9, 2020

@chgl yes, the plan is to implement Github actions. We will move this forward after the OHDSI Symposium (October 19th).

pom.xml Show resolved Hide resolved
Copy link
Collaborator

@anthonysena anthonysena left a comment

Choose a reason for hiding this comment

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

These changes look good and work well from a WebAPI perspective. I'll defer to @leeevans for final approval. Thanks and great work!

@leeevans
Copy link
Contributor

@blootsvoets could you please update your fork/branch with the latest code in master and I'll do a docker build to test.

Adding a Dockerfile directly to the repository facilitates development
build deployments and deploying custom builds. Also, it allows separate Atlas and WebAPI developers
to publish their own images. For easiest integration, automated builds
can be set up to create Docker images whenever a new release or
development version is made.
Because of a Jersey bug (https://github.com/jersey/jersey/pull/196),
Jersey does not work with Spring Boot WAR files. By unpacking the WAR
file, directly running WebAPI works again.

Running the code with Undertow reduces Tomcat configuration
(tomcat-users.xml). Running it directly with Undertow instead of within
Tomcat ensures that the container gets stopped if WebAPI fails, rather
than the Tomcat servlet system staying active while WebAPI has already crashed.
@blootsvoets
Copy link
Contributor Author

@leeevans I'm sorry I missed your message. I've rebased the code to the latest master. Please do a

git pull --rebase

to test it.

Copy link
Contributor

@leeevans leeevans left a comment

Choose a reason for hiding this comment

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

thanks @blootsvoets
Looks good to me.

@anthonysena anthonysena removed the request for review from t-abdul-basser November 10, 2020 19:03
@anthonysena anthonysena merged commit 9f6863e into OHDSI:master Nov 10, 2020
@blootsvoets blootsvoets deleted the dockerfile branch July 7, 2021 09:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants