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

Conntrack output field copy operations + fix duplicates #413

Merged

Conversation

jpinsonneau
Copy link
Collaborator

@jpinsonneau jpinsonneau commented Mar 28, 2023

This PR brings first and last operations for connection tracking OutputFields in order to be able to keep first flow direction informations as:

			OutputFields: []api.OutputField{
...
				{
					Name:      "FlowDirection",
					Operation: "first",
				},
				{
					Name:      "IfDirection",
					Operation: "first",
				},
				{
					Name:      "AgentIP",
					Operation: "first",
				},
			},

Also fix duplicates:

These changes would fix NETOBSERV-973

@jpinsonneau
Copy link
Collaborator Author

@codecov
Copy link

codecov bot commented Mar 28, 2023

Codecov Report

Merging #413 (0ea48c5) into main (cfadce6) will increase coverage by 64.59%.
The diff coverage is 86.25%.

@@            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     
Flag Coverage Δ
unittests 64.59% <86.25%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/pipeline/extract/conntrack/metrics.go 100.00% <ø> (ø)
pkg/api/conntrack.go 87.66% <50.00%> (ø)
pkg/pipeline/extract/conntrack/conn.go 78.29% <64.28%> (ø)
pkg/pipeline/extract/conntrack/store.go 80.90% <86.58%> (ø)
pkg/pipeline/extract/conntrack/aggregator.go 95.14% <100.00%> (ø)
pkg/pipeline/extract/conntrack/conntrack.go 93.37% <100.00%> (ø)

... 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.

@jpinsonneau jpinsonneau force-pushed the conntrack_outputfield_copy branch from bf7fe04 to ffe215e Compare March 29, 2023 14:58
@jpinsonneau jpinsonneau changed the title Conntrack output field copy operations Conntrack output field copy operations + fix duplicates Mar 30, 2023
@jpinsonneau jpinsonneau marked this pull request as ready for review March 30, 2023 10:53
@jpinsonneau jpinsonneau requested a review from ronensc March 30, 2023 10:53
@jpinsonneau jpinsonneau force-pushed the conntrack_outputfield_copy branch from 36535eb to 015cea3 Compare March 30, 2023 11:17
@jpinsonneau
Copy link
Collaborator Author

added duplicates fix testing 015cea3

ronensc
ronensc previously approved these changes Mar 30, 2023
Copy link
Collaborator

@ronensc ronensc left a 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...

pkg/pipeline/extract/conntrack/copier.go Outdated Show resolved Hide resolved
@jpinsonneau
Copy link
Collaborator Author

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 transformer for all the outputFields manipulations

@ronensc
Copy link
Collaborator

ronensc commented Mar 30, 2023

I'm fine with merging the maps.
I think "first" and "last" operations can be thought of as kinds of aggregation like in many SQL DBs.
So we don't have to come up with a new term.

@jpinsonneau jpinsonneau requested a review from ronensc March 30, 2023 16:53
@jpinsonneau jpinsonneau force-pushed the conntrack_outputfield_copy branch 2 times, most recently from f84abc4 to ba3488b Compare March 31, 2023 08:01
@jpinsonneau jpinsonneau requested a review from jotak April 3, 2023 08:09
Copy link
Collaborator

@ronensc ronensc left a 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?

pkg/api/conntrack.go Outdated Show resolved Hide resolved
pkg/pipeline/extract/conntrack/aggregator.go Outdated Show resolved Hide resolved
pkg/pipeline/extract/conntrack/aggregator_test.go Outdated Show resolved Hide resolved
@jpinsonneau
Copy link
Collaborator Author

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.

Yeah that was also part of my concerns. Should we duplicate the multiOrderedMap in groupType of connectionStore as:

type groupType struct {
	scheduling api.ConnTrackSchedulingGroup
	runningMom *utils.MultiOrderedMap
	expiredMom *utils.MultiOrderedMap
	labelValue string
}

@jpinsonneau jpinsonneau marked this pull request as draft April 3, 2023 10:59
@jpinsonneau
Copy link
Collaborator Author

WIP c961da9

@jpinsonneau jpinsonneau force-pushed the conntrack_outputfield_copy branch from c961da9 to 34e0453 Compare April 3, 2023 12:30
@jpinsonneau
Copy link
Collaborator Author

tested 34e0453 on my cluster and works fine 👍

@jpinsonneau jpinsonneau marked this pull request as ready for review April 3, 2023 12:48
ronensc
ronensc previously approved these changes Apr 4, 2023
Copy link
Collaborator

@ronensc ronensc left a 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.

pkg/pipeline/extract/conntrack/conntrack_test.go Outdated Show resolved Hide resolved
pkg/pipeline/extract/conntrack/metrics.go Outdated Show resolved Hide resolved
@@ -181,7 +181,7 @@ func (ct *conntrackImpl) updateConnection(conn connection, flowLog config.Generi
}

func (ct *conntrackImpl) isLastFlowLogOfConnection(flowLog config.GenericMap) bool {
Copy link
Collaborator

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?

Copy link
Collaborator Author

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

Copy link
Collaborator

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.

pkg/pipeline/extract/conntrack/store.go Outdated Show resolved Hide resolved
pkg/pipeline/extract/conntrack/store.go Outdated Show resolved Hide resolved
pkg/pipeline/extract/conntrack/store.go Outdated Show resolved Hide resolved
@jpinsonneau
Copy link
Collaborator Author

Addressed all the feedbacks and rebased d3a2096

ronensc
ronensc previously approved these changes Apr 4, 2023
Copy link
Collaborator

@ronensc ronensc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jpinsonneau
Copy link
Collaborator Author

Rebased following #421

@jpinsonneau jpinsonneau added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 11, 2023
@github-actions
Copy link

New image: quay.io/netobserv/flowlogs-pipeline:46219bb. It will expire after two weeks.

@jpinsonneau jpinsonneau force-pushed the conntrack_outputfield_copy branch from bb23523 to 0ea48c5 Compare April 11, 2023 16:31
@github-actions github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Apr 11, 2023
@jotak
Copy link
Member

jotak commented Apr 12, 2023

/lgtm
(re-applying dismissed lgtm)

@jpinsonneau
Copy link
Collaborator Author

/approve

@openshift-ci
Copy link

openshift-ci bot commented Apr 12, 2023

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 381890d into netobserv:main Apr 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants