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

Add arm64 Docker images #14005

Closed
wants to merge 1 commit into from
Closed

Add arm64 Docker images #14005

wants to merge 1 commit into from

Conversation

kezhenxu94
Copy link
Member

@kezhenxu94 kezhenxu94 commented Jan 28, 2022

Fixes #12944

Motivation

Explain here the context, and why you're making that change. What is the problem you're trying to solve.

Modifications

Describe the modifications you've done.

Verifying this change

  • Make sure that the change passes the CI checks.

(Please pick either of the following options)

This change is a trivial rework / code cleanup without any test coverage.

(or)

This change is already covered by existing tests, such as (please describe tests).

(or)

This change added tests and can be verified as follows:

(example:)

  • Added integration tests for end-to-end deployment with large payloads (10MB)
  • Extended integration test for recovery after broker failure

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (yes / no)
  • The public API: (yes / no)
  • The schema: (yes / no / don't know)
  • The default values of configurations: (yes / no)
  • The wire protocol: (yes / no)
  • The rest endpoints: (yes / no)
  • The admin cli options: (yes / no)
  • Anything that affects deployment: (yes / no / don't know)

Documentation

Check the box below or label this PR directly (if you have committer privilege).

Need to update docs?

  • doc-required

    (If you need help on updating docs, create a doc issue)

  • no-need-doc

    (Please explain why)

  • doc

    (If this PR contains doc changes)

@github-actions
Copy link

@kezhenxu94:Thanks for your contribution. For this PR, do we need to update docs?
(The PR template contains info about doc, which helps others know more about the changes. Can you provide doc-related info in this and future PR descriptions? Thanks)

@Anonymitaet Anonymitaet added doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. and removed doc-label-missing labels Feb 7, 2022
@Anonymitaet
Copy link
Member

@kezhenxu94 when submitting a PR, can you provide doc-related info (tick the box) in the PR description? So that Bot can recognize and label your PR correspondingly. Or else Bot labels your PR with doc-info-missing.

Instructions of doc labels: https://docs.google.com/document/d/1Qw7LHQdXWBW9t2-r-A7QdFDBwmZh6ytB4guwMoXHqc0/edit#bookmark=id.5d66olk7l4oz

@sit
Copy link

sit commented Mar 8, 2022

@kezhenxu94 Do you know what it would take to get all the PR checks passing?

@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Mar 10, 2022
@github-actions github-actions bot removed the doc-not-needed Your PR changes do not impact docs label Mar 10, 2022
@sit
Copy link

sit commented Mar 17, 2022

@kezhenxu94 Is there anything that can be done to help get things working?

@kezhenxu94
Copy link
Member Author

@kezhenxu94 Is there anything that can be done to help get things working?

I think the build should be working. Just need to get the CI passed. Kind of busy recently. Hopefully I can find some time to get back to this.

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.

This looks like a great work !
Thanks

We will need to coordinate the release procedure.

We do have some problems with the deployment of the docker images to dockerhub due to the fact that are have a proprietary docker space (not managed by apache infra)
if we refactor the docker build we could think about make it is possible to allow a build using the ASF Infra Docker hub bot.

Can we start a discussion on dev@pulsar.apache.org please ?

@sit
Copy link

sit commented Mar 29, 2022

@eolivelli Is there any help that the community can provide here?

@@ -69,14 +69,6 @@
<profile>
<id>docker</id>
<!-- include the docker image only when docker profile is active -->
<dependencies>
Copy link
Contributor

@heesung-sn heesung-sn Apr 18, 2022

Choose a reason for hiding this comment

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

I think we may need to keep this dependency in order to keep the build order, pulsar-docker-image -> pulsar-all-docker-image, since the pulsar-all-docker-image copies the pulsar server binaries from the pulsar-docker-image(newly built) and additionally inject the updated puslar-offloader and pulsar-io binaries.

<argument>
PULSAR_IO_DIR=target/apache-pulsar-io-connectors-${project.version}-bin
</argument>
<argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to add <argument>--build-arg</argument> as the key prefix for PULSAR_OFFLOADER_TARBALL.

<argument>
PULSAR_OFFLOADER_TARBALL=target/pulsar-offloader-distribution-${project.version}-bin.tar.gz
</argument>
<argument>--push</argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this --push might try to push the image to the DockerHub repo instead of local. Do we want to push the image to local or DockerHub here?

<argument>build</argument>
<argument>--no-cache</argument>
<argument>--build-arg</argument>
<argument>PULSAR_TARBALL=target/pulsar-server-distribution-${project.version}-bin.tar.gz</argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to pass PULSAR_OFFLOADER_TARBALL and PULSAR_IO_DIR instead of PULSAR_TARBALL for puslar-all/build. In fact, I think this is the root cause of bin/sh -c mv /apache-pulsar-offloaders-*/offloaders /offloaders error.

@@ -33,6 +33,8 @@
<properties>
<skipBuildPythonClient>false</skipBuildPythonClient>
<skipCopyPythonClients>false</skipCopyPythonClients>
<docker.build.skip>true</docker.build.skip>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you clarify the usage of these new props? What's the build order change here?

<executable>docker</executable>
<workingDirectory>${project.basedir}</workingDirectory>
<arguments>
<argument>buildx</argument>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think buildx requires BuildKit container in order to build for the multi platforms. Don't we need to exec(or check) docker buildx create --use --driver docker-container before this buildx execution?

@sit
Copy link

sit commented Apr 25, 2022

@kezhenxu94 What do you think are the next steps on this PR?

@kezhenxu94
Copy link
Member Author

@kezhenxu94 What do you think are the next steps on this PR?

Hi. Sorry haven't found time to continue work on this yet. Anyone interested in this can continue based on this or I can just close this to let others take over this task.

@github-actions
Copy link

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

@axiopisty
Copy link

Why is this ticket marked as closed rather than some other status that lets people know it is still unresolved? I think the ticket should be reopened as the pulsar images still do not support arm64.

@grishick
Copy link

grishick commented Jul 8, 2022

Why is this ticket marked as closed rather than some other status that lets people know it is still unresolved? I think the ticket should be reopened as the pulsar images still do not support arm64.

The PR is closed, but the issue is still open: #12944

@michaeljmarshall
Copy link
Member

@kezhenxu94 - can you clarify why you closed this PR? If you are unable to continue work on it, I'm happy to help out or to help find someone who is able to add the feature. It'd be great to get this in 2.11, which is probably going to come later this summer.

@kezhenxu94
Copy link
Member Author

@kezhenxu94 - can you clarify why you closed this PR? If you are unable to continue work on it, I'm happy to help out or to help find someone who is able to add the feature. It'd be great to get this in 2.11, which is probably going to come later this summer.

Hi @michaeljmarshall , I haven't found time to continue this work so I have to close it and let someone else to take over the work.

@michaeljmarshall
Copy link
Member

Thank you for your update, @kezhenxu94. I will start a thread on the dev@pulsar mailing list to discuss what needs to be done and to find someone to take over the work. It'd be great to see this done for 2.11.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. lifecycle/stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ARM based docker image
8 participants