Skip to content

Some improvements about Docker - #13059

Merged
FrankChen021 merged 15 commits into
apache:masterfrom
FrankChen021:docker
Sep 16, 2022
Merged

FrankChen021 merged 15 commits into
apache:masterfrom
FrankChen021:docker

Conversation

@FrankChen021

@FrankChen021 FrankChen021 commented Sep 8, 2022 •

Copy link
Copy Markdown
Member

These some improvements about Docker, including:

  1. Update to run with JRE 11 by default
  2. Update to use gcr.io/distroless/java11-debian11 image as base by default. This image is currently supported image for Java 11.
  3. Enable docker buildkit cache to speed up building
  4. Download bash-static to the image so that scripts that require bash can be executed(Fixes Ingestion task can't be launched in docker #13057 )
  5. bump builder image from 3.8.4-jdk-11-slim to the latest 3.8.6-jdk-11-slim
  6. switch busybox from amd64/busybox:1.30.0-glibc to busybox:1.35.0-glibc
  7. Support to build arm64-based image

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.
  • added integration tests.
  • been tested in a test Druid cluster.

@FrankChen021 FrankChen021 added the Docker https://hub.docker.com/r/apache/druid label Sep 8, 2022
@FrankChen021
FrankChen021 requested review from gianm and xvrl September 8, 2022 18:19
@kfaraz kfaraz mentioned this pull request Sep 9, 2022
1 of 9 tasks
@FrankChen021

FrankChen021 commented Sep 9, 2022 •

Copy link
Copy Markdown
Member Author

After switching to JRE 11, we need to use run-druid in the druid.sh to start Druid services because that script contains some --add-opens VM options for JRE11 and JRE 17.
Making as draft temporarily till this change is done.

@FrankChen021
FrankChen021 marked this pull request as draft September 9, 2022 15:51
Comment thread distribution/docker/Dockerfile Outdated
Comment on lines +55 to +56
RUN wget https://github.com/robxu9/bash-static/releases/download/5.1.016-1.2.3/bash-linux-x86_64 -O /bin/bash \
&& chmod 755 /bin/bash

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how much work would it be to fix the script to not use bash specific constructs?
If we are going to install bash or other things, maybe it would make sense to switch to a openjdk debian-slim image that already includes it

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I also agree with you that we switch to another base image. There're two reasons:

  1. Installing bash in this way makes it hard to support ARM64 in the future
  2. gcr.io/distroless image can't be directly accessed in mainland Chain due to network restriction, developers in China has to use some proxy servers to download this base image.

But distroless has some advantages such as smaller size, more security.
So currently I don't have an opinion on which we should use.

@xvrl xvrl Sep 9, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

how do others feel about switching base images? I agree installing bash is not multi-arch friendly, plus we do run tests against ARM64 today, so it would be nice to also do that for docker. cc @gianm

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It's better to have ARM64 supported. There are some feedback that ask for ARM64 images(see #11820), and someone made it by themselves.

To support ARM64, we need be able to build and run on ARM64 based machines.
Current Dockerfile uses amd64 busybox which blocks build image to run on ARM64 machines. It's easy to solve.

Another problem is even we solve that, we still can't build the image on Mac M1. Current the web-console dependency still prevents us from doing this, see #13012

@xvrl xvrl Sep 12, 2022 •

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

we still can't build the image on Mac M1

We can tackle building on arm and providing arm docker images separately. It should be possible to cross-compile the console until the node.js issue is resolved. That means building on M1 would be slower due to emulation, but I think that's better than not building at all.

Current Dockerfile uses amd64 busybox

it looks like you solved that problem by pointing to a multi-arch build of busybox.

I think the only issue left to solve is whether we want to switch to debian-slim, or if we want to add the logic to download the right arch binary for bash-static. The latter one might be simpler for now and we can do that by adding an if statement checking the value of $TARGETARCH (see https://docs.docker.com/engine/reference/builder/#automatic-platform-args-in-the-global-scope) to pick the right bash-static binary.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@gianm What's your opinion on changing the base image from distroless to debian-11?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@xvrl Actually I tried to build image on my Mac M1 by adding --platform=linux/amd64 to the docker build command several days ago, and I didn't make it. It takes more than 20 minutes and it seems it's stuck at building the source.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@xvrl Let's download the bash-static based on the target arch in this PR.

Comment thread distribution/docker/README.md
@FrankChen021
FrankChen021 marked this pull request as ready for review September 10, 2022 10:15
@FrankChen021 FrankChen021 removed the WIP label Sep 10, 2022
Comment thread distribution/docker/Dockerfile Outdated
Comment thread distribution/docker/Dockerfile Outdated
Comment thread distribution/docker/Dockerfile Outdated
FrankChen021 and others added 2 commits September 14, 2022 10:22
Co-authored-by: Xavier Léauté <xl+github@xvrl.net>
@FrankChen021

Copy link
Copy Markdown
Member Author

I have built and run the image on my intel MacBook and M1 MacBook, both are OK.

@FrankChen021
FrankChen021 requested a review from xvrl September 14, 2022 12:55
Comment thread distribution/docker/Dockerfile Outdated

@xvrl xvrl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just one minor typo

Co-authored-by: Xavier Léauté <xl+github@xvrl.net>
@FrankChen021

Copy link
Copy Markdown
Member Author

@gianm Do you have any comments on these changes? If no, I will merge it once the CI finishes.

@xvrl

xvrl commented Sep 15, 2022

Copy link
Copy Markdown
Member

@FrankChen021 you can go ahead and merge this, I don't think there is anything controversial about it. Adding bash-static is already backported as part of #13063. The biggest change is the switch to JDK 11, so we should probably mention that fact in the PR description and also make sure we add it to the release notes.

@FrankChen021

Copy link
Copy Markdown
Member Author

Thank you @xvrl very much for you helpful review.

@FrankChen021
FrankChen021 merged commit b8dd822 into apache:master Sep 16, 2022
@FrankChen021
FrankChen021 deleted the docker branch September 16, 2022 04:03
@kfaraz kfaraz added this to the 25.0 milestone Nov 22, 2022
riovic918data pushed a commit to riovic918data/druid that referenced this pull request Jun 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docker https://hub.docker.com/r/apache/druid Release Notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Ingestion task can't be launched in docker

3 participants