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.

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

Add support to fallback from ENI PD if subnet is out of /28 prefixes #20822

Merged
merged 1 commit into from
Sep 9, 2022

Conversation

hemanthmalla
Copy link
Member

When a subnet is very fragmented, there might not be enough free
contiguous IPs to make up a /28 block. Without this fallback, new node
creation would be blocked when PD is enabled and subnet is in this
state. Currently operator does not support PD in mixed mode. Once a node
has secondary IP addresses attached on the ENI, operator will not attempt
to allocate any more prefixes.

With this fallback, operator will try to provision new nodes with prefix delegation
enabled. If a subnet doesn't have /28 blocks available, regular /32 IPs would
be allocated instead.

AWS VPC CNI plugin uses v1 of aws-sdk-go and has better error handling.
Out of capacity errors are logged in VPC CNI plugin, but it does not seem to have
any fallback.

@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 Aug 8, 2022
@hemanthmalla hemanthmalla marked this pull request as ready for review August 9, 2022 09:24
@hemanthmalla hemanthmalla requested a review from a team as a code owner August 9, 2022 09:24
@hemanthmalla hemanthmalla requested review from christarazi and gandro and removed request for christarazi August 9, 2022 09:24
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!

@hemanthmalla hemanthmalla added area/eni Impacts ENI based IPAM. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Aug 9, 2022
@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 Aug 9, 2022
pkg/aws/eni/node.go Outdated Show resolved Hide resolved
@hemanthmalla hemanthmalla force-pushed the hemanthmalla/pd_fallback branch 2 times, most recently from cf00cf4 to 3297552 Compare August 17, 2022 20:46
@hemanthmalla
Copy link
Member Author

hemanthmalla commented Aug 25, 2022

/test

Job 'Cilium-PR-K8s-1.24-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Check BPF masquerading with ip-masq-agent VXLAN

Failure Output

FAIL: Failed to add ip route

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.24-kernel-net-next so I can create one.

@aanm
Copy link
Member

aanm commented Sep 1, 2022

@hemanthmalla Could you rebase the PR? I believe some of the build failures were already fixed in master branch.

@hemanthmalla
Copy link
Member Author

@aanm build seems to have failed again ?
https://app.travis-ci.com/github/cilium/cilium/jobs/581598034

@christarazi
Copy link
Member

Yep looks like a known failure: #21145

@aanm
Copy link
Member

aanm commented Sep 2, 2022

/test

Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed:

Click to show.

Test Name

K8sConformance Portmap Chaining Check connectivity-check compliance with portmap chaining

Failure Output

FAIL: connectivity-check pods are not ready after timeout

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.16-kernel-4.9 so I can create one.

@hemanthmalla
Copy link
Member Author

Rebasing again to include changes from #21220

When a subnet is very fragmented, there might not be enough free
contiguous IPs to make up a /28 block. Without this fallback, new node
creation would be blocked when PD is enabled and subnet is in this
state. Currently operator does not support PD in mixed mode. Once a node
has secondary IP addresses attached on the ENI, operator will not attempt
to allocate any more prefixes.

Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
@hemanthmalla
Copy link
Member 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 Sep 7, 2022
@nebril nebril merged commit fc2c9ce into cilium:master Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/eni Impacts ENI based IPAM. 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
Development

Successfully merging this pull request may close these issues.

None yet

5 participants