-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Fix IPAM IP release racing condition when IP reassigned back to ENI #40019
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
Conversation
Commit a82c11c 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 |
a82c11c
to
4107a1e
Compare
/test |
4ae5cc8
to
f9b627a
Compare
/test |
f9b627a
to
758fc0b
Compare
There were failed integration tests, but I'm not sure if they are really related to my change. Just rebased from main branch. |
Fair enough, thanks for contributing. If it happens again could you also link to the build and highlight the failure symptom(s)? This can help others to review the output of those tests and either correlate with other issues others are facing or help to identify that the build is unrelated. You can also search the symptom in the "Issues" tab to see if someone else experiences the same problem. As I see that you updated the PR several times, I'll also note that we do not require all PRs to be up-to-date with I'll retrigger the tests. |
/test |
Thanks @joestringer ! Looks like all checks passed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the fix!
The fix makes sense to me, but I'd left a feedback inline about the usage of time
functions in the test.
Head branch was pushed to by a user without write access
4b30807
to
871ea9e
Compare
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the second iteration!
Some final advice about the test and I think we can merge this! 💯
e50d23f
to
115f518
Compare
@victorcq changes LGTM, nice! Could you please squash the three commits into a single one? Thanks in advance! 🙏 |
115f518
to
4ab4303
Compare
👍 Done. Btw, I see this PR is tagged as |
/test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! ✅
No need to do the backporting yourself, a backporter will take care of doing that (see https://docs.cilium.io/en/stable/contributing/development/reviewers_committers/duties/#backporters). |
@pippolo84 can you help to trigger those failed checks again? They don't really relate to my change. 🤞 |
Racing condition when IP reassigned back to ENI, during IPAM release process. Signed-off-by: Victor Chen <victor.chenq@gmail.com>
4ab4303
to
8a9a9a9
Compare
/test |
This fixes the racing condition during IPAM IP release process:
where an IP address that was marked for release and then reassigned back to the ENI can become stuck in a state where it's both in CiliumNode's
spec.ipam.pool
and instatus.ipam.release-IPs
with status "released". This prevents the IP from being assigned to new pods, leading to pods stuck in ContainerCreating state with "No more IPs available" errors.Fixes: #39981