Skip to content

operator: add flag to control AWS API pagination #39543

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
Jul 3, 2025
Merged

operator: add flag to control AWS API pagination #39543

merged 1 commit into from
Jul 3, 2025

Conversation

antonipp
Copy link
Contributor

Description

Small follow-up to #37983 and #39120

After we enabled pagination for the AWS EC2 API calls, it would actually be better to allow users to turn it off when needed. There are a couple of use-cases where it would help:

  • Quicker mitigation for issues like cilium-operator 1.17.3 fails to start on EKS w/ ENI allocator #39106 where the paginated request fails but the non-paginated one succeeds
  • In very large AWS accounts with 10000s of ENIs, the pagination can actually make things worse by increasing the number of API calls which are made and thus causing more rate-limiting. So 1 large expensive ENI list call is actually sometimes preferable than many smaller paginated calls.

I did not add a setting for configuring the page size: it is set to 1000 by default which is the maximum allowed size. That’s already small enough for most scenarios, and I don’t expect anyone to want a smaller value. Otherwise, we could add an int parameter in a follow-up PR as well.

operator: added `--aws-pagination-enabled` flag for enabling/disabling AWS API pagination

@antonipp antonipp requested review from a team as code owners May 14, 2025 15:35
@antonipp antonipp requested review from liyihuang and joamaki May 14, 2025 15:35
@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 May 14, 2025
@antonipp
Copy link
Contributor Author

/test

@maintainer-s-little-helper
Copy link

Commit 56d426a does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 14, 2025
@antonipp
Copy link
Contributor Author

/test

@antonipp antonipp force-pushed the ai/aws-enable-pagination-flag branch from 56d426a to f126d4b Compare May 14, 2025 15:40
@maintainer-s-little-helper
Copy link

Commit 56d426a does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@antonipp antonipp force-pushed the ai/aws-enable-pagination-flag branch from f126d4b to 4ad90c6 Compare May 14, 2025 15:42
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 14, 2025
@antonipp
Copy link
Contributor Author

/test

@antonipp antonipp marked this pull request as draft May 19, 2025 15:43
@antonipp
Copy link
Contributor Author

Thank you for your reviews! I completely forgot to mention that we found a performance regression with DescribeNetworkInterfaces pagination in large AWS accounts with lots of ENIs. I’m working with AWS support to track down the root cause and will update you as soon as I know more. Depending on what we find, we may want to actually disable pagination by default.

@github-actions GitHub Actions
Copy link

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 Jun 19, 2025
@antonipp antonipp force-pushed the ai/aws-enable-pagination-flag branch 3 times, most recently from 67f8336 to 8b3e8c9 Compare June 20, 2025 11:02
@antonipp
Copy link
Contributor Author

Alright, sorry for the delay, I had a long back-and-forth discussion with AWS regarding their API behaviour when pagination is used.

After this discussion, I had to make a small change to the PR: I removed pagination from the describeNetworkInterfacesByInstance function because the attachment.instance-id filter is not an "optimized" / "fast" filter (the list of "fast" filters is at the top of this documentation page). All paginated requests with this attachment.instance-id filter are actually very slow in AWS accounts with a large number of ENIs because the AWS API will return many empty pages with 0 results (see this documentation page - "each call returns 0 to MaxResults items"). Moreover, the number of returned results is very small, so it's fine to not have pagination here.

@antonipp antonipp marked this pull request as ready for review June 20, 2025 11:08
@antonipp
Copy link
Contributor Author

/test

@HadrienPatte HadrienPatte removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Jun 20, 2025
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Jun 20, 2025
@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 Jun 20, 2025
@joestringer joestringer added the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jun 20, 2025
@antonipp
operator: add flag to control AWS API pagination
f2eb8a8 
Signed-off-by: Anton Ippolitov <anton.ippolitov@datadoghq.com>
@antonipp antonipp force-pushed the ai/aws-enable-pagination-flag branch from 8b3e8c9 to f2eb8a8 Compare June 24, 2025 13:56
@antonipp
Copy link
Contributor Author

/test

@joestringer joestringer removed the dont-merge/wait-until-release Freeze window for current release is blocking non-bugfix PRs label Jul 1, 2025
@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 Jul 1, 2025
@tklauser tklauser added this pull request to the merge queue Jul 3, 2025
Merged via the queue into cilium:main with commit 8b24557 Jul 3, 2025
69 of 70 checks passed
@marseel marseel mentioned this pull request Jul 9, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@liyihuang liyihuang liyihuang approved these changes

@HadrienPatte HadrienPatte HadrienPatte approved these changes

Assignees
No one assigned
Labels
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.
6 participants
@antonipp @joamaki @liyihuang @HadrienPatte @tklauser @joestringer