Skip to content

Conversation

@nodece
Copy link
Member

@nodece nodece commented May 29, 2023

Motivation

  1. Right now the docker/publish.sh script always publishes the latest image, this causes the pulsar version issue: [Bug] Latest docker image version should be 2.11.0 #19544
  2. When using the custom docker.organization, the pulsar-all/Dockerfile always uses the apachepulsar/pulsar:latest as the base image, so cause [Bug] pulsar-all 3.0.0 docker image contains Pulsar 2.11.0 #20420

Modifications

  • Add a prompt to confirm the publishing of the latest images in the docker/publish.sh script.
  • Pass the PULSAR_IMAGE to the docker/pulsar-all/Dockerfile to avoid using incorrect image.

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label May 29, 2023
@nodece nodece force-pushed the improve-publishing-docker-image branch 2 times, most recently from 15d3eac to 3c799d9 Compare May 29, 2023 15:37
@lhotari
Copy link
Member

lhotari commented May 29, 2023

  • Use apachepulsar/pulsar:${PULSAR_VERSION} instead of apachepulsar/pulsar:latest in the docker/pulsar-all/Dockerfile, I expect to be able to use the correct pulsar image.

I support this change, but the usage of apachepulsar/pulsar:latest in docker/pulsar-all/Dockerfile wasn't the root cause that caused Pulsar 3.0.0 pulsar-all docker images to use the 2.11 image.

The default build process should have built apachepulsar/pulsar:latest locally before the pulsar-all image.
Perhaps the build dependency isn't working as expected?

<!-- include the docker image only when docker profile is active -->
<dependencies>
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>pulsar-docker-image</artifactId>
<version>${project.version}</version>
<type>pom</type>
<scope>provided</scope>
<exclusions>
<exclusion>
<groupId>*</groupId>
<artifactId>*</artifactId>
</exclusion>
</exclusions>
</dependency>
</dependencies>

We might have to dig deeper.

A possible way to prevent using an old image would be to use a per-build invocation tag instead of using project.version alone.

@lhotari
Copy link
Member

lhotari commented May 29, 2023

Using the Git SHA as part of the tag and ensuring that the repository is clean before building would be a useful improvement over the changes already in this PR. The problem with project.version is that it's not unique. It's better than latest, but doesn't prevent the mistake from happening again.

The https://github.com/git-commit-id/git-commit-id-maven-plugin maven plugin could be used to find out the commit hash.

@cbornet
Copy link
Contributor

cbornet commented May 29, 2023

Note that I didn't use the publish.sh script to build the image.
I used the following command to have cross-compilation for ARM:

mvn install -DUBUNTU_MIRROR=http://azure.archive.ubuntu.com/ubuntu/ \
        -DskipTests \
        -Pmain,docker -Pdocker-push \
        -Ddocker.platforms=linux/amd64,linux/arm64 \
        -Ddocker.organization=cbornet \
         -pl docker/pulsar,docker/pulsar-all

We should update the script to use this command and see why it didn't pick the correct image.

@nodece
Copy link
Member Author

nodece commented May 30, 2023

Hi @lhotari, thank you for your explanation!

Perhaps the build dependency isn't working as expected?
It seems that there is no locally built image used.

I agreed that use Git SHA, which is unique, but we can also use the project.version when building the apachepulsar/puslar-all:3.0.0 image, and we use the apachepulsar/pulsar:3.0.0 as the base image, which will does not cause version confusion unless you provide the wrong version number.

@nodece
Copy link
Member Author

nodece commented May 30, 2023

Hi @cbornet, it looks your command is correct, I need to reproduce your case.

If you still have build logs on your computer, please let me know.

@RobertIndie
Copy link
Member

Hi, I don't think this is the root cause. The root cause is here: #20420 (comment)

I have pushed a PR to address the root cause: #20435

@nodece
Copy link
Member Author

nodece commented May 30, 2023

@RobertIndie Good catch! BTW, we still need to update the publish.sh script, or use docker-push instead of that?

