conntrack: Configurable timeouts #357

ronensc · 2022-12-26T09:40:31Z

This PR allows configuring different timeouts for connections based on their key fields.

conntrack:
  keyDefinition:
    fieldGroups:
    - name: protocol
      fields:
      - Proto
    # ...
  scheduling:
  - selector: # UDP connections
      Proto: 17
    endConnectionTimeout: 5s
    updateConnectionInterval: 40s
  - selector: {} # Default group
    endConnectionTimeout: 10s
    updateConnectionInterval: 30s

Implementation: Instead of using a single Multi-Order Map in the connection store for all the connections, the connection store will have a Multi-Order Map for each scheduling group. In addition, the store has a new hashId2groupIdx map that given a connection hash ID, finds the Multi-Order Map of the connection.

Other changes:

Add a scheduling group label to the conntrack_memory_connections operational metric. This allows monitoring the number of active connections per scheduling group.

codecov-commenter · 2022-12-26T10:16:38Z

Codecov Report

Merging #357 (6a0f65b) into main (b6979fe) will decrease coverage by 1.00%.
The diff coverage is 55.37%.

📣 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     #357      +/-   ##
==========================================
- Coverage   61.91%   60.91%   -1.00%     
==========================================
  Files          91       91              
  Lines        5873     6177     +304     
==========================================
+ Hits         3636     3763     +127     
- Misses       2016     2192     +176     
- Partials      221      222       +1

