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

chore(build): add support for multiarch build #71

Merged
merged 6 commits into from
Nov 9, 2020

Conversation

shubham14bajpai
Copy link
Contributor

Signed-off-by: shubham shubham.bajpai@mayadata.io

What this PR does?:
This PR adds support for multi-arch builds for cstor-base and cstor images.

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/openebs-docs is used to track them:

Signed-off-by: shubham <shubham.bajpai@mayadata.io>
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
.github/workflows/build.yml Outdated Show resolved Hide resolved
RUN cd libcstor && cp ../cstor/lib/libzpool/.libs/*.so* ../cstor/lib/libuutil/.libs/*.so* ../cstor/lib/libnvpair/.libs/*.so* ../cstor/lib/libzfs/.libs/*.so* ../cstor/lib/libzfs_core/.libs/*.so* src/.libs/*.so* ./docker/zfs/lib

#Final
FROM ubuntu:bionic-20200219
Copy link

Choose a reason for hiding this comment

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

I'm kind of tempted to say we should maintain a unique final image for this with the relevant packages that are bundled. @akhilerm what do you think? The final stage here should be as slim as possible, I think we can't continue to just use ubuntu base and keep adding more packages as the images are already becoming quite large.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which image do you suggest @xunholy .

I think there are some packages of ubuntu which we use while running zfs making us dependent on ubuntu. We need to make sure those packages are available in the new image that we choose so that we dont break existing functionality.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can have our own images with all the packages installed pushed as cstor-build & cstor-final?

docker/cstor.Dockerfile Outdated Show resolved Hide resolved
docker/cstor.Dockerfile Outdated Show resolved Hide resolved
docker/cstor.Dockerfile Outdated Show resolved Hide resolved

- name: Build Image
env:
IMG_RESULT: cache
Copy link
Contributor

Choose a reason for hiding this comment

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

If you cache the image, you wont be able to use the image. after exiting buildix. It exists only in the buildkit cache.

For pull requests, this line will always pull image from the docker hub. It wont use the locally built cstor-base image.

We may need to push the image to an intermediate repo to use that image.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO we can just remove the cstor build and have only the cstor-base build in pull request as the cstor image only copies a entrypoint script to the cstor-base image.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, Then I think for now, that would work. In the long term, will need to identify a way to push intermediate images and use them.

docker/cstor.Dockerfile Outdated Show resolved Hide resolved

# default list of platforms for which multiarch image is built
ifeq (${PLATFORMS}, )
export PLATFORMS="linux/amd64,linux/arm64,linux/ppc64le"
Copy link
Contributor

Choose a reason for hiding this comment

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

armv7 support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

armv7 will require some code changes as the build is failing. One of the users who has a arm/v7 machine is trying that out https://kubernetes.slack.com/archives/CUAKPFU78/p1604006389024000
Once we figure out how to fix that I will raise a separate PR.

libssl1.0.0 rsyslog net-tools gdb apt-utils \
sed libjemalloc-dev

RUN if [ "$TARGETARCH" != "arm64" ]; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

Dont we need to check for both arm64 and arm32?

Copy link

Choose a reason for hiding this comment

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

If it's armv7 it will be just plain arm as the variant will hold the v7 value.

Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

given some comments @shubham14bajpai .

The main thing that you need to address is the use of cache in pull_request action. Since dependent images are present it wont work.

Signed-off-by: shubham <shubham.bajpai@mayadata.io>
Copy link
Contributor

@akhilerm akhilerm left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: shubham <shubham.bajpai@mayadata.io>
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
@kmova kmova merged commit e9742ec into openebs-archive:master Nov 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants