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

[improve][build] Avoid building image multiple times #17208

Merged
merged 1 commit into from
Nov 10, 2022

Conversation

nodece
Copy link
Member

@nodece nodece commented Aug 22, 2022

Motivation

The current config will build the pulsar and pulsar-all images multiple times.

https://github.com/apache/pulsar/runs/7945037366?check_suite_focus=true#step:10:5675
https://github.com/apache/pulsar/runs/7945037366?check_suite_focus=true#step:10:6760

One is apache/pulsar, and one is without the apache repository, we should only keep an image without the apache repository, it is enough.

When we do release, only use the image without the apache repository. The following commands are from the docker/publish.sh.

docker tag pulsar:latest ${docker_registry_org}/pulsar:latest
docker tag pulsar-all:latest ${docker_registry_org}/pulsar-all:latest

docker tag pulsar:latest ${docker_registry_org}/pulsar:$MVN_VERSION
docker tag pulsar-all:latest ${docker_registry_org}/pulsar-all:$MVN_VERSION

Modifications

Documentation

Check the box below or label this PR directly.

Need to update docs?

  • doc-required
    (Your PR needs to update docs and you will update later)

  • doc-not-needed
    (Please explain why)

  • doc
    (Your PR contains doc changes)

  • doc-complete
    (Docs have been already added)

docker/pulsar-all/pom.xml Outdated Show resolved Hide resolved
@nodece nodece force-pushed the improve-build-image branch 4 times, most recently from c3356aa to a4bd30f Compare August 23, 2022 03:06
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Aug 23, 2022
Copy link
Member

@lhotari lhotari left a comment

Choose a reason for hiding this comment

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

I think it's a bad idea to remove tagging with ${project.version} since some custom build could be relying on that.

This PR should be limited to the change that just removes the unnecessary duplication docker build (which seems to be prevented by Docker caching).

@tisonkun
Copy link
Member

which seems to be prevented by Docker caching

IIRC in this #17148 (comment) I add a noCache config to keep logic as previous <noCache>true</noCache>. But we can enable it so that there's no need to wire all this logic in this patch.

@nodece
Copy link
Member Author

nodece commented Aug 23, 2022

I think it's a bad idea to remove tagging with ${project.version} since some custom build could be relying on that.

As far as I know, we didn't use ${project.version} image, if you know where using that, please let me know, thanks!

This PR should be limited to the change that just removes the unnecessary duplication docker build (which seems to be prevented by Docker caching).

You are right! But the docker-maven plugin only supports renewing an image name by rebuilding, or using https://dmp.fabric8.io/#docker:tag, but it only supports adding an image name.

which seems to be prevented by Docker caching

Multiple builds can use caching, see https://github.com/apache/pulsar/runs/7945037366?check_suite_focus=true#step:10:6768, this PR just wants to avoid the multiple builds.

We just enable noCache when building the test image. The pulsar, and pulsar-all images always use caching.

@nodece
Copy link
Member Author

nodece commented Aug 24, 2022

/pulsarbot rerun-failure-checks

Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

I have no objection to this patch, so approve to merge it. Although, we need to clarify the procedure of publishing:

  1. I don't see any -SNAPSHOT version on the dockerhub and suppose the test image refers to latest.
  2. The current build and publish process for releasing doesn't depend on maven settings.

@tisonkun
Copy link
Member

tisonkun commented Sep 3, 2022

It seems before and after this change, "Build Pulsar docker image" takes always 25~30 mins to complete.

UPDATE: It's because "Build Pulsar docker image" actually build test images instead of pulsar/pulsar-all images.

@nodece
Copy link
Member Author

nodece commented Sep 7, 2022

Hi @lhotari, @michaeljmarshall, do you have time to review this PR?

@nodece
Copy link
Member Author

nodece commented Sep 22, 2022

Ping @merlimat, could you review this PR?

@github-actions
Copy link

The pr had no activity for 30 days, mark with Stale label.

@github-actions github-actions bot added the Stale label Oct 31, 2022
@tisonkun
Copy link
Member

Merging...

@lhotari your questions are answered at #17208 (review)

