-
Notifications
You must be signed in to change notification settings - Fork 916
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
ARTEMIS-3042 Add docker multistage build #4307
base: main
Are you sure you want to change the base?
Conversation
How is this related to https://issues.apache.org/jira/browse/ARTEMIS-4045 (i.e. the Jira referenced in the PR title)? |
Sorry my bad. Didn't get that the number refers to a jira ticket. |
Is this related to https://issues.apache.org/jira/browse/ARTEMIS-3042? The PR's title seems to correspond. If it is related please include the Jira in the commit message. If not, please create a Jira for this change and include it in the commit message. You can read more details about how to format commit messages in the hacking guide. |
Thanks for the hint. I added a message according to your guidelines. This pr solves exactly that issue. |
If there are any further questions I am more than welcome to answer them. |
It's not clear to me how your changes are directly related to https://issues.apache.org/jira/browse/ARTEMIS-3042 aside from just the wording. Your commit is just adding a new Dockerfile which is similar to the three other Dockerfiles which are already present. These three Dockerfiles were also already present when ARTEMIS-3042 was opened originally. The Jira description even says, "The current docker image build is not really user friendly or convenient at all." Therefore, adding another version of what already exists doesn't seem to help resolve the issue. Can you shed any light on this? |
To be clear, I am happy to merge your commit eventually. I just want the Jira/commit association to be clear for posterity's sake. |
@@ -187,4 +187,4 @@ cp ./docker-run.sh "$ARTEMIS_DIST_DIR/docker" | |||
echo "Docker file support files at:" | |||
tree "$ARTEMIS_DIST_DIR/docker" | |||
|
|||
next_step | |||
next_step |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change also isn't necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove the unnecessary changes. Thanks!
A docker multistage build contains one or multiple steps of building an image and one final image that is used during runtime. A multistage build has the advantage that some components are only needed during build time leading to a smaller footprint. It also has the charm that I don't need to know anything about building this image. Everything is handled during build process. Before my pr you had to follow the guides, run the script, cd into the _TMP/artemis/version folder and start the build process. This can be teadious and result in user errors: At first I forgot to change to that directory I had to read through the proprietary instructions to this project. Now you can start directly with building. Just use the Dockerfile as you would with any other Dockerfile and execute If you agree I would remove the other Dockerfiles as they are not needed anymore and may confuse the end user. |
3914b7f
to
2ac4f0e
Compare
The other two files are solved. Seems Windows/IntelliJ doesn't add a newline to the file. |
Seems like the failing test is not the cause of my commit. The other pr is also failing. |
When will this be merged? Is there something to do from my side? |
At this point two main things are not clear in your commit:
To resolve # 1 you should probably just add some documentation. You could do this, for example, in the To resolve # 2 you might also just add documentation. However, if the approach you've taken in this new Docker file is superior to the approach offered by the existing Docker files then it may be worth updating those to use your new approach so that users don't need to run |
I'm done. I adapted the README.md to reflect the changes in the build process and added the multistage capability to every Dockerfile. |
I guess this pr can be merged. |
I'm still in the process of testing all the changes locally. I have other priorities on my plate at the moment so progress has stopped at the moment. @SamTV12345, is there any particular rush to get this merged? If so, how come? |
@clebertsuconic, can you take a look at this as well since I believe you originally created the Docker files. |
Thanks for reporting back. I'd like to get this pr merged as soon as possible so I can move on to the next ticket. I guess it doesn't make much sense that I create a lot of pr's that don't get merged at the end. |
Is this PR a dependency for another task you're working on? I think most folks work on tasks in parallel. Once they send a PR they move on to their next task knowing that the PR will be reviewed and merged asynchronously.
Have you created other PRs that haven't been merged? I looked through the list of open and closed PRs and I don't see any others from you. Perhaps you used a different account when you created those? |
I don't argue that, but as I said, such images are great only for final deployment or local development.
I know that, however this PR changes the purpose of these Dockerfiles. Previously they contained only configuration needed for the runtime and served more as a tool to build your own custom images. Now they contain all the build flow inside, hence they now serve just as an easier way to run Artemis. |
I gave this more thought today. In theory, if the requirement is to just conveniently allow users to build docker image and run it locally there is a docker-compose exactly for that purpose. All prepare/run steps can be automated via Docker Compose YAML file. |
The question is how minimal is the produced image? Could you create a docker-compose with a few steps that would satisfy the requirement for different base images? If we talk about a docker-compose yaml you also need suitable tooling in form of a docker compose cli plugin or v1 docker-compose. If you could easily switch between base images and select them by a build arg that would indeed be interesting. I am unfortunately currently pretty involved in other FOSS projects so if you could please go through creating said yaml I should be able to adapt the scripts and add the alpine image which was one of the reasons this pr exists. |
Minor suggestion:
can be replaced with existing tools:
|
This is performed in the builder section. This section is executed one time at the beginning of the pipeline. Any subsequent pipeline runs in the future will have this command cached and it will never be rerun as it is done just at the beginning. I tuned the Dockerfile to cache the things that are static like apt packages and add the dynamic parts of the app e.g. artemis at the end. |
anyway 'tree' can be safely removed #4453 |
Removed. |
artemis-docker/prepare-docker.sh
Outdated
@@ -181,10 +163,9 @@ if [ ! -d "${ARTEMIS_DIST_DIR}/docker" ]; then | |||
mkdir "${ARTEMIS_DIST_DIR}/docker" | |||
fi | |||
|
|||
cp ./Dockerfile-* "$ARTEMIS_DIST_DIR/docker" | |||
cp ./docker-run.sh "$ARTEMIS_DIST_DIR/docker" | |||
|
|||
echo "Docker file support files at:" | |||
tree "$ARTEMIS_DIST_DIR/docker" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
its still there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What exactly is still there? tree has been removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'tree' command in L188 will cause build error since not available in build stage. I propose to remove it like done in https://github.com/apache/activemq-artemis/pull/4453/files#diff-09d0fd7b380274547af0cdaf1cb41c22b3990dfed9848b560e9d9f77b97fb204L188
or replace with 'ls -l'
I didn't had much time lately, but I will try to come up with YAML file which does the same as this PR this week. Produced image won't change because this doesn't depend on a tooling. Compose plugin is installed by default with Docker Desktop, so it's not a problem either. |
I removed the leftover tree command. I maybe can ditch all these sh files and build it with docker-compose. @ViliusS I'll try that today. Maybe I find a clever way to build that with docker-compose |
It's implemented. It is now save to review and hopefully merge. |
I've went through couple of different YAML and bash solution iterations today. I have also tested the solution provided in this PR. I still have to conclude that at least for us multistage builds give more headache than it solves. My main concern, as mentioned previously, is that Artemis preparation process will be integrated into Docker image which doesn't allow for easy customization. For example previously we had such Jenkins CI pipeline:
If this PR is ought to be accepted we won't be able to modify Artemis image as easily anymore. We would need to download official docker file, modify it by using "patch" or similar tools and only then build our image. This means we would need to maintain our own Dockerfile which looses the purpose of having official Docker file altogether. It also integrates preparation process into build process which lowers visibility in Jenkins (or any other CI system) UI. The final word will be on Artemis maintainers, by my proposal is to leave current Docker files as their are. If building Artemis images for fast local development is really a problem (which I don't think it is to run 2 commands instead of 1), then have separate Docker files. Or better yet, have official Docker image uploaded to Docker Hub for easy run with default settings. That way Docker files can stay in the repo for those wanting to modify the image. On a side note, what I would really want to see happening is proper customization via environment variables or Docker entrypoint as mentioned in ARTEMIS-3042. |
What exactly is this step sh "........ our modifications and custom configuration goes here........." and can't you simply do this before the jar is put into the container? The final build step is simply take the compiled jar with modifications if you like and put it into the various docker images. |
Modifications to web console for example. In the past this was also the place where we applied our own patches before building Artemis from source. |
Well that should be possible without a problem. You do your changes in the jenkins pipeline first, then compile it on the machine with |
As I said multiple times before, we can't without modifying provided Dockerfile. For modification to happen tarball needs to be downloaded and extracted first. |
@SamTV12345 @jbertram what is really the advantage of using this? just to avoid the prepare phase? This is doing a build all over again? The ./prepare.sh and build seemed a lot simpler to me. if you really wanted to have this committed, we could create a new folder ./artemis-distribution-multistage ... or move all the docker possibilities under a single directory. although it gets complicated with competing things.. we already have the ./artemis-image being a different OCI image by @gtully |
@ViliusS do you understand the advantage of multistage builds? @SamTV12345 I'm trying to understand the difference here? what's the issue with ./prepare.sh? again.. this seems moot by ./artemis-image ... did you look at it? |
is this making a docker official build any easier? is that the reason behind this? |
it would be nice if we could chat somewhere? Slack at Apache? at the end of build it just hangs as it's running the brokers |
This turbocharges new builds. You can build all platforms in next to no time and push it to Dockerhub. It streamlines the build process. Before that you had to go through the steps in the ./prepare.sh. Then go to the sub directory, then start building dockerfiles one after the other. If I want to start the whole build that would mean 1 x prepare.sh, 5-6 times Dockerfiles with the right tags. Non parallel builds. With this approach I can build all 6 images in around 5 minutes with minimum resource usage of the maven repository. |
You can add my developer email samelus1998@outlook.de and we can go through the build process. Without any screenshots/output its hard to evaluate. |
If you are asking in general, then yes. It's just a mechanism to separate build stage from the runtime. In this particular case I see no advantage, sorry. These dockerfiles previously were used just for runtime, by packing downloaded already built releases. So the separation was already there. |
@SamTV12345 I had everything ready on my build locally, building the image for me would been a couple seconds... and this made everything from scratch, including downloading the entire maven artifacts somewhere. I wouldn't have a problem if this was an additional option to build the image, meaning if I want to use the "regular" way we could just do what we have been doing so far. in that sense if you could reuse the docker files as they are now, and just call the "multi-stage" with docker compose build as an option.. then I would be ok... as long as it is well documented. |
This adds the possibility to create an artemis image with just the docker compose build command. First the image is downloaded in an Eclipse Temurin installation and compiled with a mounted volume. After that a second docker compose build command can be used to put the build artifact into the different containers. Thus activemq-artemis is only built once resulting in a very fast compile time.
@clebertsuconic I rebased the commits. What should be done for this pr to be merged? |
Added multistage docker image build
This adds the possibility to create an artemis image with just the command:
docker build -f Docker-alpine -t artemis_alpine .
Furthermore it has the smallest attack surface compared to the other images as it is based on alpine and only contains the jre, bash and libaio for running the application.
When releasing a new version simply adjust the arg value at the top and run above command to build a more recent version.