[flexible-ipam] Use uplink interface name for host interface internal port by gran-vmv · Pull Request #3938 · antrea-io/antrea

gran-vmv · 2022-06-24T08:26:15Z

Use uplink interface name for host interface internal port, to support DHCP client.
Add a suffix to uplink when bridging to OVS bridge.

Signed-off-by: gran gran@vmware.com

codecov-commenter · 2022-06-24T09:15:08Z

Codecov Report

Merging #3938 (8f0bdf0) into main (2f9134b) will increase coverage by 1.51%.
The diff coverage is 19.23%.

❗ Current head 8f0bdf0 differs from pull request most recent head 7584f1d. Consider uploading reports for the commit 7584f1d to get more accurate results

@@            Coverage Diff             @@
##             main    #3938      +/-   ##
==========================================
+ Coverage   64.58%   66.09%   +1.51%     
==========================================
  Files         295      309      +14     
  Lines       43740    43875     +135     
==========================================
+ Hits        28251    29001     +750     
+ Misses      13215    12558     -657     
- Partials     2274     2316      +42

Flag	Coverage Δ	*Carryforward flag
e2e-tests	`60.35% <ø> (?)`
kind-e2e-tests	`51.15% <18.26%> (ø)`	Carriedforward from 2f9134b
unit-tests	`44.31% <3.84%> (ø)`	Carriedforward from 2f9134b

*This pull request uses carry forward flags. Click here to find out more.

Impacted Files	Coverage Δ
pkg/agent/agent_linux.go	`5.14% <0.00%> (ø)`
pkg/agent/cniserver/pod_configuration.go	`54.59% <0.00%> (ø)`
pkg/agent/util/net_linux.go	`31.61% <ø> (ø)`
pkg/agent/util/net.go	`51.94% <21.42%> (ø)`
pkg/ovs/ovsconfig/ovs_client.go	`47.79% <62.50%> (ø)`
pkg/agent/agent.go	`52.42% <100.00%> (ø)`
pkg/agent/controller/trafficcontrol/controller.go	`81.08% <100.00%> (ø)`
... and 36 more

gran-vmv · 2022-06-27T01:59:18Z

 	FamilyIPv4 uint8 = 4
 	FamilyIPv6 uint8 = 6
+
+	BridgedUplinkPrefix = "b-"


@wenyingd Which prefix you used in BM/VM?

Prefix phy is used in VM support case. But I am thinking maybe ant is better to represent it is renamed by Antrea, just like antctl? What's your thought @gran-vmv @mengdie-song ?

Could we use antrea- (as antrea-gw0, antrea-tun0), or you would choose a shorter prefix due to the interface length limit?

I know NSX-T appends a single letter to the interface name (I remember it is ~, like eth0~).

Linux has a property IFNAMSIZ to limit the network interface name length to 15 bytes, thus I prefer to use short prefix/suffix, and I feel that suffix ~ is better. @wenyingd @tnqn @mengdie-song

I am OK with the suffix, but I think we should test if it is working on Windows, let me try.

In BM/VM and this PR, we'll restrict the uplink interface name length, thus we'd better choose a shorter prefix/suffix to improve the compatibility.

wenyingd · 2022-06-28T06:54:35Z

+
 	// Create uplink port.
-	uplinkPortUUID, err := i.ovsBridgeClient.CreateUplinkPort(uplink, int32(i.nodeConfig.UplinkNetConfig.OFPort), map[string]interface{}{interfacestore.AntreaInterfaceTypeKey: interfacestore.AntreaUplink})
+	uplinkPortUUID, err := i.ovsBridgeClient.CreateUplinkPort(bridgedUplinkName, int32(uplinkNetConfig.OFPort), map[string]interface{}{interfacestore.AntreaInterfaceTypeKey: interfacestore.AntreaUplink})


Is it possible to provide an API from ovsBridgeClient to support create a pair of ports? We could give the expected names of uplink and host internal port as parameters, along with other parameters like expected port MAC?

Yes, but I think we can do this refactor in future.

wenyingd · 2022-06-28T06:57:32Z

 	return i.ovsBridgeClient.SetInterfaceMTU(iface, mtu)
 }
+
+func getTransportIPNetDeviceByNameWithExcludedList(interfaceName string, excludedIPs []*net.IPNet) ([]*net.IPNet, error) {


Move this to path pkt/agent/util/net ? The same as function getTransportIPNetDeviceByName

Rename as getTransportIPNetByNameWithExcludedList as it returns []*net.IPNet but not net interface?

Fixed name.
As you commented in getTransportIPNetDeviceByName, we should keep this func in package agent, and do refactor in future if needed.
// getTransportIPNetDeviceByName is meant to be overridden for testing.

wenyingd · 2022-06-28T07:44:48Z

 	SetInterfaceOptions(name string, options map[string]interface{}) Error
 	CreatePort(name, ifDev string, externalIDs map[string]interface{}) (string, Error)
 	CreateAccessPort(name, ifDev string, externalIDs map[string]interface{}, vlanID uint16) (string, Error)
-	CreateInternalPort(name string, ofPortRequest int32, externalIDs map[string]interface{}) (string, Error)


I would prefer to add a new signature to support configuring port MAC with a specific value, since this is not a general case.

I prefer to add MAC for this func, since MAC property is supported only in InternalPort, and the default value "" also matches OVS spec.

@tnqn @jianjuns What is your point?

No strong preference. Either way works for me. I feel no issue to add MAC to this func.

No strong preference from me.

OK. Let's keep existing changes.

gran-vmv · 2022-07-04T01:13:51Z

@wenyingd @tnqn @jianjuns Any more comments?

wenyingd

Overall LGTM, one ask.

wenyingd · 2022-07-05T12:04:59Z

 	return i.ovsBridgeClient.SetInterfaceMTU(iface, mtu)
 }
+
+func getTransportIPNetByNameWithExcludedList(interfaceName string, excludedIPs []*net.IPNet) ([]*net.IPNet, error) {


I didn't find a consumer of this function to provide valid excludedIPs, is this parameter really needed?

I also mentioned the same question in VM patch, is it necessary in this function to filter only global IP address? I didn't find an example to say it is harm, but just a bit confuse on it.

Agree with Wenying, generically we should avoid adding code/parameters that are not used anywhere to save development, review, maintenance efforts. They can be added any time when they are actually required.

Removed unused parameter.

tnqn · 2022-07-05T15:25:31Z

-		freePort, err := i.getFreeOFPort(config.UplinkOFPort)
+		uplinkNetConfig.OFPort = uint32(freePort)
+		klog.InfoS("Set OpenFlow port in UplinkNetConfig", "ofport", uplinkNetConfig.OFPort)
+		freePort, err = i.getFreeOFPort(config.UplinkOFPort, []int32{freePort})


I wonder if we should still use a predefined config.BridgeOFPort for the host interface to make the process of allocating ofport for well known ports consistent and less confusing to use config.UplinkOFPort as start port for HostInterfaceOFPort.

Guess the problem is we may potentially have multiple uplinks. @wenyingd

But why we must allocate the port in advance, rather than let CreatePort() return a OFPort?

config.UplinkOFPort is a reserved port ID which is not valid in a port request.
This line means we apply for next available port ID and the start ID is UplinkOFPort.
Jianjun suggested another design in #3938 (comment)

tnqn · 2022-07-05T15:35:08Z

 	return i.ovsBridgeClient.SetInterfaceMTU(iface, mtu)
 }
+
+func getTransportIPNetByNameWithExcludedList(interfaceName string, excludedIPs []*net.IPNet) ([]*net.IPNet, error) {


Agree with Wenying, generically we should avoid adding code/parameters that are not used anywhere to save development, review, maintenance efforts. They can be added any time when they are actually required.

tnqn · 2022-07-05T16:10:33Z

 	SetInterfaceOptions(name string, options map[string]interface{}) Error
 	CreatePort(name, ifDev string, externalIDs map[string]interface{}) (string, Error)
 	CreateAccessPort(name, ifDev string, externalIDs map[string]interface{}, vlanID uint16) (string, Error)
-	CreateInternalPort(name string, ofPortRequest int32, externalIDs map[string]interface{}) (string, Error)


No strong preference from me.

tnqn · 2022-07-05T16:12:31Z

 // is not saved in OVSDB yet before the real port is created, so it might introduce an issue for the same return value
 // if it is called multiple times before OVS port creation.
-func (i *Initializer) getFreeOFPort(startPort int) (int32, error) {
+func (i *Initializer) getFreeOFPort(startPort int, reservedPorts []int32) (int32, error) {


nit: getFreeOFPort(startPort int, reservedPorts ...int32) also makes sense in this case and can avoid updating callers which don't need to set reservedPorts.

To make it simpler, should we maintain the allocated port numbers in OVSBridgeClient (so no need to have the callers pass the reservedPorts)?

Accepted Quan's comment. We can discuss here #3938 (comment) for reservedPorts cache.

jianjuns · 2022-07-05T23:51:44Z

 // is not saved in OVSDB yet before the real port is created, so it might introduce an issue for the same return value
 // if it is called multiple times before OVS port creation.
-func (i *Initializer) getFreeOFPort(startPort int) (int32, error) {
+func (i *Initializer) getFreeOFPort(startPort int, reservedPorts []int32) (int32, error) {


This func should be moved OVSBridgeClient?

To make it simpler, should we maintain the allocated port numbers in OVSBridgeClient (so no need to have the callers pass the reservedPorts)?

@wenyingd Any idea about this?
Current implementation is more flexible, and Jianjun's suggestion is safer.

jianjuns · 2022-07-05T23:56:19Z

-		freePort, err := i.getFreeOFPort(config.UplinkOFPort)
+		uplinkNetConfig.OFPort = uint32(freePort)
+		klog.InfoS("Set OpenFlow port in UplinkNetConfig", "ofport", uplinkNetConfig.OFPort)
+		freePort, err = i.getFreeOFPort(config.UplinkOFPort, []int32{freePort})


Guess the problem is we may potentially have multiple uplinks. @wenyingd

But why we must allocate the port in advance, rather than let CreatePort() return a OFPort?

jianjuns · 2022-07-05T23:59:52Z

 // is not saved in OVSDB yet before the real port is created, so it might introduce an issue for the same return value
 // if it is called multiple times before OVS port creation.
-func (i *Initializer) getFreeOFPort(startPort int) (int32, error) {
+func (i *Initializer) getFreeOFPort(startPort int, reservedPorts []int32) (int32, error) {


To make it simpler, should we maintain the allocated port numbers in OVSBridgeClient (so no need to have the callers pass the reservedPorts)?

wenyingd · 2022-07-06T02:14:22Z

Guess the problem is we may potentially have multiple uplinks. @wenyingd

But why we must allocate the port in advance, rather than let CreatePort() return a OFPort?

The workflow that allocating an OF port first then consuming it but not creating OVS port directlly is not about multiple uplink support, it is existing only in AntreaIPAM, because we need the expected uplink OF port number in the OpenFlow pipeline init stage to install many flows which is not per Pod perspective, but the uplink interface is attached to OVS after this stage, accurately, it should be performed when the flow-restored flag is set as true.

For VM support or Windows cases, we don't have this additional requirement.

jianjuns · 2022-07-06T18:10:05Z

The workflow that allocating an OF port first then consuming it but not creating OVS port directly is not about multiple uplink support

The original question from Quan is whether we can use pre-defined constant OFPort for the host interface. And I guess one reason we do not do that is because we may support multiple uplinks later?

it is existing only in AntreaIPAM, because we need the expected uplink OF port number in the OpenFlow pipeline init stage to install many flows which is not per Pod perspective, but the uplink interface is attached to OVS after this stage, accurately, it should be performed when the flow-restored flag is set as true.

I did not remember the code well, but can we change the order to create the host port first? @wenyingd @gran-vmv

gran-vmv · 2022-07-07T04:01:29Z

The workflow that allocating an OF port first then consuming it but not creating OVS port directly is not about multiple uplink support

The original question from Quan is whether we can use pre-defined constant OFPort for the host interface. And I guess one reason we do not do that is because we may support multiple uplinks later?

it is existing only in AntreaIPAM, because we need the expected uplink OF port number in the OpenFlow pipeline init stage to install many flows which is not per Pod perspective, but the uplink interface is attached to OVS after this stage, accurately, it should be performed when the flow-restored flag is set as true.

I did not remember the code well, but can we change the order to create the host port first? @wenyingd @gran-vmv

We can use a pre-defined constant OFPort for the host interface, but this is not best practice since Wenying already added support for dynamic OFPort for HostInterface/Uplink port, and a constant may conflict with existing OFPort. (e.g. User created many regular Pods and then enable FlexibleIPAM)

Previously we create BridgePort (br-int) as HostInterfacePort and it works well. However, since we need to change the BridgePort to an InternalPort with same name as Uplink, we can only create this port after Uplink is renamed.
The process is:

Occupy OFPortID for Uplink/HostInterfacePort
[not-this-PR] Init OVS flows
[not-this-PR] Release flow-restore-wait flag (Important!)
Rename Uplink and create Uplink OVS port (eth0->eth0~, then create eth0~ port)
Create OVS port for HostInterface (create eth0 port on OVS with same MACAddress)
[not-this-PR] Run CNIServer

jianjuns · 2022-07-07T18:04:19Z

Occupy OFPortID for Uplink/HostInterfacePort

[not-this-PR] Init OVS flows

[not-this-PR] Release flow-restore-wait flag (Important!)

Rename Uplink and create Uplink OVS port (eth0->eth0~, then create eth0~ port)

Create OVS port for HostInterface (create eth0 port on OVS with same MACAddress)

[not-this-PR] Run CNIServer

I meant can we change to:

Rename Uplink and create Uplink OVS port (eth0->eth0~, then create eth0~ port)
Create OVS port for HostInterface (create eth0 port on OVS with same MACAddress)
[not-this-PR] Init OVS flows
[not-this-PR] Release flow-restore-wait flag (Important!)
[not-this-PR] Run CNIServer

If we have to reserve OFPorts, I suggest to move the func into OVSBridgeClient, and maintain allocated OFPorts there, to simplify the caller side.

gran-vmv · 2022-07-08T01:27:06Z

I meant can we change to:

Rename Uplink and create Uplink OVS port (eth0->eth0~, then create eth0~ port)

Create OVS port for HostInterface (create eth0 port on OVS with same MACAddress)

[not-this-PR] Init OVS flows

[not-this-PR] Release flow-restore-wait flag (Important!)

[not-this-PR] Run CNIServer

If we have to reserve OFPorts, I suggest to move the func into OVSBridgeClient, and maintain allocated OFPorts there, to simplify the caller side.

We cannot change the sequence since antrea-agent is unable to access K8s apiserver after step 1, but this access is required by some other steps in initialization.
I'll move this func to OVSBridgeClient.

tnqn

two nits, otherwise LGTM

tnqn · 2022-07-08T08:48:23Z

 	return nil, nil, nil, fmt.Errorf("unable to find local IP and device")
 }

+func GetTransportIPNetByName(interfaceName string) ([]*net.IPNet, error) {


I think the qualifier “transport" shouldn't here. The function in pkg/agent/agent.go has it because it's used to fetch transport interface's IPs, but this is a generic util function and its implementation has nothing to do with transport interface.

Changed to GetAllIPNetsByName, since we already have GetIPNetDeviceByName

tnqn · 2022-07-08T10:55:07Z

+// AllocateFreeOFPort returns an OpenFlow port number which is not allocated or used by any existing OVS port. Note
+// that, the returned port is cached locally but not saved in OVSDB yet before the real port is created, so it might
+// introduce an issue for the conflict if it is occupied by others before OVS port creation.
+func (br *OVSBridge) AllocateFreeOFPort(startPort int) (int32, error) {


Perhaps just AllocateOFPort

jianjuns

A nit

jianjuns · 2022-07-09T22:18:37Z

 }

+// AllocateOFPort returns an OpenFlow port number which is not allocated or used by any existing OVS port. Note that,
+// the returned port is cached locally but not saved in OVSDB yet before the real port is created, so it might introduce


port -> port number

… port Add a suffix to uplink when bridging to OVS bridge. Signed-off-by: gran <gran@vmware.com>

tnqn

LGTM

tnqn · 2022-07-11T04:41:27Z

/test-networkpolicy

gran-vmv self-assigned this Jun 24, 2022

gran-vmv added the action/backport Indicates a PR that requires backports. label Jun 24, 2022

gran-vmv added this to the Antrea v1.8 release milestone Jun 24, 2022

gran-vmv requested review from jianjuns, tnqn and wenyingd June 24, 2022 08:27

gran-vmv force-pushed the ipam-dhcp2 branch from a3dfafa to c42467c Compare June 24, 2022 08:42

gran-vmv force-pushed the ipam-dhcp2 branch from c42467c to 0374143 Compare June 24, 2022 09:57

gran-vmv commented Jun 27, 2022

View reviewed changes

gran-vmv force-pushed the ipam-dhcp2 branch from 0374143 to 4b03697 Compare June 28, 2022 06:46

wenyingd reviewed Jun 28, 2022

View reviewed changes

gran-vmv force-pushed the ipam-dhcp2 branch 5 times, most recently from 6c32afe to e5e432a Compare June 30, 2022 01:41

gran-vmv requested a review from wenyingd June 30, 2022 07:27

wenyingd reviewed Jul 5, 2022

View reviewed changes

tnqn reviewed Jul 5, 2022

View reviewed changes

jianjuns reviewed Jul 5, 2022

View reviewed changes

gran-vmv force-pushed the ipam-dhcp2 branch from e5e432a to 50614a5 Compare July 6, 2022 03:03

gran-vmv force-pushed the ipam-dhcp2 branch 2 times, most recently from 896afe6 to 318dfa1 Compare July 8, 2022 02:01

gran-vmv force-pushed the ipam-dhcp2 branch from 318dfa1 to 798a8b3 Compare July 8, 2022 02:11

gran-vmv requested review from jianjuns, tnqn and wenyingd July 8, 2022 03:07

tnqn reviewed Jul 8, 2022

View reviewed changes

gran-vmv force-pushed the ipam-dhcp2 branch 2 times, most recently from c79fc53 to 3f91116 Compare July 8, 2022 11:55

jianjuns reviewed Jul 9, 2022

View reviewed changes

[flexible-ipam] Use uplink interface name for host interface internal…

7584f1d

… port Add a suffix to uplink when bridging to OVS bridge. Signed-off-by: gran <gran@vmware.com>

gran-vmv force-pushed the ipam-dhcp2 branch from 3f91116 to 7584f1d Compare July 11, 2022 01:23

tnqn approved these changes Jul 11, 2022

View reviewed changes

jianjuns merged commit cd4e156 into antrea-io:main Jul 11, 2022

gran-vmv mentioned this pull request Jul 12, 2022

Automated cherry pick of #3938: Use uplink interface name for host interface #3991

Merged

xliuxu added the action/release-note Indicates a PR that should be included in release notes. label Jul 13, 2022

Conversation

gran-vmv commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jun 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wenyingd Jun 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gran-vmv commented Jul 4, 2022

Uh oh!

wenyingd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

gran-vmv commented Jun 24, 2022 •

edited

Loading

codecov-commenter commented Jun 24, 2022 •

edited

Loading

wenyingd Jun 28, 2022 •

edited

Loading