multiarch image pushing #84

pohly · 2020-05-25T16:06:23Z

What type of PR is this?
/kind feature

What this PR does / why we need it:

The previous PR (#82) had an issue (make invocation) and several opens. With the changes in this PR, push-multiarch integrates better with the rest of the build rules by removing duplication:

the same Go environment is used for multiarch and for testing
BUILD_PLATFORMS determines what is getting build instead
of hard-coding that in the make target
no separate Dockerfile.multiarch

Which issue(s) this PR fixes:
Related-to kubernetes-csi/csi-release-tools#86

Special notes for your reviewer:

The result of make push-multiarch PULL_BASE_REF=master REGISTRY_NAME=pohly BUILD_PLATFORMS="linux amd64; windows amd64 .exe; linux ppc64le -ppc64le; linux s390x -s390x; linux arm64 -arm64" can be found here: https://hub.docker.com/repository/docker/pohly/csi-node-driver-registrar [Update 2020-05-26: added arm64]

Does this PR introduce a user-facing change?:

Windows and multiarch Linux images are now available.
The Dockerfile must accept a "binary" build argument.

The entire parameter for make was used as target as-is because without some outer shell, argument splitting and variable expansion is gone. The gcloud invocation then fits better into cloudbuild.yaml again, where it belongs (it's only needed when pushing to GCR).

pohly · 2020-05-25T16:06:42Z

/cc @vitt-bagal

k8s-ci-robot · 2020-05-25T16:06:43Z

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

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 @vitt-bagal

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.

This removes code duplication: - the same Go environment is used for multiarch and for testing - BUILD_PLATFORMS determines what is getting build instead of hard-coding that in the make target - no separate Dockerfile.multiarch During the actual cloud build, BUILD_PLATFORMS is taken from the shared prow.sh. There's currently no support for overriding that per repo. We still need separate Dockerfiles for Linux and Windows because Windows has special requirements. In some lines, indention is changed to use tabs.

pohly · 2020-05-26T07:26:19Z

the same Go environment is used for multiarch and for testing

I need to take this back a bit: this is only true for local builds and Prow, but not for cloud build. There

node-driver-registrar/cloudbuild.yaml

Line 10 in cae4fbe

- name: 'gcr.io/k8s-testimages/gcb-docker-gcloud:v20200421-a2bf5f8'

determines which version of Go is used.

I think we should use the same approach as for Prow and install the desired Go version if it isn't available already.

By moving the check before building binaries, users get the warning immediately when doing sequential builds.

This introduces .cloudbuild.sh as the main entry point for building cloud images, similar to the role that .prow.sh has for Prow testing. The script sets up the build environment (gcloud auth, installing Go, parsing some of the env variables) before invoking "make push-multiarch", which (as before) is usable also outside of a cloud build.

pohly · 2020-05-26T08:45:45Z

I think we should use the same approach as for Prow and install the desired Go version if it isn't available already.

I've added that.

pohly · 2020-05-26T11:28:40Z

/assign @msau42

msau42 · 2020-05-26T20:58:48Z

cloudbuild.yaml

-    - PULL_BASE_REF=$_PULL_BASE_REF
+    - GIT_TAG=${_GIT_TAG}
+    - PULL_BASE_REF=${_PULL_BASE_REF}
+    - STAGING_PROJECT=${_STAGING_PROJECT}


Do we need a staging_project variable, or can we set REGISTRY directly?

You are right, setting REGISTRY_NAME directly is cleaner. I'm not even sure why we have _STAGING_PROJECT under substitutions.

@vitt-bagal: do you remember where that came from? Isn't that always k8s-staging-csi?

@vitt-bagal: do you remember where that came from? Isn't that always k8s-staging-csi?

It came from this commit. I think this can be removed safely if we set REGISTRY_NAME directly

If we can move cloudbuild.yaml into release-tools and just symlink to it from the root directory of a project (similar to .travis.yml -> release-tools/travis.yml), then it may make sense to have the substitution because different projects might have different staging projects.

msau42 · 2020-05-26T21:01:43Z

Makefile

+	dockerfile_linux=$$(if [ -e ./cmd/$*/Dockerfile ]; then echo ./cmd/$*/Dockerfile; else echo Dockerfile; fi); \
+	dockerfile_windows=$$(if [ -e ./cmd/$*/Dockerfile.Windows ]; then echo ./cmd/$*/Dockerfile.Windows; else echo Dockerfile.Windows; fi); \
+	build_platforms='$(BUILD_PLATFORMS)'; \
+	if ! [ "$$build_platforms" ]; then build_platforms="linux amd64"; fi; \


Can we set the default in L45?

$(BUILD_PLATFORMS) is expanded by make, so the usual shell mechanism (${var:-default}) does not work.

I've combined the two lines into one with:

if [ '$(BUILD_PLATFORMS)' ]; then build_platforms='$(BUILD_PLATFORMS)'; else build_platforms="linux amd64"; fi;

msau42 · 2020-05-26T21:05:42Z

Makefile

 	else \
-                       : "release image $(IMAGE_NAME):$(PULL_BASE_REF) already exists, skipping push"; \
+		       : "release image $(IMAGE_NAME):$(PULL_BASE_REF) already exists, skipping push"; \


should this be silently ignored, or return an error?

I haven't thought about this much. The whole if/else sequence here was copied originally from the corresponding "push" target and I just kept it. The original motivation for the check was that we directly push to the final destination, and those images should never be rebuilt because someone may already have pulled them. Here the situation is a bit different.

Do we expect that the cloud image build job will run more than once per tag? If no, then we can remove the "does the image exist" check because the image should never exist already. If yes, perhaps the build job was triggered intentionally again and the goal was to rebuild the image.

I'm leaning towards removing the check.

I think we should never repush an existing tag (except for canary), even if there was some error. We would have to resolve that scenario by incrementing the tag.

Okay, it's an error now.

pohly · 2020-05-27T18:47:08Z

@msau42: please take another look

msau42 · 2020-05-28T03:08:21Z

/lgtm
/approve

k8s-ci-robot · 2020-05-28T03:08:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/feature Categorizes issue or PR as related to a new feature. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels May 25, 2020

k8s-ci-robot requested review from msau42 and vladimirvivien May 25, 2020 16:06

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label May 25, 2020

pohly force-pushed the multiarch_image_tagging branch from c78c975 to cae4fbe Compare May 25, 2020 19:55

pohly changed the title ~~multiarch image pushing~~ WIP: multiarch image pushing May 26, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020

pohly added 2 commits May 26, 2020 10:39

cloud build: warn earlier about missing PULL_BASE_REF

597d425

By moving the check before building binaries, users get the warning immediately when doing sequential builds.

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 26, 2020

k8s-ci-robot assigned msau42 May 26, 2020

pohly changed the title ~~WIP: multiarch image pushing~~ multiarch image pushing May 26, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 26, 2020

msau42 reviewed May 26, 2020

View reviewed changes

This was referenced May 27, 2020

[WIP] Support for multi-arch #63

Closed

Moving images away from quay.io? kubernetes-csi/csi-release-tools#86

Closed

cloud build: simplify variable handling, return error when image exists

3018101

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 28, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 28, 2020

k8s-ci-robot merged commit 522b79b into kubernetes-csi:master May 28, 2020

This was referenced May 28, 2020

cloud build: fix shell syntax #85

Merged

cloud build: initial set of shared files kubernetes-csi/csi-release-tools#87

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

multiarch image pushing #84

multiarch image pushing #84

pohly commented May 25, 2020 •

edited by jsafrane

Loading

pohly commented May 25, 2020

k8s-ci-robot commented May 25, 2020

pohly commented May 26, 2020

pohly commented May 26, 2020

pohly commented May 26, 2020

msau42 May 26, 2020

pohly May 27, 2020

vitt-bagal May 27, 2020

pohly May 27, 2020

msau42 May 26, 2020

pohly May 27, 2020

msau42 May 26, 2020

pohly May 27, 2020

msau42 May 27, 2020

pohly May 27, 2020

pohly commented May 27, 2020

msau42 commented May 28, 2020

k8s-ci-robot commented May 28, 2020

multiarch image pushing #84

multiarch image pushing #84

Conversation

pohly commented May 25, 2020 • edited by jsafrane Loading

pohly commented May 25, 2020

k8s-ci-robot commented May 25, 2020

pohly commented May 26, 2020

pohly commented May 26, 2020

pohly commented May 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented May 27, 2020

msau42 commented May 28, 2020

k8s-ci-robot commented May 28, 2020

pohly commented May 25, 2020 •

edited by jsafrane

Loading