Tagging of multiarch images on gcr

[vitt-bagal]

What type of PR is this?
/kind feature

What this PR does / why we need it:
This PR will push canary image for master branch and vX.Y.Z image depending on tag.

@pohly Could you please review ?

[k8s-ci-robot]

Hi @vitt-bagal. Thanks for your PR.

I'm waiting for a kubernetes-csi member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[vitt-bagal]

/cc @pohly @namrata-ibm @dims

[k8s-ci-robot]

@vitt-bagal: GitHub didn't allow me to request PR reviews from the following users: namrata-ibm, dims.

Note that only kubernetes-csi members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc pohly namrata-ibm dims

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[pohly]

I like that this moves the build rules into the Makefile, that'll make it easier to share them between repos and just have the bare minimum cloudbuild.yaml in each repo separately.

However, we cannot merge a modified release-tools/build.make into node-driver-registrar because "no local modifications in release-tools" is enforced by a test. Instead, we need to do one more round of experimentation where the new push-multiarch is put into the top-level Makefile. If that works, we can copy it to csi-release-tools.

[pohly]

That comment can be removed.

[vitt-bagal]

removed comment

[pohly]

I don't think we need a separate MULTIARCH_IMAGE_NAME. Just use IMAGE_NAME and make sure that cloudbuild.yaml overrides it with make push-multiarch REGISTRY_NAME=gcr.io/k8s-staging-csi.

[vitt-bagal]

Removed MULTIARCH_IMAGE_NAME extra variable. Added REGISTRY_CI env variable in cloudbuild to override REGISTRY_NAME value in makefile

[pohly]

Why this line?

[vitt-bagal]

make build for windows is required as Windows binary is needed to copy it in windows dockerfile

[pohly]

This doesn't do anything, the variable is getting set and the shell in which it was set exits. You need to join the lines with ; \:

