-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
@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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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/push_image.sh
Outdated
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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. |
17dae92
to
779a99f
Compare
@gjtempleton any special reason to default to the -amd64 image instead of leaving it as is, pointing to the manifest? |
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. |
Can you squash commits to just 1? |
779a99f
to
ab2f8ec
Compare
Done. :) |
@detiber are you OK with the PR in the current shape? |
/lgtm |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/approve
[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 |
Thanks everyone for helping with this. |
…pstream-cluster-autoscaler-release-1.19 Automated cherry pick of #3714: Add build support for ARM64
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
andbuild-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