@tisonkun tisonkun merged commit 79a97a9 into apache:master Nov 10, 2022
nodece added a commit to nodece/pulsar that referenced this pull request Apr 28, 2024
nodece added a commit to ascentstream/pulsar that referenced this pull request May 10, 2024
* [Dockerfile] Enable retries for apt-get when building Pulsar docker image (apache#14513)

- also reduce default timeout to 30 seconds
- prevents issues where apt repository doesn't respond

(cherry picked from commit d3f6fe3)

* PIP-155: Removed Python 2 support (apache#15376)

* Remove Pulsar Client Build for Python 2.7

* Remove outdated homebrew files (source of truth is upstream homebrew)

* Remove Python 2.7 build references; print error in some cases

* Update python client tests to run with python client for python 3.5m

* PIP-155: Removed Python 2 support

* Fixed invocation in pulsar-build image

* Fixed clang-format-10 indent differences

* Fixed script invocation with wrong python

* We don't need to rebuild the manylinux image each time

* Fixed image name

* Reverted back to use newer protobuf

* Fixed image name

* Fixed missing python3 in centos:7 image

* Use python3 for gtest-parallel

* Show bash commands in docker-tests.sh

* Fixed gh action issue with git directory permissions

* Fixed python to 3

* Fixed custom_logger_test.py

* Fixed path in run_python_instance_tests.sh

* Function runtime should use python3

* Fixed function runtime test python expectation

* Fixed presto worker launcher

* Fixed notes on how to format C++ code

Co-authored-by: Michael Marshall <[email protected]>

(cherry picked from commit 2b2e0c5)
Signed-off-by: Zixuan Liu <[email protected]>

* [improve][docker] Switch to Temurin JDK (apache#17129)

Signed-off-by: Zixuan Liu <[email protected]>

(cherry picked from commit 4378856)
Signed-off-by: Zixuan Liu <[email protected]>

* [refactor][ci] Build the docker image with docker-maven-plugin (apache#17148)

(cherry picked from commit a68b58d)
Signed-off-by: Zixuan Liu <[email protected]>

* [feat][build] Support ARM64-based docker images (apache#17733)

(cherry picked from commit 9a2aeb2)
Signed-off-by: Zixuan Liu <[email protected]>

* PIP-209: Removed C++/Python clients from main repo (apache#17881)

* PIP-209: Removed C++/Python clients from main repo

* Removed python directory from Docekrfile

* Fixed python client version argument scoping

* Fixed handling of pulsar.functions.serde

(cherry picked from commit f3c547b)
Signed-off-by: Zixuan Liu <[email protected]>

* [improve][build] Avoid building image multiple times (apache#17208)

Signed-off-by: Zixuan Liu <[email protected]>
(cherry picked from commit 79a97a9)

* [improve] Allow to build and push multi-arch Docker images (apache#19432)

Co-authored-by: Lari Hotari <[email protected]>
Co-authored-by: Yong Zhang <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
Co-authored-by: tison <[email protected]>

(cherry picked from commit 4190e40)
Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix publish image script (apache#20305)

Signed-off-by: Zixuan Liu <[email protected]>

(cherry picked from commit 94c7bf3)
Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix the pulsar-all image may use the wrong upstream image (apache#20435)

Signed-off-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>

(cherry picked from commit d7f3558)
Signed-off-by: Zixuan Liu <[email protected]>

* [feat][build] Adapt to Python client to be compatible with ARM arch

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Configure git-commit-id-plugin to skip git describe (apache#20550)

(cherry picked from commit 05f7e62)

* [improve][misc] Include native epoll library for Netty for arm64

From apache#22319

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][misc] Rename all shaded Netty native libraries

From apache#22415

Signed-off-by: Zixuan Liu <[email protected]>

* [cleanup][build] Cleanup -Ddocker.nocache=true

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix ubuntu mirror

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix license

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Downgrade docker-maven to 0.43.3

Signed-off-by: Zixuan Liu <[email protected]>

---------

Signed-off-by: Zixuan Liu <[email protected]>
Signed-off-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>
Co-authored-by: Matteo Merli <[email protected]>
Co-authored-by: Michael Marshall <[email protected]>
Co-authored-by: tison <[email protected]>
Co-authored-by: Yong Zhang <[email protected]>
Co-authored-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>
nodece added a commit to nodece/pulsar that referenced this pull request May 11, 2024
* [Dockerfile] Enable retries for apt-get when building Pulsar docker image (apache#14513)

- also reduce default timeout to 30 seconds
- prevents issues where apt repository doesn't respond

(cherry picked from commit d3f6fe3)

* PIP-155: Removed Python 2 support (apache#15376)

* Remove Pulsar Client Build for Python 2.7

* Remove outdated homebrew files (source of truth is upstream homebrew)

* Remove Python 2.7 build references; print error in some cases

* Update python client tests to run with python client for python 3.5m

* PIP-155: Removed Python 2 support

* Fixed invocation in pulsar-build image

* Fixed clang-format-10 indent differences

* Fixed script invocation with wrong python

* We don't need to rebuild the manylinux image each time

* Fixed image name

* Reverted back to use newer protobuf

* Fixed image name

* Fixed missing python3 in centos:7 image

* Use python3 for gtest-parallel

* Show bash commands in docker-tests.sh

* Fixed gh action issue with git directory permissions

* Fixed python to 3

* Fixed custom_logger_test.py

* Fixed path in run_python_instance_tests.sh

* Function runtime should use python3

* Fixed function runtime test python expectation

* Fixed presto worker launcher

* Fixed notes on how to format C++ code

Co-authored-by: Michael Marshall <[email protected]>

(cherry picked from commit 2b2e0c5)
Signed-off-by: Zixuan Liu <[email protected]>

* [improve][docker] Switch to Temurin JDK (apache#17129)

Signed-off-by: Zixuan Liu <[email protected]>

(cherry picked from commit 4378856)
Signed-off-by: Zixuan Liu <[email protected]>

* [refactor][ci] Build the docker image with docker-maven-plugin (apache#17148)

(cherry picked from commit a68b58d)
Signed-off-by: Zixuan Liu <[email protected]>

* [feat][build] Support ARM64-based docker images (apache#17733)

(cherry picked from commit 9a2aeb2)
Signed-off-by: Zixuan Liu <[email protected]>

* PIP-209: Removed C++/Python clients from main repo (apache#17881)

* PIP-209: Removed C++/Python clients from main repo

* Removed python directory from Docekrfile

* Fixed python client version argument scoping

* Fixed handling of pulsar.functions.serde

(cherry picked from commit f3c547b)
Signed-off-by: Zixuan Liu <[email protected]>

* [improve][build] Avoid building image multiple times (apache#17208)

Signed-off-by: Zixuan Liu <[email protected]>
(cherry picked from commit 79a97a9)

* [improve] Allow to build and push multi-arch Docker images (apache#19432)

Co-authored-by: Lari Hotari <[email protected]>
Co-authored-by: Yong Zhang <[email protected]>
Co-authored-by: Zixuan Liu <[email protected]>
Co-authored-by: tison <[email protected]>

(cherry picked from commit 4190e40)
Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix publish image script (apache#20305)

Signed-off-by: Zixuan Liu <[email protected]>

(cherry picked from commit 94c7bf3)
Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix the pulsar-all image may use the wrong upstream image (apache#20435)

Signed-off-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>

(cherry picked from commit d7f3558)
Signed-off-by: Zixuan Liu <[email protected]>

* [feat][build] Adapt to Python client to be compatible with ARM arch

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Configure git-commit-id-plugin to skip git describe (apache#20550)

(cherry picked from commit 05f7e62)

* [improve][misc] Include native epoll library for Netty for arm64

From apache#22319

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][misc] Rename all shaded Netty native libraries

From apache#22415

Signed-off-by: Zixuan Liu <[email protected]>

* [cleanup][build] Cleanup -Ddocker.nocache=true

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix ubuntu mirror

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Fix license

Signed-off-by: Zixuan Liu <[email protected]>

* [fix][build] Downgrade docker-maven to 0.43.3

Signed-off-by: Zixuan Liu <[email protected]>

---------

Signed-off-by: Zixuan Liu <[email protected]>
Signed-off-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>
Co-authored-by: Matteo Merli <[email protected]>
Co-authored-by: Michael Marshall <[email protected]>
Co-authored-by: tison <[email protected]>
Co-authored-by: Yong Zhang <[email protected]>
Co-authored-by: Zike Yang <[email protected]>
Co-authored-by: Lari Hotari <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants