Create new ENIs in initial subnet by default#22000
Conversation
b15e0b3 to
f85fc60
Compare
|
/test |
f85fc60 to
c5a0416
Compare
gandro
left a comment
There was a problem hiding this comment.
Awesome work 🚀
Two very minor nits, otherwise this looks excellent to me.
c5a0416 to
757c6ac
Compare
757c6ac to
7cdaed2
Compare
|
/test Job 'Cilium-PR-K8s-1.16-kernel-4.9' failed: Click to show.Test NameFailure OutputIf it is a flake and a GitHub issue doesn't already exist to track it, comment |
7cdaed2 to
2250167
Compare
|
I realised that https://docs.cilium.io/en/v1.12/concepts/networking/ipam/eni/#eni-creation exists, and had to update that too- It was mildly out of date already, but should be correct now. Worthy of a quick glance though, maybe the wording could be improved. Manually requesting reviews since GH didn't. |
|
/ci-eks |
|
/test |
In the ENI mode of IPAM, when all IPs of an ENI are used, a new ENI is created to provide more IPs. Cilium chooses the subnet for this ENI based on SubnetIDs and SubnetTags, but if none are configured it needs to choose a subnet nonetheless. In this fallback case, the heuristic used was to choose the subnet in the same VPC and Availability Zone with the most available addresses. This led to surprising results, however, since the first ENI and future ENIs were not necessariliy in the same subnet. Instead, as part of node discovery, remember the subnet of the primary ENI as part of the Spec and try to create future ENIs in it as well, as long as there are sufficient addresses available. Note that explicitly configured subnet IDs and/or tags still take precedence, this changes the unconfigured default to be a bit more reasonable. Fixes: cilium#20553 Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
2250167 to
43c62dc
Compare
|
Rebased on master due to conflcit in docs (which exposed that I had accidentially put the upgrade notes into the section for 1.12 instead of 1.13. Thanks Joe!) ConformanceKind1.19 and "Smoke test / conformance-test (pull_request)" flake on #22217, the rest of CI was green before the rebase. Setting ready to merge. |
|
Is there any chance this PR can be backported in the next |
|
@alloveras As I understand, this would be a breaking change or at least a change in default behavior. We typically don't do that in a stable release. |
|
Yeah, I understand 👍 I saw this one more as a bugfix than anything else but I guess you could argue it both ways 😄 |
As a note: You can achieve a very similar behavior on v1.12 by setting a subnet tag filter via a custom CNI config: https://docs.cilium.io/en/v1.12/concepts/networking/ipam/eni/#eni-creation |
Yeah, absolutely 👍 That's exactly the workaround that we are using at the moment 😄 Thanks for the suggestion, though 🙇 |
|
Another thing to note: The subnet tag filters might still be needed even with this PR applied - the PR here will use prefer the initial subnet, but not force it. So once the initial subnet is exhausted, Cilium will allocate from any other available subnets, unless a tag filter is set. That may or may not be what you want. |
When no Subnet IDs/tags are configured, Cilium used to allocate new ENIs in the subnet which the most addresses available that is in the same VPC and AZ. This can lead to suprises, see #20553. Instead, try to allocate the new ENI in the same subnet as the primary one.
In order to do so, save the initial subnet ID in the Spec to have access to it while making the decision where to allocate the new ENI.
Fixes: #20553