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.

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

[cluster-autoscaler] Add build support for ARM64 #3714

Merged
merged 1 commit into from
Dec 1, 2020

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?

@@ -11,9 +11,9 @@
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# 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
Member

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.

cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Outdated Show resolved Hide resolved
cluster-autoscaler/Makefile Show resolved Hide resolved
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
Member

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
Member

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