ROX-32798: Fix ipMap leak on IP recycling in clusterentities#19308
Open
ROX-32798: Fix ipMap leak on IP recycling in clusterentities#19308
Conversation
Contributor
Author
|
This change is part of the following stack: Change managed by git-spice. |
Contributor
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the new tests you repeatedly call net.ParseIP("10.0.0.1") inline (including as a map key); consider parsing once into a local variable and reusing it to avoid subtle issues and make the tests a bit clearer.
- Helper-style functions in the test file like totalDeploymentsInIPMap, totalIPsInReverseIPMap, and ptr could be marked as test helpers (e.g., via t.Helper if refactored to take *testing.T) or grouped/placed closer to their callers to make the test flow easier to follow.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the new tests you repeatedly call net.ParseIP("10.0.0.1") inline (including as a map key); consider parsing once into a local variable and reusing it to avoid subtle issues and make the tests a bit clearer.
- Helper-style functions in the test file like totalDeploymentsInIPMap, totalIPsInReverseIPMap, and ptr could be marked as test helpers (e.g., via t.Helper if refactored to take *testing.T) or grouped/placed closer to their callers to make the test flow easier to follow.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
|
Images are ready for the commit at e8fb513. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #19308 +/- ##
=======================================
Coverage 49.62% 49.63%
=======================================
Files 2680 2680
Lines 202231 202230 -1
=======================================
+ Hits 100366 100370 +4
+ Misses 94379 94377 -2
+ Partials 7486 7483 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Description
Strongly related to ROX-32798, but it has not been confirmed yet as the sole root cause of the issue.
What was the bug?
Sensor maintains an in-memory map (
ipMap) that tracks which deployment currently owns each pod IP address. When a deployment was removed, the cleanup function (deleteDeploymentFromCurrent) was supposed to remove that deployment's entry from the map. However, if the IP was temporarily shared by two or more deployments, the cleanup skipped the removal entirely and only logged a warning. The stale deployment ID stayed in the map forever.When could it happen?
Kubernetes routinely recycles pod IP addresses: when a pod dies, its IP can be reassigned to a new pod in a different deployment. Sensor receives these lifecycle events asynchronously — and on clusters with high pod churn, it is common for the "new pod got IP X" event to arrive before the "old pod with IP X is gone" event. During that short window, two deployments appear to share the same IP. This is the exact condition that triggered the bug.
What were the consequences?
Once the bug wasriggered, it was self-reinforcing:
ipMapaccumulated stale deployment IDs that were never cleaned up. On an affected customer cluster, a single IP (10.131.x.y) had accumulated references to 4 deployments, only one of which was still alive.ips_having_multiple_containers_totalPrometheus metric used deployment UUIDs as a label. Each new combination of stale + active UUIDs created a new time series, resulting in ~50 distinct series from only ~30 affected IPs on the customer cluster.ipMapcould attribute traffic to deployments that no longer existed and thus incorrect data to the network graph.What does this PR change?
deleteDeploymentFromCurrentnow removes the specific deployment ID from the IP's set, and only deletes the map entry when the set is empty.containerslabel (comma-joined deployment UUIDs) is dropped fromips_having_multiple_containers_total. Theiplabel is kept — its cardinality is bounded by the pod CIDR and is sufficient to identify which IPs are being recycled.deleteDeploymentFromCurrent) and via the public API (Store.Apply), simulating realistic out-of-order event delivery.The bug fix and regression tests were generated with AI assistance. The metric change, test review, and validation were done by the author.
User-facing documentation
Testing and quality
Automated testing
How I validated my change
Ran the full cluster entities test suite locally:
All 67 tests pass, including 5 new regression tests that:
deleteDeploymentFromCurrentwith shared IPs (3 subtests)Store.Applywith out-of-order events (2 subtests)These tests fail on
master(confirming the bug exists) and pass with the fix applied.