Flag	Coverage Δ
unittests	`60.91% <55.37%> (-1.00%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/pipeline/utils/convert.go	`7.65% <5.29%> (-10.54%)`	⬇️
pkg/pipeline/extract/conntrack/conn.go	`75.70% <59.18%> (-14.47%)`	⬇️
pkg/pipeline/extract/conntrack/store.go	`84.17% <97.22%> (+22.17%)`	⬆️
pkg/api/conntrack.go	`89.68% <100.00%> (+2.68%)`	⬆️
pkg/operational/metrics.go	`68.00% <100.00%> (-5.05%)`	⬇️
pkg/pipeline/extract/conntrack/conntrack.go	`92.20% <100.00%> (-0.74%)`	⬇️
pkg/pipeline/extract/conntrack/metrics.go	`100.00% <100.00%> (ø)`
pkg/pipeline/extract/timebased/heap.go	`97.82% <0.00%> (-2.18%)`	⬇️

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

github-actions · 2023-01-16T11:22:43Z

New image: ["quay.io/netobserv/flowlogs-pipeline:56612e6"]. It will expire after two weeks.

github-actions · 2023-01-19T13:27:58Z

New image: ["quay.io/netobserv/flowlogs-pipeline:fb0e3bb"]. It will expire after two weeks.

KalmanMeth · 2023-01-19T13:30:46Z

pkg/api/conntrack.go

@@ -83,6 +82,12 @@ type ConnTrackOperationEnum struct {
 	Max   string `yaml:"max" doc:"max"`
 }

+type ConnTrackSchedulingGroup struct {
+	Selector                 map[string]string `yaml:"selector,omitempty" doc:"key-value map to match against connection fields to apply this scheduling"`


Selector matches only string: string. Is this the intended usage? So we cannot, for instance, select on integer values?

We can select integer values. The integer values are converted to string before the comparison.

At first, I used map[string]interface{}. But then I noticed that setting the selector to Proto: 6 is parsed as int. The problem is that the grpc ingester parses Proto as uint32. Comparing int and uint32 is evaluated to false regardless of the values. That's why I convert and compare strings.

pkg/api/conntrack_test.go

jotak · 2023-01-25T22:34:03Z

pkg/pipeline/extract/conntrack/conntrack.go

 	return outputRecords
 }

+// TBD: think of how to avoid confusion with the word "update". It is used with periodic connections updates and for update a connection with incoming flow logs.


Just a suggestion, you could use the terminology of "heartbeats" to refer to the periodic updates on long connections?

I like this idea! thanks @jotak :)

github-actions · 2023-01-30T10:01:30Z

New image: . It will expire after two weeks.

jotak

lgtm, nice PR!

ronensc · 2023-02-01T16:13:59Z

I tired to improve isMatchSelector(). Instead of converting both values to string and compare strings,
I added a check for the type of the value and convert the selector to that type. So at the end we have only one conversion and the comparison is in the original type.
Currently, I implemented checks only for uint32, uint64, int64, int and bool since these types are in use by the protobuf decoder. Other types fallback to the string conversion-comparison.
I added a benchmark test to compare the performance of both approaches.
It seems that the new approach is faster. But, I'm not pleased with the code. It feels too verbose to me.
@jotak @KalmanMeth wdyt?, do you have ideas for improving?

goos: linux
goarch: amd64
pkg: github.com/netobserv/flowlogs-pipeline/pkg/pipeline/extract/conntrack
cpu: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
BenchmarkIsMatchSelectorStrings
BenchmarkIsMatchSelectorStrings/Proto_1
BenchmarkIsMatchSelectorStrings/Proto_1-4         	10409644	       105.8 ns/op
BenchmarkIsMatchSelectorStrings/Proto_2
BenchmarkIsMatchSelectorStrings/Proto_2-4         	12772057	        84.20 ns/op
BenchmarkIsMatchSelectorStrings/Multiple_fields_1
BenchmarkIsMatchSelectorStrings/Multiple_fields_1-4         	 3224965	       393.5 ns/op
BenchmarkIsMatchSelectorStrings/Multiple_fields_2
BenchmarkIsMatchSelectorStrings/Multiple_fields_2-4         	 4209242	       280.4 ns/op
BenchmarkIsMatchSelectorStrings/string_match
BenchmarkIsMatchSelectorStrings/string_match-4              	 9767826	       115.9 ns/op
BenchmarkIsMatchSelectorStrings/string_mismatch
BenchmarkIsMatchSelectorStrings/string_mismatch-4           	12365690	        97.28 ns/op
BenchmarkIsMatchSelectorStrings/custom_match
BenchmarkIsMatchSelectorStrings/custom_match-4              	 2459720	       471.9 ns/op
BenchmarkIsMatchSelectorStrings/custom_mismatch
BenchmarkIsMatchSelectorStrings/custom_mismatch-4           	 2666076	       447.7 ns/op


BenchmarkIsMatchSelectorGeneric
BenchmarkIsMatchSelectorGeneric/Proto_1
BenchmarkIsMatchSelectorGeneric/Proto_1-4                   	26646829	        46.48 ns/op
BenchmarkIsMatchSelectorGeneric/Proto_2
BenchmarkIsMatchSelectorGeneric/Proto_2-4                   	34333780	        34.89 ns/op
BenchmarkIsMatchSelectorGeneric/Multiple_fields_1
BenchmarkIsMatchSelectorGeneric/Multiple_fields_1-4         	 4840779	       243.1 ns/op
BenchmarkIsMatchSelectorGeneric/Multiple_fields_2
BenchmarkIsMatchSelectorGeneric/Multiple_fields_2-4         	 5974236	       211.2 ns/op
BenchmarkIsMatchSelectorGeneric/string_match
BenchmarkIsMatchSelectorGeneric/string_match-4              	10744186	       119.4 ns/op
BenchmarkIsMatchSelectorGeneric/string_mismatch
BenchmarkIsMatchSelectorGeneric/string_mismatch-4           	11647137	        99.56 ns/op
BenchmarkIsMatchSelectorGeneric/custom_match
BenchmarkIsMatchSelectorGeneric/custom_match-4              	 1920768	       618.1 ns/op
BenchmarkIsMatchSelectorGeneric/custom_mismatch
BenchmarkIsMatchSelectorGeneric/custom_mismatch-4           	 2170971	       533.2 ns/op
PASS

pkg/pipeline/extract/conntrack/conn.go

pkg/pipeline/extract/conntrack/conntrack.go

jotak · 2023-02-07T08:59:16Z

(and sorry for the delay!)
/lgtm

… each group

Some of the grpc ingester fields are of type uint32. The yaml/json parser parses numeric values as int32. So, comparing uint32 to int32 where both static types are interface{} will always evaluate to false regardless of the actual values. Hence, we convert and compare strings.

ronensc force-pushed the conntrack-timeouts branch from a462f5d to 8c37ec2 Compare January 16, 2023 10:17

ronensc added the breaking-change This pull request has breaking changes. They should be described in PR description. label Jan 16, 2023

ronensc marked this pull request as ready for review January 16, 2023 10:56

ronensc requested review from jotak, KalmanMeth and eranra and removed request for jotak and KalmanMeth January 16, 2023 10:56

ronensc added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jan 16, 2023

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jan 19, 2023

ronensc added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jan 19, 2023

KalmanMeth reviewed Jan 19, 2023

View reviewed changes

pkg/api/conntrack_test.go Outdated Show resolved Hide resolved

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jan 23, 2023

jotak reviewed Jan 25, 2023

View reviewed changes

ronensc mentioned this pull request Jan 30, 2023

conntrack: Rename "updateConnection" to "heartbeatConnection" #370

Closed

jotak added the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Jan 30, 2023

jotak approved these changes Jan 30, 2023

View reviewed changes

github-actions bot removed the ok-to-test To set manually when a PR is safe to test. Triggers image build on PR. label Feb 1, 2023

ronensc mentioned this pull request Feb 2, 2023

Rename updateConnection -> heartbeat #373

Merged

KalmanMeth reviewed Feb 6, 2023

View reviewed changes

pkg/pipeline/extract/conntrack/conn.go Outdated Show resolved Hide resolved

KalmanMeth reviewed Feb 6, 2023

View reviewed changes

pkg/pipeline/extract/conntrack/conntrack.go Show resolved Hide resolved

ronensc added 25 commits February 8, 2023 15:01

Remove old timeout and interval from config

53e7e9c

Remove TBD comments

7fb022d

Update comments

a076496

groupId->groupIdx

4c8ef44

Add comment

52779f6

Update comment

e8c3421

Rename ConnTrackSchedulingSelector -> ConnTrackSchedulingGroup

01b96b6

Fix tests

2ca544d

Fix bug in tests

53fa859

Add test for scheduling groups

8fa88fc

Update README.md

79f45c3

Make linter happy

cf77a75

Split metric conntrack_memory_connections into scheduling groups

d661733

Change maps to slices for efficiency

52a51b7

Join multiple slices into a single slice

a4f5cc1

Fix metric conntrack_memory_connections to show the correct length of…

0b7691f

… each group

Remove entries from hashId2groupIdx map on connection removal

a8a63f9

Update configurations

152b9b6

Add test case for 2 default selectors

ad30223

Add special cases conversion and benchmark

551dcd0

Make linter gofmt happy

9618027

Fix default and string cases and add test cases

75c0aae

Cleanup

59aa2db

Fix rebase

6a0f65b

ronensc force-pushed the conntrack-timeouts branch from 827678c to 6a0f65b Compare February 8, 2023 13:27

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

ronensc merged commit eaac68e into netobserv:main Feb 8, 2023

ronensc deleted the conntrack-timeouts branch February 8, 2023 14:09

ronensc mentioned this pull request Feb 9, 2023

NETOBSERV-567 Enable connection tracking in NOO netobserv/network-observability-operator#252

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

conntrack: Configurable timeouts #357

conntrack: Configurable timeouts #357

ronensc commented Dec 26, 2022 •

edited

Loading

codecov-commenter commented Dec 26, 2022 •

edited

Loading

github-actions bot commented Jan 16, 2023

github-actions bot commented Jan 19, 2023

KalmanMeth Jan 19, 2023

ronensc Jan 19, 2023

jotak Jan 25, 2023

ronensc Jan 30, 2023

github-actions bot commented Jan 30, 2023

jotak left a comment

ronensc commented Feb 1, 2023 •

edited

Loading

jotak commented Feb 7, 2023

conntrack: Configurable timeouts #357

conntrack: Configurable timeouts #357

Conversation

ronensc commented Dec 26, 2022 • edited Loading

codecov-commenter commented Dec 26, 2022 • edited Loading

Codecov Report

github-actions bot commented Jan 16, 2023

github-actions bot commented Jan 19, 2023

KalmanMeth Jan 19, 2023

Choose a reason for hiding this comment

ronensc Jan 19, 2023

Choose a reason for hiding this comment

jotak Jan 25, 2023

Choose a reason for hiding this comment

ronensc Jan 30, 2023

Choose a reason for hiding this comment

github-actions bot commented Jan 30, 2023

jotak left a comment

Choose a reason for hiding this comment

ronensc commented Feb 1, 2023 • edited Loading

jotak commented Feb 7, 2023

ronensc commented Dec 26, 2022 •

edited

Loading

codecov-commenter commented Dec 26, 2022 •

edited

Loading

ronensc commented Feb 1, 2023 •

edited

Loading