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

alibabacloud: use NextToken and MaxResults to list instance types #25387

Merged
merged 1 commit into from Jun 20, 2023
Merged

alibabacloud: use NextToken and MaxResults to list instance types #25387

merged 1 commit into from Jun 20, 2023

Conversation

haozhangami
Copy link
Contributor

@haozhangami haozhangami commented May 11, 2023 •
edited

Currently, there is no paging query used to retrieve instance types. When there are many instance types, some instances can not obtain ENI and IPPerENI limits. The cilium operator log is as follows.

level=warning msg="Unable to maintain ip pool of node" error="Unable to determine limits" instanceID=<ID> name=<NAME> subsys=ipam

This patch fixes this problem by using NextToken and MaxResults for paging query.

Fix bug in AlibabaCloud where instance type limits could not be determined

@haozhangami haozhangami requested a review from a team as a code owner May 11, 2023 14:58
@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 11, 2023
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 11, 2023
@haozhangami haozhangami force-pushed the use-nexttoken-and-maxresults-to-list-instance-types branch from 08548ec to b7adf02 Compare May 11, 2023 15:57
christarazi
christarazi reviewed May 11, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've left one comment and I have an overarching question: how does this change resolve the error you experienced? I'm not sure I follow why this fixes it.

pkg/alibabacloud/api/api.go Show resolved Hide resolved
@christarazi christarazi added integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. sig/ipam IP address management, including cloud IPAM area/alibaba Impacts Alibaba based IPAM. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. area/operator Impacts the cilium-operator component labels May 11, 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 May 11, 2023
@christarazi
Copy link
Member

/test

@haozhangami haozhangami force-pushed the use-nexttoken-and-maxresults-to-list-instance-types branch 2 times, most recently from ba4954b to 0e45bc1 Compare May 12, 2023 04:15
@haozhangami haozhangami force-pushed the use-nexttoken-and-maxresults-to-list-instance-types branch from 0e45bc1 to b91fadb Compare May 13, 2023 00:46
@christarazi
Copy link
Member

/test

@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 15, 2023
@haozhangami
Copy link
Contributor Author

Hi @christarazi @sayboras , please help to see if this PR can be merged.

@christarazi
Copy link
Member

@haozhangami Could you rebase on PR on latest main so we can re-run the CI?

@haozhangami haozhangami force-pushed the use-nexttoken-and-maxresults-to-list-instance-types branch from b91fadb to bdd47e1 Compare June 16, 2023 01:35
@haozhangami
Copy link
Contributor Author

@haozhangami Could you rebase on PR on latest main so we can re-run the CI?

Done

@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 Jun 16, 2023
@haozhangami
alibabacloud: use NextToken and MaxResults to list instance types
803e52e 
Currently, there is no paging query used to retrieve instance types.
When there are many instance types, some instances can not obtain ENI
and IPPerENI limits. The cilium operator log is as follows.
```
level=warning msg="Unable to maintain ip pool of node" error="Unable
to determine limits" instanceID=<ID> name=<NAME> subsys=ipam
```
This patch fixes this problem by using NextToken and MaxResults for
paging query.

Signed-off-by: Hao Zhang <hao.zhang.am.i@gmail.com>
@haozhangami haozhangami force-pushed the use-nexttoken-and-maxresults-to-list-instance-types branch from bdd47e1 to 803e52e Compare June 16, 2023 14:24
@sayboras
Copy link
Member

/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 Jun 19, 2023
@ti-mo ti-mo merged commit 95b264d into cilium:main Jun 20, 2023
61 of 62 checks passed
@joestringer joestringer mentioned this pull request Jun 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@christarazi christarazi christarazi approved these changes

Assignees
No one assigned
Labels
area/alibaba Impacts Alibaba based IPAM. area/operator Impacts the cilium-operator component integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. kind/bug This is a bug in the Cilium logic. kind/community-contribution This was a contribution made by a community member. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium. sig/ipam IP address management, including cloud IPAM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@haozhangami @christarazi @sayboras @ti-mo