Skip to content

Conversation

@hakman
Copy link
Member

@hakman hakman commented Nov 23, 2020

I tried to keep existing target names to be as close as possible to existing ones and to build only AMD64 by default.
Similar targets with the -arch-% suffix were added and allow specifying the architecture, for example build-arch-amd64 and build-arch-arm64.

A new Dockerfile had to be added for ARM64 support and also a target named push-manifest for generating the multi-arch manifest after both AMD64 and ARM64 images are pushed.
Releasing both AMD64 and ARM64 is a bit awkward due to the About to push image/manifest. Are you sure? [y/N] input in push_image.sh.

The only major change is that both the binaries and the images are now suffixed with the arch name.

Fixes: #3419

/cc @MaciekPytel @gjtempleton

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 23, 2020
@hakman hakman changed the title Add build support for ARM64 [cluster-autoscaler] Add cluster-autoscaler build support for ARM64 Nov 23, 2020
@hakman hakman changed the title [cluster-autoscaler] Add cluster-autoscaler build support for ARM64 [cluster-autoscaler] Add build support for ARM64 Nov 23, 2020
@mwielgus
Copy link
Contributor

@gjtempleton Are there any implications of this pr to Helm charts/releases?

# See the License for the specific language governing permissions and
# limitations under the License.
ARG BASEIMAGE=gcr.io/distroless/static:latest
ARG BASEIMAGE=gcr.io/distroless/static:latest-amd64
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't this make use of an additional ARG for the architecture to avoid having to have two separate docker files?

Copy link
Member Author

Choose a reason for hiding this comment

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

The ARG would work here, but not further when copying the binary. Not sure if there's a good way to not add both ARG and ENV var. Separate Dockerfile seemed cleaner.

Comment on lines 39 to 42
else
DOCKER_CLI_EXPERIMENTAL=enabled "${docker_push_cmd[@]}" manifest create "${IMAGE_TO_PUSH}" "$@"
DOCKER_CLI_EXPERIMENTAL=enabled "${docker_push_cmd[@]}" manifest push --purge "${IMAGE_TO_PUSH}"
fi
Copy link
Contributor

Choose a reason for hiding this comment

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

Unless things have changed recently, this would require that the arch specific images already be pushed, in which case I think it might make sense to handle the manifest creation/pushing in make rather than here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The push process is a bit strange, as it asks for confirmation that you want to push the image. Not sure if still needed considering that after promoting images from k8s.io they cannot be changed anymore.
I think you are right and the manifest doesn't need these checks as it just uses the already pushed images.

@gjtempleton
Copy link
Member

@gjtempleton Are there any implications of this pr to Helm charts/releases?

Not at this point, no. The next release bumping the default image after this being merged would need to change the image default to the newly published amd64 image, but nothing blocking this going ahead from the helm side.

@hakman hakman force-pushed the multi-arch-support branch from 17dae92 to 779a99f Compare November 25, 2020 17:43
@hakman
Copy link
Member Author

hakman commented Nov 25, 2020

Not at this point, no. The next release bumping the default image after this being merged would need to change the image default to the newly published amd64 image, but nothing blocking this going ahead from the helm side.

@gjtempleton any special reason to default to the -amd64 image instead of leaving it as is, pointing to the manifest?

@hakman
Copy link
Member Author

hakman commented Nov 25, 2020

Thanks for the review @detiber. I added most of the suggested changes, but there are a few things that would need to be discussed, especially the way the push image script works and if it needs to change.

@hakman hakman requested a review from detiber November 25, 2020 17:49
@mwielgus
Copy link
Contributor

Can you squash commits to just 1?

@hakman hakman force-pushed the multi-arch-support branch from 779a99f to ab2f8ec Compare November 26, 2020 11:21
@hakman
Copy link
Member Author

hakman commented Nov 26, 2020

Can you squash commits to just 1?

Done. :)

@mwielgus
Copy link
Contributor

@detiber are you OK with the PR in the current shape?

@detiber
Copy link
Contributor

detiber commented Nov 30, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 30, 2020
Copy link
Contributor

@mwielgus mwielgus left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hakman, mwielgus

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:

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 k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2020
@k8s-ci-robot k8s-ci-robot merged commit 7a0731b into kubernetes:master Dec 1, 2020
@hakman
Copy link
Member Author

hakman commented Dec 1, 2020

Thanks everyone for helping with this.
I don't know the cherry-picking policies for cluster-autoscaler, but would like to know if there's any chance to cherry-pick this to 1.19. It should not change in any way the current functionality.

@hakman hakman deleted the multi-arch-support branch December 1, 2020 10:31
k8s-ci-robot added a commit that referenced this pull request Dec 21, 2020
…pstream-cluster-autoscaler-release-1.19

Automated cherry pick of #3714: Add build support for ARM64
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

cluster-autoscaler for arm64

5 participants