-
Notifications
You must be signed in to change notification settings - Fork 24
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
Conntrack output field copy operations + fix duplicates #413
Conntrack output field copy operations + fix duplicates #413
Conversation
Codecov Report
@@ Coverage Diff @@
## main #413 +/- ##
=========================================
+ Coverage 0 64.59% +64.59%
=========================================
Files 0 94 +94
Lines 0 6629 +6629
=========================================
+ Hits 0 4282 +4282
- Misses 0 2102 +2102
- Partials 0 245 +245
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 88 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
bf7fe04
to
ffe215e
Compare
36535eb
to
015cea3
Compare
added duplicates fix testing 015cea3 |
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 @jpinsonneau ! I'm very impressed by how quickly you understand the code and came up with a solution.
Your approach to add the copy operations seems the most straight forward to me. Even though, I wonder whether there is a clean and simple way to integrate copiers and aggregators into the same interface...
Thanks @ronensc ! If it's fine for you we can merge the following maps: aggFields map[string]float64
cpFields map[string]interface{} and find a new name like |
I'm fine with merging the maps. |
f84abc4
to
ba3488b
Compare
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 Julien, AWESOME work! :)
Regarding the MoveToX()
, when I discussed this with @eranra, we had concerns that it would be too costly in terms of CPU...
Hm... Alternatively, we could maintain a queue of the connections that got FIN
and set them with smaller expiry timeout. WDYT?
Yeah that was also part of my concerns. Should we duplicate the type groupType struct {
scheduling api.ConnTrackSchedulingGroup
runningMom *utils.MultiOrderedMap
expiredMom *utils.MultiOrderedMap
labelValue string
} |
WIP c961da9 |
c961da9
to
34e0453
Compare
tested 34e0453 on my cluster and works fine 👍 |
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 @jpinsonneau ! I have a few comments/questions, but none are critical.
@@ -181,7 +181,7 @@ func (ct *conntrackImpl) updateConnection(conn connection, flowLog config.Generi | |||
} | |||
|
|||
func (ct *conntrackImpl) isLastFlowLogOfConnection(flowLog config.GenericMap) bool { |
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.
should we rename isLastFlowLogOfConnection()
since it's not really guaranteed to be the last?
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.
I would remove isLastFlowLogOfConnection
and shouldSwapAB
since containsTcpFlag
with the proper flag is better in terms of readability and these functions are only used in a single place
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.
Sure, no problem. If in the future we'll have these features for other protocols, we could add the abstraction layer.
1b1cc51
to
d3a2096
Compare
Addressed all the feedbacks and rebased d3a2096 |
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.
LGTM
d3a2096
to
bb23523
Compare
Rebased following #421 |
New image: quay.io/netobserv/flowlogs-pipeline:46219bb. It will expire after two weeks. |
bb23523
to
0ea48c5
Compare
/lgtm |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jpinsonneau The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
This PR brings
first
andlast
operations for connection trackingOutputFields
in order to be able to keep first flow direction informations as:Also fix duplicates:
bypasing these on top of Extract loop that can generate multiple connection events=> NETOBSERV-973 skip duplicates #421terminatingTimeout
These changes would fix NETOBSERV-973