-
Notifications
You must be signed in to change notification settings - Fork 27
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
Conversation
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
750f128
to
73ada39
Compare
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
73ada39
to
abf49b0
Compare
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
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 |
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.
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.
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.
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.
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.
I think we can have our own images with all the packages installed pushed as cstor-build
& cstor-final
?
|
||
- name: Build Image | ||
env: | ||
IMG_RESULT: cache |
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.
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.
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.
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.
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.
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.
|
||
# default list of platforms for which multiarch image is built | ||
ifeq (${PLATFORMS}, ) | ||
export PLATFORMS="linux/amd64,linux/arm64,linux/ppc64le" |
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.
armv7 support?
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.
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 \ |
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.
Dont we need to check for both arm64
and arm32
?
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.
If it's armv7 it will be just plain arm as the variant will hold the v7 value.
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.
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>
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.
LGTM
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
Signed-off-by: shubham <shubham.bajpai@mayadata.io>
920d4e1
to
15ac321
Compare
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:
<type>(<scope>): <subject>