Skip to content

Adding support for writing e2e tests exclusive to ENI mode#18557

Closed
hemanthmalla wants to merge 1 commit intocilium:masterfrom
DataDog:hemanthmalla/eni_e2e
Closed

Adding support for writing e2e tests exclusive to ENI mode#18557
hemanthmalla wants to merge 1 commit intocilium:masterfrom
DataDog:hemanthmalla/eni_e2e

Conversation

@hemanthmalla
Copy link
Copy Markdown
Member

@hemanthmalla hemanthmalla commented Jan 20, 2022 •
edited
Loading

Currently in CI only cilium-cli based connectivity tests are run for AWS ENI mode. AFAIK, There seems to be no easy way to write e2e tests exclusive to a cloud provider. This commit introduces a new ginkgo focus group for ENI, which can be used to house e2e tests for cloud provider specific features like AWS excess IP release, ENI prefix delegation, etc. There are more features that aren't currently tested e2e in CI and this focus group should make it easy to add them.

This could also be achieved with build tags maybe ? Please suggest if there's an easier / better way to achieve this.

This PR also adds an e2e test for changes added from #17939 and is needed to trigger the e2e test in #18463

Link to successful workflow run with incoming changes.

Todo :

  • Github workflow changes to trigger new tests
  • Add support for managing cloud provider nodegroups from ginkgo
  • Support for using cilium-cli from ginkgo

@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 Jan 20, 2022
@hemanthmalla hemanthmalla force-pushed the hemanthmalla/eni_e2e branch 2 times, most recently from bef3499 to 3a9ff38 Compare January 20, 2022 15:15
@hemanthmalla hemanthmalla mentioned this pull request Jan 20, 2022
Merged
3 tasks
@gandro
Copy link
Copy Markdown
Member

gandro commented Jan 24, 2022

Awesome, thank you! A bit of feedback from an offline discussions with some folks:

  • There seems to be a preference to add this set of tests as an additional step to the existing ci-eks GitHub actions, to reduce the maintenance burden.
  • We are a bit unsure about the future of Ginkgo inside Cilium CI. While there is no official proposal to move a way from it, all recent CI additions have opted for other solutions, so we're a bit unsure of its future use in the Cilium project. However, given the unclear path forward, this doesn't seem to be a blocker at this point.
  • Given that this tests very specific features, I think we should consider having on-demand trigger for this. I don't think it needs to run on every PR, as at least in my experience, most PRs are unrelated to specific behavior of the ENI IPAM control plane. It's also not hard to tell if a PR does modify ENI/AWS related code, so ENI/AWS CODEOWNERs can also easily trigger this test if deemed necessary (though we can likely automate that process of determining if this test suite needs to be run for a PR)

@gandro gandro added area/CI-improvement Topic or proposal to improve the Continuous Integration workflow area/eni Impacts ENI based IPAM. release-note/ci This PR makes changes to the CI. labels Jan 25, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Jan 25, 2022
@hemanthmalla hemanthmalla marked this pull request as ready for review February 15, 2022 21:10
@hemanthmalla hemanthmalla requested review from a team as code owners February 15, 2022 21:10
@aanm aanm requested a review from nbusseneau February 15, 2022 22:36
@aanm aanm requested a review from gandro February 15, 2022 22:36
@aanm aanm removed their request for review February 15, 2022 22:36
@aanm aanm removed their assignment Feb 15, 2022
gandro
gandro reviewed Feb 16, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@gandro gandro left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Awesome work 🚀 Overall looks good to me, few small nits.

Given that this adds about 10 minutes to the overall test duration, I think one improvement to consider is to only run the Run e2e tests step if any of the ENI affected code has been changed (i.e. by adding a ENI-specific path filter similar to what we already do for documentation and gating the Run e2e test step with an if) or some other opt-in mechanism, but I let the ci-structure team chime in here as well.

@hemanthmalla
Copy link
Copy Markdown
Member Author

hemanthmalla commented Feb 16, 2022

@gandro suggestion to limit e2e test execution makes sense. But would pkg/aws and pkg/ipam suffice for ENI mode ? We might end up ignoring packages that indirectly impact ENI mode ?

@gandro
Copy link
Copy Markdown
Member

gandro commented Feb 17, 2022 •
edited
Loading

@gandro suggestion to limit e2e test execution makes sense. But would pkg/aws and pkg/ipam suffice for ENI mode ? We might end up ignoring packages that indirectly impact ENI mode ?

Yeah, this is a good question. Looking at the CODEOWNERs and from my experience, pkg/aws and pkg/ipam will cover the vast majority of changes. We should also add pkg/policy/groups/aws and vendor/github.com/aws/aws-sdk-go-v2.

I think automatically detecting affected PRs is good thing to have, as not everyone will be aware of the details of this workflow.
In addition, maybe we could however also have a additional override condition (maybe checking for a PR label like ci/aws similar to other existing ci/ labels) to run the step regardless of the detected code changes?

I'm also fine to start with maybe the label option first, and then experiment with the code changes later in a follow-up PR.

@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jul 7, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Aug 7, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 7, 2022
@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Aug 8, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Sep 8, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 8, 2022
@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Sep 8, 2022
@pchaigno pchaigno marked this pull request as draft September 15, 2022 10:47
@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 16, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 16, 2022
@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Oct 17, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Nov 17, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 17, 2022
@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Nov 17, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Dec 18, 2022

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Dec 18, 2022
@christarazi christarazi removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Dec 19, 2022
@github-actions
Copy link
Copy Markdown

github-actions bot commented Jan 19, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 19, 2023
@christarazi
Copy link
Copy Markdown
Member

christarazi commented Jan 23, 2023

@hemanthmalla Do you plan to take this PR forward?

@github-actions github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jan 24, 2023
@hemanthmalla
Copy link
Copy Markdown
Member Author

hemanthmalla commented Jan 26, 2023

@christarazi we definitely need this. But looks like due to recent changes we might need significant rework anyway ?
I'm fine with closing this now and opening a new one later.

@christarazi
Copy link
Copy Markdown
Member

christarazi commented Jan 26, 2023

@hemanthmalla Sounds good, maybe a fresh start would be best. Up to you.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Feb 26, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Feb 26, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2023

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 2, 2023
@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 16, 2023

This pull request has not seen any activity since it was marked stale.
Closing.

@github-actions github-actions bot closed this Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nbusseneau nbusseneau nbusseneau left review comments

@christarazi christarazi christarazi requested changes

@gandro gandro gandro approved these changes

@tklauser tklauser tklauser approved these changes

Assignees

No one assigned

Labels

area/CI-improvement Topic or proposal to improve the Continuous Integration workflow area/eni Impacts ENI based IPAM. release-note/ci This PR makes changes to the CI. stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@hemanthmalla @gandro @tklauser @christarazi @nbusseneau @aanm