Save secondary network interfaces in InterfaceStore #5998

jianjuns · 2024-02-17T19:24:35Z

Change to using InterfaceStore to store secondary interfaces. This enables more code sharing of OVS interface configuration between primary and secondary interfaces, and faciliates interface configuration persistence and restoration after agent restarts.
VLAN interface restoration will be implemented by a future commit.

Closes: #5278

antoninbas · 2024-02-21T01:33:27Z

pkg/agent/cniserver/pod_configuration.go

+	// Not needed for a secondary network interface.
+	if pc.routeClient != nil {


I understand why we reuse the interface store for secondary interfaces, but do we need to also reuse the same PodConfigurator code, given that it leads to all these conditionals?

If you look at this func, only this block cannot be reused for secondary interface. I think it is just a tradeoff to make. I do not have a strong opinion. Let me know what you prefer.

More generally, I was referring to other PodConfigurator methods as well, in particular func (pc *podConfigurator) connectInterfaceToOVSCommon.

To me it ends up being a bit more confusing that having a dedicated workflow for secondary network interfaces (with a shared interface store of course).

Yes, there are other funcs, which also means there is more code can be shared in this way. I feel it does not look bad to check some interfaces (routeClient, ofClient, podUpdateNotifier) are not nil before calling their funcs. But I can change to duplicating the required code for secondary interfaces if you want that.

I chatted with Jianjun, and I'm fine with either approach, but we fell like we should check with @tnqn in case he has a preference.

I'm fine with the way the code is shared. However, I feel it looks a little obscure to use various nil check to determine the workflow. It's not very straightforward to understand and we need to elaborate instances of podConfigurator to make sure it can work correctly for two cases.

Instead, could we make the code more explicit and perhaps more readable by adding a parameter to the functions, like isSecondaryInterface bool, or calculate it from containerIFDev != "eth0" in the beginning of the functions? Another benefit is that we wouldn't need to create two podConfigurator instances and can remove NewSecondaryInterfaceConfigurator.

I still want a separate podConfigurator as we need a separate OVS bridge client, and I want not to use the primary interface InterfaceStore for secondary interfaces.

I can add a new argument like isSecondaryInterface to the funcs.

I thought over, and now feel better to add an attribute to podConfigurator to indicate it is for secondary network. Will update the code.

pkg/agent/cniserver/pod_configuration.go

tnqn · 2024-02-28T09:59:21Z

pkg/agent/cniserver/pod_configuration.go

+	// Not needed for a secondary network interface.
+	if pc.routeClient != nil {


I'm fine with the way the code is shared. However, I feel it looks a little obscure to use various nil check to determine the workflow. It's not very straightforward to understand and we need to elaborate instances of podConfigurator to make sure it can work correctly for two cases.

Instead, could we make the code more explicit and perhaps more readable by adding a parameter to the functions, like isSecondaryInterface bool, or calculate it from containerIFDev != "eth0" in the beginning of the functions? Another benefit is that we wouldn't need to create two podConfigurator instances and can remove NewSecondaryInterfaceConfigurator.

pkg/agent/cniserver/pod_configuration.go

tnqn · 2024-02-28T10:09:36Z

pkg/agent/cniserver/secondary.go

+	klog.InfoS("Deleted SR-IOV interface", "Pod",
+		klog.KRef(interfaceConfig.PodNamespace, interfaceConfig.PodName),


Perhaps better to have a key-value pair in the same line.

Sure. Fixed.

tnqn

Some nits about format.

pkg/agent/cniserver/pod_configuration.go

tnqn · 2024-03-01T14:14:00Z

pkg/agent/cniserver/pod_configuration_linux.go

+	podName, podNamespace, containerID, netNS string,
+	hostIface, containerIface *current.Interface,
+	ips []*current.IPConfig, vlanID uint16,
+	containerAccess *containerAccessArbitrator) (*interfacestore.InterfaceConfig, error) {


Ditto, the original one argument per line is more common in golang. Normally it's either all parameters in one line for short parameter list or one parameter per line for long parameter list. For example:
https://github.com/golang/go/blob/b5a64ba62eafe5dee13562091ca03aef6cac87b6/src/go/format/internal.go#L94-L101
https://github.com/kubernetes/kubernetes/blob/d51e2da869350b2b20a33d060e17bd1d2165e02d/pkg/kubelet/kubelet.go#L338-L365

In addition to being neat, there will be less diff when adding/updating/deleting the parameters.

I changed to breaking lines by types. Check if that works for you.

tnqn · 2024-03-01T14:19:30Z

pkg/agent/cniserver/pod_configuration_windows.go

+	podName, podNamespace, containerID, netNS string,
+	hostIface, containerIface *current.Interface,
+	ips []*current.IPConfig, vlanID uint16,
+	containerAccess *containerAccessArbitrator) (*interfacestore.InterfaceConfig, error) {


tnqn · 2024-03-01T14:34:55Z

pkg/agent/secondarynetwork/podwatch/controller.go

+	pod *corev1.Pod, network *netdefv1.NetworkSelectionElement,
+	podCNIInfo *podCNIInfo, networkConfig *SecondaryNetworkConfig) error {


antoninbas

some minor comments, otherwise LGTM

antoninbas · 2024-03-01T19:51:10Z

pkg/agent/cniserver/pod_configuration.go

@@ -72,6 +75,8 @@ type podConfigurator struct {
 	// podUpdateNotifier is used for notifying updates of local Pods to other components which may benefit from this
 	// information, i.e. NetworkPolicyController, EgressController.
 	podUpdateNotifier channel.Notifier
+	// SecondaryInterfaceConfigurator


Suggested change

// SecondaryInterfaceConfigurator

// isSecondaryNetwork is true if this instance of podConfigurator is used to configure secondary Pod network interfaces.

antoninbas · 2024-03-01T19:53:16Z

pkg/agent/agent_test.go

-			interfacestore.NewContainerInterface("p1", uuid1, "pod1", "ns1", p1NetMAC, []net.IP{p1NetIP}, 0)))}
+			interfacestore.NewContainerInterface("p1", uuid1, "pod1", "ns1", "eth0",
+				p1NetMAC, []net.IP{p1NetIP}, 0)))}
 	ovsPort2 := ovsconfig.OVSPortData{UUID: uuid2, Name: "p2", IFName: "p2", OFPort: 12,
 		ExternalIDs: convertExternalIDMap(cniserver.BuildOVSPortExternalIDs(
-			interfacestore.NewContainerInterface("p2", uuid2, "pod2", "ns2", p2NetMAC, []net.IP{p2NetIP}, 0)))}
+			interfacestore.NewContainerInterface("p2", uuid2, "pod2", "ns2", "eth0",
+				p2NetMAC, []net.IP{p2NetIP}, 0)))}


nit: based on Quan's earlier comment, these should probably be broken up differently. I usually go with:

ovsPort2 := ovsconfig.OVSPortData{UUID: uuid2, Name: "p2", IFName: "p2", OFPort: 12, ExternalIDs: convertExternalIDMap(cniserver.BuildOVSPortExternalIDs( interfacestore.NewContainerInterface("p2", uuid2, "pod2", "ns2", "eth0", p2NetMAC, []net.IP{p2NetIP}, 0), )), }

antoninbas · 2024-03-01T19:54:08Z

pkg/agent/cniserver/pod_configuration.go

+	klog.InfoS("Configured container interface", "Pod", klog.KRef(podNamespace, podName),
+		"container", containerID, "interface", containerIface.Name, "hostInterface", hostIface.Name)


nit: we also tend to stick to a single line for these

I prefer a new line here, otherwise it is too long.

antoninbas · 2024-03-01T19:54:21Z

pkg/agent/cniserver/pod_configuration.go

+		if err != nil {
+			return fmt.Errorf("failed to get of_port of OVS port %s: %v", ovsPortName, err)
+		}
+		klog.V(2).Infof("Setting up Openflow entries for container %s", containerID)


maybe update to klog.InfoS

antoninbas · 2024-03-01T19:56:20Z

pkg/agent/cniserver/pod_configuration_linux_test.go

+	assert.NoError(t, pc.ConfigureVLANSecondaryInterface(podName, testPodNamespace, containerID, containerNS, "eth2", 1600, ipamResult))
+	assert.Equal(t, 2, ifaceStore.Len())
+	intfConfigs := ifaceStore.GetContainerInterfacesByPod(podName, testPodNamespace)
+	assert.Equal(t, 2, len(intfConfigs))


there is an assert.Len assertion available for this case

antoninbas · 2024-03-01T19:56:41Z

pkg/agent/cniserver/pod_configuration_linux_test.go

+	mockOVSBridgeClient := ovsconfigtest.NewMockOVSBridgeClient(controller)
+	ifaceStore := interfacestore.NewInterfaceStore()
+	pc, err := NewSecondaryInterfaceConfigurator(mockOVSBridgeClient, ifaceStore)
+	require.Nil(t, err, "No error expected in podConfigurator constructor")


require.NoError

antoninbas · 2024-03-01T20:03:00Z

pkg/agent/cniserver/secondary.go

