-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 s390x support on multiarch docker images #2948
Add s390x support on multiarch docker images #2948
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2948 +/- ##
==========================================
- Coverage 95.94% 95.92% -0.02%
==========================================
Files 236 236
Lines 10238 10238
==========================================
- Hits 9823 9821 -2
- Misses 345 347 +2
Partials 70 70
Continue to review full report at Codecov.
|
This change is to add s390x support on multiarch Jaeger docker images Signed-off-by: Kun Lu <kun.lu@ibm.com> Signed-off-by: Kun-Lu <kun.lu@ibm.com>
93842d0
to
bae83d3
Compare
Signed-off-by: Kun-Lu kun.lu@ibm.com
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.
Thank you for working on it.
scripts/build-all-in-one-image.sh
Outdated
## the other possible value, currently, is 'master' (or another branch name) | ||
if [[ $BRANCH == v* ]]; then | ||
COMPONENT_VERSION=$(echo ${BRANCH} | grep -Po "([\d\.]+)") | ||
MAJOR_MINOR=$(echo ${COMPONENT_VERSION} | awk -F. '{print $1"."$2}') |
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.
Every jaeger release has major, minor and patch components. This looks like patch isn't taken into consideration.
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.
The COMPONENT_VERSION
variable in L71
is actually for patch components, like 1.22.0
. I've changed the name of this variable to MAJOR_MINOR_PATCH
to avoid ambiguity.
scripts/build-all-in-one-image.sh
Outdated
if [[ $BRANCH == v* ]]; then | ||
COMPONENT_VERSION=$(echo ${BRANCH} | grep -Po "([\d\.]+)") | ||
MAJOR_MINOR=$(echo ${COMPONENT_VERSION} | awk -F. '{print $1"."$2}') | ||
MAJOR1=$(echo ${COMPONENT_VERSION} | awk -F. '{print $1}') |
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.
Should we use MAJOR instead of MAJOR1 ?
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.
Yes, I've updated the code here.
scripts/build-all-in-one-image.sh
Outdated
IMAGE_TAGS="${IMAGE_TAGS} --tag docker.io/${SNAPSHOT_TAG}" | ||
|
||
## for quay.io | ||
IMAGE_TAGS="${IMAGE_TAGS} --tag quay.io/${BASE_BUILD_IMAGE} --tag quay.io/${BUILD_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.
Do we need to add quay.io tags separately. Can we add as part of L83.
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.
Yes, we can. I've updated the script to combine them.
Makefile
Outdated
@@ -72,6 +72,17 @@ MOCKERY=mockery | |||
|
|||
.DEFAULT_GOAL := test-and-lint | |||
|
|||
BASE_IMAGE_MULTIARCH := localhost:5000/baseimg:$(VERSION)-$(shell echo $(ROOT_IMAGE) | tr : -) | |||
PLATFORMS=linux/amd64,linux/s390x | |||
INGESTER_TAG=$(subst JAGERCOMP,ingester,$(IMAGE_TAGS)) |
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.
Do we need to do this for each jaeger component, isn't it the same value after all ? Also where is JAGERCOMP variable populated ?
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.
Yes, I think we need to, since each jaeger component has its unique name, hence the unique tag value.
JAGERCOMP
is not a variable but a const string. We use it in the tag computation procedure in scripts/build-upload-docker-images.sh
to get the value of variable IMAGE_TAGS
. Then in Makefile
L77-L84
, we replace it with the name of each jaeger component in variable IMAGE_TAGS
to get each actual tag 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.
I am not a fan of these cross-dependences. If the string that contains JAGERCOMP
is defined in the script, why can't the script resolve it as needed, without pushing the substitution into Makefile?
Also, Jaeger is spelled with 'e'.
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.
Got it. I've corrected the spelling and moved the related code to the script.
cmd/all-in-one/Dockerfile
Outdated
@@ -28,6 +28,8 @@ EXPOSE 16686 | |||
COPY all-in-one-linux-$TARGETARCH /go/bin/all-in-one-linux | |||
COPY sampling_strategies.json /etc/jaeger/ | |||
|
|||
RUN chmod +x /go/bin/all-in-one-linux |
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.
Why are we adding this chmod here. Isn't the binary already executable ?
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.
Yes, you are right, the binary is already executable. I've removed this line from Dockerfile.
Remove chmod from all-in-one Dockerfile Optimize tag computation code Signed-off-by: Kun-Lu <kun.lu@ibm.com>
da1e074
to
c0de24a
Compare
@Ashmita152 Thank you very much for your review comments! I've responded to them and made corresponding code changes. Please take a look when you are available. |
scripts/build-all-in-one-image.sh
Outdated
@@ -45,17 +45,96 @@ upload_to_docker() { | |||
fi | |||
} | |||
|
|||
make build-all-in-one GOOS=linux GOARCH=$GOARCH | |||
upload_multiarch_to_docker() { |
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.
Why is there so much code just for all-in-one? I assume it would be the same for other Docker images, so let's keep a single copy.
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.
Yes, you're right. I've extracted the duplicate code from scripts/build-all-in-one-image.sh
and scripts/build-upload-docker-images.sh
and put them in newly created scripts.
@@ -6,9 +6,14 @@ on: | |||
pull_request: | |||
branches: [ master ] | |||
|
|||
jobs: | |||
jobs: |
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.
remove whitespace
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.
Whitespace removed.
Makefile
Outdated
@@ -72,6 +72,17 @@ MOCKERY=mockery | |||
|
|||
.DEFAULT_GOAL := test-and-lint | |||
|
|||
BASE_IMAGE_MULTIARCH := localhost:5000/baseimg:$(VERSION)-$(shell echo $(ROOT_IMAGE) | tr : -) | |||
PLATFORMS=linux/amd64,linux/s390x | |||
INGESTER_TAG=$(subst JAGERCOMP,ingester,$(IMAGE_TAGS)) |
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 am not a fan of these cross-dependences. If the string that contains JAGERCOMP
is defined in the script, why can't the script resolve it as needed, without pushing the substitution into Makefile?
Also, Jaeger is spelled with 'e'.
Makefile
Outdated
@@ -348,6 +361,73 @@ docker-images-only: docker-images-cassandra \ | |||
docker-images-tracegen \ | |||
docker-images-anonymizer | |||
|
|||
.PHONY: multiarch-docker |
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.
Couldn't we just move all this code into the script? Why do they need to be in the Makefile? I would restrict Makefile to building the binaries only, and then have all docker packaging etc. be done in the script.
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.
Yes, we could. I've moved all code related to building and uploading multi-arch docker images from Makefile to the scripts.
Makefile
Outdated
$(ANONYMIZER_TAG) \ | ||
cmd/anonymizer/ | ||
@echo "Finished building multiarch jaeger-anonymizer ==============" | ||
|
||
.PHONY: docker-push |
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.
we should delete this target, it's not used from GH Actions, and it probably incorrect given these changes.
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, I've deleted this target.
1. Extracted duplicate codes from build-all-in-one-image.sh and build-upload-docker-images.sh and put them in new scripts. 2. Moved the multi-arch images building and uploading codes from Makefile to scripts. 3. Removed some unused Makefile targets and extra whitespaces. Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Hi @yurishkuro , thank you so much for your review! I've made changes as per your comments, please take a look when you are available. Thanks again! |
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.
It's looking great. One minor feedback: Do we need to append for-multiarch-image
in the script names.
docker buildx build --output "${PUSHTAG}" \ | ||
--progress=plain --target release \ | ||
--build-arg base_image="localhost:5000/baseimg:1.0.0-alpine-3.12" \ | ||
--build-arg debug_image="golang:1.15-alpine" \ |
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.
We don't use this debug_image. We build that too ourselves like baseimg (eg. https://github.com/jaegertracing/jaeger/blob/master/scripts/build-all-in-one-image.sh#L66)
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.
Yes, you're right, we've noticed this. But building debugimg:1.0.0-golang-1.15-alpine
needs go-delve
which does not have s390x
support yet. So at this moment we only make release version of docker images (like all-in-one
, jaeger-agent
, jaeger-tracegen
, etc.) multi-arch and use debug_image="golang:1.15-alpine"
instead in build-arg
. In fact, the argument debug_image
is not used when building release version of images.
The debug version of images (like all-in-one-debug
or jaeger-agent-debug
) still use debug_image=localhost/debugimg:1.0.0-golang-1.15-alpine
and are amd64
specific.
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.
In fact, the argument debug_image is not used when building release version of images.
If this argument is not used, is it mandatory to pass it ? It just creates confusion, isn't it ?
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.
Yes, since the debug images also use the same Dockerfile, we need to keep this arg and pass it. Although it would create confusion at this moment, once go-delve
is supported by s390x
, we can use this arg for building multi-arch debug images. Otherwise we need to separate each of these Dockerfiles into two Dockerfiles, one for release
target, the other for debug
target.
--platform=${PLATFORMS} \ | ||
$(echo ${IMAGE_TAGS} | sed "s/JAEGERCOMP/es-index-cleaner/g") \ | ||
plugin/storage/es | ||
docker buildx build --output "${PUSHTAG}" \ |
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.
Can we make es-index-cleaner, es-rollover, tracegen and anonymizer build in one for loop like you did for jaeger components above.
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.
Yes, we can, I've updated the code to implement this.
|
||
## if we are on a release tag, let's extract the version number | ||
## the other possible value, currently, is 'master' (or another branch name) | ||
if [[ $BRANCH == v* ]]; 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.
I think we should use strict regex match here - something like
$BRANCH =~ ^v[0-9]+\.[0-9]+\.[0-9]+$
Otherwise any PR with branchname starting with letter v will match here
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.
Yes, you're right, I've updated the code to use strict regex match.
1. fix the indentation issues in build-upload-docker-images.sh 2. make other multi-arch images built in one for loop 3. use strict regex to match branch names 4. remove “for-multiarch-image” from script names Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
@Ashmita152 Thank you very much! I've removed |
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
@yurishkuro Could you please review this again? Thank you very much! |
@kun-lu20 could you please describe what kind of testing you did to make sure this all works? Ideally you would run this in your fork with a tag and override the target repos in the registries to your private ones, so that the end result are the images pushed to registries that can be validated. |
@yurishkuro Thanks for your response! Yes, each time before I committed the code changes, I would create a testing branch in our fork with the name |
|
||
# Only push multi-arch images to dockerhub/quay.io for master branch or for release tags vM.N.P | ||
if [[ "$BRANCH" == "master" || $BRANCH =~ ^v[0-9]+\.[0-9]+\.[0-9]+$ ]]; then | ||
echo "build multiarch images and upload to dockerhub/quay.io, BRANCH=$BRANCH" |
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 can see that in some places within this script you are using space while at other places you are using tab for indentation.
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've updated this script to use tab for indentation. Thanks.
scripts/compute-tags.sh
Outdated
|
||
set -exu | ||
|
||
###############Compute the tag |
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.
can we change this to just single #
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.
Yes, changes done.
scripts/compute-tags.sh
Outdated
|
||
IMAGE_TAGS="${IMAGE_TAGS} --tag docker.io/${SNAPSHOT_TAG} --tag quay.io/${SNAPSHOT_TAG}" | ||
|
||
################################ |
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.
would suggest to remove it.
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, removed
else | ||
docker_file_arg="" | ||
fi | ||
if [[ "${component}" =~ ^es-.* ]]; 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.
Won't this regex match for es-rollover too ? Is that intended ?
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.
This multiple if-else condition is making things confusing. Can we make it cleaner with either switch-case or some other way ?
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.
a simpler approach is to define a function with args like docker_file_arg and then call the function for each component with necessary args
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.
@Ashmita152 Yes, this regex match for both es-index-cleaner
and es-rollover
, since the value of their dir_arg
is the same (plugin/storage/es
).
The if-else
condition before this is specific for es-rollover
because it has an extra --file
argument.
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've separated out a function docker_buildx_build() in this script and call it for each component with necessary args. Thanks.
1. Add a function docker_buildx_build() in scripts/ build-upload-docker-images.sh 2. Use tab for indentation in build-upload-docker-images.sh 3. Remove extra '#' from compute-tags.sh Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Hi @yurishkuro , thanks for your suggestions! I believe we could use Since both the image building and uploading process could be done in one Keeping |
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Create shared script build-upload-a-docker-image.sh, other scripts could call it to build and upload multi-arch docker images. Signed-off-by: Kun-Lu <kun.lu@ibm.com>
docker/Makefile
Outdated
BASE_IMAGE := localhost/baseimg:$(VERSION)-$(shell echo $(ROOT_IMAGE) | tr : -) | ||
DEBUG_IMAGE := localhost/debugimg:$(VERSION)-$(shell echo $(GOLANG_IMAGE) | tr : -) | ||
BASE_IMAGE := localhost:5000/baseimg:$(VERSION)-$(shell echo $(ROOT_IMAGE) | tr : -) | ||
DEBUG_IMAGE := localhost:5000/debugimg:$(VERSION)-$(shell echo $(GOLANG_IMAGE) | tr : -) |
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.
Why do we encode the versions of base images instead of giving them a stable local name? When we include versions we're creating cross-script dependencies which require other scripts to be cognizant of the versions, e.g. base_debug_img_arg="--build-arg base_image=localhost:5000/baseimg:1.0.0-alpine-3.13 --build-arg debug_image=golang:1.15-alpine "
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.
Yes, you're right, I'll change the names of base image and debug image to stable local names. Thanks!
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.
Hi @yurishkuro , I've changed the names of baseimg
and debugimg
to stable local names. I still set the debug_image
arg to golang:1.15-alpine
for building multi-arch release version images in build-all-in-one-image.sh
and build-upload-docker-images.sh
, since the built debugimg
doe not support s390x
yet (due to lack of go-delve
support on s390x). For building amd64 debug version images, the debug_image
arg is set to the stable local name.
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.
Hi @yurishkuro , I've made some changes to remove cross-script dependencies of debug_image
image names. Please take a look when you are available. Thanks!
Change the names of base image and debug image to stable local names and update build-crossdock.sh Signed-off-by: Kun-Lu <kun.lu@ibm.com>
03c360e
to
5a6b268
Compare
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
1. Update docker/debug/Dockerfile to use `if statement` to support s390x 2. Use the stable local name for debugimg across all archs 3. Populate arg `base_debug_img_arg` with the stable local names directly in the shared script `build-upload-a-docker-image.sh` Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Hi @jpkrohling @yurishkuro , could you please have a look at the recent code changes? I am looking forward to hearing the comments from you. Thank you very much! |
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.
Looks OK to me, had only a couple of comments. But in general, I think we could think a bit about what the ideal world would be in terms of building both the binaries and the images. I feel like there are quite a few things that can be improved.
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
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. I'll give a couple of days for @yurishkuro and @Ashmita152 to review it again, in case they are interested. If I don't hear complaints from them, I'll merge this on Wed.
@kun-lu20, could you please rebase this PR? Looks like I can't rebase it myself. Once it's done, I'll merge this. |
Signed-off-by: Kun-Lu <kun.lu@ibm.com>
@jpkrohling Sure, I've merged the latest updates in branch |
Thanks @yurishkuro @jpkrohling ! |
This change is to add s390x support on multiarch Jaeger docker images
Signed-off-by: Kun-Lu kun.lu@ibm.com
Which problem is this PR solving?
Short description of the changes
ci-all-in-one-build.yml
,ci-docker-build.yml
,ci-release.yml
) to build and publish multi-arch docker images ondocker.io
andquay.io
.s390x
support on multi-arch docker images ofbaseimg
,all-in-one
and all the Jaeger components, except debug images andJaeger-Cassandra-Schema
whose base image doesn’t haves390x
support yet.baseimg
,all-in-one
image for local integration test.Makefile
andscripts/build-all-in-one-image.sh
, addsscripts/build-upload-docker-images.sh
to usedocker buildx
to build and publish multi-arch docker images.scripts/upload-all-docker-images.sh
since its uploading function has been implemented inscripts/build-upload-docker-images.sh
.ARG TARGETARCH
in Dockerfiles.s390x
).