Add throughput calculation into flow-aggregator #2692

heanlan · 2021-09-01T04:46:51Z

Flow-visibility solution is moving away from elk flow collector, we are moving throughput calculation from logstash into flow-aggregator: vmware/go-ipfix#270

This PR makes the necessary changes on Antrea side corresponding to the changes in the above PR. More description can be found on the PR description of the above Go-ipfix PR.

fixes: #2211

codecov-commenter · 2021-09-01T05:09:40Z

Codecov Report

Merging #2692 (8089b7e) into main (fb57d91) will decrease coverage by 9.45%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##             main    #2692      +/-   ##
==========================================
- Coverage   60.85%   51.39%   -9.46%     
==========================================
  Files         292      462     +170     
  Lines       24766    53579   +28813     
==========================================
+ Hits        15072    27539   +12467     
- Misses       8050    23706   +15656     
- Partials     1644     2334     +690

Flag	Coverage Δ
e2e-tests	`51.03% <0.00%> (?)`
integration-tests	`31.59% <ø> (?)`
kind-e2e-tests	`47.36% <66.66%> (-0.67%)`	⬇️
unit-tests	`40.35% <60.60%> (+0.26%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/flowaggregator/flowaggregator.go	`51.66% <50.00%> (-16.54%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam_controller.go	`18.60% <0.00%> (-46.78%)`	⬇️
pkg/agent/cniserver/pod_configuration_linux.go	`26.31% <0.00%> (-40.36%)`	⬇️
pkg/ipam/poolallocator/allocator.go	`22.27% <0.00%> (-40.15%)`	⬇️
pkg/ipfix/ipfix_intermediate.go	`51.51% <0.00%> (-37.96%)`	⬇️
pkg/agent/cniserver/ipam/antrea_ipam.go	`41.46% <0.00%> (-35.01%)`	⬇️
pkg/ipfix/ipfix_collector.go	`50.00% <0.00%> (-31.82%)`	⬇️
pkg/ipam/ipallocator/allocator.go	`54.22% <0.00%> (-29.99%)`	⬇️
pkg/controller/networkpolicy/endpoint_querier.go	`61.46% <0.00%> (-29.97%)`	⬇️
pkg/controller/egress/validate.go	`47.54% <0.00%> (-26.27%)`	⬇️
... and 448 more

antoninbas

could you clarify why we get a more accurate throughput calculation by performing it at the aggregator? It seems to me that the formula you give in vmware/go-ipfix#270 (diff(octetTotalCount) / diff(flowEndSeconds)) could also be computed at the collector.

antoninbas · 2021-11-30T21:06:16Z

build/yamls/elk-flow-collector/logstash/ipfix.yml

+  145:
+    - :uint64
+    - :throughput
+  146:
+    - :uint64
+    - :reverseThroughput
+  147:
+    - :uint64
+    - :throughputFromSourceNode
+  148:
+    - :uint64
+    - :throughputFromDestinationNode
+  149:
+    - :uint64
+    - :reverseThroughputFromSourceNode
+  150:
+    - :uint64
+    - :reverseThroughputFromDestinationNode
+  151:
+    - :uint32
+    - :flowEndSecondsFromSourceNode
+  152:
+    - :uint32
+    - :flowEndSecondsFromDestinationNode


I didn't remember us having all these *SourceNode and *DestinationNodes IE variations for octet and packet counts. I wonder if this is really necessary. For octant and packet counts, do we actually use all these field variations?

Yes, we have all these IE variations. Currently the only use case is we are using octetDeltaCountFromSourceNode/DestinationNode to compute node throughput in logstash. I agree these IE variations can be removed if we decide to add throughput information into the exported records.

heanlan · 2021-12-01T20:39:20Z

could you clarify why we get a more accurate throughput calculation by performing it at the aggregator? It seems to me that the formula you give in vmware/go-ipfix#270 (diff(octetTotalCount) / diff(flowEndSeconds)) could also be computed at the collector.

Hi @antoninbas , about why we want to do the throughput calculation in aggregator, I re-summarized the intuitions by a little bit, hope it will make a little bit more sense:

*Node specific fields: *SourceNode and *DestinationNodes IE variations for octet and packet counts
*common fields: octetDeltaCount/octetTotalCount

Separate from elk-flow-collector. Previously we do throughput calculation in logstash, now we want to develop a new pipeline without elk. So we need to find a new place to do the throughput calculation. And essentially there will be more than one applications, we want to do the throughput calculation in a single place and the results can be used by all the apps.
Accuracy. In the case of inter-node flows, there is some data discrepancy between source and destination stats. In this PR, source node throughput is calculated based on byte count and timestamp sent from the source node, destination node throughput is calculated based on byte count and timestamp sent from destination node. If we decide to remove the node specific fields, only export the common fields, and do the calculation at the front end, both source and destination node throughput will be calculated from the common fields. The result will be less accurate compare to calculated in aggregator.

antoninbas · 2021-12-02T00:48:22Z

@heanlan My concern is the growing size of the flow records and potentially the increasing CPU & memory consumption in the Flow Aggregator that goes with it. If you think this isn't an issue, we can add these fields. I originally thought throughput computation at the collector or collectors wouldn't be too much of a problem. Or that we would use a Kafka transformer to add throughput to the data once.

heanlan · 2021-12-02T02:22:20Z

@heanlan My concern is the growing size of the flow records and potentially the increasing CPU & memory consumption in the Flow Aggregator that goes with it. If you think this isn't an issue, we can add these fields. I originally thought throughput computation at the collector or collectors wouldn't be too much of a problem. Or that we would use a Kafka transformer to add throughput to the data once.

With IPFIX encoding we convert data into bytes, and Kai and Srikar suggested we would use protobuf later for better efficiency. So this would not be a big issue?
Also we want to remove the *SourceNode and *DestinationNodes IE variations for octet and packet counts in a follow-up PR, since there are no use cases for those fields.

heanlan · 2021-12-02T19:07:20Z

@antoninbas Do you think we can include this PR into v1.5 release?

antoninbas · 2021-12-03T01:16:02Z

@heanlan Antrea v1.5 is scheduled for December 22nd, so I don't see why not. This is not a very large PR.

heanlan · 2021-12-10T22:40:36Z

E2E test will fail as the ipfix-collector image is not available yet in registry. Will re-run when it's available.

docs/network-flow-visibility.md

antoninbas · 2021-12-10T23:48:44Z

docs/network-flow-visibility.md

+|-------------------------------------------|----------|-------------|-------------|
+| packetTotalCountFromSourceNode            | 120      | unsigned64  | The cumulative number of packets from source to destination for this flow since the flow starts, based on the records sent from the source node. |
+| octetTotalCountFromSourceNode             | 121      | unsigned64  | The cumulative number of octets from source to destination for this flow since the flow starts, based on the records sent from the source node. |
+| packetDeltaCountFromSourceNode            | 122      | unsigned64  | The number of packets from source to destination since the previous report for this flow at the observation point, based on the records sent from the source node. |


I would simplify a bit the phrasing: "The number of packets for this flow as reported by the source Node, since the previous report for this flow at the observation point"

let me know if that works

Thanks, Antonin. I simplified it accordingly.

heanlan · 2021-12-14T03:19:57Z

Image ipfix-collector:v0.5.11 still not available in the registry. Will ask help from the team tomorrow to fix it up.

heanlan · 2021-12-15T17:17:15Z

/test-all
/test-conformance
/test-integration
/test-e2e
/test-networkpolicy

antoninbas

Approving again

tnqn

LGTM, a typo

tnqn · 2022-01-07T14:03:53Z

docs/network-flow-visibility.md

+| egressNetworkPolicyRuleName      | 142      | string      | Name of the egress network policy rule applied to the source Pod for this flow. |
+| ingressNetworkPolicyRuleAction   | 139      | unsigned8   | 1 stands for Allow. 2 stands for Drop. 3 stands for Reject. |
+| egressNetworkPolicyRuleAction    | 140      | unsigned8   |             |
+| tcpState                         | 136      | string      | The state of the TCP connection. The states are: LISTEN, SYN-SENT, SYNRECEIVED, ESTABLISHED, FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT, and CLOSED. |


Suggested change

| tcpState | 136 | string | The state of the TCP connection. The states are: LISTEN, SYN-SENT, SYNRECEIVED, ESTABLISHED, FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT, and CLOSED. |

| tcpState | 136 | string | The state of the TCP connection. The states are: LISTEN, SYN-SENT, SYN-RECEIVED, ESTABLISHED, FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT, and CLOSED. |

thanks Quan, updated

Signed-off-by: heanlan <[email protected]>

tnqn

LGTM

tnqn · 2022-01-14T17:03:11Z

@heanlan Some aggregator tests always failed in my local testbed because the following errors:

=== RUN   TestFlowAggregator/IPv4/LocalServiceAccess
    flowaggregator_test.go:633: Iperf throughput: 12800.00 Mbits/s, IPFIX record throughput: 0.00 Mbits/s
    flowaggregator_test.go:634:
                Error Trace:    flowaggregator_test.go:634
                                                        flowaggregator_test.go:525
                Error:          Max difference between 0 and 12800 allowed is 1920, but difference was -12800
                Test:           TestFlowAggregator/IPv4/LocalServiceAccess
                Messages:       Difference between Iperf bandwidth and IPFIX record bandwidth should be lower than 15%%
    flowaggregator_test.go:633: Iperf throughput: 12800.00 Mbits/s, IPFIX record throughput: 0.00 Mbits/s

I ran same tests for other PRs, they can pass most of times. So I'm not sure if it's related to this PR.
CI has recovered, but I'm not sure if the PR will run with the latest merged code (with a patch to fix CI). I will take a try anyway.
/test-e2e

heanlan · 2022-01-14T17:35:15Z

@heanlan Some aggregator tests always failed in my local testbed because the following errors:

Thanks Quan. I haven't seen this error before in my local vagrant env. And I triggered another run last night, it also passed:

=== RUN   TestFlowAggregator/IPv4/IntraNodeFlows
    flowaggregator_test.go:866: Network Policies are realized.
    flowaggregator_test.go:633: Iperf throughput: 27648.00 Mbits/s, IPFIX record throughput: 27868.87 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 27648.00 Mbits/s, IPFIX record throughput: 24829.58 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 27648.00 Mbits/s, IPFIX record throughput: 24934.12 Mbits/s
   
=== RUN   TestFlowAggregator/IPv4/InterNodeFlows
I0113 20:15:35.498249   83327 k8s_util.go:675] Creating/updating Antrea NetworkPolicy antrea-test/test-flow-aggregator-antrea-networkpolicy-ingress
I0113 20:15:35.514235   83327 k8s_util.go:675] Creating/updating Antrea NetworkPolicy antrea-test/test-flow-aggregator-antrea-networkpolicy-egress
    flowaggregator_test.go:901: Antrea Network Policies are realized.
    flowaggregator_test.go:633: Iperf throughput: 800.00 Mbits/s, IPFIX record throughput: 776.51 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 800.00 Mbits/s, IPFIX record throughput: 788.06 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 800.00 Mbits/s, IPFIX record throughput: 738.05 Mbits/s
    
