Fix azure ipam flake caused by instance resync race condition. #31580
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maintainer-s-little-helper
bot
added
the
dont-merge/needs-release-note-label
tommyp1ckles
force-pushed
the
pr/tp/fix-azure-ipam-flake-v2
branch
2 times, most recently
from
4008aa2
to
3523bcd
Compare
|
/test |
|
Looking for a review for @cilium/azure. @hemanthmalla or @doniacld could either of you take a look 🙏 |
tommyp1ckles
added
release-note/bug
maintainer-s-little-helper
bot
removed
dont-merge/needs-release-note-label
hemanthmalla
approved these changes
Mar 29, 2024
TestIpamManyNodes fails occasionally with unexpected available IP counts. This appears to be an underlying bug in the azure/ipam implementation. This results from two racing resyncInstance calls racing to update the cached instances list. The chain of triggers for Upsert is as follows: *note*: Node has a reference to nodemanager, and nodemanager maintains references to all nodes, thus each node has a circular ref w/ the node manager. This complicates locking. nodemanager.Upsert -> 1. (NodeManager) Sets up node triggers. 2. (Node) Does node IP IPAM accounting. 3. (Node) Triggers poolMaintenance. poolMaintenance -> 1. (Node) Attach any necessary private IPs to instance based on current accounting (i.e. node stats). 2. (Node) Performs node device/address accounting and triggers instance sync (i.e. in this case: syncing Cloud API instance/device/addr with local cache). instanceSync -> 1. (NodeManager/InstanceManager) API is called to get subnets and instances. 2. (NodeManager/InstanceManager) Instances are updated in critical section. Finally, based on the outcome of the instances sync, following addr accounting, another maintainIPPool may be triggered causing a series of confusing allocation/accounting errors. In this last part, because getting instance data from the API is not locked, and because the instanceSync operation is an operation on the NodeManager. Two such syncs can occur at the same time (i.e. from different vms), in such a case, two instanceSync runs may occur out of order and the most recent result will be overwritten. This moves the azure API implementation lock to encompass the entire operation Resync operation - such that getting instances/subnets and assigning them is atomic. This shouldn't have any risk of deadlocks as azure api doesn't have any references to node or node manager. Fixes: #31466 Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
The code being test has been subject to several bugs. These tend to be more likely to reproduce when running on a smaller test case (presumably due to a lower chance of another trigger correcting previous errors). This adds another test case to TestIpamManyNodes that should make surfacing of regressions in the azure ipam implementation more likely. Signed-off-by: Tom Hadlaw <tom.hadlaw@isovalent.com>
tommyp1ckles
force-pushed
the
pr/tp/fix-azure-ipam-flake-v2
branch
from
3523bcd
to
c2b9fa5
Compare
|
/test |
tommyp1ckles
added
area/azure
maintainer-s-little-helper
bot
added
the
ready-to-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TestIpamManyNodes fails occasionally with unexpected available IP counts.
This appears to be an underlying bug in the azure/ipam implementation. This results from two racing resyncInstance calls racing to update the cached instances list.
This moves the azure API implementation lock to encompass the entire operation Resync operation - such that getting instances/subnets assigning them is atomic.
This shouldn't have any risk of deadlocks as azure api doesn't have any references to node or node manager.