Fix the bug in Flow Exporter that cause intermittent errors in e2e tests #1959

srikartati · 2021-03-16T04:21:31Z

The bug is because IPFIX exporting process interface in Flow Exporter is assigned with typecasted nil (typecasted to corresponding structure ipfixExportingProcess). This code was modified in PR #1714 . Therefore, the code comparing with nil breaks in exporting process initialization code of Flow Exporter.
This PR fixes the bug along with another issue in fetching certificates that exacerbates this bug when the Flow Exporter cannot connect to the Flow Aggregator.

Fixes #1956

codecov-io · 2021-03-16T04:25:55Z

Codecov Report

Merging #1959 (14fb9b5) into main (020946f) will decrease coverage by 14.77%.
The diff coverage is 43.54%.

❗ Current head 14fb9b5 differs from pull request most recent head e19983a. Consider uploading reports for the commit e19983a to get more accurate results

@@             Coverage Diff             @@
##             main    #1959       +/-   ##
===========================================
- Coverage   64.37%   49.60%   -14.78%     
===========================================
  Files         193      189        -4     
  Lines       16967    16109      -858     
===========================================
- Hits        10923     7991     -2932     
- Misses       4885     7056     +2171     
+ Partials     1159     1062       -97

Flag	Coverage Δ
e2e-tests	`49.60% <43.54%> (?)`
kind-e2e-tests	`?`
unit-tests	`?`

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

Impacted Files	Coverage Δ
pkg/agent/agent.go	`51.55% <0.00%> (+3.83%)`	⬆️
pkg/agent/agent_linux.go	`100.00% <ø> (ø)`
pkg/agent/openflow/client_other.go	`0.00% <0.00%> (ø)`
pkg/agent/openflow/pipeline.go	`66.10% <14.28%> (+0.65%)`	⬆️
pkg/agent/flowexporter/exporter/exporter.go	`70.48% <64.28%> (+2.41%)`	⬆️
pkg/agent/openflow/client.go	`62.57% <66.66%> (+2.39%)`	⬆️
pkg/agent/openflow/pipeline_other.go	`66.66% <66.66%> (ø)`
pkg/controller/networkpolicy/crd_utils.go	`0.00% <0.00%> (-90.22%)`	⬇️
pkg/controller/networkpolicy/status_controller.go	`0.00% <0.00%> (-88.82%)`	⬇️
...g/controller/networkpolicy/clusternetworkpolicy.go	`0.00% <0.00%> (-87.15%)`	⬇️
... and 113 more

srikartati · 2021-03-16T04:48:10Z

/test-e2e

srikartati · 2021-03-16T17:14:51Z

/test-e2e

srikartati · 2021-03-16T20:21:55Z

/test-e2e

srikartati · 2021-03-17T01:29:03Z

/test-e2e

srikartati · 2021-03-17T03:40:25Z

/test-e2e

srikartati · 2021-03-17T05:11:06Z

/test-e2e

srikartati · 2021-03-17T06:04:18Z

/test-e2e

srikartati · 2021-03-17T06:49:25Z

/test-e2e

srikartati · 2021-03-17T14:42:32Z

/test-all

srikartati · 2021-03-17T16:09:12Z

/test-e2e

srikartati · 2021-03-17T16:58:29Z

All e2e test runs are successful (total 8 runs) after this fix except for two times when the connection to harbor registry failed.

First run: https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2203/
Failed runs due to harbor registry unreachability:https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2205/
https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2206/
Last run successful: https://jenkins.antrea-ci.rocks/job/antrea-e2e-for-pull-request/2214/

