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.

Sign up for GitHub

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

Jump to bottom

*: add multi-arch support in release binaries #7714

Merged
merged 5 commits into from
Apr 21, 2017
Merged

*: add multi-arch support in release binaries #7714

merged 5 commits into from
Apr 21, 2017

Conversation

glevand
Copy link
Contributor

@glevand glevand commented Apr 12, 2017

No description provided.

heyitsanthony
heyitsanthony reviewed Apr 12, 2017
View reviewed changes
scripts/release.sh

for TARGET_ARCH in "amd64" "arm64" "ppc64le"; do
echo Building ${TARGET_ARCH} docker image...
GOARCH=${TARGET_ARCH} BINARYDIR=release/etcd-${VERSION}-linux-${TARGET_ARCH} BUILDDIR=release ./scripts/build-docker ${VERSION}
Copy link
Contributor

@heyitsanthony heyitsanthony Apr 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I was under the impression it's not possible to build docker images on a mismatched architecture. Does this work because the Dockerfile doesn't use RUN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, if the Dockerfile just uses ADD it works. If we want to use RUN, then I think we could setup QEMU user emulation to do that, but that is significantly more complicated and would add to the requirements of the host.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK, great! glad it's possible to avoid that whole qemu mess

@glevand
Copy link
Contributor Author

glevand commented Apr 12, 2017

Rebased to latest, renamed patch to satisfy travis-ci check.

@glevand glevand mentioned this pull request Apr 13, 2017
Merged
@glevand
Copy link
Contributor Author

glevand commented Apr 13, 2017

All the CI stuff passed except the jenkins-proxy-ci with --- FAIL: TestRestartMember (16.86s), which seems irrelevant to this PR.

heyitsanthony
heyitsanthony reviewed Apr 13, 2017
View reviewed changes
Documentation/v2/dev/release.md Outdated
@@ -70,7 +70,7 @@ cd release
# personal GPG is okay for now
for i in etcd-*{.zip,.tar.gz}; do gpg --sign ${i}; done
# use `CoreOS ACI Builder <release@coreos.com>` secret key
gpg -u 88182190 -a --output etcd-${VERSION}-linux-amd64.aci.asc --detach-sig etcd-${VERSION}-linux-amd64.aci
gpg -u 88182190 -a --output etcd-${VERSION}-linux-${arch}.aci.asc --detach-sig etcd-${VERSION}-linux-${arch}.aci
Copy link
Contributor

Choose a reason for hiding this comment

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

for aci in etcd-${VERSION}.*.aci; do gpg -u 88182190 -a --output ${aci}.asc --detach-sig ${aci}; done

?

scripts/build-docker
@@ -24,17 +27,18 @@ fi

if [ ${ARCH} != "amd64" ]; then
DOCKERFILE+=".${ARCH}"
TAG+="-${ARCH}"
VERSION+="-${ARCH}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this the usual way to do multi arch? Won't this break ${TAG}:latest?

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 was following the presence set by flannel: https://quay.io/repository/coreos/flannel?tab=tags
I guess then we could have ${TAG}:latest-${arch}.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

Dockerfile-release.arm64
EXPOSE 2379 2380

# Define default command.
CMD ["/usr/local/bin/etcd"]
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be tagged as arm64 or aarch64? is there an official convention yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is totally open.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

@glevand
Copy link
Contributor Author

glevand commented Apr 14, 2017 •
edited
Loading

Rebased to latest, added for loop in release.md.

@heyitsanthony
Copy link
Contributor

lgtm, but maybe squash some of the build-docker patches if possible? Thanks!

@glevand
Copy link
Contributor Author

glevand commented Apr 14, 2017

Squashed build-docker patches into build-docker: Updates for multi-arch release.

@codecov-io
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (master@cfbc5e5). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##             master   #7714   +/-   ##
========================================
  Coverage          ?   75.9%           
========================================
  Files             ?     331           
  Lines             ?   25998           
  Branches          ?       0           
========================================
  Hits              ?   19733           
  Misses            ?    4845           
  Partials          ?    1420

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cfbc5e5...397e50e. Read the comment docs.

@glevand
build-binary: Add arm64
fb62872 
Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
build-aci: Add multi arch support
8309ca9 
Uses GOARCH to build for a targeted arch.

Usage: GOARCH=... BINARYDIR=... BUILDDIR=... ./scripts/build-aci version

Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
build-docker: Updates for multi-arch release
0c8988a 
 o Set -e to abort script if a command fails.
 o Allow custom docker 'TAG' from the environment.
 o Move arch suffix to version to allow all images to
   be put into a single repository.
 o Enable cross builds.  When doing cross builds where the
   host and target architectures are different 'RUN mkdir'
   will fail since the target container cannot be run on
   the host.  To work around this, create the directories
   in build-docker, then use ADD in the Dockerfile.
 o Add Dockerfile-release.arm64

Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
release: Add multi arch support
07c07ce 
Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
release.md: Update for multi arch release
bf98718 
Signed-off-by: Geoff Levand <geoff@infradead.org>
@glevand
Copy link
Contributor Author

glevand commented Apr 21, 2017 •
edited
Loading

Rebased to latest. Is there anything else this needs to be merged?

@heyitsanthony
Copy link
Contributor

/cc @xiang90

@xiang90
Copy link
Contributor

xiang90 commented Apr 21, 2017

LGTM

@xiang90 xiang90 merged commit a9087ee into etcd-io:master Apr 21, 2017
@glevand glevand deleted the for-merge-cross branch April 21, 2017 18:25
@gyuho gyuho changed the title Add multi arch release support *: add multi-arch support in release binaries Apr 25, 2017
@glevand glevand mentioned this pull request Jun 12, 2017
Closed
@vielmetti vielmetti mentioned this pull request Dec 13, 2018
Closed
5 tasks
@lubinszARM lubinszARM mentioned this pull request Apr 9, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heyitsanthony heyitsanthony heyitsanthony approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@glevand @heyitsanthony @codecov-io @xiang90 @gyuho