Skip to content

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

Merged
gandro merged 1 commit intocilium:mainfrom
liyihuang:pr/liyihuang/dynamic_update_ec2_instance_in_limits
Mar 12, 2025
Merged

ipam/aws: Remove UpdateEC2AdapterLimitViaAPI option and static mapping for aws enviroment and always fetch from EC2 API#36922
gandro merged 1 commit intocilium:mainfrom
liyihuang:pr/liyihuang/dynamic_update_ec2_instance_in_limits

Conversation

@liyihuang
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

liyihuang commented Jan 13, 2025

/test

1 similar comment
@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Jan 14, 2025

/test

christarazi
christarazi reviewed Jan 14, 2025
View reviewed changes
gandro
gandro requested changes Jan 14, 2025
View reviewed changes
Copy link
Copy Markdown
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 4 times, most recently from 47b9cae to 3e17c3c Compare March 6, 2025 18:44
@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/test

2 similar comments
@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/test

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/test

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/ci-eks

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/ci-awscni

1 similar comment
@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/ci-awscni

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 6, 2025

/ci-runtime

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 7, 2025

@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
Copy Markdown
Contributor Author

liyihuang commented Mar 10, 2025

/test

@liyihuang
Copy link
Copy Markdown
Contributor Author

liyihuang commented Mar 10, 2025

/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
Copy Markdown
Contributor Author

liyihuang commented Mar 11, 2025

/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  <[email protected]>
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
Copy Markdown
Contributor Author

liyihuang commented Mar 11, 2025

/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
@HadrienPatte HadrienPatte mentioned this pull request Mar 17, 2025
Closed
8 tasks
This was referenced Mar 25, 2025
Closed
Merged
Merged
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