-
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 node_e2e test #29192
Add ARM64 node_e2e test #29192
Conversation
Hi @ike-ma. 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. |
/cc @bobbypage |
/ok-to-test |
- --scenario=kubernetes_e2e | ||
- -- | ||
- --deployment=node | ||
- --env=KUBE_BUILD_FOR_ARM64=true |
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.
@ike-ma where is this used? KUBE_BUILD_FOR_ARM64
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 was using it to control whether to build for ARM64 or AMD64: https://github.com/kubernetes/kubernetes/pull/117017/files/c7c8c7fe388079adf3a24e0d465dd35afea15357#diff-4e140fc3b0b901a6cdad3f45948687ebd709753a4c6b48a52e876a1fec856247
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 still relevant? It seems to be removed from kubernetes/kubernetes#117017
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.
+1, this should no longer be needed?
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 has been removed
/assign @bobbypage |
/assign @mmiranda96 |
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.
Added a couple of comments.
# `gcloud compute --project <to-project> disks create <image name> --image=https://www.googleapis.com/compute/v1/projects/<from-project>/global/images/<image-name>` | ||
# `gcloud compute --project <to-project> images create <image-name> --source-disk=<image-name>` | ||
images: | ||
ubuntu: |
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.
Do you plan to use COS as well? We can add both configs 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.
Currently only Ubuntu is in scope, will add COS after more testing in a separate PR.
# `gcloud compute --project <to-project> images create <image-name> --source-disk=<image-name>` | ||
images: | ||
ubuntu: | ||
image: ubuntu-gke-2204-1-24-arm64-v20230217 |
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 suggest that we use image-family
instead, otherwise we'd need to update this job constantly.
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.
Done. Updated.
runcmd: | ||
- echo "Test run from /tmp folder, remounting it" | ||
- mount /tmp /tmp -o remount,exec,suid | ||
|
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.
Not sure if having empty lines across a YAML list is supported.
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 empty lines will be ignored by the parser. I have tested it without any issue.
- --scenario=kubernetes_e2e | ||
- -- | ||
- --deployment=node | ||
- --env=KUBE_BUILD_FOR_ARM64=true |
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 still relevant? It seems to be removed from kubernetes/kubernetes#117017
/retest |
/assign @ndixita |
@ike-ma can you please review the @mmiranda96 's comments. I'd like to merge this so we can run this test for the changes in k/k repository |
/cc |
description: "Run serial node e2e tests on ARM64 environment on Ubuntu" | ||
labels: | ||
preset-service-account: "true" | ||
preset-k8s-ssh: "true" |
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 will likely need
preset-dind-enabled: "true"
because ARM build needs to use dockerized build so we need to enable docker-in-docker.
Also for docker-in-docker it will also need to be marked as
securityContext:
privileged: true
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 the pointer, added.
- --deployment=node | ||
- --env=KUBE_BUILD_FOR_ARM64=true | ||
- --gcp-zone=us-central1-a | ||
- --node-args=--image-config-file=/workspace/test-infra/jobs/e2e_node/arm/image-config.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 will likely need the new flags introduced in https://github.dev/kubernetes/kubernetes/pull/117017, i.e.
--use-dockerized-build=true --target-build-arch=linux/arm64
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.
Done, updated with the latest command.
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.
seems like there is a typo in the config file,
--image-config-file=/workspace/test-infra/jobs/e2e_node/arm/image-config.yaml
while it should be
--image-config-file=/workspace/test-infra/jobs/e2e_node/arm/image-config-serial.yaml
Can you confirm?
/hold
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.
Good catch. Named as image-config.yaml
in my own test. Updated to match.
Addressed all review comments, and updated the test config based on #28888. Tested with
|
containers: | ||
- image: gcr.io/k8s-staging-test-infra/kubekins-e2e:v20230513-7e1db2f1bb-master | ||
args: | ||
- --repo=k8s.io/kubernetes=master |
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.
we should stop writing new jobs with bootstrap.py instead of decorate: true
these jobs have been logging a warning to migrate for years
this doesn't have to block adding the job, but please seriously consider migrating.
https://gist.github.com/dims/c1296f8ed42238baea0a5fcae45f4cf4
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.
Acknowledge, thanks for the gist pointer, will follow up in a separate PR.
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.
/approve
with a request to please switch to prow native cloning etc. (decorate: true
) and stop using the bootstrap.py entrypoint script.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, ike-ma 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 |
Run node_e2e serial tests on GCP T2A (arm64) machine
Addressed review comments, and rebased to upstream/master |
/hold cancel |
/test pull-test-infra-unit-test |
@ike-ma: Updated the
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. |
Cool! Thank you for merging. It is failing: https://prow.k8s.io/view/gs/kubernetes-jenkins/logs/ci-kubernetes-node-arm64-ubuntu-serial/1662289187593785344 |
Run node_e2e serial tests on GCP T2A (arm64) machine
Cross-ref: kubernetes/kubernetes#117017 (Setup node_e2e to support ARM64)