[Flow-Aggregator] Add e2e tests for flow aggregator #1628

dreamtalen · 2020-12-08T03:18:24Z

In this commit, we add e2e tests for the flow aggregator. For these tests, we setup an environment where the flow exporters in antrea agents export flow records to the flow aggregator. The flow aggregator will run the collecting, aggregating jobs and will export records to the external IPFIX collector.

The existing "flow exporter" tests (where the agents export to the external collector directly) are replaced with "flow aggregator" tests, as we assume that the aggregator is always deployed.

Also, we removed the configuration in the agents causing flow records to only be exported from the source node since we now have the aggregator to correlate and de-duplicate flows. Related commit and PR are b44fd78, #1268.

antrea-bot · 2020-12-08T03:18:36Z

Thanks for your PR.
Unit tests and code linters are run automatically every time the PR is updated.
E2e, conformance and network policy tests can only be triggered by a member of the vmware-tanzu organization. Regular contributors to the project should join the org.

The following commands are available:

/test-e2e: to trigger e2e tests.
/skip-e2e: to skip e2e tests.
/test-conformance: to trigger conformance tests.
/skip-conformance: to skip conformance tests.
/test-all-features-conformance: to trigger conformance tests with all alpha features enabled.
/skip-all-features-conformance: to skip conformance tests with all alpha features enabled.
/test-whole-conformance: to trigger all conformance tests on linux.
/skip-whole-conformance: to skip all conformance tests on linux.
/test-networkpolicy: to trigger networkpolicy tests.
/skip-networkpolicy: to skip networkpolicy tests.
/test-windows-conformance: to trigger windows conformance tests.
/skip-windows-conformance: to skip windows conformance tests.
/test-windows-networkpolicy: to trigger windows networkpolicy tests.
/skip-windows-networkpolicy: to skip windows networkpolicy tests.
/test-hw-offload: to trigger ovs hardware offload test.
/skip-hw-offload: to skip ovs hardware offload test.
/test-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

codecov-io · 2020-12-08T03:22:35Z

Codecov Report

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

@@                    Coverage Diff                     @@
##             feature/flow-aggregator    #1628   +/-   ##
==========================================================
  Coverage                           ?   61.82%           
==========================================================
  Files                              ?      184           
  Lines                              ?    15607           
  Branches                           ?        0           
==========================================================
  Hits                               ?     9649           
  Misses                             ?     4926           
  Partials                           ?     1032

Flag	Coverage Δ
kind-e2e-tests	`53.72% <0.00%> (?)`
unit-tests	`40.40% <0.00%> (?)`

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

srikartati · 2020-12-08T06:07:56Z

test/e2e/framework.go

+	if err != nil {
+		return err
+	}
+	// fmt.Printf("configMap.Data['flow-aggregator.conf']: %+v\n", configMap.Data[flowAggregatorConfName])


probably a stray line from debugging

srikartati · 2020-12-08T06:17:13Z

test/e2e/infra/vagrant/push_antrea.sh

@@ -97,12 +98,12 @@ waitForNodes "${pids[@]}"
 echo "Done!"

 echo "Copying Antrea deployment YAML to every node..."
-scp -F ssh-config $ANTREA_YML $ANTREA_IPSEC_YML k8s-node-master:~/ &
+scp -F ssh-config $ANTREA_YML $ANTREA_IPSEC_YML $FLOW_AGGREGATOR_YML k8s-node-master:~/ &


I am guessing this is for your local test runs and not CI.
We should probably automate the workflow of enabling flow exporter, deploying Antrea, and deploying Flow Aggregator on the vagrant setup for dev/demo purposes. This can be considered as a future TODO.

If its not for your CI, please remove this. #1646 can handle this for dev automated workflow.

Thanks. removed.

srikartati · 2020-12-08T06:24:39Z

test/e2e/fixtures.go

+	if err := data.deployFlowAggregator(ipStr + ":" + ipfixCollectorPort + ":tcp"); err != nil {
+		return data, err
+	}
+	flowAggregatorIP, err := data.getServiceIP("flow-aggregator", flowAggregatorNamespace)