=== RUN   TestFlowAggregator/IPv4/LocalServiceAccess
    flowaggregator_test.go:633: Iperf throughput: 28979.20 Mbits/s, IPFIX record throughput: 28160.11 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 28979.20 Mbits/s, IPFIX record throughput: 29254.94 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 28979.20 Mbits/s, IPFIX record throughput: 26198.28 Mbits/s
    
    
=== RUN   TestFlowAggregator/IPv4/RemoteServiceAccess
    flowaggregator_test.go:633: Iperf throughput: 813.00 Mbits/s, IPFIX record throughput: 854.69 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 813.00 Mbits/s, IPFIX record throughput: 713.43 Mbits/s
    flowaggregator_test.go:633: Iperf throughput: 813.00 Mbits/s, IPFIX record throughput: 749.17 Mbits/s

It seems the e2e test was not running with the CI fix-patch.

Does the error only happen in LocalServiceAccess? We might need to dump more log in order to triage the fail run in your local env. e.g. Add a line in L578 in flowaggregator_test:

t.Log(record)

to see the received IPFIX records. The throughput field can be 0 when all the received records having the same timestamp.

heanlan · 2022-01-17T17:23:42Z

/test-e2e

tnqn · 2022-01-17T17:44:39Z

@heanlan This is the previous e2e log I collected: e2e.log

I reran the test with the line added: e2e-2.log, and the antrea component logs: antrea-test-506706938.tar.gz

heanlan · 2022-01-17T18:24:13Z

/test-e2e

heanlan · 2022-01-17T19:12:13Z

@heanlan This is the previous e2e log I collected: e2e.log

I reran the test with the line added: e2e-2.log, and the antrea component logs: antrea-test-506706938.tar.gz

Thanks Quan.

In your local testbed:
It seems to be the flow-aggregator image issue. The newly-added throughput fields were not reflected in the records you printed out. Usually when I run test locally, in order to reflect the new changes in flow-aggregator code, I will build the image manually and use it in the flow-aggregator yaml file.
If you would please to try again, you could change antrea/build/yamls/flow-aggregator.yml: L266 to the line below, and scp the yaml file to the node to run again.

hanlan/flow-aggregator:latest

In the recovered Jenkins testbed:
The bandwidth checks have all passed. The test failure in log is unrelated to the changes in this PR. The failure never happened before on other testbeds, e.g. vagrant, kind, your local testbed. I'm re-running the test to see if the failure is stable.

=== CONT  TestFlowAggregator
    fixtures.go:396: Deleting 'antrea-test' K8s Namespace
    fixtures.go:382: Error when gracefully exiting Flow Aggregator: error when gracefully exiting Flow Aggregator - copy flow-aggregator coverage files out: cannot retrieve content of file 'flow-aggregator.cov.out' from Pod 'flow-aggregator-5d46df4797-bcct8', stderr: <cat: flow-aggregator.cov.out: No such file or directory
        >, err: <command terminated with exit code 1>