Right now we have docker/build.sh and docker/publish.sh scripts to do the build and publish, and have the maven command, there is some confusion now, we must clean up that.

I think we should the maven command instead shell script, we also cherry-pick the build and script changes for other branches to unify the release process.

please let me know your thoughts.

Signed-off-by: Zixuan Liu <nodeces@gmail.com>
@lhotari
Copy link
Member

lhotari commented May 30, 2023

Hi @lhotari, thank you for your explanation!

Perhaps the build dependency isn't working as expected?
It seems that there is no locally built image used.

I agreed that use Git SHA, which is unique, but we can also use the project.version when building the apachepulsar/puslar-all:3.0.0 image, and we use the apachepulsar/pulsar:3.0.0 as the base image, which will does not cause version confusion unless you provide the wrong version number.

The version number doesn't address the problem since the version number doesn't uniquely identify the build product (artifact). The project.version is not much better than latest in this sense, it won't fix the issue.

@nodece Your PR would be a good solution by simply continuing the work and making it use a tag that is for example a combination of the project.version and the git sha. I hope you could consider this.
If there's confusion, I could help by pushing the changes to this PR. It's not many extra changes.

@lhotari
Copy link
Member

lhotari commented May 30, 2023

I guess we would need to backport the solution to all maintenance branches so that maintenance release docker images don't get mixed up. The script shouldn't attempt to push the latest tag when the script is run in maintenance branches.

Copy link
Contributor

@eolivelli eolivelli left a comment

Choose a reason for hiding this comment

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

+1

@eolivelli
Copy link
Contributor

@nicoloboschi PTAL

Copy link
Member

@RobertIndie RobertIndie left a comment

Choose a reason for hiding this comment

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

I see that some parts of the changes are moved from #20435 to here.

I agree that we need to update the publish.sh script. But after Pulsar 3.0, the method for building docker image is different. Please see #20420 (comment)

I am afraid that the publish.sh in this PR doesn't build the arm64 image.
I think this PR can be focused on the improvement of publish.sh. And #20435 can focus on the fix of the current build.

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.

  • Git SHA should be part of the tag (it could be an extra tag) so that it is unique and ensures that the correct parent image is used, also in the case that the build ordering wouldn't be effective for some reason.
    • what if "pulsar-all" gets built before "pulsar" in the build?
  • Pushing the latest tag should be prevented in maintenance branch publishing process and scripts used to do the publishing

@lhotari
Copy link
Member

lhotari commented May 30, 2023

I am afraid that the publish.sh in this PR doesn't build the arm64 image.

@RobertIndie @cbornet @tisonkun Is https://pulsar.apache.org/contribute/release-process/#stage-docker-images up-to-date with this information?

@lhotari
Copy link
Member

lhotari commented May 30, 2023

Note that I didn't use the publish.sh script to build the image. I used the following command to have cross-compilation for ARM:

mvn install -DUBUNTU_MIRROR=http://azure.archive.ubuntu.com/ubuntu/ \
        -DskipTests \
        -Pmain,docker -Pdocker-push \
        -Ddocker.platforms=linux/amd64,linux/arm64 \
        -Ddocker.organization=cbornet \
         -pl docker/pulsar,docker/pulsar-all

We should update the script to use this command and see why it didn't pick the correct image.

@cbornet Would you mind getting the process description in https://pulsar.apache.org/contribute/release-process/#stage-docker-images up-to-date with this information? /cc @tisonkun

@nodece
Copy link
Member Author

nodece commented May 30, 2023

I think we need to handle the two points :

  1. Use the correct image as the base image in the pulsar-all/Dockerfile and tests/docker-images/latest-version-image/Dockerfile
  2. Cleanup the build and publish scripts, using maven command instead of shell script

@nodece
Copy link
Member Author

nodece commented Jun 2, 2023

Move to #20462

@nodece nodece closed this Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

doc-not-needed Your PR changes do not impact docs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants