Move the go-ipfix tag after adding performance fixes #2574

srikartati · 2021-08-11T16:25:52Z

Performance fixes for memory fragmentation issue in go-ipfix. Moving the release tag of go-ipfix.

lgtm-com · 2021-08-11T16:59:14Z

This pull request introduces 90 alerts when merging 39bd139 into fe4457f - view on LGTM.com

new alerts:

90 for Useless assignment to field

codecov-commenter · 2021-08-11T21:17:38Z

Codecov Report

Merging #2574 (7d758ef) into main (535b7a3) will increase coverage by 0.08%.
The diff coverage is 95.00%.

@@            Coverage Diff             @@
##             main    #2574      +/-   ##
==========================================
+ Coverage   60.10%   60.18%   +0.08%     
==========================================
  Files         282      282              
  Lines       22337    22345       +8     
==========================================
+ Hits        13426    13449      +23     
+ Misses       7491     7480      -11     
+ Partials     1420     1416       -4

Flag	Coverage Δ
e2e-tests	`∅ <ø> (?)`
kind-e2e-tests	`47.09% <95.00%> (+0.15%)`	⬆️
unit-tests	`42.16% <30.00%> (-0.01%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/flowaggregator/flowaggregator.go	`64.72% <94.11%> (+0.78%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`80.23% <100.00%> (+0.09%)`	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
pkg/ipfix/ipfix_process.go	`81.81% <0.00%> (-18.19%)`	⬇️
pkg/ovs/ovsctl/appctl.go	`32.60% <0.00%> (-11.96%)`	⬇️
pkg/agent/client.go	`77.41% <0.00%> (-3.23%)`	⬇️
pkg/agent/agent.go	`50.53% <0.00%> (+0.21%)`	⬆️
pkg/agent/openflow/client.go	`59.07% <0.00%> (+0.72%)`	⬆️
pkg/apiserver/storage/ram/store.go	`80.45% <0.00%> (+1.50%)`	⬆️
... and 3 more

antoninbas · 2021-08-11T22:53:02Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -548,13 +548,14 @@ func (exp *flowExporter) addRecordToSet(record flowexporter.FlowRecord) error {
 		case "flowType":
 			ie.Value = exp.findFlowType(record.Conn)
 		}
+		eL[i] = ie


I don't know if that makes a copy, but I image so and to be safe you should use:

for i := range eL { ie := &eL[i] // ... }

same comment for other loop below

Any reference for this? Thanks.
I can confirm that all the e2e tests and unit tests covering this code are passing.

it's not about correctness, but you are copying the element back into the list so it's slower for no reason

see this benchmark:

package main import ( "testing" ) // Without Big // BenchmarkCopy-12 1000000 1151 ns/op // BenchmarkRef-12 1000000 1027 ns/op // With Big // BenchmarkCopy-12 46578 23557 ns/op // BenchmarkRef-12 869930 1425 ns/op type Data struct { A int64 B int64 C int64 D int64 Big [32]int64 } func makeList(N int) []Data { return make([]Data, N) } func iterCopy(l []Data) []Data { for i, e := range l { e.A += 1 e.B += 1 e.C += 1 e.D += 1 l[i] = e } return l } func iterRef(l []Data) []Data { for i := range l { e := &l[i] e.A += 1 e.B += 1 e.C += 1 e.D += 1 } return l } func BenchmarkCopy(b *testing.B) { l := makeList(1000) b.ResetTimer() for i := 0; i < b.N; i++ { iterCopy(l) } } func BenchmarkRef(b *testing.B) { l := makeList(1000) b.ResetTimer() for i := 0; i < b.N; i++ { iterRef(l) } }

The performance difference can vary significantly based on element size.

I missed the performance reference in your comment.
Here the object is not big. It has a pointer and value of type interface. As you mentioned, better to be safe.
Thanks for the catch.

antoninbas · 2021-08-11T22:55:29Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -388,7 +388,7 @@ func (exp *flowExporter) sendTemplateSet(isIPv6 bool) (int, error) {
 	if err := exp.ipfixSet.PrepareSet(ipfixentities.Template, templateID); err != nil {
 		return 0, err
 	}
-	err := exp.ipfixSet.AddRecord(elements, templateID)
+	err := exp.ipfixSet.AddRecord(elements, 0, templateID)


it would have been good (or it would be good if it's not too late) to define 2 different methods in go-ipfix: AddRecord and AddRecordWithExtraElements. It would avoid these cryptic changes in Antrea and it's probably good to keep backward compatibility for your library when you can...

Good suggestion.

antoninbas

there are several CI failures that need to be addressed

antoninbas · 2021-08-12T17:41:16Z

pkg/flowaggregator/flowaggregator.go

@@ -341,7 +342,7 @@ func (fa *flowAggregator) initExportingProcess() error {
 			CollectorProtocol:   fa.externalFlowCollectorProto,
 			ObservationDomainID: fa.observationDomainID,
 			TempRefTimeout:      1800,
-			PathMTU:             0,
+			PathMTU:             1400,


this needs a comment...
it also seems unrelated to the rest of the PR

Yes, it was not meant to be added. Removed it.

srikartati · 2021-08-12T20:03:39Z

Thanks for the review @antoninbas
There are some more fixes for perf improvement in go-ipfix. I will update the release tag further after they go in.

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

srikartati · 2021-08-13T00:21:56Z

/test-all

antoninbas

LGTM

antoninbas · 2021-08-13T00:35:39Z

pkg/flowaggregator/flowaggregator.go

@@ -279,6 +279,7 @@ func (fa *flowAggregator) InitCollectingProcess() error {
 			IsEncrypted:   false,
 		}
 	}
+	cpInput.NumExtraElements = len(antreaSourceStatsElementList) + len(antreaDestinationStatsElementList) + len(antreaLabelsElementList)


it's a bit strange that this doesn't match aggregationElements which is what I would expect. I guess I am not familiar enough with go-ipfix.

aggregationElements contains all elements that needs an update as we receive flow record. The numExtraElements are the elements that are added newly after receiving the flow records.

srikartati · 2021-08-13T01:50:22Z

/test-conformance

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

srikartati force-pushed the patch_flow_agg branch 4 times, most recently from 3ad510f to 39bd139 Compare August 11, 2021 16:34

srikartati marked this pull request as draft August 11, 2021 17:13

srikartati force-pushed the patch_flow_agg branch from 39bd139 to fd82de1 Compare August 11, 2021 20:43

srikartati marked this pull request as ready for review August 11, 2021 20:45

srikartati force-pushed the patch_flow_agg branch 2 times, most recently from 8356d86 to 1323069 Compare August 11, 2021 22:41

antoninbas reviewed Aug 11, 2021

View reviewed changes

srikartati force-pushed the patch_flow_agg branch from 1323069 to 58af088 Compare August 12, 2021 05:32

antoninbas reviewed Aug 12, 2021

View reviewed changes

srikartati force-pushed the patch_flow_agg branch from 58af088 to 5d42d19 Compare August 13, 2021 00:17

Move the go-ipfix tag after adding performance fixes:

7d758ef

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

srikartati force-pushed the patch_flow_agg branch from 5d42d19 to 7d758ef Compare August 13, 2021 00:18

antoninbas approved these changes Aug 13, 2021

View reviewed changes

antoninbas merged commit bdddc01 into antrea-io:main Aug 13, 2021

antoninbas mentioned this pull request Aug 13, 2021

Automated cherry pick of #2574: Move the go-ipfix tag after adding performance fixes: #2597

Merged

srikartati deleted the patch_flow_agg branch August 13, 2021 05:17

annakhm pushed a commit to annakhm/antrea that referenced this pull request Aug 16, 2021

Move the go-ipfix tag after adding performance fixes: (antrea-io#2574)

c304322

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

zyiou pushed a commit to zyiou/antrea that referenced this pull request Aug 17, 2021

Move the go-ipfix tag after adding performance fixes: (antrea-io#2574)

61fce03

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

zyiou pushed a commit to zyiou/antrea that referenced this pull request Aug 17, 2021

Move the go-ipfix tag after adding performance fixes: (antrea-io#2574)

f37658b

Identified and fixed performance fixes. Moving the tag. Signed-off-by: Srikar Tati <stati@vmware.com>

Move the go-ipfix tag after adding performance fixes #2574

Move the go-ipfix tag after adding performance fixes #2574

srikartati commented Aug 11, 2021 •

edited

lgtm-com bot commented Aug 11, 2021

codecov-commenter commented Aug 11, 2021 •

edited

antoninbas Aug 11, 2021

srikartati Aug 11, 2021 •

edited

antoninbas Aug 12, 2021

srikartati Aug 12, 2021

antoninbas Aug 11, 2021

srikartati Aug 11, 2021

antoninbas left a comment

antoninbas Aug 12, 2021

srikartati Aug 13, 2021

srikartati commented Aug 12, 2021

srikartati commented Aug 13, 2021

antoninbas left a comment

antoninbas Aug 13, 2021

srikartati Aug 13, 2021

srikartati commented Aug 13, 2021

Move the go-ipfix tag after adding performance fixes #2574

Move the go-ipfix tag after adding performance fixes #2574

Conversation

srikartati commented Aug 11, 2021 • edited

lgtm-com bot commented Aug 11, 2021

codecov-commenter commented Aug 11, 2021 • edited

Codecov Report

Choose a reason for hiding this comment

srikartati Aug 11, 2021 • edited

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

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati commented Aug 12, 2021

srikartati commented Aug 13, 2021

antoninbas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati commented Aug 13, 2021

srikartati commented Aug 11, 2021 •

edited

codecov-commenter commented Aug 11, 2021 •

edited

srikartati Aug 11, 2021 •

edited