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

Add support for CPU variants #1676

Merged

Conversation

Silvanoc
Copy link
Contributor

Signed-off-by: Silvano Cirujano Cuesta silvano.cirujano-cuesta@siemens.com
Inspired-by: mickkael 19755421+mickkael@users.noreply.github.com

Fixes #1591

Description

Adds the option to build an image with a CPU variant for the customplatform option. Look for variant here for the meaning of CPU variant: https://github.com/opencontainers/image-spec/blob/master/image-index.md#image-index-property-descriptions.

The CPU variant specification cannot be included in the resulting image due to a limitation of the OCI-image specification. The effect of this PR is that in the case of multi-arch base images the right architecture and CPU variant gets used.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

  • Includes unit tests
  • Adds integration tests if needed.

See the contribution guide for more details.

Reviewer Notes

  • The code flow looks good.
  • Unit tests and or integration tests added.

Release Notes

Describe any changes here so maintainer can include it in the release notes, or delete this block.

- kaniko supports cpu variants in custom platform, like `--customPlatform=linux/arm/v5`

Signed-off-by: Silvano Cirujano Cuesta <silvano.cirujano-cuesta@siemens.com>
Inspired-by: mickkael 19755421+mickkael@users.noreply.github.com
@google-cla
Copy link

google-cla bot commented Jun 18, 2021

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no CLA not signed by all commit authors label Jun 18, 2021
@mickkael
Copy link
Contributor

I had tried the same, but with the build included, which fails because of the OCI limitation.
mickkael@ac6fc1f

remote only is a good compromise for now. Else qemu is likely a solution for armv6, but armv5's support is limited. (https://hub.docker.com/r/tonistiigi/binfmt)

@Silvanoc
Copy link
Contributor Author

remote only is a good compromise for now. Else qemu is likely a solution for armv6, but armv5's support is limited. (https://hub.docker.com/r/tonistiigi/binfmt)

As mentioned in this issue comment, I mostly focus on the base image (sub)issue, because it's the one we're stumbling upon now.

In fact I'd propose to split #1591 in two:

  1. No support for CPU variant (like v7 for ARM) on customPlatform builds #1591 keeps focused on ensuring that the right base image gets used
  2. A new issue focused on the missing support for CPU variant in the OCI-image manifest, this one mostly keeping track of Add CPU variant to image config opencontainers/image-spec#777. This would be a long-running issue that will probably take long until being supported at all levels, some projects decide not to track them as issues, I don't know how Kaniko is tracking them.

I consider that either a subset of your patch or mine should fix #1591 (after the split).

  • If you prefer to have contributions from different companies, you just have to wait for me to get the signing of the CLA clarified.
  • Otherwise taking only the part of your patch affecting remote would also do it.

I don't see in the link provided by you any information about qemu-user having issues with ARMv5. An issue with Qemu v5 is being reported, but it doesn't have anything to do with ARMv5. Although don't see ARMv5 reported as supported in that repo, but "binfmt_misc" doesn't require it for working (in fact using a container to configure the host can trigger ugly race conditions that have costed us hours, I would discourage from doing so on systems that need to use binfmt_misc concurrently). And as long as I don't see a Qemu reporting about it, I consider it a limitation of that concrete set-up.

@mickkael
Copy link
Contributor

To install qemu at built time, I use the image tonistiigi/binfmt , this is also the default in the github action set-qemu, so it's widely used.
This image supports only arm/v7 and arm/v6

@Silvanoc
Copy link
Contributor Author

To install qemu at built time, I use the image tonistiigi/binfmt , this is also the default in the github action set-qemu, so it's widely used.
This image supports only arm/v7 and arm/v6

Kaniko shouldn't be anyway concerned about how the user gets the needed host support to build the image. It should only support getting the right base image.

The Docker Hub official Debian image is available for ARMv5 and ARMv7 and they rely on completely different packages (being "armel" and "armhf" the respective names). If Kaniko isn't capable of getting the right one, then no matter how much I do on my host, it won't work.

QEMU is capable of handling both ARMv5 and ARMv7 (I haven't tested if the resulting artifacts also run properly on real hardware though), therefore I still see the limitation on Kaniko not pulling the right base image. And that's what I'd like first to overcome.

@Silvanoc
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes CLA signed by all commit authors and removed cla: no CLA not signed by all commit authors labels Jun 24, 2021
@Silvanoc
Copy link
Contributor Author

@mickkael the build pipeline fails due to some credential issues that I don't know how to fix, could you help me on that?

@mickkael
Copy link
Contributor

all the other PR are failing the same test... it isn't your code causing the issue.

@Silvanoc
Copy link
Contributor Author

Silvanoc commented Jul 1, 2021

all the other PR are failing the same test... it isn't your code causing the issue.

So what does it mean for this PR? It will be reviewed somewhere in time? I shouldn't expect any merging until those CI issues are resolved?

@tejal29
Copy link
Member

tejal29 commented Jul 8, 2021

Sorry folks, due to some internal project organization, tests accessing a particular image are failing.
Will fix it.

@tejal29 tejal29 merged commit 1d9bc17 into GoogleContainerTools:master Jul 8, 2021
@Silvanoc
Copy link
Contributor Author

@tejal29 do you have any plans to do a release in the upcoming future? Just to estimate when this patch will flow into a release. Thanks

@Silvanoc
Copy link
Contributor Author

@tejal29 do you have any plans to do a release in the upcoming future? Just to estimate when this patch will flow into a release. Thanks

I'd love to see a new release as demanded in #1740 including this patch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes CLA signed by all commit authors
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No support for CPU variant (like v7 for ARM) on customPlatform builds
3 participants