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

Improvements to excess IP release handshake #18296

Merged
merged 2 commits into from
Dec 21, 2021

Conversation

hemanthmalla
Copy link
Member

@hemanthmalla hemanthmalla commented Dec 17, 2021

Follow up from #17939

  • Fix for blocked state transition from ready-for-release to released
  • Fix for unnecessary updates between agent and operator during handshake

@hemanthmalla hemanthmalla requested review from a team and twpayne December 17, 2021 11:49
@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 Dec 17, 2021
@gandro gandro added area/eni Impacts ENI based IPAM. needs-backport/1.11 release-note/bug This PR fixes an issue in a previous release of Cilium. labels Dec 20, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 20, 2021
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 a ton! Unfortunately this approach seems to introduce a data race.

pkg/ipam/node.go Show resolved Hide resolved
@hemanthmalla hemanthmalla force-pushed the hemanth.malla/handshake_improvements branch from 55ee370 to 0c8953f Compare December 20, 2021 14:01
* Fix for blocked state transition from ready-for-release to released
* Fix for unnecessary updates between agent and operator during handshake

Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
@hemanthmalla hemanthmalla force-pushed the hemanth.malla/handshake_improvements branch from 0c8953f to fed1226 Compare December 20, 2021 20:14
crdAllocator.Allocate() aquires lock on allocator first and then on
nodestore. But updateLocalNodeResource() acquires locks in the opposite
order. This commit releases nodestore lock before acquiring allocator lock
to avoid potential deadlocks due to inconsistent lock ordering.

Signed-off-by: Hemanth Malla <hemanth.malla@datadoghq.com>
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.

Awesome! This looks much cleaner than before. Thanks a ton!

@gandro
Copy link
Member

gandro commented Dec 21, 2021

/ci-eks

@gandro
Copy link
Member

gandro commented Dec 21, 2021

/test-runtime

@gandro
Copy link
Member

gandro commented Dec 21, 2021

Not running the full CI suite, as the none of the CRD-backed IPAM modes (eni, aks, plain crd) are covered by Jenkins at all. Once runtime is green, I think we can safely merge this.

@gandro
Copy link
Member

gandro commented Dec 21, 2021

/ci-aks

Edit: This workflow apparently was disabled. https://github.com/cilium/cilium/actions/workflows/conformance-aks.yaml

@gandro
Copy link
Member

gandro commented Dec 21, 2021

Marking this ready to merge. As mentioned above, the code modified by this PR is not active in any currently enabled test suite except ConformanceEKS which as passed.

Ideally we had more coverage for the IP release feature as discussed in the last community meeting, but that should not block this bug fix from getting merged.

@gandro gandro added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Dec 21, 2021
@gandro gandro removed the request for review from twpayne December 21, 2021 16:26
@tklauser tklauser merged commit 2581084 into cilium:master Dec 21, 2021
@qmonnet qmonnet added the backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. label Mar 28, 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. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants