Fix FlowExporter memory bloat when export process is dead #3994
+74
−6
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
Codecov Report
@@ Coverage Diff @@
## main #3994 +/- ##
==========================================
+ Coverage 64.39% 65.06% +0.67%
==========================================
Files 295 295
Lines 43781 44037 +256
==========================================
+ Hits 28191 28654 +463
+ Misses 13304 13113 -191
+ Partials 2286 2270 -16
|
|
/test-e2e |
|
/test-networkpolicy |
|
/test-conformance |
antoninbas
reviewed
Jul 12, 2022
| func (pq *ExpirePriorityQueue) AddItemToQueue(connKey flowexporter.ConnectionKey, conn *flowexporter.Connection) { | ||
| // AddOrOverwriteItemToQueue adds conn with connKey into the queue. If an existing item | ||
| // has the same connKey, it will be overwritten by the new item. | ||
| func (pq *ExpirePriorityQueue) AddOrOverwriteItemToQueue(connKey flowexporter.ConnectionKey, conn *flowexporter.Connection) { |
There was a problem hiding this comment.
I think we could just call this WriteItemToQueue.
wsquan171
force-pushed
the
fe-memory-fix
branch
3 times, most recently
from
267842d to
9f011e5
Compare
|
/test-conformance |
heanlan
reviewed
Jul 12, 2022
| @@ -74,6 +74,7 @@ func TestDenyConnectionStore_AddOrUpdateConn(t *testing.T) { | |||
| denyConnStore.AddOrUpdateConn(&testFlow, refTime.Add(-(time.Second * 20)), uint64(60)) | |||
| expConn := testFlow | |||
| expConn.DestinationServicePortName = servicePortName.String() | |||
| expConn.LastExportTime = refTime.Add(-(time.Second * 20)) | |||
There was a problem hiding this comment.
I think denyConnStore.AddOrUpdateConn has already taken care of assigning value to LastExportTime?
Signed-off-by: Shawn Wang <[email protected]>
|
/test-conformance |
heanlan
approved these changes
Jul 13, 2022
antoninbas
approved these changes
Jul 13, 2022
antoninbas
added
the
action/release-note
|
@wsquan171 I assume that this should be backported? Could you take care of it? |
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.
When flow exporter is enabled, but failed to connect to downstream IPFIX collector, connections added to the priority queue inside flow exporter won't be expired and removed from queue, causing memory to bloat. Furthermore, connection store polling will remove conn from its
connectionsmap when the flow is no longer in conntrack, but not the related items in priority queue. When a new flow with same flow key is reestablished, a duplicated item with same key will be added to the queue, while the reference to the old one is lost, essentially causing memory leak.This change addresses above issue in the following aspects:
connectionsmap and the priority queue. Since CS polling is independent of exporting process liveness, this allows clean up to be done without connection to collector.Connection.LastExportTimeto be connection start time to make sure CS polling logic works properly when the exporting process is dead. Previously LastExportTime will only be filled by exporting process at the time of export, causing zero value to be compare in certain cases.Benchmark test
BenchmarkExportConntrackConnsdid not show observable difference before and after change.Fixes item 1 and 2 in #3972. Severity of item 3 is lower, which will be addressed in a later change.