+		klog.ErrorS(err, "Failed to delete container interface link",
+			"Pod", klog.KRef(interfaceConfig.PodNamespace, interfaceConfig.PodName),
+			"interface", interfaceConfig.IFDev)
+		// No retry for interface deletion.


Is my understanding correct that if the Pod is deleted (i.e., the netns is deleted), there can be no leftover veth anyway?

antoninbas · 2024-03-01T20:06:40Z

pkg/agent/cniserver/server_test.go

 			}
+			assert.True(t, existed, fmt.Sprintf("IP %s should exist in the restored InterfaceConfig", ip1.String()))


I don't think you need to call fmt.Sprintf manually. assert.Truef (or even assert.True?) will do the formatting for you

antoninbas · 2024-03-01T20:07:55Z

pkg/agent/cniserver/server_test.go

+	if !existed || parsedIFDev != "eth1" {
+		t.Errorf("Failed to store interface name to external IDs")
+	}


why not stick to assert.True(t, existed && parsedIFDev == "eth1")?

antoninbas · 2024-03-01T20:08:49Z

pkg/agent/interfacestore/interface_cache_test.go

+	for _, containerInterface := range []*InterfaceConfig{containerInterface1, containerInterface2} {
+		interfaceKey := util.GenerateContainerInterfaceKey(containerInterface.ContainerID, containerInterface.IFDev)
+		storedIface, exists := store.GetInterface(interfaceKey)
+		assert.True(t, exists)


feels like this would be better as a require.True

Change to using InterfaceStore to store secondary interfaces. This enables more code sharing of OVS interface configuration between primary and secondary interfaces, and faciliates interface configuration persistence and restoration after agent restarts. VLAN interface restoration will be implemented by a future commit. Signed-off-by: Jianjun Shen <[email protected]>

antoninbas

LGTM

tnqn

LGTM

jianjuns · 2024-03-05T16:33:47Z

/test-all

jianjuns added area/interface Issues or PRs related to network interfaces area/secondary-network Issues or PRs related to support for secondary networks in Antrea labels Feb 17, 2024

jianjuns force-pushed the secnet-store branch from 49655e7 to 9cfe293 Compare February 17, 2024 20:54

jianjuns requested a review from antoninbas February 17, 2024 23:31

jianjuns marked this pull request as ready for review February 17, 2024 23:31

jianjuns requested a review from arunvelayutham February 17, 2024 23:32

antoninbas reviewed Feb 21, 2024

View reviewed changes

tnqn reviewed Feb 28, 2024

View reviewed changes

jianjuns force-pushed the secnet-store branch 4 times, most recently from 5165589 to 191f26f Compare March 1, 2024 06:48

tnqn reviewed Mar 1, 2024

View reviewed changes

jianjuns force-pushed the secnet-store branch from 191f26f to 595033b Compare March 1, 2024 19:29

antoninbas reviewed Mar 1, 2024

View reviewed changes

jianjuns force-pushed the secnet-store branch from 595033b to 0471d44 Compare March 1, 2024 23:01

antoninbas approved these changes Mar 4, 2024

View reviewed changes

tnqn approved these changes Mar 5, 2024

View reviewed changes

arunvelayutham approved these changes Mar 5, 2024

View reviewed changes

jianjuns merged commit 55d46ec into antrea-io:main Mar 5, 2024
50 of 55 checks passed

		// Not needed for a secondary network interface.
		if pc.routeClient != nil {

		klog.InfoS("Deleted SR-IOV interface", "Pod",
		klog.KRef(interfaceConfig.PodNamespace, interfaceConfig.PodName),

		pod corev1.Pod, network netdefv1.NetworkSelectionElement,
		podCNIInfo podCNIInfo, networkConfig SecondaryNetworkConfig) error {

	// SecondaryInterfaceConfigurator
	// isSecondaryNetwork is true if this instance of podConfigurator is used to configure secondary Pod network interfaces.

		klog.InfoS("Configured container interface", "Pod", klog.KRef(podNamespace, podName),
		"container", containerID, "interface", containerIface.Name, "hostInterface", hostIface.Name)

		}
		assert.True(t, existed, fmt.Sprintf("IP %s should exist in the restored InterfaceConfig", ip1.String()))

Save secondary network interfaces in InterfaceStore #5998

Save secondary network interfaces in InterfaceStore #5998

Conversation

jianjuns commented Feb 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jianjuns Feb 22, 2024 • 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

tnqn 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

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

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

tnqn left a comment

Choose a reason for hiding this comment

jianjuns commented Mar 5, 2024

jianjuns commented Feb 17, 2024 •

edited

Loading

jianjuns Feb 22, 2024 •

edited

Loading