ipam/crd: Fix ENI leak due to miscounting of empty interface slots#21800
ipam/crd: Fix ENI leak due to miscounting of empty interface slots#21800aanm merged 3 commits intocilium:masterfrom
Conversation
Currently, in CRD ipam modes, before the operator creates a new interface (or ENI for brevity), it checks the number of remaining empty ENI slots to ensure the max ENI limit does not exceed. However, the check is made against the field `AllocationAction.AvailableInterfaces`, which is set as `(empty ENI slots) + (attached ENIs that have not yet reached the IP address capacity)` in `PrepareIPAllocation`. This makes the ENI limit check ineffective in situations where an ENI with free space somehow can't be assigned with more IPs, e.g. the subnet of ENI is exhausted. As a result, the operator continuously tries to create and attach ENIs to the instance, the createInterface calls would succeed but attachInterface calls might fail due to ENI limit, and ENIs are leaked and exhausted eventually. The cause of miscouting might be the ambiguous field name `AvailableInterfaces`, which might be interpreted with different meanings in different contexts: 1. empty ENI slots, in interface creation context 2. attached ENIs with IP space, in IP allocation context 3. sum of 1. and 2., in node ipam stats context This patch fixes this by checking max ENI limit against the correct value, and also replaces `AllocationAction.AvailableInterfaces` with clearer names `InterfaceCandidates` `EmptyInterfaceSlots` to avoid potential misunderstanding in the future. Fixes: cilium#21747 Signed-off-by: Jaff Cheng <jaff.cheng.sh@gmail.com>
qmonnet
left a comment
There was a problem hiding this comment.
Looks good as far as I can tell, but I'm not familiar with Alibaba's IPAM.
From the issue report I understand the bug has been present since at least 1.10, is this fix something that we should consider for backports?
Deprecate `cilium_operator_ipam_available_interfaces` and add the following metrics: * cilium_operator_ipam_interface_candidates * cilium_operator_ipam_empty_interface_slots Related: cilium#21747 Signed-off-by: Jaff Cheng <jaff.cheng.sh@gmail.com>
c14f045 to
f35bf0b
Compare
|
Thanks for the comment.
Yes, I think this needs to be backported. I'm a little surprised this issue hasn't been reported by another user, since it should affect AWS ENI as well if I understand correctly. |
christarazi
left a comment
There was a problem hiding this comment.
Fix makes sense to me, thanks! Should we go a step further and ensure that we deallocate the ENI if we fail to attach it?
Delete ENI after a failed AttachNetworkInterface API call. Fixes: cilium#21747 Signed-off-by: Jaff Cheng <jaff.cheng.sh@gmail.com>
@christarazi Thanks for the advice! Just realized we already have ENI rollback logic, but alibabacloud missed it after a failed |
|
Looks good, thanks! |
|
/test |
|
Removing backport labels here as this PR has upgrade impact and deprecations. Please yell if some part of the PR does need to be bsckported. |
|
@jaffcheng Do we need to do the same thing for AWS ENI? |
|
I think this fix should also affect AWS ENI. IIUC, if an AWS node encounters this, the cilium-operator without this PR would keep creating and attaching(limit error) and deleting an ENI, but no ENI leak would happen. |
|
@jaffcheng Could you open a PR to resolve discrepancies between AWS and Alibaba? |
@christarazi Sorry for my wording not being clear enough, I think this PR should have fixed the ENI slot miscounting problem for AWS: 118c148#diff-30611d880237aa1610d4ba922a8639e304d4f1e398b98418bdd4200ee4536066R225-R252 The discrepancies between Alibaba and AWS are fixed in this commit: |
|
Ah perfect, thanks @jaffcheng! |
please see commit msg
Fixes: #21747