support multiple network on pod using OVN k8s CNI #2775

cathy-zhou · 2022-01-26T17:40:11Z

- What this PR does and why is it needed

- Special notes for reviewers

- How to verify it

- Description for the changelog

coveralls · 2022-01-27T04:38:06Z

Coverage decreased (-1.3%) to 49.4% when pulling ada462b on cathy-zhou:multinetwork into 5efa78f on ovn-org:master.

cathy-zhou · 2022-02-12T05:03:53Z

/retest

s1061123 · 2022-03-15T13:20:28Z

go-controller/pkg/cni/types/types.go

+	MTU int `json:"mtu,omitempty"`
+	// captures net-attach-def name in the form of namespace/name
+	NadName string `json:"netAttachDefName,omitempty"`
+	// set true if it is not the default net-attach-def


"the default net-attach-def" is unclear to me. Is that 'default network interface', other than 'secondary using net-attach-def"? If so, NadName could distinguish between default network and secondary network (i.e. NadName == "" must be secondary).

Signed-off-by: Yun Zhou <[email protected]>

To support multi OVN networks on a single Pod, we'd need to allow setting/deleting Pod annoation per OVN network. Also, ovnkube-master needs to update Pod annotation with RetryOnConflict(), because multi-networks could update the same Pod annotation and introduce conflict. Signed-off-by: Yun Zhou <[email protected]>

To support multi OVN networks on a single Pod, we'd need to allow setting/deleting node subnet annoation per OVN network. Also, ovnkube-master needs to update node annotation with RetryOnConflict(), because multi-networks could update the same node subnet annotation and introduce conflict. Signed-off-by: Yun Zhou <[email protected]>

to support multiple OVN networks on a single Pod, we'd need to set/unset smart-nic annoation on the per network basis. Signed-off-by: Yun Zhou <[email protected]>

allow OVN k8s CNI attaching multiple network interfaces to pods. Signed-off-by: Yun Zhou <[email protected]>

Signed-off-by: Yun Zhou <[email protected]>

maiqueb

First of, thank you for your work in this feature. The PR is huge, but, given the sheer amount of new functionality you're adding, I would have expected it to be a even bigger. Very nice work.

I have left a bunch of opinionated suggestions, but I am also asking some important questions.

Most importantly, I have the following concerns:

scale. I am unsure your solution to indicate to which ovs bridge will be traffic be output to (when using the localnet topology) will scale. Our use cases imply tens (possibly hundreds) of net-attach-defs, meaning the length of the bindings string will be huge. I'm unsure it has a hard limit, but, it will impair performance if its length is too big.
the localnet topology requires IP addresses. We have a use case for IPAM less localnet topologies . We need to sync on this.
absence of multi-net policies without IPAM. I'm aware you don't consider this use case. We would also like to sync on this, since that use case is important for us at Red Hat. We could use your help / input to come up with a simple(st) way to define a way for this to happen .
we also have a use case for pure overlays. IIUC, if we don't use the localnet port, it'll behave this way. Can you confirm that if we don't setup the ovn-bridge-bindings on each node's it will communicate via tunnel ?

maiqueb · 2022-02-11T13:30:24Z

go-controller/pkg/cni/cni.go

+	namespace := pr.PodNamespace
+	podName := pr.PodName
+	if namespace == "" || podName == "" {
+		return nil, fmt.Errorf("required CNI variable missing")
+	}


I see this code duplicated below; would it make sense to export it to an helper function ?

func (pr *PodRequest) validate() error { if pr.PodNamespace == "" || pr.PodName == "" { return fmt.Errorf("required CNI variable missing") } return nil }

maiqueb · 2022-02-11T13:35:57Z

go-controller/pkg/cni/cni.go

+	mtu := pr.CNIConf.MTU
+	if mtu == 0 {
+		mtu = config.Default.MTU
+	}


I would also export this into an helper:

func (pr *PodRequest) mtu() int { // I'm unsure about the return type, but you get the idea ... if pr.CNIConf.MTU == 0 { return config.Default.MTU } return pr.CNIConf.MTU }

This enables you to inline this is the call below:

... podInterfaceInfo, err := PodAnnotation2PodInfo(annotations, useOVSExternalIDs, pr.PodUID, vfNetdevName, pr.effectiveNADName, pr.mtu(), netNameInfo) ...

maiqueb · 2022-02-11T13:36:22Z

go-controller/pkg/cni/cni.go

+	namespace := pr.PodNamespace
+	podName := pr.PodName
+	if namespace == "" || podName == "" {
+		return nil, fmt.Errorf("required CNI variable missing")
+	}


the helper func could be re-used here.

maiqueb · 2022-02-11T13:36:58Z

go-controller/pkg/cni/cni.go

+			condString := "external-ids:sandbox=" + pr.SandboxID
+			if pr.CNIConf.IsSecondary {
+				condString += " external_ids:network_name=" + pr.effectiveNADName
+			} else {
+				condString += " external_ids:network_name{=}[]"
+			}


I would use add yet another helper here, which allows you to simply:

ovsIfNames, err := ovsFind("Interface", "name", pr.condString())

I unfortunately could not come up with a decent name for it ...

maiqueb · 2022-02-11T13:40:58Z

go-controller/pkg/cni/cniserver.go

+	req.effectiveNetName = types.DefaultNetworkName
+	req.effectiveNADName = types.DefaultNetworkName
+	if conf.IsSecondary {
+		req.effectiveNetName = conf.Name
+		if conf.NadName == "" {
+			return nil, fmt.Errorf("OVN Netconf %q doesn't contain network-attachment-definition name", conf.Name)
+		}
+		req.effectiveNADName = conf.NadName
+	}


Would it make sense to add (yet another ...) helper req receiver function here ?

maiqueb · 2022-05-26T11:12:29Z

go-controller/pkg/ovn/ovn.go

+
+	// Find all the logical node switches for the non-default networks and delete the ones that belong to the
+	// obsolete networks
+	nodeSwitches, err := libovsdbops.FindSwitchesWithOtherConfig(mc.nbClient)


are these only the "logical node switches" - i.e. the logical switches for routed topology ? I find the comment a bit misleading - I wouldn't expect to see the localnet topology switches listed here after reading that comment.

I may have grossly misinterpreted your comment though.

maiqueb · 2022-05-26T11:27:17Z

go-controller/pkg/ovn/pods.go

+		err1 := oc.addLogicalPort4Nad(pod, nadName, nodeName, network)
+		if err1 != nil {
+			err = err1
+		}


what if more than one failed ? You'd swallow all errors except the last one. Is that OK ? Shouldn't we at least log the error ?

maiqueb · 2022-05-26T11:35:13Z

go-controller/pkg/ovn/policy.go

+			if portInfo, err := oc.logicalPortCache.get(portName); err != nil {
+				klog.Errorf(err.Error())


could you explain what would this scenario represent - i.e. a port missing from the cache ? I'm having trouble understanding this one.

maiqueb · 2022-05-26T11:41:26Z

go-controller/pkg/util/node_annotations.go

@@ -258,17 +256,17 @@ func SetL3GatewayConfig(nodeAnnotator kube.Annotator, cfg *L3GatewayConfig) erro
 func ParseNodeL3GatewayAnnotation(node *kapi.Node) (*L3GatewayConfig, error) {
 	l3GatewayAnnotation, ok := node.Annotations[ovnNodeL3GatewayConfig]
 	if !ok {
-		return nil, newAnnotationNotSetError("%s annotation not found for node %q", ovnNodeL3GatewayConfig, node.Name)


nit: imho, this read better previously.

maiqueb · 2022-05-26T13:40:30Z

go-controller/pkg/util/util.go

+type NetNameInfo struct {
+	// netconf's name
+	NetName     string
+	Prefix      string
+	IsSecondary bool
+}
+
+type NetAttachDefInfo struct {
+	NetNameInfo
+	// net-attach-defs shared the same CNI Conf, key is <Namespace>/<Name> of net-attach-def.
+	// Note that it means they share the same logical switch (subnet cidr/MTU etc), but they might
+	// have different resource requirement (requires or not require VF, or different VF resource set)
+	NetAttachDefs sync.Map
+	NetCidr       string
+	MTU           int
+	TopoType      string
+	VlanId        int
+	ExcludeIPs    []net.IP
+}


I think these structs should be defined in a dedicated package, instead of a utils file with a bunch of unreleated code.

maiqueb · 2022-06-09T15:01:01Z

go-controller/pkg/cni/types/types.go

+	// Network MTU
+	MTU int `json:"mtu,omitempty"`
+	// set to localnet if it needs public interface
+	TopoType string `json:"topology,omitempty"`


It's very confusing to have this attribute set to localnet when you just want an overlay (and relying on the ovn bridge bindings to have the underlay configured).

I think TopologyType should allow 3 values: routed, overlay, and localnet, for instance.

cathy-zhou force-pushed the multinetwork branch from ab3b549 to 05b0096 Compare January 26, 2022 17:42

cathy-zhou changed the title ~~draft: support multiple network on pod using OVN k8s CNI~~ [WIP] support multiple network on pod using OVN k8s CNI Jan 26, 2022

cathy-zhou force-pushed the multinetwork branch from 05b0096 to 634ea96 Compare January 27, 2022 04:22

cathy-zhou force-pushed the multinetwork branch from 634ea96 to 5b7a9f6 Compare January 27, 2022 19:34

cathy-zhou changed the title ~~[WIP] support multiple network on pod using OVN k8s CNI~~ support multiple network on pod using OVN k8s CNI Jan 27, 2022

cathy-zhou force-pushed the multinetwork branch 3 times, most recently from a752cf6 to f7bfedb Compare February 3, 2022 06:05

cathy-zhou force-pushed the multinetwork branch 5 times, most recently from ada462b to 9b96c2a Compare February 12, 2022 04:53

girishmg mentioned this pull request Feb 15, 2022

Support for endPort in multi-networkpolicy k8snetworkplumbingwg/multi-networkpolicy#14

Closed

s1061123 reviewed Mar 15, 2022

View reviewed changes

cathy-zhou added 8 commits March 30, 2022 12:49

remove OVN acl direction const definition

2845e82

Signed-off-by: Yun Zhou <[email protected]>

Prepare smart-nic annotation for multi-network support

61abe8d

to support multiple OVN networks on a single Pod, we'd need to set/unset smart-nic annoation on the per network basis. Signed-off-by: Yun Zhou <[email protected]>

support multiple networks on pods using OVN k8s CNI

b5f6a1e

allow OVN k8s CNI attaching multiple network interfaces to pods. Signed-off-by: Yun Zhou <[email protected]>

update net-attach-def client vendor package version

4d6d310

Signed-off-by: Yun Zhou <[email protected]>

support multinetwork policy

c56a0f2

Signed-off-by: Yun Zhou <[email protected]>

add localnet topology network support

60c0697

Signed-off-by: Yun Zhou <[email protected]>

cathy-zhou force-pushed the multinetwork branch from 9b96c2a to 60c0697 Compare April 1, 2022 18:34

maiqueb reviewed May 26, 2022

View reviewed changes

maiqueb reviewed Jun 9, 2022

View reviewed changes

cathy-zhou marked this pull request as draft June 29, 2022 20:16

cathy-zhou closed this Dec 9, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support multiple network on pod using OVN k8s CNI #2775

support multiple network on pod using OVN k8s CNI #2775

cathy-zhou commented Jan 26, 2022

coveralls commented Jan 27, 2022 •

edited

Loading

cathy-zhou commented Feb 12, 2022

s1061123 Mar 15, 2022

maiqueb left a comment

maiqueb Feb 11, 2022

maiqueb Feb 11, 2022

maiqueb Feb 11, 2022

maiqueb Feb 11, 2022

maiqueb Feb 11, 2022

maiqueb May 26, 2022

maiqueb May 26, 2022

maiqueb May 26, 2022

maiqueb May 26, 2022

maiqueb May 26, 2022

maiqueb Jun 9, 2022

		if portInfo, err := oc.logicalPortCache.get(portName); err != nil {
		klog.Errorf(err.Error())

support multiple network on pod using OVN k8s CNI #2775

support multiple network on pod using OVN k8s CNI #2775

Conversation

cathy-zhou commented Jan 26, 2022

coveralls commented Jan 27, 2022 • edited Loading

cathy-zhou commented Feb 12, 2022

Choose a reason for hiding this comment

maiqueb 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

coveralls commented Jan 27, 2022 •

edited

Loading