Cache Flow modification message in Antrea Agent #4495

wenyingd · 2022-12-19T08:56:05Z

he motivation for this patch is to reduce the memory used for storing the installed OpenFlow flow entries to replay them when OVS is re-connected. In the existing code, struct ofctrl.Flow is stored in the memory, which has taken a lot of memory because all kinds of the candidate Match fields are allocated in one such object although they may not use in the final Flow.

This patch is to store the FlowModification message as a substituation of the ofctrl.Flow objects. As the FlowModification message is the obejct which was sent to OVS via the OF connections in final, it contains enough information to replay the OpenFlow flow entry. Besides, a FlowModification only contains the necessary fields which are set with values.

A test with this patch may reduce about 140M memory on a setup with about 130K OpenFlow entries in antrea-agent (from 380M to 240M)

Signed-off-by: wenyingd [email protected]

codecov · 2022-12-19T10:20:36Z

Codecov Report

Merging #4495 (656a1f9) into main (4de4392) will decrease coverage by 0.45%.
The diff coverage is n/a.

❗ Current head 656a1f9 differs from pull request most recent head 7740f91. Consider uploading reports for the commit 7740f91 to get more accurate results

@@            Coverage Diff             @@
##             main    #4495      +/-   ##
==========================================
- Coverage   68.31%   67.87%   -0.45%     
==========================================
  Files         400      403       +3     
  Lines       58324    57690     -634     
==========================================
- Hits        39843    39155     -688     
- Misses      15708    15823     +115     
+ Partials     2773     2712      -61

Flag	Coverage Δ
e2e-tests	`?`
integration-tests	`34.58% <0.00%> (+0.13%)`	⬆️
kind-e2e-tests	`46.69% <0.00%> (+0.11%)`	⬆️
unit-tests	`56.69% <0.00%> (-1.24%)`	⬇️

Impacted Files	Coverage Δ
cmd/antrea-agent/options_linux.go	`0.00% <0.00%> (-100.00%)`	⬇️
pkg/agent/wireguard/client_windows.go	`0.00% <0.00%> (-100.00%)`	⬇️
cmd/antrea-agent/util.go	`0.00% <0.00%> (-85.00%)`	⬇️
pkg/agent/util/ethtool/ethtool_linux.go	`0.00% <0.00%> (-70.00%)`	⬇️
pkg/monitor/controller.go	`51.73% <0.00%> (-26.09%)`	⬇️
pkg/monitor/agent.go	`84.21% <0.00%> (-15.79%)`	⬇️
pkg/agent/wireguard/client_linux.go	`66.24% <0.00%> (-15.28%)`	⬇️
cmd/antrea-agent/options.go	`23.20% <0.00%> (-13.27%)`	⬇️
pkg/agent/agent_linux.go	`4.65% <0.00%> (-10.80%)`	⬇️
pkg/graphviz/traceflow.go	`55.28% <0.00%> (-7.62%)`	⬇️
... and 95 more

wenyingd · 2023-01-31T01:03:22Z

/test-all
/test-windows-all

wenyingd · 2023-02-01T08:47:54Z

/test-all
/test-windows-all

wenyingd · 2023-02-07T09:13:25Z

/test-windows-all

pkg/agent/openflow/client_test.go

pkg/agent/openflow/framework.go

pkg/ovs/openflow/interfaces.go

pkg/ovs/openflow/ofctrl_bridge.go

pkg/agent/openflow/client_test.go

hongliangl · 2023-02-08T04:39:21Z

pkg/agent/openflow/network_policy_test.go

@@ -1046,6 +1116,10 @@ func preparePipelines() {
 		}
 	}
 	pipelineMap[pipelineID] = generatePipeline(pipelineID, requiredTables)
+	for _, obj := range tableCache.List() {


Why we need to add this?

Set ofctrl.Table with a valid TableID to ensure the OpenFlow Modification messages can be generated successfully in tests. Since the TableID is used when generating OF messages, without this step, the test may be crashed. newFakeClient is not suitable to mock client in the related cases.

pkg/agent/openflow/framework.go

tnqn · 2023-03-28T07:05:32Z

@wenyingd this is a large size change, could you add more details about it for reviewers to understand?

What's the purpose of the change? (I know it but other reviewers don't)
How "caching flow modification message in Antrea Agent" could achieve the goal?
A evaluation of the improvement, for example, on a Node with XXX flows, how much memory it consumes before and now?

wenyingd · 2023-03-28T07:35:27Z

@wenyingd this is a large size change, could you add more details about it for reviewers to understand?

1. What's the purpose of the change? (I know it but other reviewers don't)

2. How "caching flow modification message in Antrea Agent" could achieve the goal?

3. A evaluation of the improvement, for example, on a Node with XXX flows, how much memory it consumes before and now?

Updated in the commit message.

tnqn · 2023-03-29T15:36:48Z

pkg/agent/openflow/pipeline.go

+	key        string
+	message    ofctrl.OpenFlowModMessage
+	table      binding.Table
+	isDropFlow bool


I reviewed the current usage of key, table, and isDropFlow. The first one seems only used by methods added for antctl, which can be easily changed to return flows directly instead. The latter two are used only by flowsToTrace, which could actually get the same information from structs like conjunction or context. So perhaps we don't need to create this struct?

For table and isDropFlow fields, I don't think it is a good idea to re-generate the flows from conjunction or context for flowsToTrace which means consuming CPU calculation for the memory as the two fields are only two bytes per object in total. Instead, maybe I can get these information from the OpenFlowModMessage. Let me try to remove these two fields.

About key field, I would like to use another patch to construct from the flow modification mesage because it may introduce more changes. What do you think @tnqn

Removed the fields according to the offline discussion, and publish the functions to dynamically generate Flow string or match string from a FlowModification mesage. Could you review it again? @tnqn

pkg/agent/openflow/client.go

pkg/agent/openflow/pipeline.go

pkg/ovs/openflow/ofctrl_flow.go

tnqn · 2023-05-16T07:50:00Z

pkg/ovs/openflow/ofctrl_flow.go

 func (f *ofFlow) MatchString() string {
-	repr := fmt.Sprintf("table=%d", f.table.id)
-	if f.protocol != "" {
-		repr = fmt.Sprintf("%s,%s", repr, f.protocol)
-	}
-
-	if len(f.matchers) > 0 {
-		repr = fmt.Sprintf("%s,%s", repr, strings.Join(f.matchers, ","))
+	flowMod, err := f.getFlowMod()
+	if err != nil {
+		return ""
 	}
-	return repr
+	return FlowModMatchString(flowMod)
 }


Got it, now I see the called functions are different

tnqn

Please add the description to the commit message as well for future reference

pkg/ovs/openflow/utils.go

tnqn

Commit message is not formated properly. Follow the link in https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers

tnqn · 2023-05-16T15:02:22Z

The commit message is not wrapped with the same length limit. I will re-format it when merging. Would suggest to automate the formatting (e.g. set your .vimrc) to avoid efforts spent on this.

wenyingd · 2023-05-17T02:28:24Z

/test-all
/test-windows-all
/test-ipv6-all
/test-multicluster-e2e

tnqn · 2023-05-17T06:25:13Z

need rebase

tnqn · 2023-05-18T02:31:54Z

/test-all

tnqn · 2023-05-18T02:32:09Z

/test-ipv6-only-all
/test-ipv6-all
/test-windows-all

The motivation for this patch is to reduce the memory used for storing the installed OpenFlow flow entries to replay them when OVS is re-connected. In the existing code, struct ofctrl.Flow is stored in the memory, which has taken a lot of memory because all kinds of the candidate Match fields are allocated in one such object although they may not use in the final Flow. This patch is to store the FlowModification message as a substituation of the ofctrl.Flow objects. As the FlowModification message is the obejct which was sent to OVS via the OF connections in final, it contains enough information to replay the OpenFlow flow entry. Besides, a FlowModification only contains the necessary fields which are set with values. A test with this patch may reduce about 140M memory on a setup with about 130K OpenFlow entries in antrea-agent (from 380M to 240M) Signed-off-by: wenyingd <[email protected]>

tnqn · 2023-05-18T13:49:45Z

/test-all
/test-windows-all
/test-ipv6-all
/test-multicluster-e2e

The motivation for this patch is to reduce the memory used for storing the installed OpenFlow flow entries to replay them when OVS is re-connected. In the existing code, struct ofctrl.Flow is stored in the memory, which has taken a lot of memory because all kinds of the candidate Match fields are allocated in one such object although they may not use in the final Flow. This patch is to store the FlowModification message as a substituation of the ofctrl.Flow objects. As the FlowModification message is the obejct which was sent to OVS via the OF connections in final, it contains enough information to replay the OpenFlow flow entry. Besides, a FlowModification only contains the necessary fields which are set with values. A test with this patch may reduce about 140M memory on a setup with about 130K OpenFlow entries in antrea-agent (from 380M to 240M) Signed-off-by: wenyingd <[email protected]>

wenyingd force-pushed the agent_performance branch 2 times, most recently from 0933bdb to 3204cfd Compare December 21, 2022 07:02

wenyingd force-pushed the agent_performance branch from 3204cfd to 7740f91 Compare January 30, 2023 10:11

wenyingd force-pushed the agent_performance branch 2 times, most recently from 98c694c to f69a41f Compare February 1, 2023 07:07

wenyingd requested a review from hongliangl February 6, 2023 02:25

wenyingd force-pushed the agent_performance branch from f69a41f to 3459ed6 Compare February 8, 2023 02:59

hongliangl reviewed Feb 8, 2023

View reviewed changes

wenyingd force-pushed the agent_performance branch from 3459ed6 to 92e6c1e Compare February 9, 2023 02:28

wenyingd force-pushed the agent_performance branch 2 times, most recently from 350b7ea to 6ab2d29 Compare March 24, 2023 08:28

wenyingd requested a review from tnqn March 27, 2023 02:15

wenyingd added this to the Antrea v1.12 release milestone Mar 27, 2023

wenyingd requested a review from hongliangl March 27, 2023 02:16

hongliangl reviewed Mar 27, 2023

View reviewed changes

pkg/agent/openflow/framework.go Outdated Show resolved Hide resolved

wenyingd force-pushed the agent_performance branch from 6ab2d29 to 7cf875b Compare March 27, 2023 05:41

tnqn reviewed Mar 29, 2023

View reviewed changes

wenyingd force-pushed the agent_performance branch 4 times, most recently from cbbf692 to ec409ec Compare April 4, 2023 02:07

luolanzone mentioned this pull request Apr 4, 2023

Antrea Agent memory optimization #4479

Closed

2 tasks

wenyingd force-pushed the agent_performance branch from ec409ec to 7ecbdda Compare April 4, 2023 10:03

wenyingd force-pushed the agent_performance branch 5 times, most recently from 6809cd6 to 85a6be6 Compare May 16, 2023 07:08

tnqn reviewed May 16, 2023

View reviewed changes

pkg/ovs/openflow/utils.go Outdated Show resolved Hide resolved

wenyingd force-pushed the agent_performance branch from 85a6be6 to 9898148 Compare May 16, 2023 09:02

tnqn reviewed May 16, 2023

View reviewed changes

wenyingd force-pushed the agent_performance branch from 9898148 to b09854c Compare May 16, 2023 10:31

tnqn approved these changes May 16, 2023

View reviewed changes

tnqn previously approved these changes May 17, 2023

View reviewed changes

wenyingd dismissed tnqn’s stale review via 13e3b5a May 17, 2023 16:58

wenyingd force-pushed the agent_performance branch from b09854c to 13e3b5a Compare May 17, 2023 16:58

tnqn previously approved these changes May 18, 2023

View reviewed changes

wenyingd dismissed tnqn’s stale review via 23652ce May 18, 2023 10:59

wenyingd force-pushed the agent_performance branch from 13e3b5a to 23652ce Compare May 18, 2023 10:59

tnqn approved these changes May 19, 2023

View reviewed changes

tnqn added the action/release-note Indicates a PR that should be included in release notes. label May 19, 2023

tnqn merged commit 11026df into antrea-io:main May 19, 2023

wenyingd deleted the agent_performance branch May 30, 2023 06:45

antoninbas mentioned this pull request Jun 14, 2023

Fix incorrect FlowMod message passing in openflow client modifyFlows #5125

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache Flow modification message in Antrea Agent #4495

Cache Flow modification message in Antrea Agent #4495

wenyingd commented Dec 19, 2022 •

edited

Loading

codecov bot commented Dec 19, 2022 •

edited

Loading

wenyingd commented Jan 31, 2023

wenyingd commented Feb 1, 2023

wenyingd commented Feb 7, 2023

hongliangl Feb 8, 2023

wenyingd Feb 9, 2023

tnqn commented Mar 28, 2023

wenyingd commented Mar 28, 2023

tnqn Mar 29, 2023

wenyingd Mar 30, 2023

wenyingd Apr 10, 2023

tnqn May 16, 2023

tnqn left a comment

tnqn left a comment

tnqn commented May 16, 2023

wenyingd commented May 17, 2023

tnqn commented May 17, 2023

tnqn commented May 18, 2023

tnqn commented May 18, 2023

tnqn commented May 18, 2023

Cache Flow modification message in Antrea Agent #4495

Cache Flow modification message in Antrea Agent #4495

Conversation

wenyingd commented Dec 19, 2022 • edited Loading

codecov bot commented Dec 19, 2022 • edited Loading

Codecov Report

wenyingd commented Jan 31, 2023

wenyingd commented Feb 1, 2023

wenyingd commented Feb 7, 2023

hongliangl Feb 8, 2023

Choose a reason for hiding this comment

wenyingd Feb 9, 2023

Choose a reason for hiding this comment

tnqn commented Mar 28, 2023

wenyingd commented Mar 28, 2023

tnqn Mar 29, 2023

Choose a reason for hiding this comment

wenyingd Mar 30, 2023

Choose a reason for hiding this comment

wenyingd Apr 10, 2023

Choose a reason for hiding this comment

tnqn May 16, 2023

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented May 16, 2023

wenyingd commented May 17, 2023

tnqn commented May 17, 2023

tnqn commented May 18, 2023

tnqn commented May 18, 2023

tnqn commented May 18, 2023

wenyingd commented Dec 19, 2022 •

edited

Loading

codecov bot commented Dec 19, 2022 •

edited

Loading