tnqn · 2021-03-17T16:05:18Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -115,33 +118,6 @@ func prepareExporterInputArgs(collectorAddr, collectorProto string, enableTLSToF
 	if err != nil {
 		return expInput, err
 	}
-
-	if enableTLSToFlowAggregator {


should the unused parameters be removed?

Yes. Missed it.

tnqn · 2021-03-17T16:14:43Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -191,7 +169,9 @@ func (exp *flowExporter) Export() {
 			// There could be other errors while initializing flow exporter other than connecting to IPFIX collector,
 			// therefore closing the connection and resetting the process.
 			if exp.process != nil {
-				exp.process.CloseConnToCollector()
+				if !(reflect.ValueOf(exp.process).Kind() == reflect.Ptr && reflect.ValueOf(exp.process).IsNil()) {


I think this can be avoided by only setting exp.process when the funtion that constructs it doesn't return error, then we won't assign nil of a concrete type to it and can simply check nil when using exp.process.
For instance:

process, err = ipfix.NewIPFIXExportingProcess(exp.exporterInput) if err != nil { return fmt.Errorf("error when starting exporter: %v", err) } exp.process = process

You are right. This also explains why this crash was not happening before with similar scenarios even though they are not that often. The code was like this before PR #1714 , so it was not an issue before. Thanks for the pointer. Fixed it.

CI e2e tests are failing during or after TestFlowAggregator Errors are happening when flow aggregator service DNS resolution is not successful. I was able to get the backtrace that leads to crashing the agent. Added fix when checking the interface for nil to resolve it.

srikartati · 2021-03-17T17:34:26Z

/test-all

antoninbas

LGTM, but I wish the commit title and message were more helpful and accurate.
They should probably focus on the nature of the bug being fixed instead of the e2e test failure which is a consequence of the bug.

srikartati · 2021-03-17T18:46:02Z

/test-conformance

srikartati · 2021-03-17T19:11:25Z

LGTM, but I wish the commit title and message were more helpful and accurate.
They should probably focus on the nature of the bug being fixed instead of the e2e test failure which is a consequence of the bug.

Thanks for the review. Added that. I originally created PR to debug the e2e tests in CI, so overlooked that part.

vmwclabot added the cla-not-required label Mar 16, 2021

srikartati force-pushed the fix_e2e_test branch from 129cf4b to c1c140d Compare March 16, 2021 04:47

srikartati force-pushed the fix_e2e_test branch 2 times, most recently from 157b87d to fbc63ad Compare March 16, 2021 17:14

srikartati force-pushed the fix_e2e_test branch 3 times, most recently from 92c0032 to 869ab55 Compare March 16, 2021 20:15

srikartati force-pushed the fix_e2e_test branch 2 times, most recently from 6ef5f00 to ca54d93 Compare March 17, 2021 01:27

srikartati force-pushed the fix_e2e_test branch from ca54d93 to bf78427 Compare March 17, 2021 03:27

srikartati changed the title ~~[Not for review]Debug errors in CI runs~~ Fix Flow Aggregator e2e test error in CI Mar 17, 2021

srikartati requested a review from antoninbas March 17, 2021 03:32

srikartati mentioned this pull request Mar 17, 2021

e2e cases failed quite often after testing TestFlowAggregator #1956

Closed

srikartati requested review from tnqn and jianjuns March 17, 2021 03:39

srikartati force-pushed the fix_e2e_test branch 2 times, most recently from 4342b4d to e19983a Compare March 17, 2021 04:02

tnqn reviewed Mar 17, 2021

View reviewed changes

srikartati force-pushed the fix_e2e_test branch from e19983a to bd905ca Compare March 17, 2021 17:33

antoninbas reviewed Mar 17, 2021

View reviewed changes

srikartati changed the title ~~Fix Flow Aggregator e2e test error in CI~~ Fix the bug introduced in PR #1714 that cause intermittent errors in e2e tests Mar 17, 2021

srikartati changed the title ~~Fix the bug introduced in PR #1714 that cause intermittent errors in e2e tests~~ Fix the bug in Flow Exporter that cause intermittent errors in e2e tests Mar 17, 2021

srikartati requested a review from antoninbas March 17, 2021 19:28

antoninbas approved these changes Mar 17, 2021

View reviewed changes

srikartati merged commit a9adf5a into antrea-io:main Mar 17, 2021

srikartati deleted the fix_e2e_test branch March 18, 2021 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix the bug in Flow Exporter that cause intermittent errors in e2e tests #1959

Fix the bug in Flow Exporter that cause intermittent errors in e2e tests #1959

srikartati commented Mar 16, 2021 •

edited

Loading

codecov-io commented Mar 16, 2021 •

edited

Loading

srikartati commented Mar 16, 2021

srikartati commented Mar 16, 2021

srikartati commented Mar 16, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021 •

edited

Loading

tnqn Mar 17, 2021

srikartati Mar 17, 2021

tnqn Mar 17, 2021

srikartati Mar 17, 2021

srikartati commented Mar 17, 2021

antoninbas left a comment

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

Fix the bug in Flow Exporter that cause intermittent errors in e2e tests #1959

Fix the bug in Flow Exporter that cause intermittent errors in e2e tests #1959

Conversation

srikartati commented Mar 16, 2021 • edited Loading

codecov-io commented Mar 16, 2021 • edited Loading

Codecov Report

srikartati commented Mar 16, 2021

srikartati commented Mar 16, 2021

srikartati commented Mar 16, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021 • edited Loading

tnqn Mar 17, 2021

Choose a reason for hiding this comment

srikartati Mar 17, 2021

Choose a reason for hiding this comment

tnqn Mar 17, 2021

Choose a reason for hiding this comment

srikartati Mar 17, 2021

Choose a reason for hiding this comment

srikartati commented Mar 17, 2021

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Mar 17, 2021

srikartati commented Mar 17, 2021

srikartati commented Mar 16, 2021 •

edited

Loading

codecov-io commented Mar 16, 2021 •

edited

Loading

srikartati commented Mar 17, 2021 •

edited

Loading