-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 arm64/v8 support for krte image #24783
Conversation
Hi @ruquanzhao. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: ruquanzhao The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cc @chendave |
/ok-to-test |
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.
Have your verified with gcb?
images/krte/variants.yaml
Outdated
@@ -1 +0,0 @@ | |||
../kubekins-e2e/variants.yaml |
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.
Can you pls explain why you remove this link and create a new one later?
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.
I noticed the bootstrap image is DEPRECATED, and kubekins-e2e based on bootstrap.
So, I thought it's a better way to maintain two variants.yaml.
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.
this is reverting a previous intentional change (which admittedly I didn't like), but is completely orthogonal to this PR. this change, if merged, should be a different PR.
I don't think arm64 should be a variant anyhow. we should just make the tags multi-arch if we're going to do this.
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.
using bootstrap directly is deprecated, but kubekins-e2e is the majority of kubernetes CI.
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.
@BenTheElder thanks for the comments! let's see if we can make this change more clear to follow your suggestion!
@@ -1,23 +1,29 @@ | |||
timeout: 1800s | |||
steps: | |||
- name: gcr.io/cloud-builders/docker | |||
- name: gcr.io/k8s-testimages/gcb-docker-gcloud |
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.
Is this the new way to build the multi-arch image?
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.
refers to images/git/cloudbuild.yaml
There is a serial of patches to make the test-infra/images support for multi-arch, see the relevant change of this issue - #16588. And per our evaluation, this krte image is one of the bits that are not ignorable. Might you take a look at this and let us know your thoughts? |
Yes. And here is the log of arm64.
Full build log in https://pastebin.ubuntu.com/p/4WSskkNXg3/ |
images/krte/cloudbuild.yaml
Outdated
- --build-arg=UPGRADE_DOCKER_ARG=$_UPGRADE_DOCKER | ||
- --build-arg=IMAGE_ARG=gcr.io/$PROJECT_ID/krte:$_GIT_TAG-$_CONFIG | ||
- . | ||
- build |
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.
Is the indent here and below necessary?
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.
Thanks for your review!
It's not necessay, I have removed it.
Signed-off-by: Ruquan Zhao ruquan.zhao@arm.com
@@ -30,6 +36,3 @@ substitutions: | |||
_CFSSL_VERSION: R1.2 | |||
options: | |||
substitution_option: ALLOW_LOOSE | |||
images: | |||
- 'gcr.io/$PROJECT_ID/krte:$_GIT_TAG-$_CONFIG' | |||
- 'gcr.io/$PROJECT_ID/krte:latest-$_CONFIG' |
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.
why those lines are removed?
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.
refer to #22444
Dependence of |
I'm still missing the why, I mean we could build for arbitrarily many architectures but building them is not free, and this image is used to test kind on Kubernetes's CI, and isn't supported for other users. What's the use case? I don't want to add build time and complexity and pay to store more image contents without a concrete use case, most of these CI images are amd64 because SIG k8s infra provides amd64 clusters to host CI and these images are just used as the project's CI environment. See also the image's readme: https://github.com/kubernetes/test-infra/blob/master/images/krte/README.md#warning
OK, I see that these PRs exist, but in #16588 the conversation settled that while redhat needed the core prow components (the "pod utilities") to be multi-arch, there was no decision regarding the rest. My experience so far has been that making docker images multi-arch "just because" causes CI flakes and much slower builds due to the penalty of building under qemu, and costs far more energy than the initial effort put in to port it with essentially no reward for the upstream project. In the main kubernetes repo we are not accepting new architectures without a KEP. Is there a plan to run multi-arch CI within the Kubernetes project? Or some other reason SIG testing should continue to port everything? |
@@ -72,7 +73,7 @@ RUN echo "Installing Packages ..." \ | |||
unzip \ | |||
&& rm -rf /var/lib/apt/lists/* \ | |||
&& echo "Installing Go ..." \ | |||
&& export GO_TARBALL="go${GO_VERSION}.linux-amd64.tar.gz"\ | |||
&& export GO_TARBALL="go${GO_VERSION}.linux-${TARGETARCH:-amd64}.tar.gz"\ |
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.
nit: can we just default this once with ARG TARGETARCH=amd64 and simplify here and below?
args: | ||
- build | ||
- --tag=gcr.io/$PROJECT_ID/krte:$_GIT_TAG-$_CONFIG | ||
- --platform=linux/amd64,linux/arm64/v8 |
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.
possibly something we should parameterize
Since @ruquanzhao is on vacation, so pls let me try to reply, just for something I heard from his early investigation, might not correct though.
Based on what @ruquanzhao told me, we need this to provide a containerized environment for test execution. So, why we need this change?
Whether we need ARM CI coverage for k8s is another topic and it's totally community's call but we can make this utility work-able for mulit-arch so that they are not struggle with containerized environment.
Sure, we can provide a KEP if this is needed, we might need to have a consensus on how far we could go.
Of course, we'd love to. Again, it's community's call to decide where we should go. @ruquanzhao @BenTheElder correct me? |
We are plagued by timeouts in the image build today from a very lengthy build in these images as-is supporting all the necessary Kubernetes environments in CI, such as this currently most recent build.
I don't see how these are related, the KRTE image is the environment used for prow.k8s.io to test KIND (again, please read the README for KRTE). Kubernetes can be tested on any architecture without this image. And there is no ARM CI in the Kubernetes project, certainly not in the KIND project, for which this image is supported only.
Again, this image is only supported for the purposes of testing KIND in the project's CI, we do not have the maintainer bandwidth to support other things with this, the KIND project is already overloaded as-is. So it is not another topic. This image is an implementation detail of the KIND project's CI and is expressly not supported for other purposes.
|
@BenTheElder thanks for the detailed explanation! I was thinking if we need to test kubernetes with KIND on ARM, we must need this, maybe I am wrong. @ruquanzhao could you pls take a look at @BenTheElder 's comments when you back? thanks! |
This is a problem about maintaining the image after this merge, last experience was very disappointing, people added changes to support ARM and after some months nobody keep working on it, CI kept failing forever and we end with more code to maintain.
I'm with Ben on this, if people wants to add a CI with kind with ARM I'm happy to support them, answer questions, .... , the image can be copied, and new jobs can be added in parallel, but people has to maintain their own CI,not at the cost of putting more load on us. This can also be reviewed in the future, and if there is more alignment, it is just a matter of merging things ... |
Just got back from a long vacation. Thank you all for the comments and reviews! @BenTheElder @aojea @chendave
We(@chendave and I) are deploying k8s CI with Prow on arm locally and trying to align with upstream, preparing to make it public in the future if possible. We found that
Yes, indeed. Kubernetes can be tested on any architecture without
We have dedicated resources to work or maintain such things if ARM CI is acceptable for KIND or K8S, so this is not a problem anymore. Thanks again for your comments! @aojia Hope to hear your opinion! @BenTheElder |
Please reach out to SIG k8s infra about a plan for community arm testing resources and staffing maintaining them if that approach is to be taken. I don't think the community will need or want an additional prow deployment to accomplish testing, so effort to "align" is relatively wasted with respect to being part of community CI in the future. Prow can readily schedule to any kubernetes cluster from the same services cluster. At this point in time the Kubernetes project and by extension the kind project do not have any non amd64 CI nor any plans to provide any given lack of funding. This image is maintained for our own upstream testing purposes as documented and is not intended for reuse elsewhere. I do not think the community should pay the cost to maintain and host this for external CI, Antonio and I are stretched extremely thin as-is. It is not a blocker to run downstream kind tests using some other image, and we make no stability guarantees about it's reuse. This image is an internal detail of the kind project, which is fully on amd64 provided by sig k8s infra. |
@ruquanzhao: The following tests failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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. I understand the commands that are listed here. |
/close |
@dims: Closed this PR. In response to this:
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. |
How about we add multi-arch support of krte?
Signed-off-by: Ruquan Zhao ruquan.zhao@arm.com