push-multiarch-%:
    gcloud auth configure-docker
    set -ex; \
    DOCKER_CLI_EXPERIMENTAL=enabled; \
    export DOCKER_CLI_EXPERIMENTAL; \
    docker buildx create --use --name multiarchimage-buildertest; \
    pushMultiArch () { \

I intentionally avoided export DOCKER_CLI_EXPERIMENTAL=enabled because that only works in bash.

[pohly]

We loose the ability here to build different images in the same repo with different Docker files. Can you add something similar to build-% where it locates the Docker file with $(shell if [ -e ./cmd/$*/Dockerfile ]; then echo ./cmd/$*/Dockerfile; else echo Dockerfile; fi)?

[vitt-bagal]

Updated docker buildx build command using approach of locating file.

[pohly]

For canary images, the rev label used to specify the source code revision (= result of git describe). This is useful to determine what the image contains because there is no other indicator.

Can this be done also for cloud builds? If we override REV in cloudbuild.yaml, then we can simply use --label rev=$(REV) here.

[vitt-bagal]

As rev value from git describe command(build.make) is same as tag-n-hash value from _GIT_TAG( vYYYYMMDD-tag-n-hash form). Added code to extract tag-n-hash value from REV(REV_CI cloudbuild env)in push-multiarch target and passing this as label value to docker build.

[pohly]

Introducing another name for PULL_BASE_REF IMHO obscures what's going on. Better just use PULL_BASE_REF directly below.

[vitt-bagal]

Removed RELEASE_ALIAS_TAG. Using PULL_BASE_REF directly in push-multiarch target

[pohly]

This can be removed now, right?

[vitt-bagal]

yes

[pohly]

Instead of exporting an unused VERSION env variable, can we use this to override the useless REV make variable, like this:

make push-multiarch 'REV=$_GIT_TAG'

Or do substitutions only work under env?

[vitt-bagal]

Added changes in Makefile to override REV value with REV_CI value passed as env in cloudbuild

do substitutions only work under env?

I am also not sure about how substitutions works. May be you can tag someone who can comment on this

[pohly]

IMHO this is better done locally in push-multiarch-%.

[vitt-bagal]

Added this in push-multiarch target

[vitt-bagal]

@pohly Thanks for review comment. Added few changes as per your suggestions.

[pohly]

Is this specific to the cloud build? Then put it into cloudbuild.yaml and explain what it does.

[vitt-bagal]

Moved rev calculation to cloudbuild.yaml

[pohly]

Can you do this without REGISTRY_CI and REV_CI? The goal is to write a make target that can be copied 1:1 into release-tools/build.make, with as few special cases as possible.

You can use REV and REGISTRY_NAME and override them with make REV=... REGISTRY_NAME=... in the cloudbuild.yaml invocation.

[vitt-bagal]

Removed extra CI variables. Updated push-multiarch invocation as make push-multiarch REV=v$(echo $_GIT_TAG | cut -f3- -d 'v') REGISTRY_NAME=gcr.io/k8s-staging-csi in cloudbuild

[pohly]

This needs further work before it's ready for csi-release-tools, but should be good enough for another test run.

/ok-to-test
/approve
/lgtm

[pohly]

I'm still not sure what this make invocation is needed for.

Ah, it's because Dockerfile.Windows doesn't build its binary. We probably should fix this and ideally remove the special Dockerfile for Windows entirely. But this can probably wait until later. We have to sort it out before including it in csi-release-tools.

@ddebroy: is there a chance to use gcr.io/distroless/static:latest as base for a Windows image?

[jingxu97]

In order to use buildx to build windows image on a linux machine, we cannot include any RUN command in the Dockerfile, so I think this limitation means we cannot build binary as a step in Dockerfile.

[pohly]

Perhaps create a local variable dockerfile=$(shell if [ -e ./cmd/$*/Dockerfile.multiarch ]; then echo ./cmd/$*/Dockerfile.multiarch; else echo Dockerfile.multiarch; fi) above and use it here and below?

[vitt-bagal]

I think we can use this once windows is also a part of Dockerfile.multiarch. otherwise we have to set an extra variable for windows dockerfile too.

[msau42]

Any way we can make the list of linux platforms as a variable instead of hard coding the command for each platform?

Unfortunately I don't think there's much we can do to generalize windows atm.

[vitt-bagal]

Currently with the help of this PR we are just testing that whether push-multiarch (keeping cloudbuild minimal) can be used to correctly tag multiarch images.
Once all these changes are working fine as expected then we can modify docker buildx command as docker buildx build --push -t $(IMAGE_NAME):$$tag --platform=linux/amd64,linux/s390x -f ... which will have single docker buildx command for linux (with multiaple arch)

[pohly]

Have you tried doing this with Dockerfile.multiarch instead of Dockerfile.Windows?

You must extract the target OS from TARGETPLATFORM instead of hard-coding linux and the .exe suffix may need special handling.

[vitt-bagal]

Have you tried doing this with Dockerfile.multiarch instead of Dockerfile.Windows?

I haven't tried this. We can't test windows images at our end. May be @jingxu97 can try this out.

Ideally instead of writing different docker buildx build command for each arch build and then creating manifest for them, single command with --push --platform=linux/amd64,linux/s390x should work. However I feel we can try this out later, once current changes are working as expected.

[jingxu97]

Since Dockerfile.multiarch contains a 'RUN' command, we cannot use it to build windows image on a linux machine with buildx.

[pohly]

So we can build Windows binaries on the Linux host, but not inside a Docker container? Or does building for Windows need a Windows host?

[pohly]

/release-note-none

[pohly]

In https://github.com/kubernetes-csi/node-driver-registrar/pull/82/files/3e9e449f9f279c0c6e0859c1bce7c9b4b0007141#r425692239, we were discussing how to unify handling of Windows and Linux images. @jingxu97 pointed out that "Since Dockerfile.multiarch contains a 'RUN' command, we cannot use it to build windows image on a linux machine with buildx.". I was then wondering what we can use.

I tried it: we can and do cross-compile Windows binaries on a linux/amd64 host, just as we do for all other currently supported platforms (https://github.com/kubernetes-csi/csi-release-tools/blob/9084fecb84ddb47af8360d9ebe4f48a6b7a65c1d/prow.sh#L88).

As we are stuck with cross-compilation (correct, @jingxu97?), I am not sure whether we need to use "docker buildx". While in theory it may be nicer (building for multiple platforms with one instruction), we are not taking advantage of that and may as well do something like https://github.com/MarcusWichelmann/PrometheusSolarExporter/blob/3f2ee21b071af79a741cd1fefc02cba74c9bf166/.github/workflows/build.yml#L48-L65

@vitt-bagal, agreed that this could work?

However, one open question is whether can use the same base image or need something special for Windows. Is "gcr.io/distroless/static:latest" just for Linux and we have to use
"mcr.microsoft.com/windows/nanoserver:1809" as base for Windows?

[pohly]

I tried building multi-arch images with a local Docker registry, but I am running into docker/cli#2396.

[vitt-bagal]

I tried building multi-arch images with a local Docker registry, but I am running into docker/cli#2396.

@pohly Even i also got the same issue with local docker registry. You can try with dockerhub.

[namrata-ibm]

As we are stuck with cross-compilation (correct, @jingxu97?), I am not sure whether we need to use "docker buildx". While in theory it may be nicer (building for multiple platforms with one instruction), we are not taking advantage of that and may as well do something like https://github.com/MarcusWichelmann/PrometheusSolarExporter/blob/3f2ee21b071af79a741cd1fefc02cba74c9bf166/.github/workflows/build.yml#L48-L65

@vitt-bagal, agreed that this could work?

@pohly Docker buildx adds correct manifest for mutiarch images as we are using distroless image in Dockerfile. There are issues using manifest commands, for more reference: bazelbuild/rules_docker#300 (comment)

However, one open question is whether can use the same base image or need something special for Windows. Is "gcr.io/distroless/static:latest" just for Linux and we have to use
"mcr.microsoft.com/windows/nanoserver:1809" as base for Windows?

Distroless seems to be for Linux only: https://github.com/GoogleContainerTools/distroless/tree/master/base#image-contents

@pohly how about testing Linux and Windows through different commands for now, considering various open issues?

[msau42]

This is going to require gcloud? Can we put this in the cloudbuild.yml instead?

[vitt-bagal]

Earlier gcloud command was a part of cloudbuild only but later we have moved it to make target in order to keep cloudbuild simple and minimal.

[msau42]

I think it would be preferable to keep gcloud command outside of this otherwise anybody that wants to build a multiarch image is going to require gcloud to be installed.

If you still want put it in the makefile to reduce the commands in cloudbuild, then I would maybe wrap "push-multiarch" with a "cloudbuild-push-multiarch"

[pohly]

I agree with @msau42 that this belongs into cloudbuild.yml. push-multiarch-% should be independent of the registry. cloudbuild.yml is where we prepare for pushing to gcr.

[msau42]

Any way we can make the list of linux platforms as a variable instead of hard coding the command for each platform?

Unfortunately I don't think there's much we can do to generalize windows atm.

[msau42]

For making progress on testing, this lgtm. My comments about making this more extensible can be addressed as a followup.
/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: msau42, pohly, vitt-bagal

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[vitt-bagal]

@msau42 @pohly Latest image push job failed. I could not see the build logs. Could you please help me with the build logs ?

[pohly]

 *** No rule to make target 'push-multiarch REV=v$(echo v20200521-v1.3.0-34-g4dbc271e | cut -f3- -d 'v') REGISTRY_NAME=gcr.io/k8s-staging-csi'.  Stop.

The problem is the usage of "make" as entry point for the build. Variable expansion and splitting at spaces don't work when doing that. Obvious in retrospect, right? 😝

This might fix it (untested):

diff --git a/Makefile b/Makefile
index 32517b63..b8785ef8 100644
--- a/Makefile
+++ b/Makefile
@@ -26,7 +26,6 @@ include release-tools/build.make
 push-multiarch-%:
 	make BUILD_PLATFORMS="windows amd64 .exe"
 	set -ex; \
-	gcloud auth configure-docker; \
 	DOCKER_CLI_EXPERIMENTAL=enabled; \
 	export DOCKER_CLI_EXPERIMENTAL; \
 	docker buildx create --use --name multiarchimage-buildertest; \
diff --git a/cloudbuild.yaml b/cloudbuild.yaml
index e0d069df..00ab0287 100644
--- a/cloudbuild.yaml
+++ b/cloudbuild.yaml
@@ -8,13 +8,17 @@ options:
   substitution_option: ALLOW_LOOSE
 steps:
   - name: 'gcr.io/k8s-testimages/gcb-docker-gcloud:v20200421-a2bf5f8'
-    entrypoint: make
+    entrypoint: sh
     env:
     - PULL_BASE_REF=$_PULL_BASE_REF
     - HOME=/root
     args:
-    # Extract tag-n-hash value from _GIT_TAG(form vYYYYMMDD-tag-n-hash) for REV value
-    - push-multiarch REV=v$(echo $_GIT_TAG | cut -f3- -d 'v') REGISTRY_NAME=gcr.io/k8s-staging-csi
+    - -e
+    - -c
+    - |
+      gcloud auth configure-docker
+      # Extract tag-n-hash value from _GIT_TAG(form vYYYYMMDD-tag-n-hash) for REV value
+      make push-multiarch REV=v$(echo $_GIT_TAG | cut -f3- -d 'v') REGISTRY_NAME=gcr.io/k8s-staging-csi
 substitutions:
   # _GIT_TAG will be filled with a git-based tag for the image, of the form vYYYYMMDD-hash, and
   # can be used as a substitution