--- FAIL: TestFlowAggregator (219.90s)
    --- PASS: TestFlowAggregator/IPv4 (108.15s)
        --- PASS: TestFlowAggregator/IPv4/IntraNodeFlows (13.46s)
        --- PASS: TestFlowAggregator/IPv4/IntraNodeDenyConnIngressANP (6.09s)
        --- PASS: TestFlowAggregator/IPv4/IntraNodeDenyConnEgressANP (2.89s)
        --- PASS: TestFlowAggregator/IPv4/IntraNodeDenyConnNP (8.24s)
        --- PASS: TestFlowAggregator/IPv4/InterNodeFlows (13.79s)
        --- PASS: TestFlowAggregator/IPv4/InterNodeDenyConnIngressANP (6.43s)
        --- PASS: TestFlowAggregator/IPv4/InterNodeDenyConnEgressANP (2.95s)
        --- PASS: TestFlowAggregator/IPv4/InterNodeDenyConnNP (8.23s)
        --- PASS: TestFlowAggregator/IPv4/ToExternalFlows (9.35s)
        --- PASS: TestFlowAggregator/IPv4/LocalServiceAccess (14.18s)
        --- PASS: TestFlowAggregator/IPv4/RemoteServiceAccess (13.52s)
        --- PASS: TestFlowAggregator/IPv4/Antctl (5.93s)

heanlan · 2022-01-17T19:17:44Z

/test-e2e

heanlan · 2022-01-17T19:44:20Z

/test-e2e

tnqn · 2022-01-18T09:12:45Z

@heanlan thanks for pointing out the issue in my local testbed and sorry for forgetting to rebuild flow-aggregator image. I have reran the test and it passed.
The e2e failure in CI was unrelated, I will skip it.
/skip-e2e

…ntrea-io#2692) Signed-off-by: heanlan <[email protected]>

heanlan marked this pull request as draft September 1, 2021 04:47

heanlan force-pushed the flowaggregator-throughput branch from 8c01906 to 568198e Compare September 1, 2021 04:48

heanlan force-pushed the flowaggregator-throughput branch from 568198e to da48bcd Compare September 1, 2021 19:27

heanlan marked this pull request as ready for review September 1, 2021 19:39

heanlan mentioned this pull request Sep 3, 2021

Improve octetDeltaCount calculation vmware/go-ipfix#251

Closed

heanlan force-pushed the flowaggregator-throughput branch from da48bcd to c1eaec6 Compare September 3, 2021 22:48

heanlan marked this pull request as draft October 28, 2021 18:25

heanlan force-pushed the flowaggregator-throughput branch 4 times, most recently from 431d44e to 17eace0 Compare November 22, 2021 21:35

heanlan force-pushed the flowaggregator-throughput branch 7 times, most recently from 09b4e17 to 86c414e Compare November 29, 2021 21:15

heanlan marked this pull request as ready for review November 30, 2021 01:50

heanlan requested review from dreamtalen, zyiou, yanjunz97 and antoninbas November 30, 2021 01:55

antoninbas reviewed Nov 30, 2021

View reviewed changes

heanlan requested review from antoninbas and zyiou December 10, 2021 22:40

antoninbas reviewed Dec 10, 2021

View reviewed changes

heanlan force-pushed the flowaggregator-throughput branch 2 times, most recently from 897c729 to 14ce2ad Compare December 11, 2021 01:49

antoninbas previously approved these changes Dec 13, 2021

View reviewed changes

heanlan dismissed antoninbas’s stale review via 1ed4c16 December 14, 2021 03:17

heanlan force-pushed the flowaggregator-throughput branch from 14ce2ad to 1ed4c16 Compare December 14, 2021 03:17

heanlan requested a review from antoninbas December 21, 2021 21:56

antoninbas previously approved these changes Dec 22, 2021

View reviewed changes

tnqn reviewed Jan 7, 2022

View reviewed changes

Add throughput calculation into flow-aggregator aggregation process.

8089b7e

Signed-off-by: heanlan <[email protected]>

heanlan dismissed antoninbas’s stale review via 8089b7e January 7, 2022 15:05

heanlan force-pushed the flowaggregator-throughput branch from 1ed4c16 to 8089b7e Compare January 7, 2022 15:05

tnqn approved these changes Jan 7, 2022

View reviewed changes

tnqn merged commit 0f5df48 into antrea-io:main Jan 18, 2022

heanlan deleted the flowaggregator-throughput branch January 18, 2022 15:22

yanjunz97 pushed a commit to yanjunz97/antrea that referenced this pull request Feb 14, 2022

Add throughput calculation into flow-aggregator aggregation process. (a…

613fc16

…ntrea-io#2692) Signed-off-by: heanlan <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add throughput calculation into flow-aggregator #2692

Add throughput calculation into flow-aggregator #2692

heanlan commented Sep 1, 2021 •

edited

Loading

codecov-commenter commented Sep 1, 2021 •

edited

Loading

antoninbas left a comment

antoninbas Nov 30, 2021

heanlan Nov 30, 2021 •

edited

Loading

heanlan commented Dec 1, 2021

antoninbas commented Dec 2, 2021

heanlan commented Dec 2, 2021

heanlan commented Dec 2, 2021

antoninbas commented Dec 3, 2021

heanlan commented Dec 10, 2021

antoninbas Dec 10, 2021

heanlan Dec 11, 2021

heanlan commented Dec 14, 2021

heanlan commented Dec 15, 2021

antoninbas left a comment

tnqn left a comment

tnqn Jan 7, 2022

heanlan Jan 7, 2022

tnqn left a comment

tnqn commented Jan 14, 2022

heanlan commented Jan 14, 2022 •

edited

Loading

heanlan commented Jan 17, 2022

tnqn commented Jan 17, 2022

heanlan commented Jan 17, 2022

heanlan commented Jan 17, 2022 •

edited

Loading

heanlan commented Jan 17, 2022

heanlan commented Jan 17, 2022

tnqn commented Jan 18, 2022

	\| tcpState \| 136 \| string \| The state of the TCP connection. The states are: LISTEN, SYN-SENT, SYNRECEIVED, ESTABLISHED, FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT, and CLOSED. \|
	\| tcpState \| 136 \| string \| The state of the TCP connection. The states are: LISTEN, SYN-SENT, SYN-RECEIVED, ESTABLISHED, FIN-WAIT-1, FIN-WAIT-2, CLOSE-WAIT, CLOSING, LAST-ACK, TIME-WAIT, and CLOSED. \|

Add throughput calculation into flow-aggregator #2692

Add throughput calculation into flow-aggregator #2692

Conversation

heanlan commented Sep 1, 2021 • edited Loading

codecov-commenter commented Sep 1, 2021 • edited Loading

Codecov Report

antoninbas left a comment

Choose a reason for hiding this comment

antoninbas Nov 30, 2021

Choose a reason for hiding this comment

heanlan Nov 30, 2021 • edited Loading

Choose a reason for hiding this comment

heanlan commented Dec 1, 2021

antoninbas commented Dec 2, 2021

heanlan commented Dec 2, 2021

heanlan commented Dec 2, 2021

antoninbas commented Dec 3, 2021

heanlan commented Dec 10, 2021

antoninbas Dec 10, 2021

Choose a reason for hiding this comment

heanlan Dec 11, 2021

Choose a reason for hiding this comment

heanlan commented Dec 14, 2021

heanlan commented Dec 15, 2021

antoninbas left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn Jan 7, 2022

Choose a reason for hiding this comment

heanlan Jan 7, 2022

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Jan 14, 2022

heanlan commented Jan 14, 2022 • edited Loading

heanlan commented Jan 17, 2022

tnqn commented Jan 17, 2022

heanlan commented Jan 17, 2022

heanlan commented Jan 17, 2022 • edited Loading

heanlan commented Jan 17, 2022

heanlan commented Jan 17, 2022

tnqn commented Jan 18, 2022

heanlan commented Sep 1, 2021 •

edited

Loading

codecov-commenter commented Sep 1, 2021 •

edited

Loading

heanlan Nov 30, 2021 •

edited

Loading

heanlan commented Jan 14, 2022 •

edited

Loading

heanlan commented Jan 17, 2022 •

edited

Loading