From ff15b16a28f325a0bce6e6cb3f8a330b61508969 Mon Sep 17 00:00:00 2001 From: Antonin Bas Date: Thu, 1 Sep 2022 09:51:16 -0700 Subject: [PATCH] Add more unit tests for secondarynetwork feature (#4180) Fix one of the tests (`TestPodControllerAddPod/missing_Status.PodIPs`) and add a few more unit tests. For #4142 Signed-off-by: Antonin Bas --- .../secondarynetwork/podwatch/controller.go | 26 ++-- .../podwatch/controller_test.go | 111 +++++++++++++++++- 2 files changed, 119 insertions(+), 18 deletions(-) diff --git a/pkg/agent/secondarynetwork/podwatch/controller.go b/pkg/agent/secondarynetwork/podwatch/controller.go index 952e2624429..0ac92fc1f70 100644 --- a/pkg/agent/secondarynetwork/podwatch/controller.go +++ b/pkg/agent/secondarynetwork/podwatch/controller.go @@ -18,13 +18,11 @@ import ( "context" "encoding/json" "fmt" - "strconv" "strings" "time" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/util/rand" "k8s.io/apimachinery/pkg/util/wait" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" @@ -54,8 +52,7 @@ const ( cniPath = "/opt/cni/bin/" defaultSecondaryInterfaceName = "eth1" startIfaceIndex = 1 - endIfaceIndex = 11 - maxRandIndex = 25 + endIfaceIndex = 101 ) // Set resyncPeriod to 0 to disable resyncing. @@ -117,23 +114,21 @@ func podKeyGet(pod *corev1.Pod) string { return pod.Namespace + "/" + pod.Name } -func generatePodSecondaryIfaceName(podCNIInfo *cnipodcache.CNIConfigInfo) string { +func generatePodSecondaryIfaceName(podCNIInfo *cnipodcache.CNIConfigInfo) (string, error) { // Assign default interface name, if podCNIInfo.NetworkConfig is empty. if count := len(podCNIInfo.NetworkConfig); count == 0 { - return defaultSecondaryInterfaceName + return defaultSecondaryInterfaceName, nil } else { - // Generate new interface name (eth1,eth2..eth10) and return to caller. + // Generate new interface name (eth1,eth2..eth100) and return to caller. for ifaceIndex := startIfaceIndex; ifaceIndex < endIfaceIndex; ifaceIndex++ { ifName := fmt.Sprintf("%s%d", "eth", ifaceIndex) _, exist := podCNIInfo.NetworkConfig[ifName] if !exist { - return ifName + return ifName, nil } } } - // Generates random interface name and return. Above execution will try to ensure allocating interface names in order. - // If none available between eth1 to eth10 (already used per Pod), generate random name with the integer range. - return string("eth") + strconv.Itoa(rand.IntnRange(endIfaceIndex, maxRandIndex)) + return "", fmt.Errorf("no more interface names") } func whereaboutsArgsBuilder(cmd string, interfaceName string, podCNIInfo *cnipodcache.CNIConfigInfo) *invoke.Args { @@ -309,7 +304,12 @@ func (pc *PodController) configureSriovAsSecondaryInterface(pod *corev1.Pod, net func (pc *PodController) configureSecondaryInterface(pod *corev1.Pod, network *netdefv1.NetworkSelectionElement, podCNIInfo *cnipodcache.CNIConfigInfo, cniConfig []byte) error { // Generate and assign new interface name, If secondary interface name was not provided in Pod annotation. if len(network.InterfaceRequest) == 0 { - network.InterfaceRequest = generatePodSecondaryIfaceName(podCNIInfo) + var err error + if network.InterfaceRequest, err = generatePodSecondaryIfaceName(podCNIInfo); err != nil { + klog.ErrorS(err, "Cannot generate interface name", "Pod", klog.KObj(pod)) + // do not return error: no need to requeue + return nil + } } // PluginArgs added to provide additional arguments required for whereabouts v0.5.1 and above. cmdArgs := whereaboutsArgsBuilder("ADD", network.InterfaceRequest, podCNIInfo) @@ -366,7 +366,7 @@ func (pc *PodController) configureSecondaryNetwork(pod *corev1.Pod, networklist if networkConfig.NetworkType != sriovNetworkType { // same as above, if updated, we will not process the request again. klog.ErrorS(err, "NetworkType not supported for Pod", "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KObj(pod)) - return nil + continue } // secondary network information retrieved from API server. Proceed to configure secondary interface now. if err = pc.configureSecondaryInterface(pod, network, podCNIInfo, cniConfig); err != nil { diff --git a/pkg/agent/secondarynetwork/podwatch/controller_test.go b/pkg/agent/secondarynetwork/podwatch/controller_test.go index 8fae7ddf3d8..416b7230923 100644 --- a/pkg/agent/secondarynetwork/podwatch/controller_test.go +++ b/pkg/agent/secondarynetwork/podwatch/controller_test.go @@ -275,11 +275,11 @@ func TestPodControllerAddPod(t *testing.T) { podIP, netdefv1.NetworkSelectionElement{ Name: "net1", - InterfaceRequest: "eth1", + InterfaceRequest: "eth10", }, netdefv1.NetworkSelectionElement{ Name: "net2", - InterfaceRequest: "eth2", + InterfaceRequest: "eth11", }, ) network1 := testNetwork("net1") @@ -290,7 +290,7 @@ func TestPodControllerAddPod(t *testing.T) { testNamespace, containerID, containerNetNs(containerID), - "eth1", + "eth10", defaultMTU, gomock.Any(), gomock.Any(), @@ -300,7 +300,7 @@ func TestPodControllerAddPod(t *testing.T) { testNamespace, containerID, containerNetNs(containerID), - "eth2", + "eth11", defaultMTU, gomock.Any(), gomock.Any(), @@ -351,7 +351,7 @@ func TestPodControllerAddPod(t *testing.T) { defer ctrl.Finish() podController, _, _ := newPodController(ctrl) - pod, cniConfig := testPod(podName, containerID, podIP) + pod, cniConfig := testPod(podName, containerID, "") network := testNetwork(networkName) podController.podCache.AddCNIConfigInfo(cniConfig) @@ -403,4 +403,105 @@ func TestPodControllerAddPod(t *testing.T) { require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") assert.NoError(t, podController.handleAddUpdatePod(pod)) }) + + t.Run("no interface name", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + podController, mockIPAM, interfaceConfigurator := newPodController(ctrl) + + pod, cniConfig := testPod( + podName, + containerID, + podIP, + netdefv1.NetworkSelectionElement{ + Name: networkName, + InterfaceRequest: "", + }, + netdefv1.NetworkSelectionElement{ + Name: networkName, + InterfaceRequest: "", + }, + ) + network := testNetwork(networkName) + + interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( + podName, + testNamespace, + containerID, + containerNetNs(containerID), + "eth1", + defaultMTU, + gomock.Any(), + gomock.Any(), + ) + interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( + podName, + testNamespace, + containerID, + containerNetNs(containerID), + "eth2", + defaultMTU, + gomock.Any(), + gomock.Any(), + ) + + mockIPAM.EXPECT().GetIPAMSubnetAddress(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().GetIPAMSubnetAddress(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.101/24"), nil) + + podController.podCache.AddCNIConfigInfo(cniConfig) + _, err := podController.kubeClient.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test Pod") + _, err = podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), network, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") + assert.NoError(t, podController.handleAddUpdatePod(pod)) + }) + + t.Run("error when creating interface", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + podController, mockIPAM, interfaceConfigurator := newPodController(ctrl) + + network := testNetwork(networkName) + + interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( + podName, + testNamespace, + containerID, + containerNetNs(containerID), + interfaceName, + defaultMTU, + gomock.Any(), + gomock.Any(), + ).Return(fmt.Errorf("error when creating interface")) + + mockIPAM.EXPECT().GetIPAMSubnetAddress(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().DelIPAMSubnetAddress(gomock.Any(), gomock.Any()) + + podController.podCache.AddCNIConfigInfo(cniConfig) + _, err := podController.kubeClient.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test Pod") + _, err = podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), network, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") + assert.Error(t, podController.handleAddUpdatePod(pod)) + }) + + t.Run("invalid networks annotation", func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + podController, _, _ := newPodController(ctrl) + + pod, cniConfig := testPod(podName, containerID, podIP) + pod.Annotations = map[string]string{ + networkAttachDefAnnotationKey: "", + } + network := testNetwork(networkName) + + podController.podCache.AddCNIConfigInfo(cniConfig) + _, err := podController.kubeClient.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test Pod") + _, err = podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), network, metav1.CreateOptions{}) + require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") + // we don't expect an error here, no requeueing + assert.NoError(t, podController.handleAddUpdatePod(pod)) + }) }