Support sending IPFIX flow records for Antrea flow exporter #825

srikartati · 2020-06-11T00:36:01Z

Added support to export IPFIX flow records that are built from
connection map using IPFIX library.

Tested with local IPFIX collector running in k8s cluster. Unit tests were added. Test with
elastiflow collector in progress.

Fixes# 712

antrea-bot · 2020-06-11T00:36:16Z

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-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-all: to trigger all tests.
/skip-all: to skip all tests.

These commands can only be run by members of the vmware-tanzu organization.

srikartati · 2020-06-15T07:17:34Z

I did testing with IPFIX collector (C-based ipfixlib) in a local k8s cluster running iperf service with 2 tcp clients and one server. Collector is able to see all the data records sent at ipfix export interval. Posting output for reference below. In the process, fixed some bugs in previous patch and ipfix library.
Could not generate mocks for ipfix based interfaces without the IPFIX library in github.com. I will add mentioned unit tests in exporter_test.go, when the library is available in vmware github org.

IPFIX-HDR:
 version=10, length=185
 unixtime=1592204240 (2020-06-14 23:57:20 PDT)
 seqno=2, odid=1269807227
DATA RECORD: 
 template id:  256 
 nfields:      19
 flowStartSeconds: 18446744011573954816
 flowEndSeconds: 18446744011573954816
 sourceIPv4Address: 100.10.1.62
 destinationIPv4Address: 100.10.1.64
 sourceTransportPort: 47614
 destinationTransportPort: 8080
 protocolIdentifier: 6
 packetTotalCount: 1613320
 octetTotalCount: 76543694782
 packetDeltaCount: 1613320
 octetDeltaCount: 76543694782
 reverse packetTotalCount: 89493
 reverse octetTotalCount: 4654376
 reverse packetDeltaCount: 89493
 reverse octetDeltaCount: 4654376
 55829_101: 0x7765622d636c69656e742d353463663739376637642d6462776d70
 55829_100: 0x64656661756c74
 55829_103: 0x7765622d7365727665722d353836366436636434352d6a63747a34
 55829_102: 0x64656661756c74

srikartati · 2020-06-16T20:00:47Z

Added few unit test. Using the go-ipfixlib at "github.com/srikartati/go-ipfixlib" by making it public.

jianjuns

A general question - have we observed higher CPU usage/contention when flow exporting is enabled?

cmd/antrea-agent/agent.go

pkg/agent/flowexporter/exporter/exporter.go

pkg/agent/flowexporter/connections/connections.go

jianjuns · 2020-06-19T22:28:46Z

pkg/agent/flowexporter/connections/connections.go

