Skip to content

ipam/aws: Remove UpdateEC2AdapterLimitViaAPI option and static mapping for aws enviroment and always fetch from EC2 API #36922

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
Merged
merged 1 commit into from
Mar 12, 2025
Merged

ipam/aws: Remove UpdateEC2AdapterLimitViaAPI option and static mapping for aws enviroment and always fetch from EC2 API #36922

merged 1 commit into from
Mar 12, 2025

Conversation

liyihuang
Copy link
Contributor

@liyihuang liyihuang commented Jan 9, 2025 •
edited
Loading

  • Reimplement the limits package using a proper struct instead of global vars to prevent race conditions
  • Only fetch instance limits dynamically through EC2 API instead of static mapping or user config
  • Move the limits init to the instance manager from aws allocator
  • Remove deprecated CLI flags and config options:
    • aws-instance-limit-mapping
    • update-ec2dapter-limit-via-api
  • Update documentation for ENI and upgrade pages
  • Update aws mock API and unit tests

Fixes: #33654

Remove UpdateEC2AdapterLimitViaAPI option and static mapping between instance type and limits in AWS environment. Always fetch the limits via EC2API

@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 9, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch 3 times, most recently from 30f7bf6 to 6794066 Compare January 10, 2025 20:03
@liyihuang liyihuang marked this pull request as ready for review January 10, 2025 20:42
@liyihuang liyihuang requested a review from a team as a code owner January 10, 2025 20:42
@liyihuang liyihuang marked this pull request as draft January 12, 2025 13:18
@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch from cfaaa48 to 0ba9144 Compare January 13, 2025 19:16
@liyihuang liyihuang changed the title ipam/aws: improve ENI limits lookup with dynamic EC2 API only required ipam/aws: Remove UpdateEC2AdapterLimitViaAPI option and static mapping for aws enviroment and always fetch from EC2 API Jan 13, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch 3 times, most recently from a7cd530 to 41f1318 Compare January 13, 2025 21:12
@liyihuang liyihuang marked this pull request as ready for review January 13, 2025 22:07
@liyihuang liyihuang requested review from a team as code owners January 13, 2025 22:07
@liyihuang
Copy link
Contributor Author

liyihuang commented Jan 13, 2025 •
edited
Loading

Some notes:

  1. I was told I should do single PR for one issue, so I have 2 seperate commits in one PR. Hopefully different commits could help you to understand my changes better. Feel free to let me know if I need to do multiple smaller PRs.
  2. I'm still new to the codebase. let me know if I didn't do things correctly.
  3. User define static mapping deprecation will be in different PR since I can see User define static mapping could be used for overriding, I will look into previous issues to see if anyone really using it for override or for unknown instance type.

@liyihuang
Copy link
Contributor Author

/test

1 similar comment
@liyihuang
Copy link
Contributor Author

/test

christarazi
christarazi reviewed Jan 14, 2025
View reviewed changes
gandro
gandro requested changes Jan 14, 2025
View reviewed changes
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks! I'm fine with the overall approach, but two things that I believe need to be addressed:

  1. Be a bit smarter about how often we fetch the limits (in particular if we are not successful)
  2. Updating the docs since this is a breaking change

@gandro gandro added release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/eni Impacts ENI based IPAM. labels Jan 14, 2025
@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 14, 2025
@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch from 41f1318 to 040d836 Compare January 17, 2025 22:30
@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch from 47b9cae to 3e17c3c Compare March 6, 2025 18:44
@liyihuang
Copy link
Contributor Author

/test

2 similar comments
@liyihuang
Copy link
Contributor Author

/test

@liyihuang
Copy link
Contributor Author

/test

@liyihuang
Copy link
Contributor Author

/ci-eks

@liyihuang
Copy link
Contributor Author

/ci-awscni

1 similar comment
@liyihuang
Copy link
Contributor Author

/ci-awscni

@liyihuang
Copy link
Contributor Author

/ci-runtime

@liyihuang
Copy link
Contributor Author

@gandro let me know if there is anything that I need to update. I also moved the log to slog since I see a lot of modules are moving to it.

Hopefully I can merge without another rebase.

@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch from 3e17c3c to 4f825d9 Compare March 10, 2025 02:15
@liyihuang
Copy link
Contributor Author

/test

@liyihuang
Copy link
Contributor Author

/ci-integration

@gandro gandro removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 11, 2025
@gandro gandro enabled auto-merge March 11, 2025 08:37
@liyihuang liyihuang mentioned this pull request Mar 11, 2025
Merged
@liyihuang
Copy link
Contributor Author

/ci-e2e-upgrade

@liyihuang
ipam/aws: Refactor limits package to only use EC2 API dynamically
8f4cabb 
- Reimplement the limits package using a proper struct instead of global
  vars to prevent race conditions
- Only fetch instance limits dynamically through EC2 API instead of
  static mapping or user config
- Move the limits init to the instance manager from aws allocator
- Remove deprecated CLI flags and config options:
  - aws-instance-limit-mapping
  - update-ec2dapter-limit-via-api
- Update documentation for ENI and upgrade pages
- Update aws mock API and unit tests

Signed-off-by: Liyi Huang  <liyi.huang@isovalent.com>
auto-merge was automatically disabled March 11, 2025 18:04

Head branch was pushed to by a user without write access

@liyihuang liyihuang force-pushed the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch from 4f825d9 to 8f4cabb Compare March 11, 2025 18:04
@liyihuang
Copy link
Contributor Author

/test

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 11, 2025
@gandro gandro added this pull request to the merge queue Mar 12, 2025
Merged via the queue into cilium:main with commit e0a3d70 Mar 12, 2025
67 checks passed
@HadrienPatte HadrienPatte mentioned this pull request Mar 17, 2025
Closed
8 tasks
This was referenced Mar 25, 2025
Closed
Merged
Merged
@liyihuang liyihuang deleted the pr/liyihuang/dynamic_update_ec2_instance_in_limits branch April 2, 2025 17:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pippolo84 pippolo84 pippolo84 left review comments

@gandro gandro gandro approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

+1 more reviewer
Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
area/eni Impacts ENI based IPAM. area/operator Impacts the cilium-operator component kind/enhancement This would improve or streamline existing functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rate limit AWS EC2 API call to refresh AWS instance types
5 participants
@liyihuang @gandro @pippolo84 @nathanjsweet @christarazi