[Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy #1682

zyiou · 2020-12-21T20:35:04Z

This PR is a follow-up fix for bumping go-ipfix to v0.4.2 with following changes:

add destinationServicePort, egressNetworkPolicyName and egressNetworkPolicyNamespace in correlation fields
add stats update function in flow-exporter without which the delta values are 0 (found in e2e tests)
add network policy testing for inter Node flows
fix bandwidth unit parsing problem

srikartati

LGTM. Thanks for root causing it quickly.
As I already have the PR to do the merging of upstream/feature/flow-aggregator with upstream/master, additional commits on upstream/feature/flow-aggregator mean more massaging on the local branch--I already have one local commit fixing the flakiness.
Can you merge it with master and include the network policies in interNodeFlows to check for fixed correlation? You should wait for my PR to get merged with the master.

srikartati · 2020-12-21T20:42:18Z

pkg/agent/flowexporter/exporter/exporter.go

@@ -273,6 +273,9 @@ func (exp *flowExporter) sendFlowRecords() error {
 				return err
 			}
 		}
+		if err := exp.flowRecords.ValidateAndUpdateStats(key, record); err != nil {


Oh.. this was there before right? That is a good amount of logic missing :)

Yea.. I missed this part when changing the code.

codecov-io · 2020-12-21T21:13:56Z

Codecov Report

❗ No coverage uploaded for pull request base (feature/flow-aggregator@179a472). Click here to learn what that means.
The diff coverage is n/a.

@@                    Coverage Diff                     @@
##             feature/flow-aggregator    #1682   +/-   ##
==========================================================
  Coverage                           ?   40.34%           
==========================================================
  Files                              ?      107           
  Lines                              ?    13150           
  Branches                           ?        0           
==========================================================
  Hits                               ?     5306           
  Misses                             ?     7363           
  Partials                           ?      481

Flag	Coverage Δ
unit-tests	`40.34% <0.00%> (?)`

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

zyiou · 2020-12-21T21:39:21Z

LGTM. Thanks for root causing it quickly.
As I already have the PR to do the merging of upstream/feature/flow-aggregator with upstream/master, additional commits on upstream/feature/flow-aggregator mean more massaging on the local branch--I already have one local commit fixing the flakiness.
Can you merge it with master and include the network policies in interNodeFlows to check for fixed correlation? Is that ok for you? You should wait for my PR to get merged with master.

Sure. will add the network policy changes in e2e tests.

antoninbas

LGTM - I assume we need to merge this before #1671?

srikartati · 2020-12-21T22:43:13Z

LGTM - I assume we need to merge this before #1671?

This can go in after PR #1671. Bandwidth tests are being skipped with octetDeltaCount check and correlation of network policy fields is not being validated in current e2e tests. We need to validate those and check-in this change to master after merging PR #1671.

antoninbas · 2020-12-21T23:00:37Z

LGTM - I assume we need to merge this before #1671?

This can go in after PR #1671. Bandwidth tests are being skipped with octetDeltaCount check and correlation of network policy fields is not being validated in current e2e tests. We need to validate those and check-in this change to master after merging PR #1671.

If this PR fixes a bug in #1671, then we should amend #1671 with this commit (to avoid checking in broken code in master)

zyiou · 2020-12-22T00:10:11Z

LGTM - I assume we need to merge this before #1671?

This can go in after PR #1671. Bandwidth tests are being skipped with octetDeltaCount check and correlation of network policy fields is not being validated in current e2e tests. We need to validate those and check-in this change to master after merging PR #1671.

If this PR fixes a bug in #1671, then we should amend #1671 with this commit (to avoid checking in broken code in master)

Sure. I will then rebase with flow/aggregator branch after I fix the e2e tests problem for bandwidth.

srikartati · 2020-12-22T03:45:04Z

Sure a couple of ways to go about this. I am updating the upstream/feature/flow-aggregator branch with my commits and new rebased master commits. We can push this commit and use PR #1671 to merge everything.

srikartati

LGTM

zyiou requested a review from srikartati December 21, 2020 20:35

vmwclabot added the cla-not-required label Dec 21, 2020

srikartati reviewed Dec 21, 2020

View reviewed changes

zyiou force-pushed the zyiou/test branch 6 times, most recently from 0de8880 to 6d41413 Compare December 21, 2020 22:05

antoninbas approved these changes Dec 21, 2020

View reviewed changes

zyiou force-pushed the zyiou/test branch from 6d41413 to 34d79b1 Compare December 22, 2020 02:08

zyiou changed the title ~~[Flow-Aggregator]Add correlate fields and stats update method~~ [Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy Dec 22, 2020

srikartati force-pushed the feature/flow-aggregator branch from 179a472 to 92b6976 Compare December 22, 2020 03:46

zyiou and others added 2 commits December 21, 2020 19:51

add correlate fields and stats update method

a9f5f22

add inter node network policy test and fix e2e testing problems

347746c

zyiou force-pushed the zyiou/test branch from 34d79b1 to 347746c Compare December 22, 2020 03:52

srikartati approved these changes Dec 22, 2020

View reviewed changes

srikartati merged commit a51e08d into antrea-io:feature/flow-aggregator Dec 22, 2020

zyiou deleted the zyiou/test branch February 24, 2021 19:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy #1682

[Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy #1682

zyiou commented Dec 21, 2020 •

edited

Loading

srikartati left a comment •

edited

Loading

srikartati Dec 21, 2020

zyiou Dec 21, 2020

codecov-io commented Dec 21, 2020 •

edited

Loading

zyiou commented Dec 21, 2020

antoninbas left a comment

srikartati commented Dec 21, 2020 •

edited

Loading

antoninbas commented Dec 21, 2020 •

edited

Loading

zyiou commented Dec 22, 2020

srikartati commented Dec 22, 2020

srikartati left a comment

[Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy #1682

[Flow-Aggregator] Add correlate fields and fix e2e tests with stats and network policy #1682

Conversation

zyiou commented Dec 21, 2020 • edited Loading

srikartati left a comment • edited Loading

Choose a reason for hiding this comment

srikartati Dec 21, 2020

Choose a reason for hiding this comment

zyiou Dec 21, 2020

Choose a reason for hiding this comment

codecov-io commented Dec 21, 2020 • edited Loading

Codecov Report

zyiou commented Dec 21, 2020

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Dec 21, 2020 • edited Loading

antoninbas commented Dec 21, 2020 • edited Loading

zyiou commented Dec 22, 2020

srikartati commented Dec 22, 2020

srikartati left a comment

Choose a reason for hiding this comment

zyiou commented Dec 21, 2020 •

edited

Loading

srikartati left a comment •

edited

Loading

codecov-io commented Dec 21, 2020 •

edited

Loading

srikartati commented Dec 21, 2020 •

edited

Loading

antoninbas commented Dec 21, 2020 •

edited

Loading