NETOBSERV-838 Group ingress+egress flows in same connection #387

jpinsonneau · 2023-02-14T09:51:35Z

This PR skip duplicates in connection tracking aggregations.

It rely on Duplicate field provided by eBPF agent

codecov-commenter · 2023-02-14T11:19:36Z

Codecov Report

Merging #387 (308d02b) into main (d30b115) will increase coverage by 0.04%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #387      +/-   ##
==========================================
+ Coverage   60.89%   60.94%   +0.04%     
==========================================
  Files          91       91              
  Lines        6281     6288       +7     
==========================================
+ Hits         3825     3832       +7     
  Misses       2223     2223              
  Partials      233      233

Flag	Coverage Δ
unittests	`60.94% <100.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/config/generic_map.go	`100.00% <100.00%> (ø)`
pkg/pipeline/extract/conntrack/conntrack.go	`92.25% <100.00%> (+0.05%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

jpinsonneau · 2023-02-14T12:39:50Z

@ronensc please let me know if you are fine with that approach. The initial ticket suggested to rely on 2 hash ids but I feel it's less performant as we already have the duplicate field on flows.

ronensc

@jpinsonneau I agree with you that this is much simpler and elegant than the approach suggested in the ticket.
@jotak , do you remember if there was a reason why we didn't follow this path the first place?

ronensc · 2023-02-15T14:14:44Z

pkg/pipeline/extract/conntrack/conntrack.go

@@ -160,8 +160,10 @@ func (ct *conntrackImpl) prepareHeartbeatRecords() []config.GenericMap {

 func (ct *conntrackImpl) updateConnection(conn connection, flowLog config.GenericMap, flowLogHash totalHashType) {
 	d := ct.getFlowLogDirection(conn, flowLogHash)


I think the call to getFlowLogDirection() could be moved under the new "if isn't duplicated"

flaky tests on data race => https://github.com/netobserv/flowlogs-pipeline/actions/runs/4185399722/jobs/7252391870

ronensc · 2023-02-15T14:24:35Z

pkg/config/generic_map.go

+
+func (m GenericMap) IsDuplicate() bool {
+	if duplicate, hasKey := m["Duplicate"]; hasKey {
+		if isDuplicate, ok := duplicate.(bool); ok && isDuplicate {


We have ConvertToBool() that can handle cases where the type of m["Duplicate"] is not bool but is convertible to bool.

flowlogs-pipeline/pkg/pipeline/utils/convert.go

Line 220 in 3f2481e

func ConvertToBool(unk interface{}) (bool, error) {

I just noticed that ConvertToBool() is missing the trivial case bool:. If you think it's worth using ConvertToBool() here, the fix of adding the trivial case could be done in this PR as well.

I'm getting some conflicts when I try to import pipeline/utils:

$ make build /home/julien/dev/flowlogs-pipeline/bin/golangci-lint-v1.50.1 run --enable goimports --enable gofmt --enable ineffassign --timeout 5m ERRO Running error: context loading failed: failed to load packages: failed to load with go/packages: internal error: go list gives conflicting information for package github.com/netobserv/flowlogs-pipeline/pkg/config [github.com/netobserv/flowlogs-pipeline/pkg/config.test] make: *** [Makefile:70: lint] Error 3

Any idea ? Should we move utils outside of the pipeline package ?

I think this errors implies a cyclic reference: config imports pipeline/utils and pipeline/utils imports config.
Looking at pipeline/utils files, it seems that only batcher.go has an import to config.

flowlogs-pipeline/pkg/pipeline/utils/batcher.go

Line 6 in 3f2481e

"github.com/netobserv/flowlogs-pipeline/pkg/config"

Hm... I'm not sure what would be the most proper solution in this case.

Yeah so we should either move away Convert* functions or batcher.go from pipeline/utils...
Or keep as is.

I think I would go with the first option of moving convert.go to an "FLP-independent" utils package.

70514bf here we go

jpinsonneau · 2023-02-15T15:35:24Z

/retest

jpinsonneau · 2023-02-16T15:04:08Z

rebased to pass the go 1.19 e2e checks

jotak · 2023-02-16T20:37:19Z

@ronensc @jpinsonneau I haven't reviewed the code, but what does it mean "This PR skip duplicates in connection tracking aggregations." ? iirc we wanted to keep duplicated on purpose because it gives information about the network interfaces. But probably I don't have all the context, I will do a better review hopefully tomorrow

jpinsonneau · 2023-02-17T16:16:40Z

@ronensc @jpinsonneau I haven't reviewed the code, but what does it mean "This PR skip duplicates in connection tracking aggregations." ? iirc we wanted to keep duplicated on purpose because it gives information about the network interfaces. But probably I don't have all the context, I will do a better review hopefully tomorrow

Connection tracking implements aggregators (sum, count, min, max) based on its flows. However, duplicates were initially counted and the aggregators were wrong.

This PR simply skip flows containing "Duplicate": true in aggregators calculations. The duplicated flows are still part of the connection since they share the same hashId.

For performance reasons, we reuse Duplicate field from netobserv/netobserv-ebpf-agent#66

jpinsonneau · 2023-02-17T16:42:08Z

/approve

openshift-ci · 2023-02-17T16:42:13Z

[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:

~~OWNERS~~ [jpinsonneau]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jotak · 2023-02-17T18:14:59Z

Hmm ok this change looks good, but it doesn't address the "group ingress+egress" req, as the PR title shows, does it? Is there another PR for that?

openshift-ci bot added the do-not-merge/work-in-progress label Feb 14, 2023

openshift-merge-robot added the needs-rebase label Feb 14, 2023

jpinsonneau force-pushed the 838 branch from ae2d7c2 to 308d02b Compare February 14, 2023 11:09

openshift-merge-robot removed the needs-rebase label Feb 14, 2023

jpinsonneau requested a review from ronensc February 14, 2023 12:37

jpinsonneau marked this pull request as ready for review February 14, 2023 12:37

openshift-ci bot removed the do-not-merge/work-in-progress label Feb 14, 2023

ronensc previously approved these changes Feb 15, 2023

View reviewed changes

openshift-ci bot assigned ronensc Feb 15, 2023

openshift-ci bot added lgtm and removed lgtm labels Feb 15, 2023

openshift-merge-robot dismissed ronensc’s stale review via ab58999 February 15, 2023 17:11

jpinsonneau mentioned this pull request Feb 16, 2023

NETOBSERV-871 UI: Connection tracking query option netobserv/network-observability-console-plugin#285

Merged

jpinsonneau added 3 commits February 16, 2023 16:03

conntrack skip duplicates

f6d00ba

addressed feedback

9b86654

use ConvertToBool to check duplicate

2292626

jpinsonneau force-pushed the 838 branch from 70514bf to 2292626 Compare February 16, 2023 15:03

ronensc approved these changes Feb 16, 2023

View reviewed changes

openshift-ci bot added the lgtm label Feb 16, 2023

openshift-ci bot added the approved label Feb 17, 2023

openshift-merge-robot merged commit 3f1a39f into netobserv:main Feb 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NETOBSERV-838 Group ingress+egress flows in same connection #387

NETOBSERV-838 Group ingress+egress flows in same connection #387

jpinsonneau commented Feb 14, 2023 •

edited

Loading

codecov-commenter commented Feb 14, 2023

jpinsonneau commented Feb 14, 2023

ronensc left a comment

ronensc Feb 15, 2023

jpinsonneau Feb 15, 2023

jpinsonneau Feb 15, 2023

ronensc Feb 15, 2023

jpinsonneau Feb 15, 2023

ronensc Feb 15, 2023

jpinsonneau Feb 16, 2023

ronensc Feb 16, 2023

jpinsonneau Feb 16, 2023

jpinsonneau commented Feb 15, 2023

jpinsonneau commented Feb 16, 2023

jotak commented Feb 16, 2023

jpinsonneau commented Feb 17, 2023

jpinsonneau commented Feb 17, 2023

openshift-ci bot commented Feb 17, 2023

jotak commented Feb 17, 2023

		@@ -160,8 +160,10 @@ func (ct *conntrackImpl) prepareHeartbeatRecords() []config.GenericMap {

		func (ct *conntrackImpl) updateConnection(conn connection, flowLog config.GenericMap, flowLogHash totalHashType) {
		d := ct.getFlowLogDirection(conn, flowLogHash)

NETOBSERV-838 Group ingress+egress flows in same connection #387

NETOBSERV-838 Group ingress+egress flows in same connection #387

Conversation

jpinsonneau commented Feb 14, 2023 • edited Loading

codecov-commenter commented Feb 14, 2023

Codecov Report

jpinsonneau commented Feb 14, 2023

ronensc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jpinsonneau commented Feb 15, 2023

jpinsonneau commented Feb 16, 2023

jotak commented Feb 16, 2023

jpinsonneau commented Feb 17, 2023

jpinsonneau commented Feb 17, 2023

openshift-ci bot commented Feb 17, 2023

jotak commented Feb 17, 2023

jpinsonneau commented Feb 14, 2023 •

edited

Loading