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.

Sign up for GitHub

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

Jump to bottom

Update EKS conformance tests to use both amd64 and arm64 hosts. #24853

Merged
merged 2 commits into from Apr 28, 2023
Merged

Update EKS conformance tests to use both amd64 and arm64 hosts. #24853

merged 2 commits into from Apr 28, 2023

Conversation

chancez
Copy link
Contributor

@chancez chancez commented Apr 12, 2023

Update EKS conformance tests to use both amd64 and arm64 hosts.

@chancez chancez self-assigned this Apr 12, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 12, 2023
@chancez chancez force-pushed the pr/chancez/eks_conformance_arm64 branch 3 times, most recently from b9bc538 to d3e61bb Compare April 12, 2023 23:37
@chancez
Copy link
Contributor Author

chancez commented Apr 13, 2023

So far it's passing on all but ci-awscni-1.11 and ci-awscni-1.12. These are failing because they're using the chart from the PR, with bandwithManager=false, but in master it should be bandwidthManager.enabled=false. I'm not sure the proper way to test these workflow changes with their appropriate branches however. I might try updating their checkout steps to use the branches temporarily.

@chancez chancez force-pushed the pr/chancez/eks_conformance_arm64 branch 4 times, most recently from b1be0c9 to f15970e Compare April 14, 2023 23:21
@chancez
Copy link
Contributor Author

chancez commented Apr 17, 2023

Okay, I got it passing all the conformance tests with the two DO NOT MERGE commits. Should be ready for review.

Just in case I end up removing the commits, here's the commit with all EKS/AWS conformance tests passing: f15970e

@chancez chancez marked this pull request as ready for review April 17, 2023 20:01
@chancez chancez requested review from a team as code owners April 17, 2023 20:01
@chancez
Copy link
Contributor Author

chancez commented Apr 17, 2023

I'm gonna leave the DO NOT MERGE commits until someone says I should remove them, since I might need them if PR review requires me to update something (which will require those commits to test it).

@chancez chancez added the dont-merge/preview-only Only for preview or testing, don't merge it. label Apr 17, 2023
pchaigno
pchaigno previously requested changes Apr 18, 2023
View reviewed changes
.github/workflows/conformance-aws-cni-v1.11.yaml Outdated
uses: actions/checkout@24cb9080177205b6e8c946b17badbe402adc938f # v3.4.0
with:
ref: ${{ github.event.repository.default_branch }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why that is required? I'm concerned over the potential security impact of this change since we are now checking out untrusted user code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was needed to actually test my changes to the CI workflows, otherwise it would fail to checkout the setup-eks-cluster action I created.

Beyond that, I'm not sure if this change makes sense beyond testing. I had thought that since we only run these CI jobs via issue comments it would be safe, but I realize that anyone can make these comments on their own PRs, so that change should probably be a "DO NOT MERGE" commit. I can make sure to remove these changes before we merge the PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the reason we have this step is to protect against a malicious user checking out their malicious PR in the CI.

@chancez chancez changed the title WIP: Update EKS conformance tests to use both amd64 and arm64 hosts. Update EKS conformance tests to use both amd64 and arm64 hosts. Apr 18, 2023
nbusseneau
nbusseneau approved these changes Apr 19, 2023
View reviewed changes
Copy link
Member

@nbusseneau nbusseneau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as long as temp commit and the checkout step change are dropped :)

.github/actions/setup-eks-cluster/action.yml
effect: "NoExecute"
- name: ng-arm64
instanceTypes:
- t4g.medium
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these instances available in enough capacity in your experience? I've had bad experiences on my end with arm64 hosts availability, but maybe it changed since then...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I honestly haven't used them a ton, but I've not really heard of too many issues with capacity. I suspect it's a lot better than it used to be. We could also add other instance ARM types if it becomes a problem. This is just the cheapest option.

@chancez
Refactor EKS based conformance to use reusable github-action
f9cac84 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
.github: Test cilium against arm64 hosts in EKS based conformance tests
559ce0f 
Signed-off-by: Chance Zibolski <chance.zibolski@gmail.com>
@chancez
Copy link
Contributor Author

chancez commented Apr 19, 2023

Rebasing to fix conflicts, keeping DO NOT MERGE commits to re-test and make sure it still works.

@chancez chancez force-pushed the pr/chancez/eks_conformance_arm64 branch from f15970e to 2a6067e Compare April 19, 2023 16:31
@chancez
Copy link
Contributor Author

chancez commented Apr 19, 2023 •
edited

All the conformance CI passed for 2a6067e.

I'm going to remove the DO NOT MERGE commits. Note, that CI will fail after this because the new setup-eks-cluster action isn't in master, and the default CI checkout master for the workflows/actions, so it will fail to find the action and fail.

@chancez chancez force-pushed the pr/chancez/eks_conformance_arm64 branch from 2a6067e to 559ce0f Compare April 19, 2023 17:36
@pchaigno pchaigno dismissed their stale review April 20, 2023 17:19

Concern was addressed.

@chancez chancez added the release-note/ci This PR makes changes to the CI. label Apr 20, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Apr 20, 2023
@chancez chancez removed the dont-merge/preview-only Only for preview or testing, don't merge it. label Apr 20, 2023
qmonnet
qmonnet approved these changes Apr 21, 2023
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good from my side, thanks!

@chancez chancez requested a review from pchaigno April 27, 2023 20:48
@chancez
Copy link
Contributor Author

chancez commented Apr 27, 2023

Okay, I think this is waiting on a few more "required" tests. Since this is literally just updating AWS/EKS CI workflows themselves, I don't think that's necessary. So I'm going to mark it as ready-to-merge.

@chancez chancez added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 27, 2023
@pchaigno pchaigno merged commit d87dbb4 into main Apr 28, 2023
43 checks passed
@pchaigno pchaigno deleted the pr/chancez/eks_conformance_arm64 branch April 28, 2023 14:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nbusseneau nbusseneau nbusseneau approved these changes

@qmonnet qmonnet qmonnet approved these changes

@pchaigno pchaigno Awaiting requested review from pchaigno

Assignees

@chancez chancez

Labels
ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/ci This PR makes changes to the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@chancez @pchaigno @nbusseneau @qmonnet