I see e2e test failures on kind setups in this PR.. both for TestFlowExporter and TestFlowAggregator.
Seems like it needs some debugging.

As part of that, could you cherry-pick my commit (#1629) and make changes for e2e tests? You will not need to acquire serviceIP of flow aggregator.

srikartati · 2020-12-08T06:27:47Z

test/e2e/flowaggregator_test.go

+AntreaProxy enabled (Inter-Node): Flow record from destination Node is ignored, so only flow record from the source Node has its K8s info like in Inter-Node case along with K8s Service info such as destinationClusterIP, destinationServicePort, destinationServicePortName etc.
+*/
+
+func TestFlowAggregator(t *testing.T) {


I think TestFlowAggregator and TestFlowExporter can be consolidated as in both cases, we need an external flow collector and just the checks will be different. This will save some time in deploying and tearing down the external flow collector.

srikartati · 2020-12-09T21:09:08Z

test/e2e/flowaggregator_test.go

@@ -271,7 +267,7 @@ func checkRecordsForFlowAggregator(t *testing.T, data *TestData, srcIP string, d
 		}
 	}
 	t.Logf("Receive template record number: %d, data record number: %d", templateRecords, dataRecordsCount)
-	assert.Equal(t, templateRecords, 1, "Flow aggregator should send out 1 template record")
+	assert.GreaterOrEqual(t, templateRecords, 1, "Flow aggregator should send out at least 1 template record")


why not exactly one? Is there any issue with clean up code?

srikartati · 2020-12-10T07:27:36Z

@dreamtalen I still see both flow exporter and flow aggregator e2e tests failing. Is there any current issue that is not resolved yet?

dreamtalen · 2020-12-10T08:07:11Z

@dreamtalen I still see both flow exporter and flow aggregator e2e tests failing. Is there any current issue that is not resolved yet?

Yes, these tests failings are related to CI, e2e tests can pass on my local environment, will fix them soon.

dreamtalen · 2020-12-15T22:43:26Z

/test-all

zyiou · 2020-12-17T19:17:49Z

test/e2e/flowaggregator_test.go

+		}},
+	})
+	if err != nil {
+		t.Fatalf("Error when creating Network Policy: %v", err)


Please change fatal message to error as master branch to make sure the test deployments are teared down before exiting. Same for most of the error logs in this file.

Thanks, addressed.

antoninbas

I agree with @srikartati - these tests (exporter & aggregator) need to be consolidated.

I didn't look into the details, but I see a lot of copy-paste and a lot of code doing exactly the same thing (e.g. to validate records).

antoninbas · 2020-12-17T19:24:24Z

pkg/agent/flowexporter/connections/connections.go

-		// Do not export the flow records of connections whose destination is local
-		// Pod and source is remote Pod. We export flow records only from source node,
-		// where the connection originates from. This is to avoid duplicate copies
-		// of flow records at flow collector. This restriction will be removed when
-		// flow aggregator is implemented. We miss some key information such as
-		// destination Pod info, ingress NetworkPolicy info, stats from destination
-		// node etc.
-		// TODO: Remove this when flow aggregator that correlates the flow records
-		// is implemented.
-		if !srcFound && dstFound {
-			conn.DoExport = false
-		}


That's a big change IMO so I am surprised to find it in an unrelated PR with no mention in the PR description?

Sorry, added related PR and commit information in this PR's description.

antoninbas · 2020-12-17T19:28:01Z

test/e2e/fixtures.go

@@ -168,6 +168,44 @@ func setupTestWithIPFIXCollector(tb testing.TB) (*TestData, error, bool) {
 	return data, nil, isIPv6
 }

+func setupTestWithFlowAggregator(tb testing.TB) (*TestData, error) {
+	data := &TestData{}


Why do we need a brand new TestData instance instead of using the existing one, which already has an initialized K8s client?

The same question applies to setupTestWithIPFIXCollector

Thanks, addressed.

antoninbas · 2020-12-17T19:28:55Z

test/e2e/fixtures.go

@@ -168,6 +168,44 @@ func setupTestWithIPFIXCollector(tb testing.TB) (*TestData, error, bool) {
 	return data, nil, isIPv6
 }

+func setupTestWithFlowAggregator(tb testing.TB) (*TestData, error) {


can't setupTestWithIPFIXCollector and setupTestWithFlowAggregator be unified with an extra boolean parameter? we just keep adding more code to do pretty much the same thing

Thanks, addressed.

dreamtalen · 2020-12-17T19:50:32Z

I agree with @srikartati - these tests (exporter & aggregator) need to be consolidated.

I didn't look into the details, but I see a lot of copy-paste and a lot of code doing exactly the same thing (e.g. to validate records).

Thanks for your comments, Antonin. Will try to combine the flow exporter and aggregator e2e test.

antoninbas

did a detailed review, looks much better than before

Lots of typos in the commit, use following:

In this commit, we add e2e tests for the flow aggregator. For these tests, we setup an environment where the flow exporters in antrea agents export flow records to the flow aggregator. The flow aggregator will run the collecting, aggregating jobs and will export records to the external IPFIX collector.

The existing "flow exporter" tests (where the agents export to and external collector directly) are replaced with "flow aggregator" tests, as we assume that the aggregator is always deployed.

Also, we removed the configuration in the agents causing flow records to only be exported from the source node since we now have the aggregator to correlate and de-duplicate flows. Related commit and PR are b44fd78, #1268.

antoninbas · 2020-12-18T21:06:18Z

test/e2e/fixtures.go

 	tb.Logf("Creating K8s clientset")
-	if err := data.createClient(); err != nil {
+	if err := testData.createClient(); err != nil {
 		return nil, err, isIPv6
 	}


should not be needed - the clientset in testdata has already been configured in testMain

Thanks, addressed.

antoninbas · 2020-12-18T21:06:36Z

test/e2e/fixtures.go

@@ -130,42 +129,42 @@ func setupTest(tb testing.TB) (*TestData, error) {
 }

 func setupTestWithIPFIXCollector(tb testing.TB) (*TestData, error, bool) {
-	data := &TestData{}
+	// TODO: remove this hardcoding IPv4 setting after flow aggregator supports IPv6


Suggested change

// TODO: remove this hardcoding IPv4 setting after flow aggregator supports IPv6

// TODO: remove hardcoding to IPv4 after flow aggregator supports IPv6

Thanks, addressed.

antoninbas · 2020-12-18T21:08:39Z

test/e2e/flowaggregator_test.go

+	// Single iperf resulting in two connections with separate ports. Suspecting second flow to be control flow to exchange
+	// stats info. As 5s is export interval and iperf traffic runs for 10s, we expect about 4 records exporting to the flow aggregator.


That's correct, iperf uses a "control" connection

suugestion: Single iperf run results in two connections with separate ports (control connection and actual data connection).

Thanks, addressed.

antoninbas · 2020-12-18T21:09:07Z

test/e2e/flowaggregator_test.go

+	expectedNumTemplateRecords = 1
+	// Single iperf resulting in two connections with separate ports. Suspecting second flow to be control flow to exchange
+	// stats info. As 5s is export interval and iperf traffic runs for 10s, we expect about 4 records exporting to the flow aggregator.
+	// Since flow aggregator will aggregate records based on 5-tuple connection key, so we expect 2 records.


Suggested change

// Since flow aggregator will aggregate records based on 5-tuple connection key, so we expect 2 records.

// Since flow aggregator will aggregate records based on 5-tuple connection key, we expect 2 records.

Thanks, addressed.

antoninbas · 2020-12-18T21:11:04Z

test/e2e/flowaggregator_test.go

+	// Flow exporter can't connect with flow aggregator service if proxy disabled.
+	skipIfProxyDisabled(t, data)


That doesn't sound right to me, could you elaborate?

Ability to connect to the flow aggregator should be independent of whether AntreaProxy is enabled.

Thanks, addressed.
Yes you're right, only LocalServiceAccess and RemoteServiceAccess testcases neet to be skipped.

antoninbas · 2020-12-18T21:11:40Z

test/e2e/flowaggregator_test.go

-				t.Errorf("Records with dstIP does not have Pod name")
+			if isIntraNode {
+				if !strings.Contains(record, "perftest-a") {
+					t.Errorf("Records with srcIP does not have Pod name")


Suggested change

t.Errorf("Records with srcIP does not have Pod name")

t.Errorf("Record with srcIP does not have Pod name")

Thanks, addressed.

antoninbas · 2020-12-18T21:12:01Z

test/e2e/flowaggregator_test.go

+					t.Errorf("Records with dstIP does not have Pod name")
+				}
+			} else {
+				// Flow record comes from destination node may contain incomplete pod name information
+				if !strings.Contains(record, "perftest-a") && !strings.Contains(record, "perftest-b") {
+					t.Errorf("Records does not have source and destination Pod name")


s/Records/Record in log messages

Thanks, addressed.

antoninbas · 2020-12-18T21:12:47Z

test/e2e/flowaggregator_test.go

+					t.Errorf("Records with dstIP does not have Pod name")
+				}
+			} else {
+				// Flow record comes from destination node may contain incomplete pod name information


s/Flow record comes/Flow record coming

However, I thought that the aggregator ensures no incomplete information in records? @srikartati

May be this comment was copied from Flow Exporter code. We should have complete information with flow aggregator.

Actually I added this if-condition because sometimes aggregator doesn't have complete information in the Kind cluster e2e test. Not sure go-ipfix v0.4.2 solved this issue or not, will try again after PR #1676 merged.

go-ipfix v0.4.2 should resolve this.. we introduced readyToSend field in aggregator process to wait until we receive the records from source and destination node.

antoninbas · 2020-12-18T21:13:12Z

test/e2e/flowaggregator_test.go

@@ -253,17 +252,47 @@ func checkRecordsForFlows(t *testing.T, data *TestData, srcIP string, dstIP stri
 					}
 				}
 			}
+			// Check the aggregation results in infra tests
+			if !strings.Contains(record, fmt.Sprintf("%s: %s", "destinationPodNamespace", testNamespace)) {
+				t.Errorf("Records does not have correct destinationPodNamespace")


s/Records does not have/Record does not have

here and in all the error messages below

Thanks, addressed.

antoninbas

LGTM

antoninbas · 2020-12-19T01:15:43Z

test/e2e/flowaggregator_test.go

+	egressNetworkPolicyName    = "test-flow-aggregator-networkpolicy-egress"
+	collectorCheckDelay        = 1 * time.Second
+	expectedNumTemplateRecords = 1
+	// Single iperf run results in two connections with separate ports (control connection and actual data connection). Suspecting second flow to be control


you can remove "Suspecting second flow to be control flow to exchange stats info." now

Thanks, addressed.

In this commit, we add e2e tests for the flow aggregator. For these tests, we setup an environment where the flow exporters in antrea agents export flow records to the flow aggregator. The flow aggregator will run the collecting, aggregating jobs and will export records to the external IPFIX collector. The existing "flow exporter" tests (where the agents export to the external collector directly) are replaced with "flow aggregator" tests, as we assume that the aggregator is always deployed. Also, we removed the configuration in the agents causing flow records to only be exported from the source node since we now have the aggregator to correlate and de-duplicate flows. Related commit and PR are b44fd78, PR#1268.

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

dreamtalen changed the title ~~Add e2e tests for flow aggregator~~ [Flow-Aggregator] Add e2e tests for flow aggregator Dec 8, 2020

dreamtalen force-pushed the local-flow-aggregator-e2e branch from 2c79e8d to 36f4474 Compare December 8, 2020 03:28

srikartati reviewed Dec 8, 2020

View reviewed changes

srikartati reviewed Dec 9, 2020

View reviewed changes

dreamtalen force-pushed the local-flow-aggregator-e2e branch from ddfc977 to f353764 Compare December 10, 2020 01:22

dreamtalen force-pushed the local-flow-aggregator-e2e branch from f353764 to 36d52d5 Compare December 10, 2020 18:36

srikartati force-pushed the feature/flow-aggregator branch from 269eb5e to 6e4ca65 Compare December 10, 2020 23:32

antoninbas force-pushed the feature/flow-aggregator branch 2 times, most recently from 0cab885 to a3f0042 Compare December 11, 2020 03:20

dreamtalen force-pushed the local-flow-aggregator-e2e branch 6 times, most recently from cc37c0a to 048b74f Compare December 15, 2020 01:28

srikartati added this to the Antrea v0.12.0 release milestone Dec 15, 2020

dreamtalen force-pushed the local-flow-aggregator-e2e branch 3 times, most recently from 517852b to 8998511 Compare December 16, 2020 22:42

srikartati force-pushed the feature/flow-aggregator branch from 0c23c32 to 0f331b9 Compare December 17, 2020 18:32

dreamtalen force-pushed the local-flow-aggregator-e2e branch from 8998511 to bfc6e40 Compare December 17, 2020 19:14

zyiou reviewed Dec 17, 2020

View reviewed changes

antoninbas requested changes Dec 17, 2020

View reviewed changes

dreamtalen force-pushed the local-flow-aggregator-e2e branch from bfc6e40 to e499e94 Compare December 17, 2020 19:40

dreamtalen force-pushed the local-flow-aggregator-e2e branch from e499e94 to b144b6b Compare December 17, 2020 19:46

dreamtalen force-pushed the local-flow-aggregator-e2e branch 8 times, most recently from 4cdf3f5 to 9f95ec1 Compare December 18, 2020 19:09

antoninbas reviewed Dec 18, 2020

View reviewed changes

dreamtalen force-pushed the local-flow-aggregator-e2e branch 2 times, most recently from fb93299 to 4ad13ca Compare December 19, 2020 00:51

antoninbas approved these changes Dec 19, 2020

View reviewed changes

dreamtalen force-pushed the local-flow-aggregator-e2e branch from 4ad13ca to 95246f6 Compare December 19, 2020 01:26

antoninbas merged commit 179a472 into antrea-io:feature/flow-aggregator Dec 19, 2020

	// TODO: remove this hardcoding IPv4 setting after flow aggregator supports IPv6
	// TODO: remove hardcoding to IPv4 after flow aggregator supports IPv6

		// Single iperf resulting in two connections with separate ports. Suspecting second flow to be control flow to exchange
		// stats info. As 5s is export interval and iperf traffic runs for 10s, we expect about 4 records exporting to the flow aggregator.

	// Since flow aggregator will aggregate records based on 5-tuple connection key, so we expect 2 records.
	// Since flow aggregator will aggregate records based on 5-tuple connection key, we expect 2 records.

		// Flow exporter can't connect with flow aggregator service if proxy disabled.
		skipIfProxyDisabled(t, data)

	t.Errorf("Records with srcIP does not have Pod name")
	t.Errorf("Record with srcIP does not have Pod name")

[Flow-Aggregator] Add e2e tests for flow aggregator #1628

[Flow-Aggregator] Add e2e tests for flow aggregator #1628

Conversation

dreamtalen commented Dec 8, 2020 • edited Loading

antrea-bot commented Dec 8, 2020

codecov-io commented Dec 8, 2020 • edited Loading

Codecov Report

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

srikartati Dec 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srikartati commented Dec 10, 2020

dreamtalen commented Dec 10, 2020

dreamtalen commented Dec 15, 2020

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

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

dreamtalen commented Dec 17, 2020

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

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

dreamtalen Dec 19, 2020 • edited Loading

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

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

dreamtalen commented Dec 8, 2020 •

edited

Loading

codecov-io commented Dec 8, 2020 •

edited

Loading

srikartati Dec 8, 2020 •

edited

Loading

dreamtalen Dec 19, 2020 •

edited

Loading