push images with multi-arch to support amd64/arm64 #491
Conversation
| $(DOCKER_BUILD) '$(BUILD)' | ||
| container: sonobuoy | ||
| for arch in $(LINUX_ARCH); do \ | ||
| if [ $$arch = amd64 ]; then \ |
There was a problem hiding this comment.
given how different these two substitutions are, maybe just having seperate Dockerfiles makes more sense?
There was a problem hiding this comment.
hi @liztio
In my opinion, inconvenient maintenance in the later stage if use seperate Dockerfiles.
For example: if we need to add a new feature in Dockerfile, we should modify them twice.
3 differences between the two substitutions:
a, baseimage
b, command: In x86, we can't use any 'RUN' command in Dockerfile to build target for CROSS-platform(arm, power).
c, binary file: different binary files correspond to different platforms
Makefile
Outdated
| container: sonobuoy | ||
| for arch in $(LINUX_ARCH); do \ | ||
| if [ $$arch = amd64 ]; then \ | ||
| sed 's|BASEIMAGE|alpine:3.7|g' Dockerfile > Dockerfile-$$arch; \ |
There was a problem hiding this comment.
These sed commands don't seem to work on BSD sed:
sed: 1: "Dockerfile-amd64": extra characters at the end of D command
There was a problem hiding this comment.
Hi,
I have fixed it. Please check it.
Makefile
Outdated
| @@ -91,6 +127,31 @@ push: | |||
| $(DOCKER) push $(REGISTRY)/$(TARGET):latest; \ | |||
| fi | |||
|
|
|||
| push_manifest: | |||
| /go/bin/manifest-tool push from-args --platforms $(PLATFORMS) --template $(REGISTRY)/$(TARGET)-ARCH:$(VERSION) --target $(REGISTRY)/$(TARGET):$(VERSION) | |||
There was a problem hiding this comment.
Hi,
I have fixed it. Please check it.
Makefile
Outdated
|
|
||
| push: pre container | ||
| for arch in $(LINUX_ARCH); do \ | ||
| $(MAKE) push_images TARGET="sonobuoy-$$arch"; \ |
There was a problem hiding this comment.
An image does not exist locally with the tag: gcr.io/heptio-images/sonobuoy-amd64
When should these be created?
| @@ -12,11 +12,12 @@ | |||
| # See the License for the specific language governing permissions and | |||
| # limitations under the License. | |||
|
|
|||
| FROM golang:1.10-alpine | |||
| FROM BASEIMAGE | |||
There was a problem hiding this comment.
I think seperate Dockerfiles would be easier to reason about
There was a problem hiding this comment.
Inconvenient maintenance in the later stage if use seperate Dockerfiles as I think.
Also, same approach in k8s project: single Dockerfile, and use 'sed' to change it for each platform.
Makefile
Outdated
|
|
||
| GIT_VERSION ?= $(shell git describe --always --dirty) | ||
| IMAGE_VERSION ?= $(shell git describe --always --dirty) | ||
| IMAGE_BRANCH ?= $(shell git rev-parse --abbrev-ref HEAD | sed 's/\///g') | ||
| GIT_REF = $(shell git rev-parse --short=8 --verify HEAD) | ||
|
|
||
| GOBIN = /go/bin |
There was a problem hiding this comment.
I don't want to need to be root to run this script. GOBIN ?= /go/bin maybe? Are you building this on a machine that doesn't have go set up?
There was a problem hiding this comment.
hi Liztio,
I have updated it, please check it.
Thanks.
lubinsz
force-pushed
the
master
branch
2 times, most recently
from
de03800 to
fb55de6
Compare
|
@liztio |
Makefile
Outdated
| @@ -71,17 +81,49 @@ lint: | |||
| vet: | |||
| $(DOCKER_BUILD) '$(VET)' | |||
|
|
|||
| container: sonobuoy | |||
| pre: | |||
| curl -sSL https://github.com/estesp/manifest-tool/releases/download/$(MANIFEST_TOOL_VERSION)/manifest-tool-linux-amd64 > $(MANIFEST_TOOL_DIR)/manifest-tool | |||
There was a problem hiding this comment.
This doesn't work on OSX: /var/folders/v9/30xb9zqj20nf56xbvmz6qpym0000gn/T/tmp.cdp3AjCw/manifest-tool: /var/folders/v9/30xb9zqj20nf56xbvmz6qpym0000gn/T/tmp.cdp3AjCw/manifest-tool: cannot execute binary file
Why did you switch from go get?
There was a problem hiding this comment.
@liztio
"go get" is back. Please check it.
lubinsz
force-pushed
the
master
branch
2 times, most recently
from
1b247eb to
820d1e5
Compare
|
Hi there @lubinsz! It took me a little bit of fiddling this morning but I managed to get a test push to work. Because I work from osx, I needed to make the following change: diff --git a/Makefile b/Makefile
index 40a982d..965c831 100644
--- a/Makefile
+++ b/Makefile
@@ -129,7 +129,7 @@ push_images:
fi
push_manifest:
- $(GOPATH)/bin/manifest-tool push from-args --platforms $(PLATFORMS) --template $(REGISTRY)/$(TARGET)-ARCH:$(VERSION) --target $(REGISTRY)/$(TARGET):$(VERSION)
+ $(GOPATH)/bin/manifest-tool -username oauth2accesstoken --password "`gcloud auth print-access-token`" push from-args --platforms $(PLATFORMS) --template $(REGISTRY)/$(TARGET)-ARCH:$(VERSION) --target $(REGISTRY)/$(TARGET):$(VERSION)
push: pre container
for arch in $(LINUX_ARCH); do \If you could add that, we should be good to add this! |
Signed-off-by: Bin Lu <[email protected]>
|
@liztio |
|
Sorry for all the back and forth! We should be cutting v1.11.5 soon, and hopefully that'll be the first release that supports arm! |
|
@liztio Could we know when will the v1.11.5 release happen? Thanks! |
|
@kalyxin02 we recently release v1.11.6 due to an issue with .5, it should be live now, give it a go! |
|
@lubinsz I'm just looking into the makefile/images a bit since we need to update them for #609 Why did we need to use different base images at all? From my short trial, it seems like we can just use the basic I'm just wondering if I'll only notice the need in a certain situation; I dont actually have an arm64 device to actually test on either. |
Signed-off-by: Bin Lu [email protected]
What this PR does / why we need it:
add arm64 support
Which issue(s) this PR fixes
Special notes for your reviewer:
Release note: