Support Pod secondary interfaces on VLAN network #5365

jianjuns · 2023-08-08T00:13:50Z

This commit adds support for connecting Pod secondary interfaces to a VLAN network. A Pod secondary interface configured with the VLAN networkType will be connected to the secondary network OVS bridge, and the specified VLAN tag will be set on the OVS port.
This commit mostly follows the existing SR-IOV secondary network implementation, in which a Pod controller configures Pod secondary interfaces based on the local Pod events. It thus inherits the existing limitations, like secondary interface configuration is not persisted on the Node and may get lost after antrea-agent restarts.

Reorganize some files:
Renamed pkg/agent/cniserver/sriov.go to secondary.go, and put secondary interface configuration funcs there.
Move SR-IOV interface related code in pkg/agent/secondarynetwork/podwatch/controller.go to sriov.go.

Issue: #5278

antoninbas · 2023-08-10T20:38:54Z

pkg/agent/cniserver/interface_configuration_linux.go

+	// Include the inner veth interface name in the name generation, as one Pod can have more
+	// than one interfaces inc. secondary interfaces, while the outer interface name must be
+	// be unique.
+	hostIfaceName := util.GenerateContainerOuterVethName(podName, podNamespace, containerID, containerIfaceName)


My understanding is that "inner veth" is usually referred to as containerVeth and the "outer veth" referred to as hostVeth. The CNI project seems to be using this terminology (https://github.com/containernetworking/plugins/blob/v1.3.0/pkg/ip/link_linux.go#L139).

Changed to containerVeth and hostVeth.

pkg/agent/cniserver/pod_configuration_linux_test.go

pkg/agent/cniserver/pod_configuration.go

antoninbas · 2023-08-10T20:49:20Z

pkg/agent/cniserver/secondary.go

+	}
+	hostIface := result.Interfaces[0]
+	containerIface := result.Interfaces[1]
+	klog.InfoS("Configured SR-IOV interface", "Pod", podName, "Namespace", podNamespace, "interface", containerInterfaceName, "hostInterface", hostIface)


use klog.KRef (or klog.KObj when appropriate) to log references to K8s objects:

klog.InfoS("Configured SR-IOV interface", "Pod", klog.KRef(podNamespace, podName), "interface", containerInterfaceName, "hostInterface", hostIface)

antoninbas · 2023-08-10T20:57:07Z

pkg/agent/secondarynetwork/podwatch/types.go

 const (
 	sriovNetworkType = "sriov"
+	vlanNetworkType  = "vlan"
 )


should we create a dedicated type for this?

type NetworkType string const ( sriovNetworkType NetworkType = "sriov" vlanNetworkType NetworkType = "vlan" )

Added a type.

antoninbas · 2023-08-10T21:04:22Z

pkg/agent/secondarynetwork/podwatch/types.go

@@ -38,5 +39,7 @@ type SecondaryNetworkConfig struct {
 	Type string `json:"type,omitempty"`
 	// Set networkType to "sriov"
 	NetworkType string     `json:"networkType,omitempty"`
+	MTU         int        `json:"mtu, omitempty"`
+	VLAN        uint16     `json:"vlan, omitempty"`


probably better to use int32 in a public API, especially since the max value is 4096 and you have to perform validation anyway
even though this is not a K8s API: https://github.com/zecke/Kubernetes/blob/master/docs/devel/api-conventions.md#primitive-types

Changed to int32

antoninbas · 2023-08-10T21:04:30Z

pkg/agent/secondarynetwork/podwatch/types.go

@@ -38,5 +39,7 @@ type SecondaryNetworkConfig struct {
 	Type string `json:"type,omitempty"`
 	// Set networkType to "sriov"
 	NetworkType string     `json:"networkType,omitempty"`
+	MTU         int        `json:"mtu, omitempty"`


s/int/int32

pkg/agent/secondarynetwork/podwatch/controller.go

antoninbas · 2023-08-10T21:15:53Z

pkg/agent/secondarynetwork/podwatch/controller_test.go

+	if cniType != "" {
+		configStr = strings.Replace(configStr, "type\": \"antrea", "type\": \""+cniType, 1)
+	}
+	configStr = strings.Replace(configStr, "mtu\": 1500", "mtu\": "+strconv.Itoa(mtu), 1)
+	configStr = strings.Replace(configStr, "vlan\": 100", "vlan\": "+strconv.Itoa(int(vlan)), 1)


IMO, you should use the text/template package to do this more cleanly

Changed to text/template. Thanks for the suggestion.

antoninbas · 2023-08-10T21:17:28Z

pkg/agent/secondarynetwork/podwatch/controller_test.go

+				CNIConfig:   config2,
+			},
+		}
+		assert.True(t, reflect.DeepEqual(&savedCNIConfig, infos[0]))


I thought assert.Equal already used reflect.DeepEqual to check for comparison?

https://github.com/stretchr/testify/blob/f97607b89807936ac4ff96748d766cf4b9711f78/assert/assertions.go#L58

You are right. Changed to use assert.Equal.

jianjuns · 2023-08-09T23:36:14Z

pkg/agent/cniserver/secondary.go

+}
+
+// ConfigureSriovSecondaryInterface configures a SR-IOV secondary interface for a Pod.
+func (pc *podConfigurator) ConfigureSriovSecondaryInterface(


This func is copied from the original cniserver/sriov.go without changes.

jianjuns · 2023-08-09T23:37:37Z

pkg/agent/secondarynetwork/podwatch/sriov.go

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package podwatch


All funcs in this file are moved from podwatch/controller.go without changes.

jianjuns · 2023-08-11T00:06:44Z

pkg/agent/cniserver/interface_configuration_linux.go

+	// Include the inner veth interface name in the name generation, as one Pod can have more
+	// than one interfaces inc. secondary interfaces, while the outer interface name must be
+	// be unique.
+	hostIfaceName := util.GenerateContainerOuterVethName(podName, podNamespace, containerID, containerIfaceName)


Changed to containerVeth and hostVeth.

pkg/agent/cniserver/pod_configuration_linux_test.go

pkg/agent/cniserver/pod_configuration.go

jianjuns · 2023-08-11T00:08:35Z

pkg/agent/secondarynetwork/podwatch/types.go

@@ -38,5 +39,7 @@ type SecondaryNetworkConfig struct {
 	Type string `json:"type,omitempty"`
 	// Set networkType to "sriov"
 	NetworkType string     `json:"networkType,omitempty"`
+	MTU         int        `json:"mtu, omitempty"`
+	VLAN        uint16     `json:"vlan, omitempty"`


Changed to int32

jianjuns · 2023-08-11T00:08:39Z

pkg/agent/secondarynetwork/podwatch/types.go

@@ -38,5 +39,7 @@ type SecondaryNetworkConfig struct {
 	Type string `json:"type,omitempty"`
 	// Set networkType to "sriov"
 	NetworkType string     `json:"networkType,omitempty"`
+	MTU         int        `json:"mtu, omitempty"`


pkg/agent/secondarynetwork/podwatch/controller.go

jianjuns · 2023-08-11T00:09:44Z

pkg/agent/secondarynetwork/podwatch/controller_test.go

+	if cniType != "" {
+		configStr = strings.Replace(configStr, "type\": \"antrea", "type\": \""+cniType, 1)
+	}
+	configStr = strings.Replace(configStr, "mtu\": 1500", "mtu\": "+strconv.Itoa(mtu), 1)
+	configStr = strings.Replace(configStr, "vlan\": 100", "vlan\": "+strconv.Itoa(int(vlan)), 1)


Changed to text/template. Thanks for the suggestion.

jianjuns · 2023-08-11T00:10:07Z

pkg/agent/secondarynetwork/podwatch/controller_test.go

+				CNIConfig:   config2,
+			},
+		}
+		assert.True(t, reflect.DeepEqual(&savedCNIConfig, infos[0]))


You are right. Changed to use assert.Equal.

antoninbas

only a couple of nits, otherwise LGTM

antoninbas · 2023-08-17T00:11:57Z

pkg/agent/util/net.go

 // The probability of collision should be neglectable.
 func GenerateContainerInterfaceName(podName, podNamespace, containerID string) string {
 	// Use the podName as the prefix and the containerID as the hashing key.
 	// podNamespace is not used currently.
 	return generateInterfaceName(containerID, podName, true)
 }

+// GenerateContainerOuterVethName generates a unique interface name using the


s/GenerateContainerOuterVethName/GenerateContainerHostVethName

antoninbas · 2023-08-17T00:13:11Z

pkg/agent/util/net_test.go

+	assert.True(t, len(ifaceName0) <= interfaceNameLength)
+	assert.True(t, strings.HasPrefix(ifaceName0, podName0+"-"))


use require as you typically want all the subtests below to be skipped if one of these assertions is false

antoninbas · 2023-08-17T00:14:10Z

pkg/agent/util/net_test.go

+	containerID0 := "container0"
+	eth0 := "eth0"
+	ifaceName0 := GenerateContainerHostVethName(podName0, podNamespace0, containerID0, eth0)
+	assert.True(t, len(ifaceName0) <= interfaceNameLength)


require.Less

pkg/agent/secondarynetwork/podwatch/controller.go

antoninbas · 2023-08-17T00:17:42Z

pkg/agent/secondarynetwork/podwatch/controller.go

 			continue
 		}