+	defer cs.mutex.Unlock()
+
+	for k, v := range cs.connections {
+		cs.mutex.Unlock()


Not safe to iterate a map without lock? What happens if another routine add/delete keys?

Yes, it is possible when conntrack poller go routine accessing the map. Doing this to avoid lock contention on whole map, when we are building flow record for a single connection. The disadvantage is that we may send connection data of last poll cycle (difference in seconds) for very few flow records--as this is continuous monitoring, I thought this is ok. What is your opinion?

I want to provide some further clarification, by considering a scenario, w.r.t following code. Here as well if routine executing iterate grabs the lock first, then we will build records using old connection data. However, by releasing lock after every item, we can decrease the contention.

lock
addOrUpdateConnectionMap
unlock

lock
IterateConnectionMap
unlock

Also, I would like to note that range connectionMap will iterate on old copy of connection map even connection map is updated in other routine. I missed that in my previous comment. Thanks.

I did not know map iteration in Golang is thread safe.
Anyway could you add some comments to explain why you release the lock there, and what could be the impact?

You are right, Jianjun. Iteration over the map is not thread-safe depending on concurrent operations. Releasing lock in our case may work because of current deletion logic, and the presumption that sending old connection data is fine. Added the comment in the code. Please take a look.

This sounds tricky. Have you verified it really works?

It worked as expected with respect to deletion logic. However, because of the IPFIX collector requirement of sending accurate flow records rather than from the last poll cycle, I changed the synchronization logic.

jianjuns · 2020-06-19T22:30:33Z

pkg/agent/flowexporter/types.go

@@ -36,3 +41,11 @@ type Connection struct {
 	DestinationPodNamespace string


Add some comments somewhere indicating Pod name/NS is filled only when the Pod is local?

A general question - have we observed higher CPU usage/contention when flow exporting is enabled?

I did not look closely into CPU utilization stats yet. I will compare the following scenarios w.r.t cpu utilization and percentage of used and runnable times for processes:

No flow exporter

Flow exporter with no workload

Flow exporter with workload.

pkg/agent/flowexporter/exporter/exporter.go

pkg/agent/flowexporter/connections/connections.go

srikartati

I will checkin the fixes along with the e2e test in the next patch. Please let me know if you have further comments. Thanks.

srikartati · 2020-06-23T02:16:16Z

pkg/agent/flowexporter/connections/connections.go

+	defer cs.mutex.Unlock()
+
+	for k, v := range cs.connections {
+		cs.mutex.Unlock()


Yes, it is possible when conntrack poller go routine accessing the map. Doing this to avoid lock contention on whole map, when we are building flow record for a single connection. The disadvantage is that we may send connection data of last poll cycle (difference in seconds) for very few flow records--as this is continuous monitoring, I thought this is ok. What is your opinion?

pkg/agent/flowexporter/connections/connections.go

pkg/agent/flowexporter/exporter/exporter.go

antrea-bot · 2020-06-23T22:56:16Z

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-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-all: to trigger all tests (except whole conformance).
/skip-all: to skip all tests (except whole conformance).

These commands can only be run by members of the vmware-tanzu organization.

Regenerated manifest files too.

Changed synchronization logic for poll(ConnectionStore.Run) and export(FlowExporter.Run) go routines. Fixed e2e tests.

First record should send 0 delta bytes/packets otherwise already established flow will show incorrect throughput (Mb/s or PPS)

This will be removed when network policy info is added in flow records.

antrea-bot · 2020-08-10T22:17:38Z

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-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).

Major change is handling of error in exporter go routine.

antoninbas · 2020-08-10T22:56:57Z

pkg/agent/flowexporter/ipfix/ipfix_process.go

+
+func (exp *ipfixExportingProcess) CloseConnToCollector() {
+	exp.ExportingProcess.CloseConnToCollector()
+	return


return not needed (same below and maybe in other places?)

antoninbas · 2020-08-10T22:58:10Z

pkg/agent/flowexporter/exporter/exporter.go

+}
+
+// CheckAndDoExport enables us to export flow records periodically at a given flow export frequency.
+func (exp *flowExporter) CheckAndDoExport(collector net.Addr, pollDone chan struct{}) {


I think the Check part of the name is a bit misleading and not needed. I would suggest removing it altogether. Or at least mention in the comment that the function ensures there is a live connection to the collector.

Changed the name.

antoninbas · 2020-08-10T22:58:40Z

pkg/agent/flowexporter/exporter/exporter.go

+		klog.V(2).Infof("Successfully exported IPFIX flow records")
+	}
+
+	return


return not needed

antoninbas · 2020-08-10T23:00:12Z

cmd/antrea-agent/agent.go

+		flowExporter := exporter.NewFlowExporter(
+			flowrecords.NewFlowRecords(connStore),
+			o.config.FlowExportFrequency)
+		go wait.Until(func() { flowExporter.CheckAndDoExport(o.flowCollector, pollDone) }, o.pollInterval, stopCh)


that's not exactly what I had in mind here. You should still have a select loop in CheckAndDoExport, the time duration provided here should be one second or a few seconds, and should only be used to establish the connection again in case of error.

in case of error, CheckAndDoExport logs the error, then returns. wait.Until will then take care of calling it again (which will establish the connection, and start a new for / select loop). If the connection never breaks or there is no export error, CheckAndDoExport will not return.

I handled the connection creation through if exp.process == nil only when there is error.
I have to change this when we move the handling of connection logic to go-ipfix. Keeping this as is for now as discussed offline.

antoninbas · 2020-08-10T23:03:12Z

pkg/agent/flowexporter/connections/conntrack_ovs.go

+	for _, flow := range outputFlow {
+		conn, err := flowStringToAntreaConnection(flow, zoneFilter)
+		if err != nil {
+			klog.Warningf("Ignoring the flow from conntrack dump due to the error: %v", err)


Suggested change

klog.Warningf("Ignoring the flow from conntrack dump due to the error: %v", err)

klog.Warningf("Ignoring the flow from conntrack dump due parsing error: %v", err)

srikartati · 2020-08-11T00:55:56Z

/test-all

srikartati · 2020-08-11T00:57:11Z

/test-networkpolicy
/test-e2e
/test-conformance

srikartati · 2020-08-11T02:30:24Z

/test-all

antoninbas · 2020-08-11T02:57:18Z

pkg/agent/flowexporter/exporter/exporter.go

+	for {
+		select {
+		case <-stopCh:
+			break


s/break/return

antoninbas · 2020-08-11T03:30:03Z

test/e2e/flowexporter_test.go

+
+	rc, collectorOutput, _, err := provider.RunCommandOnNode(masterNodeName(), fmt.Sprintf("kubectl logs ipfix-collector -n antrea-test"))
+	if err != nil || rc != 0 {
+		t.Fatalf("error when getting logs %v, rc: %v", err, rc)


nit: first letter of log message should be capitalized

antoninbas · 2020-08-11T03:31:53Z

test/e2e/flowexporter_test.go

+								t.Fatalf("Error in converting octetDeltaCount to int type")
+							}
+							// compute the bandwidth using 5s as interval
+							recBandwidth := (deltaBytes * 8.0) / float64((int64(5.0))*time.Second.Nanoseconds())


recBandwidth := (deltaBytes * 8.0) / float64(5*time.Second.Nanoseconds())

does not work?

Yes it works. For some reason, I started with 5.0 to make everything float and navigated around that. Changed.

Missed this change. As tests have finished in this PR, modified this file in this PR #984 .

srikartati · 2020-08-11T14:03:38Z

/test-all

antoninbas

LGTM.

A few things we talked about offline to keep in mind for post 0.9:

handle reconnections to the IPFix collector in the go-ipfix library
add unit tests for the reconnection case if possible
improve OVS appctl connection parsing logic

srikartati · 2020-08-11T17:07:13Z

LGTM.

A few things we talked about offline to keep in mind for post 0.9:

handle reconnections to the IPFix collector in the go-ipfix library

add unit tests for the reconnection case if possible

improve OVS appctl connection parsing logic

Thanks for the review. Yes there is an issue for the first one in go-ipfix. I will keep a note of the last one to enhance OVS appctl dump flows function. This is needed for windows support.

vmwclabot added the cla-not-required label Jun 11, 2020

srikartati changed the title ~~Support sending IPFIX flow records as part of flow exporter feature~~ Support sending IPFIX flow records for Antrea flow exporter feature Jun 11, 2020

srikartati changed the title ~~Support sending IPFIX flow records for Antrea flow exporter feature~~ [WIP]Support sending IPFIX flow records for Antrea flow exporter feature Jun 11, 2020

srikartati force-pushed the ipfix_record branch 2 times, most recently from 51f7ec1 to ac163fd Compare June 15, 2020 07:10

srikartati force-pushed the ipfix_record branch 4 times, most recently from 1c86e77 to 09a1c32 Compare June 16, 2020 19:53

srikartati changed the title ~~[WIP]Support sending IPFIX flow records for Antrea flow exporter feature~~ Support sending IPFIX flow records for Antrea flow exporter feature Jun 16, 2020

srikartati force-pushed the ipfix_record branch from 09a1c32 to 9e0b600 Compare June 16, 2020 23:02

zyiou mentioned this pull request Jun 17, 2020

Add Elastic Stack deployment for flow visualization #836

Merged

srikartati changed the title ~~Support sending IPFIX flow records for Antrea flow exporter feature~~ Support sending IPFIX flow records for Antrea flow exporter Jun 17, 2020

srikartati force-pushed the ipfix_record branch from 9e0b600 to 8c3e8bc Compare June 17, 2020 19:53

srikartati requested review from antoninbas and jianjuns June 18, 2020 04:30

jianjuns reviewed Jun 19, 2020

View reviewed changes

pkg/agent/flowexporter/connections/connections.go Outdated Show resolved Hide resolved

srikartati commented Jun 23, 2020

View reviewed changes

srikartati force-pushed the ipfix_record branch from 8c3e8bc to 2a4f20c Compare June 23, 2020 22:56

srikartati force-pushed the ipfix_record branch 4 times, most recently from bd0f7d8 to fb947d1 Compare June 24, 2020 08:14

srikartati mentioned this pull request Jun 24, 2020

Create and mainatain connection store for Antrea flow exporter #773

Merged

srikartati force-pushed the ipfix_record branch from fb947d1 to ce09e27 Compare June 25, 2020 19:46

srikartati added 10 commits August 10, 2020 11:00

Fixed memory lead issue and addressed new comments

9240e7e

Regenerated manifest files too.

Resolving conflicts with ToT and address template refresh issue

7495f1e

Changed synchronization logic for poll(ConnectionStore.Run) and export(FlowExporter.Run) go routines. Fixed e2e tests.

Fixed starting record issue

1fea8a4

First record should send 0 delta bytes/packets otherwise already established flow will show incorrect throughput (Mb/s or PPS)

Replaced personal ipfix library to vmware/go-ipfix library

d3fafae

Added integration test to increase test coverage

6abc0f8

Address comments from 7/17

52bb040

Improve unit test coverage

7eb03b5

Do not send double copies of flow records

a8ea1d5

This will be removed when network policy info is added in flow records.

Addressed comments from 7/30

090ef09

Address comments 8/6

719f8e0

srikartati force-pushed the ipfix_record branch from 9a3212c to e90d1b2 Compare August 10, 2020 22:17

Addressed comments

841469c

Major change is handling of error in exporter go routine.

srikartati force-pushed the ipfix_record branch from e90d1b2 to 841469c Compare August 10, 2020 22:38

antoninbas reviewed Aug 10, 2020

View reviewed changes

srikartati force-pushed the ipfix_record branch 2 times, most recently from f1744e4 to b8d9ee4 Compare August 11, 2020 02:18

antoninbas reviewed Aug 11, 2020

View reviewed changes

pkg/agent/flowexporter/exporter/exporter.go Outdated

for {

select {

case <-stopCh:

break

Copy link

Contributor

antoninbas Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/break/return

srikartati force-pushed the ipfix_record branch from b8d9ee4 to 747569d Compare August 11, 2020 03:19

antoninbas reviewed Aug 11, 2020

View reviewed changes

Addreseed the comments

0ef571a

srikartati force-pushed the ipfix_record branch from 747569d to 0ef571a Compare August 11, 2020 13:58

antoninbas approved these changes Aug 11, 2020

View reviewed changes

srikartati merged commit c9d365d into antrea-io:master Aug 11, 2020

		@@ -36,3 +41,11 @@ type Connection struct {
		DestinationPodNamespace string

	klog.Warningf("Ignoring the flow from conntrack dump due to the error: %v", err)
	klog.Warningf("Ignoring the flow from conntrack dump due parsing error: %v", err)

Support sending IPFIX flow records for Antrea flow exporter #825

Support sending IPFIX flow records for Antrea flow exporter #825

Conversation

srikartati commented Jun 11, 2020 • edited Loading

antrea-bot commented Jun 11, 2020

srikartati commented Jun 15, 2020

srikartati commented Jun 16, 2020

jianjuns 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

srikartati left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

antrea-bot commented Jun 23, 2020

antrea-bot commented Aug 10, 2020

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

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

srikartati commented Aug 11, 2020

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

srikartati commented Aug 11, 2020

antoninbas left a comment

Choose a reason for hiding this comment

srikartati commented Aug 11, 2020

srikartati commented Jun 11, 2020 •

edited

Loading