Use OVS port external_ids to save Antrea interface type #3027

wenyingd · 2021-11-15T03:48:01Z

Introduce Antrea interface yype to map the OVS port to Antrea created
interfaces, and use OVS port external_ids field to save the
configuration. This is helpful for Antrea to define more interface types.

Signed-off-by: wenyingd [email protected]

codecov-commenter · 2021-11-15T03:54:26Z

Codecov Report

Merging #3027 (58c399d) into main (9a52de9) will decrease coverage by 1.19%.
The diff coverage is 40.42%.

@@            Coverage Diff             @@
##             main    #3027      +/-   ##
==========================================
- Coverage   60.94%   59.75%   -1.20%     
==========================================
  Files         292      292              
  Lines       24742    24801      +59     
==========================================
- Hits        15079    14819     -260     
- Misses       8012     8374     +362     
+ Partials     1651     1608      -43

Flag	Coverage Δ
kind-e2e-tests	`46.40% <27.65%> (-1.69%)`	⬇️
unit-tests	`40.07% <19.14%> (-0.05%)`	⬇️

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

Impacted Files	Coverage Δ
pkg/agent/agent_linux.go	`3.62% <0.00%> (-0.06%)`	⬇️
pkg/agent/interfacestore/types.go	`72.72% <ø> (ø)`
pkg/ovs/ovsconfig/ovs_client.go	`46.13% <0.00%> (-1.22%)`	⬇️
pkg/agent/agent.go	`50.90% <48.68%> (-0.96%)`	⬇️
pkg/agent/cniserver/pod_configuration.go	`53.51% <100.00%> (+0.18%)`	⬆️
...g/agent/apiserver/handlers/featuregates/handler.go	`0.00% <0.00%> (-82.36%)`	⬇️
...kg/apiserver/registry/system/supportbundle/rest.go	`19.54% <0.00%> (-55.18%)`	⬇️
pkg/support/dump_others.go	`0.00% <0.00%> (-51.73%)`	⬇️
pkg/support/dump.go	`8.13% <0.00%> (-49.60%)`	⬇️
...g/agent/apiserver/handlers/addressgroup/handler.go	`0.00% <0.00%> (-40.00%)`	⬇️
... and 17 more

wenyingd · 2021-11-17T07:44:51Z

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

jianjuns · 2021-11-17T19:14:33Z

pkg/agent/agent.go

+			case ovsconfig.AntreaHostIF:
+				// Not load the host interface.
+				intf = nil
+			default:


Why we do not have a type for container interface here?

I originally thought the default case was the container Interface like the old way. Added.

pkg/agent/agent.go

jianjuns · 2021-11-18T17:20:48Z

pkg/agent/agent.go

+				intf = nil
+			case ovsconfig.AntreaContainer:
+				// The port should be for a container interface.
+				intf = cniserver.ParseOVSPortInterfaceConfig(port, ovsPort, true)


Is it possible to have other unknown type string here, or we already checked earlier?

Do you mean we need another type like "AntreaUnknown" except for "AntreaUnSet" (https://github.com/antrea-io/antrea/pull/3027/files/f594a81c0f13f1c2e784c07f9e3d2a04ef2499dd#). From the existing code, all above types are what we have used, and we use "AntreaUnSet" if we didn't find key "AntreaIFType" from the external_ids.

I meant what happens if the value of AntreaIFType is not any of our predefined one? Do we check earlier or we should check here.

No, I didn't check it earlier. If the value is not by use defined, the intf is nil, and we have a precheck on "intf != nil" before adding the intf to interfaceStore. So here I would add one case for unknown (maybe default) and add log. I don't think we need a specific type string for "AntreaUnknown", because it is only one string value and can match all unpredefined values.

Sounds good to me.

wenyingd · 2021-11-19T01:12:12Z

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

pkg/agent/agent.go

tnqn · 2021-11-23T17:02:39Z

pkg/agent/agent.go

@@ -226,50 +226,94 @@ func (i *Initializer) initInterfaceStore() error {
 		return err
 	}

+	parseGatewayIFFunc := func(port *ovsconfig.OVSPortData, ovsPort *interfacestore.OVSPortConfig) *interfacestore.InterfaceConfig {


Could we write shorthand of single word in camel case to be differentiated from acronyms?
For example, we use svc instead of SVC for service, use ch instead of CH for channel, use cfg instead CFG for configration.

change to "parseGatewayInterfaceFunc"

tnqn · 2021-11-23T17:19:47Z

pkg/agent/agent.go

-				InterfaceName: port.Name,
-				OVSPortConfig: ovsPort,
+		} else {
+			// Antrea Interface type is not saved in OVS Interface external_ids in earlier Antrea versions, so we use


Since this change will affect the existing storage and introduces upgrade complexity, I'd like to understand why we need these types explicitly stored. Doesn't the "type" of OVS interface already differentiate internal port, tunnel port, and attached port (including container port and uplink port)? What do we want to differentiate further?

The existing code is actually using the predefined ofport number to parse some kinds of the interfaces, e.g. gw0, uplink, this has two weaks: 1) If the OF port number is taken before creating the OVS port, the actual port number allocated by OVS is different from ofport_request, then we might fail in restart case when using these predefined numbers; 2) It is difficult to expand to multiple interfaces with such types, like supporting multiple uplink interfaces. Using a fixed type value can resolve the above issues.

pkg/ovs/ovsconfig/ovs_client.go

pkg/agent/openflow/fields.go

pkg/ovs/ovsconfig/interfaces.go

pkg/ovs/ovsconfig/ovs_client.go

wenyingd · 2021-11-26T11:06:02Z

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

wenyingd · 2021-11-29T03:28:05Z

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

wenyingd · 2021-11-30T00:33:20Z

/test-conformance
/test-ipv6-all
/test-windows-all

pkg/ovs/ovsconfig/interfaces.go

pkg/agent/agent_test.go

tnqn · 2021-11-30T15:04:04Z

pkg/agent/agent_linux.go

@@ -208,7 +211,7 @@ func (i *Initializer) BridgeUplinkToOVSBridge() error {
 		return err
 	}
 	// Create uplink port.
-	uplinkPortUUId, err := i.ovsBridgeClient.CreateUplinkPort(uplink, config.UplinkOFPort, map[string]interface{}{"antrea-uplink": "true"})
+	uplinkPortUUId, err := i.ovsBridgeClient.CreateUplinkPort(uplink, config.UplinkOFPort, map[string]interface{}{"antrea-uplink": "true", interfacestore.AntreaInterfaceTypeKey: interfacestore.AntreaUplink})


Should we deprecate "antrea-uplink"?

@gran-vmv is it possible to use the Antrea interface type instead of the "antrea-uplink"? May I know where is "antrea-uplink" used in the existing code?

It's used in start_ovs for ensuring the uplink is removed before stopping ovs.

Yes, it is used to check the existence of uplink when OVS shutdown, you can refactor here if you want to remove existing key-value.

antrea/build/images/scripts/start_ovs

Line 114 in eded51a

while [ "`ovsdb-client dump Port|grep \"{antrea-uplink=\\\"true\\\"}\"`" != "" ]; do

pkg/agent/agent.go

Introduce Antrea interface type to map the OVS port to Antrea created interfaces, and use OVS port external_ids field to save the configuration. This is helpful for Antrea to define more interface types. Signed-off-by: wenyingd <[email protected]>

wenyingd · 2021-12-01T06:46:40Z

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

wenyingd · 2021-12-01T08:41:09Z

/test-windows-all
/test-windows-proxyall-e2e

tnqn

LGTM

tnqn · 2021-12-02T16:43:28Z

/test-flexible-ipam-e2e

wenyingd · 2021-12-03T11:57:51Z

/test-windows-all

tnqn · 2021-12-06T16:55:29Z

/test-flexible-ipam-e2e

wenyingd · 2021-12-07T02:20:00Z

/test-flexible-ipam-e2e

wenyingd force-pushed the antrea_if_type branch from 8ab252f to 5e69e3c Compare November 15, 2021 03:50

wenyingd force-pushed the antrea_if_type branch from 5e69e3c to 648c4bc Compare November 15, 2021 05:54

wenyingd requested a review from jianjuns November 17, 2021 07:44

jianjuns reviewed Nov 17, 2021

View reviewed changes

wenyingd force-pushed the antrea_if_type branch 2 times, most recently from aa13484 to f594a81 Compare November 18, 2021 09:37

jianjuns reviewed Nov 18, 2021

View reviewed changes

wenyingd force-pushed the antrea_if_type branch from f594a81 to faa7965 Compare November 19, 2021 01:05

wenyingd requested a review from jianjuns November 22, 2021 23:58

jianjuns reviewed Nov 23, 2021

View reviewed changes

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

jianjuns previously approved these changes Nov 23, 2021

View reviewed changes

wenyingd requested a review from tnqn November 23, 2021 08:58

tnqn reviewed Nov 23, 2021

View reviewed changes

tnqn reviewed Nov 24, 2021

View reviewed changes

wenyingd dismissed jianjuns’s stale review via 55a2311 November 25, 2021 09:38

wenyingd force-pushed the antrea_if_type branch 2 times, most recently from 55a2311 to 434ed24 Compare November 26, 2021 11:04

wenyingd requested a review from tnqn November 26, 2021 11:05

wenyingd force-pushed the antrea_if_type branch from 434ed24 to c6c6aa7 Compare November 29, 2021 01:59

wenyingd changed the title ~~Use OVS Interface external_ids to save Antrea Interface type~~ Use OVS port external_ids to save Antrea interface type Nov 29, 2021

wenyingd force-pushed the antrea_if_type branch from c6c6aa7 to e503455 Compare November 29, 2021 02:03

tnqn reviewed Nov 30, 2021

View reviewed changes

wenyingd force-pushed the antrea_if_type branch from e503455 to 2fd1027 Compare December 1, 2021 05:24

wenyingd force-pushed the antrea_if_type branch from 2fd1027 to 58c399d Compare December 1, 2021 06:34

tnqn approved these changes Dec 2, 2021

View reviewed changes

jianjuns approved these changes Dec 3, 2021

View reviewed changes

tnqn merged commit 3fed840 into antrea-io:main Dec 7, 2021

wenyingd deleted the antrea_if_type branch August 15, 2022 03:32

antoninbas mentioned this pull request Apr 9, 2024

Use reserved OVS controller ports for default Antrea ports #6202

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use OVS port external_ids to save Antrea interface type #3027

Use OVS port external_ids to save Antrea interface type #3027

wenyingd commented Nov 15, 2021 •

edited

Loading

codecov-commenter commented Nov 15, 2021 •

edited

Loading

wenyingd commented Nov 17, 2021

jianjuns Nov 17, 2021

wenyingd Nov 18, 2021

jianjuns Nov 18, 2021

wenyingd Nov 19, 2021

jianjuns Nov 19, 2021

wenyingd Nov 19, 2021 •

edited

Loading

jianjuns Nov 23, 2021

wenyingd commented Nov 19, 2021

tnqn Nov 23, 2021

wenyingd Nov 25, 2021

tnqn Nov 23, 2021

wenyingd Nov 25, 2021

wenyingd commented Nov 26, 2021

wenyingd commented Nov 29, 2021

wenyingd commented Nov 30, 2021

tnqn Nov 30, 2021

wenyingd Dec 1, 2021

tnqn Dec 1, 2021

gran-vmv Dec 1, 2021

wenyingd commented Dec 1, 2021

wenyingd commented Dec 1, 2021

tnqn left a comment

tnqn commented Dec 2, 2021

wenyingd commented Dec 3, 2021

tnqn commented Dec 6, 2021

wenyingd commented Dec 7, 2021

Use OVS port external_ids to save Antrea interface type #3027

Use OVS port external_ids to save Antrea interface type #3027

Conversation

wenyingd commented Nov 15, 2021 • edited Loading

codecov-commenter commented Nov 15, 2021 • edited Loading

Codecov Report

wenyingd commented Nov 17, 2021

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

wenyingd Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wenyingd commented Nov 19, 2021

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

wenyingd commented Nov 26, 2021

wenyingd commented Nov 29, 2021

wenyingd commented Nov 30, 2021

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

wenyingd commented Dec 1, 2021

wenyingd commented Dec 1, 2021

tnqn left a comment

Choose a reason for hiding this comment

tnqn commented Dec 2, 2021

wenyingd commented Dec 3, 2021

tnqn commented Dec 6, 2021

wenyingd commented Dec 7, 2021

wenyingd commented Nov 15, 2021 •

edited

Loading

codecov-commenter commented Nov 15, 2021 •

edited

Loading

wenyingd Nov 19, 2021 •

edited

Loading