+		if networkConfig.NetworkType == vlanNetworkType {
+			if networkConfig.VLAN > vlanIDMax || networkConfig.VLAN < 0 {
+				klog.ErrorS(nil, "Invalid VLAN ID", "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KObj(pod))


nit: maybe add the VLAN value to the log

antoninbas · 2023-08-17T00:17:58Z

pkg/agent/secondarynetwork/podwatch/controller.go

+			}
+		}
+		if networkConfig.MTU < 0 {
+			klog.ErrorS(nil, "Invalid MTU", "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KObj(pod))


same as above, but for MTU

antoninbas · 2023-08-17T00:20:55Z

pkg/agent/secondarynetwork/podwatch/controller_test.go

-		network2 := testNetwork("net2")
+		savedCNIConfig := *cniConfig
+		network1 := testNetwork("net1", sriovNetworkType)
+		network2 := testNetworkExt("net2", "", vlanNetworkType, defaultMTU, 100)


nit: define a local constant for the vlan id

This commit adds support for connecting Pod secondary interfaces to a VLAN network. A Pod secondary interface configured with the VLAN networkType will be connected to the secondary network OVS bridge, and the specified VLAN tag will be set on the OVS port. This commit mostly follows the existing SR-IOV secondary network implementation, in which a Pod controller configures Pod secondary interfaces based on the local Pod events. It thus inherits the existing limitations, like secondary interface configuration is not persisted on the Node and may get lost after antrea-agent restarts. Signed-off-by: Jianjun Shen <shenj@vmware.com>

antoninbas

LGTM

jianjuns · 2023-08-17T00:58:26Z

/test-all

arunvelayutham

changes LGTM. thank you @jianjuns

jianjuns added area/component/cni Issues or PRs related to the cni component area/interface Issues or PRs related to network interfaces area/ovs Issues or PRs related to OVS labels Aug 8, 2023

jianjuns force-pushed the sec-net-vlan branch from 60b47c6 to 976cf2e Compare August 9, 2023 23:31

jianjuns marked this pull request as ready for review August 9, 2023 23:32

jianjuns requested review from arunvelayutham and antoninbas August 9, 2023 23:43

antoninbas reviewed Aug 10, 2023

View reviewed changes

jianjuns force-pushed the sec-net-vlan branch 2 times, most recently from fb2bd0c to c5f19c5 Compare August 11, 2023 00:12

jianjuns commented Aug 11, 2023

View reviewed changes

jianjuns force-pushed the sec-net-vlan branch 3 times, most recently from 5db5adf to 39a71fc Compare August 12, 2023 23:16

jianjuns requested a review from antoninbas August 15, 2023 23:47

antoninbas reviewed Aug 17, 2023

View reviewed changes

jianjuns force-pushed the sec-net-vlan branch from 39a71fc to 09a0599 Compare August 17, 2023 00:50

antoninbas approved these changes Aug 17, 2023

View reviewed changes

arunvelayutham approved these changes Aug 22, 2023

View reviewed changes

jianjuns merged commit 391666f into antrea-io:main Aug 22, 2023

		assert.True(t, len(ifaceName0) <= interfaceNameLength)
		assert.True(t, strings.HasPrefix(ifaceName0, podName0+"-"))

Support Pod secondary interfaces on VLAN network #5365

Support Pod secondary interfaces on VLAN network #5365

Conversation

jianjuns commented Aug 8, 2023 • edited Loading

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

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

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

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

Choose a reason for hiding this comment

antoninbas left a 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

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

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

antoninbas left a comment

Choose a reason for hiding this comment

jianjuns commented Aug 17, 2023

arunvelayutham left a comment

Choose a reason for hiding this comment

jianjuns commented Aug 8, 2023 •

edited

Loading