Add support to modify OF table name #2585

wenyingd · 2021-08-12T13:29:23Z

Use readable name for openflow tables.
Fixes #2617

Signed-off-by: wenyingd [email protected]

codecov-commenter · 2021-08-12T13:34:32Z

Codecov Report

Merging #2585 (50c1726) into main (95e836d) will increase coverage by 0.44%.
The diff coverage is 82.67%.

@@            Coverage Diff             @@
##             main    #2585      +/-   ##
==========================================
+ Coverage   61.18%   61.62%   +0.44%     
==========================================
  Files         284      283       -1     
  Lines       23494    23625     +131     
==========================================
+ Hits        14374    14559     +185     
+ Misses       7565     7492      -73     
- Partials     1555     1574      +19

Flag	Coverage Δ
e2e-tests	`∅ <ø> (?)`
kind-e2e-tests	`49.33% <80.26%> (+0.67%)`	⬆️
unit-tests	`40.87% <25.98%> (+<0.01%)`	⬆️

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

Impacted Files	Coverage Δ
pkg/agent/openflow/client.go	`58.33% <ø> (+0.67%)`	⬆️
pkg/agent/types/networkpolicy.go	`83.33% <ø> (ø)`
pkg/antctl/antctl.go	`66.66% <0.00%> (-33.34%)`	⬇️
pkg/ovs/ovsctl/interface.go	`0.00% <ø> (ø)`
pkg/ovs/ovsctl/ofctl.go	`37.07% <33.33%> (-1.20%)`	⬇️
pkg/agent/controller/networkpolicy/packetin.go	`72.00% <75.00%> (ø)`
pkg/agent/controller/networkpolicy/reconciler.go	`77.29% <77.77%> (-0.10%)`	⬇️
pkg/agent/openflow/pipeline.go	`78.27% <82.42%> (+2.70%)`	⬆️
pkg/agent/controller/traceflow/packetin.go	`64.60% <83.33%> (+5.11%)`	⬆️
pkg/ovs/openflow/ofctrl_bridge.go	`54.38% <87.03%> (+4.72%)`	⬆️
... and 30 more

wenyingd · 2021-08-20T01:55:21Z

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

wenyingd · 2021-08-20T10:35:42Z

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

wenyingd · 2021-08-23T02:04:55Z

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

wenyingd · 2021-08-23T05:30:48Z

/test-ipv6-e2e
/test-ipv6-only-conformance
/test-ipv6-only-e2e
/test-networkpolicy
/test-windows-networkpolicy

wenyingd · 2021-08-23T08:39:33Z

/test-ipv6-only-e2e

jianjuns

BTW, I was thinking could we define variables for each binding.Table, and so needs not to get the table by c.pipeline[id] every time to save a hash table lookup. But did not get a good idea to maintain the binding.Table - name - ID mapping with some clean/simple code.

pkg/ovs/openflow/ofctrl_bridge.go

pkg/agent/openflow/pipeline.go

wenyingd · 2021-08-25T05:33:45Z

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

pkg/agent/openflow/pipeline.go

wenyingd · 2021-08-31T10:15:33Z

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

jianjuns

LGTM. Just several nits.

pkg/agent/openflow/pipeline.go

antoninbas

LGTM, thanks for addressing comments and resolving the multipart reply receive issue in libOpenflow / ofnet

pkg/ovs/openflow/ofctrl_bridge.go

wenyingd · 2021-09-29T06:41:06Z

/skip-all
/skip-ipv6-all
/skip-ipv6-only-all
/skip-windows-all

Since the change is only for comments but not about logic code, and tests in last round already passed, I skip the tests in this round.

wenyingd · 2021-09-30T05:58:50Z

/skip-all
/skip-ipv6-all
/skip-ipv6-only-all
/skip-windows-all

antoninbas

LGTM

@tnqn any follow-up comment?

pkg/ovs/openflow/ofctrl_bridge.go

wenyingd · 2021-10-08T06:39:21Z

/test-all
/test-ipv6-all
/test-ipv6-only-all
/test-windows-all
/test-integration
Rebase the code, so re-trigger the tests here.

wenyingd · 2021-10-08T14:11:57Z

/test-conformance

wenyingd · 2021-10-18T01:06:35Z

@tnqn Could you help review this PR again?

tnqn · 2021-10-18T05:22:28Z

pkg/agent/apiserver/handlers/ovsflows/handler_test.go

+	// Create openflow.Client to ensure the OVS tables are added into the cache.
+	bridgeName := "testbr"
+	bridgeMgmtAddr := binding.GetMgmtAddress(ovsconfig.DefaultOVSRunDir, bridgeName)
+	openflow.NewClient(bridgeName, bridgeMgmtAddr, ovsconfig.OVSDatapathSystem, true, false, false, false, true)


Why is this needed when it's not used by any instance below? If this is really needed, it might indicate there is an anti-OOP code in the openflow package where some global variables require some objects to initialize implicitly.

My bad, these code is added to ensure the functions openflow.GetFlowTableName and openflow.GetFlowTableID are called correctly after the binding.Table is added into cache ofTableCache, and the related logic is called in function createOFTable. To unbreak the code style, I would use function variables in handler.go and use mock functions in the unit test.

pkg/ovs/openflow/ofctrl_bridge.go

wenyingd · 2021-10-18T07:24:44Z

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

1. Use Multipart messages to query OVS table features and modify the table name property if the table is defined in Antrea pipeline 2. Use openflow.Table in the ofClient under pkg/agent/openflow directly, and remove the pipeline map and FlowTables in pipeline.go 3. Remove the typed type TableIDType, and use uint8 for table id directly. Signed-off-by: wenyingd <[email protected]>

tnqn

LGTM

wenyingd · 2021-10-18T12:27:28Z

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

wenyingd · 2021-10-18T23:56:59Z

/test-ipv6-e2e
/test-windows-networkpolicy

wenyingd · 2021-10-19T06:07:43Z

/test-ipv6-e2e
/test-windows-networkpolicy

tnqn · 2021-10-19T08:58:41Z

@wenyingd I will edit the commit message when merging it. Could you make the commit message of other patches wrapped properly following https://github.com/antrea-io/antrea/blob/main/CONTRIBUTING.md#getting-reviewers in the future?

wenyingd force-pushed the name_table branch from bb64b04 to 860cbde Compare August 12, 2021 13:57

wenyingd force-pushed the name_table branch from 860cbde to 2d220a7 Compare August 20, 2021 01:50

wenyingd changed the title ~~[WIP] Add support to modify OF table name~~ Add support to modify OF table name Aug 20, 2021

wenyingd requested review from antoninbas, jianjuns and tnqn August 20, 2021 01:56

wenyingd changed the title ~~Add support to modify OF table name~~ [WIP]Add support to modify OF table name Aug 20, 2021

lzhecheng force-pushed the name_table branch from 2d220a7 to f0cf918 Compare August 20, 2021 05:32

wenyingd force-pushed the name_table branch from f0cf918 to 0357bc3 Compare August 23, 2021 01:38

wenyingd changed the title ~~[WIP]Add support to modify OF table name~~ Add support to modify OF table name Aug 23, 2021

jianjuns reviewed Aug 23, 2021

View reviewed changes

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

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

wenyingd force-pushed the name_table branch from 0357bc3 to 890e0f1 Compare August 24, 2021 02:18

wenyingd requested a review from jianjuns August 24, 2021 02:39

wenyingd force-pushed the name_table branch 4 times, most recently from a877a48 to f1ea04f Compare August 24, 2021 15:11

wenyingd force-pushed the name_table branch from f1ea04f to a17c3b3 Compare August 30, 2021 02:47

jianjuns reviewed Aug 31, 2021

View reviewed changes

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

wenyingd force-pushed the name_table branch from a17c3b3 to 25d7867 Compare August 31, 2021 07:22

jianjuns reviewed Aug 31, 2021

View reviewed changes

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

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

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

wenyingd closed this Sep 1, 2021

antoninbas previously approved these changes Sep 29, 2021

View reviewed changes

antoninbas reviewed Sep 29, 2021

View reviewed changes

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

wenyingd dismissed antoninbas’s stale review via a656941 September 29, 2021 02:19

wenyingd force-pushed the name_table branch from 766b38e to a656941 Compare September 29, 2021 02:19

wenyingd force-pushed the name_table branch from a656941 to f9c29af Compare September 30, 2021 05:42

antoninbas previously approved these changes Sep 30, 2021

View reviewed changes

pkg/ovs/openflow/ofctrl_bridge.go Show resolved Hide resolved

antoninbas mentioned this pull request Oct 5, 2021

Bump antrea.io/libOpenflow from 0.2.0 to 0.5.1 #2805

Closed

wenyingd dismissed antoninbas’s stale review via 1400cf5 October 8, 2021 05:38

wenyingd force-pushed the name_table branch from f9c29af to 1400cf5 Compare October 8, 2021 05:38

tnqn reviewed Oct 18, 2021

View reviewed changes

wenyingd force-pushed the name_table branch from 1400cf5 to c78480b Compare October 18, 2021 06:52

wenyingd force-pushed the name_table branch from c78480b to 4b8f7bf Compare October 18, 2021 09:22

wenyingd force-pushed the name_table branch from 4b8f7bf to 50c1726 Compare October 18, 2021 09:27

tnqn approved these changes Oct 18, 2021

View reviewed changes

wenyingd requested review from antoninbas, tnqn and jianjuns October 19, 2021 06:15

tnqn merged commit 95156bd into antrea-io:main Oct 19, 2021

wenyingd deleted the name_table branch August 15, 2022 03:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support to modify OF table name #2585

Add support to modify OF table name #2585

wenyingd commented Aug 12, 2021 •

edited

Loading

codecov-commenter commented Aug 12, 2021 •

edited

Loading

wenyingd commented Aug 20, 2021

wenyingd commented Aug 20, 2021

wenyingd commented Aug 23, 2021

wenyingd commented Aug 23, 2021

wenyingd commented Aug 23, 2021

jianjuns left a comment

wenyingd commented Aug 25, 2021

wenyingd commented Aug 31, 2021

jianjuns left a comment

antoninbas left a comment

wenyingd commented Sep 29, 2021

wenyingd commented Sep 30, 2021

antoninbas left a comment

wenyingd commented Oct 8, 2021

wenyingd commented Oct 8, 2021

wenyingd commented Oct 18, 2021

tnqn Oct 18, 2021

wenyingd Oct 18, 2021 •

edited

Loading

wenyingd commented Oct 18, 2021

tnqn left a comment

wenyingd commented Oct 18, 2021

wenyingd commented Oct 18, 2021

wenyingd commented Oct 19, 2021

tnqn commented Oct 19, 2021

Add support to modify OF table name #2585

Add support to modify OF table name #2585

Conversation

wenyingd commented Aug 12, 2021 • edited Loading

codecov-commenter commented Aug 12, 2021 • edited Loading

Codecov Report

wenyingd commented Aug 20, 2021

wenyingd commented Aug 20, 2021

wenyingd commented Aug 23, 2021

wenyingd commented Aug 23, 2021

wenyingd commented Aug 23, 2021

jianjuns left a comment

Choose a reason for hiding this comment

wenyingd commented Aug 25, 2021

wenyingd commented Aug 31, 2021

jianjuns left a comment

Choose a reason for hiding this comment

antoninbas left a comment

Choose a reason for hiding this comment

wenyingd commented Sep 29, 2021

wenyingd commented Sep 30, 2021

antoninbas left a comment

Choose a reason for hiding this comment

wenyingd commented Oct 8, 2021

wenyingd commented Oct 8, 2021

wenyingd commented Oct 18, 2021

tnqn Oct 18, 2021

Choose a reason for hiding this comment

wenyingd Oct 18, 2021 • edited Loading

Choose a reason for hiding this comment

wenyingd commented Oct 18, 2021

tnqn left a comment

Choose a reason for hiding this comment

wenyingd commented Oct 18, 2021

wenyingd commented Oct 18, 2021

wenyingd commented Oct 19, 2021

tnqn commented Oct 19, 2021

wenyingd commented Aug 12, 2021 •

edited

Loading

codecov-commenter commented Aug 12, 2021 •

edited

Loading

wenyingd Oct 18, 2021 •

edited

Loading