From 595033bd535a699ae73d713bc3f5ba4859a4cf3a Mon Sep 17 00:00:00 2001 From: Jianjun Shen Date: Tue, 30 Jan 2024 18:46:35 -0500 Subject: [PATCH] Save secondary network interfaces in InterfaceStore 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 --- pkg/agent/agent_test.go | 6 +- pkg/agent/cniserver/pod_configuration.go | 116 ++-- .../cniserver/pod_configuration_linux.go | 13 +- .../cniserver/pod_configuration_linux_test.go | 197 ++++++- .../cniserver/pod_configuration_windows.go | 13 +- pkg/agent/cniserver/secondary.go | 78 ++- pkg/agent/cniserver/server_linux_test.go | 11 +- pkg/agent/cniserver/server_test.go | 83 ++- pkg/agent/cniserver/server_windows_test.go | 4 +- pkg/agent/interfacestore/interface_cache.go | 5 +- .../interfacestore/interface_cache_test.go | 57 +- pkg/agent/interfacestore/types.go | 6 +- .../secondarynetwork/cnipodcache/cache.go | 109 ---- .../secondarynetwork/cnipodcache/types.go | 44 -- .../secondarynetwork/podwatch/controller.go | 366 ++++++------ .../podwatch/controller_test.go | 557 ++++++++++-------- pkg/agent/secondarynetwork/podwatch/sriov.go | 51 +- .../podwatch/testing/mock_podwatch.go | 38 +- pkg/agent/secondarynetwork/podwatch/types.go | 11 +- pkg/agent/util/net.go | 6 +- 20 files changed, 1000 insertions(+), 771 deletions(-) delete mode 100644 pkg/agent/secondarynetwork/cnipodcache/cache.go delete mode 100644 pkg/agent/secondarynetwork/cnipodcache/types.go diff --git a/pkg/agent/agent_test.go b/pkg/agent/agent_test.go index 11c449aee54..930954c6554 100644 --- a/pkg/agent/agent_test.go +++ b/pkg/agent/agent_test.go @@ -91,10 +91,12 @@ func TestInitstore(t *testing.T) { ovsPort1 := ovsconfig.OVSPortData{UUID: uuid1, Name: "p1", IFName: "p1", OFPort: 11, ExternalIDs: convertExternalIDMap(cniserver.BuildOVSPortExternalIDs( - 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)))} initOVSPorts := []ovsconfig.OVSPortData{ovsPort1, ovsPort2} mockOVSBridgeClient.EXPECT().GetPortList().Return(initOVSPorts, ovsconfig.NewTransactionError(fmt.Errorf("Failed to list OVS ports"), true)) diff --git a/pkg/agent/cniserver/pod_configuration.go b/pkg/agent/cniserver/pod_configuration.go index 0009021e0e7..41e068edb3e 100644 --- a/pkg/agent/cniserver/pod_configuration.go +++ b/pkg/agent/cniserver/pod_configuration.go @@ -51,11 +51,14 @@ const ( ovsExternalIDContainerID = "container-id" ovsExternalIDPodName = "pod-name" ovsExternalIDPodNamespace = "pod-namespace" + ovsExternalIDIFDev = "if-dev" ) const ( defaultOVSInterfaceType int = iota //nolint suppress deadcode check for windows internalOVSInterfaceType + + defaultIFDevName = "eth0" ) var ( @@ -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 + isSecondaryNetwork bool } func newPodConfigurator( @@ -116,10 +121,8 @@ func buildContainerConfig( containerIface *current.Interface, ips []*current.IPConfig, vlanID uint16) *interfacestore.InterfaceConfig { - containerIPs, err := parseContainerIPs(ips) - if err != nil { - klog.Errorf("Failed to find container %s IP", containerID) - } + // A secondary interface can be created without IPs. Ignore the IP parsing error here. + containerIPs, _ := parseContainerIPs(ips) // containerIface.Mac should be a valid MAC string, otherwise it should throw error before containerMAC, _ := net.ParseMAC(containerIface.Mac) return interfacestore.NewContainerInterface( @@ -127,6 +130,7 @@ func buildContainerConfig( containerID, podName, podNamespace, + containerIface.Name, containerMAC, containerIPs, vlanID) @@ -141,6 +145,10 @@ func BuildOVSPortExternalIDs(containerConfig *interfacestore.InterfaceConfig) ma externalIDs[ovsExternalIDIP] = getContainerIPsString(containerConfig.IPs) externalIDs[ovsExternalIDPodName] = containerConfig.PodName externalIDs[ovsExternalIDPodNamespace] = containerConfig.PodNamespace + if containerConfig.IFDev != defaultIFDevName { + // Save interface name for a secondary interface. + externalIDs[ovsExternalIDIFDev] = containerConfig.IFDev + } externalIDs[interfacestore.AntreaInterfaceTypeKey] = interfacestore.AntreaContainer return externalIDs } @@ -168,10 +176,14 @@ func ParseOVSPortInterfaceConfig(portData *ovsconfig.OVSPortData, portConfig *in klog.V(2).Infof("OVS port %s has no %s in external_ids", portData.Name, ovsExternalIDContainerID) return nil } - containerIPStrs := strings.Split(portData.ExternalIDs[ovsExternalIDIP], ",") + var containerIPs []net.IP - for _, ipStr := range containerIPStrs { - containerIPs = append(containerIPs, net.ParseIP(ipStr)) + // A secondary interface may not have an IP assigned. + if portData.ExternalIDs[ovsExternalIDIP] != "" { + containerIPStrs := strings.Split(portData.ExternalIDs[ovsExternalIDIP], ",") + for _, ipStr := range containerIPStrs { + containerIPs = append(containerIPs, net.ParseIP(ipStr)) + } } containerMAC, err := net.ParseMAC(portData.ExternalIDs[ovsExternalIDMAC]) @@ -181,12 +193,14 @@ func ParseOVSPortInterfaceConfig(portData *ovsconfig.OVSPortData, portConfig *in } podName, _ := portData.ExternalIDs[ovsExternalIDPodName] podNamespace, _ := portData.ExternalIDs[ovsExternalIDPodNamespace] + ifDev, _ := portData.ExternalIDs[ovsExternalIDIFDev] interfaceConfig := interfacestore.NewContainerInterface( portData.Name, containerID, podName, podNamespace, + ifDev, containerMAC, containerIPs, portData.VLANID) @@ -226,8 +240,11 @@ func (pc *podConfigurator) configureInterfacesCommon( } }() - if err := pc.routeClient.AddLocalAntreaFlexibleIPAMPodRule(containerConfig.IPs); err != nil { - return err + // Not needed for a secondary network interface. + if !pc.isSecondaryNetwork { + if err := pc.routeClient.AddLocalAntreaFlexibleIPAMPodRule(containerConfig.IPs); err != nil { + return err + } } // Note that the IP address should be advertised after Pod OpenFlow entries are installed, otherwise the packet might @@ -236,9 +253,11 @@ func (pc *podConfigurator) configureInterfacesCommon( // Do not return an error and fail the interface creation. klog.ErrorS(err, "Failed to advertise IP address for container", "container", containerID) } + // Mark the manipulation as success to cancel deferred operations. success = true - klog.Infof("Configured interfaces for container %s", containerID) + klog.InfoS("Configured container interface", "Pod", klog.KRef(podNamespace, podName), + "container", containerID, "interface", containerIface.Name, "hostInterface", hostIface.Name) return nil } @@ -489,28 +508,36 @@ func (pc *podConfigurator) connectInterfaceToOVSCommon(ovsPortName, netNS string } }() - // GetOFPort will wait for up to 1 second for OVSDB to report the OFPort number. var ofPort int32 - ofPort, err = pc.ovsBridgeClient.GetOFPort(ovsPortName, false) - 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) - if err = pc.ofClient.InstallPodFlows(ovsPortName, containerConfig.IPs, containerConfig.MAC, uint32(ofPort), containerConfig.VLANID, nil); err != nil { - return fmt.Errorf("failed to add Openflow entries for container %s: %v", containerID, err) + // Not needed for a secondary network interface. + if !pc.isSecondaryNetwork { + // GetOFPort will wait for up to 1 second for OVSDB to report the OFPort number. + ofPort, err = pc.ovsBridgeClient.GetOFPort(ovsPortName, false) + 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) + if err = pc.ofClient.InstallPodFlows(ovsPortName, containerConfig.IPs, containerConfig.MAC, uint32(ofPort), containerConfig.VLANID, nil); err != nil { + return fmt.Errorf("failed to add Openflow entries for container %s: %v", containerID, err) + } } + containerConfig.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: portUUID, OFPort: ofPort} // Add containerConfig into local cache pc.ifaceStore.AddInterface(containerConfig) - // Notify the Pod update event to required components. - event := agenttypes.PodUpdate{ - PodName: containerConfig.PodName, - PodNamespace: containerConfig.PodNamespace, - ContainerID: containerConfig.ContainerID, - NetNS: netNS, - IsAdd: true, - } - pc.podUpdateNotifier.Notify(event) + + // Not needed for a secondary network interface. + if !pc.isSecondaryNetwork { + // Notify the Pod update event to required components. + event := agenttypes.PodUpdate{ + PodName: containerConfig.PodName, + PodNamespace: containerConfig.PodNamespace, + ContainerID: containerConfig.ContainerID, + NetNS: netNS, + IsAdd: true, + } + pc.podUpdateNotifier.Notify(event) + } return nil } @@ -518,29 +545,32 @@ func (pc *podConfigurator) connectInterfaceToOVSCommon(ovsPortName, netNS string func (pc *podConfigurator) disconnectInterfaceFromOVS(containerConfig *interfacestore.InterfaceConfig) error { containerID := containerConfig.ContainerID klog.V(2).Infof("Deleting Openflow entries for container %s", containerID) - if err := pc.ofClient.UninstallPodFlows(containerConfig.InterfaceName); err != nil { - return fmt.Errorf("failed to delete Openflow entries for container %s: %v", containerID, err) - // We should not delete OVS port if Pod flows deletion fails, otherwise - // it is possible a new Pod will reuse the reclaimed ofport number, and - // the OVS flows added for the new Pod can conflict with the stale - // flows of the deleted Pod. + if !pc.isSecondaryNetwork { + if err := pc.ofClient.UninstallPodFlows(containerConfig.InterfaceName); err != nil { + return fmt.Errorf("failed to delete Openflow entries for container %s: %v", containerID, err) + // We should not delete OVS port if Pod flows deletion fails, otherwise + // it is possible a new Pod will reuse the reclaimed ofport number, and + // the OVS flows added for the new Pod can conflict with the stale + // flows of the deleted Pod. + } } - klog.V(2).Infof("Deleting OVS port %s for container %s", containerConfig.PortUUID, containerID) // TODO: handle error and introduce garbage collection for failure on deletion if err := pc.ovsBridgeClient.DeletePort(containerConfig.PortUUID); err != nil { - return fmt.Errorf("failed to delete OVS port for container %s: %v", containerID, err) + return fmt.Errorf("failed to delete OVS port for container %s interface %s: %v", containerID, containerConfig.InterfaceName, err) } // Remove container configuration from cache. pc.ifaceStore.DeleteInterface(containerConfig) - event := agenttypes.PodUpdate{ - PodName: containerConfig.PodName, - PodNamespace: containerConfig.PodNamespace, - ContainerID: containerConfig.ContainerID, - IsAdd: false, - } - pc.podUpdateNotifier.Notify(event) - klog.Infof("Removed interfaces for container %s", containerID) + if !pc.isSecondaryNetwork { + event := agenttypes.PodUpdate{ + PodName: containerConfig.PodName, + PodNamespace: containerConfig.PodNamespace, + ContainerID: containerConfig.ContainerID, + IsAdd: false, + } + pc.podUpdateNotifier.Notify(event) + } + klog.InfoS("Deleted container OVS port", "container", containerID, "interface", containerConfig.InterfaceName) return nil } diff --git a/pkg/agent/cniserver/pod_configuration_linux.go b/pkg/agent/cniserver/pod_configuration_linux.go index c692f541731..75dd9fd6d5e 100644 --- a/pkg/agent/cniserver/pod_configuration_linux.go +++ b/pkg/agent/cniserver/pod_configuration_linux.go @@ -25,18 +25,13 @@ import ( "antrea.io/antrea/pkg/agent/interfacestore" ) -// connectInterfaceToOVS connects an existing interface to ovs br-int. +// connectInterfaceToOVS connects an existing interface to the OVS bridge. func (pc *podConfigurator) connectInterfaceToOVS( - podName string, - podNamespace string, - containerID string, - netNS string, - hostIface *current.Interface, - containerIface *current.Interface, + podName, podNamespace, containerID, netNS string, + hostIface, containerIface *current.Interface, ips []*current.IPConfig, vlanID uint16, - containerAccess *containerAccessArbitrator, -) (*interfacestore.InterfaceConfig, error) { + containerAccess *containerAccessArbitrator) (*interfacestore.InterfaceConfig, error) { // Use the outer veth interface name as the OVS port name. ovsPortName := hostIface.Name containerConfig := buildContainerConfig(ovsPortName, containerID, podName, podNamespace, containerIface, ips, vlanID) diff --git a/pkg/agent/cniserver/pod_configuration_linux_test.go b/pkg/agent/cniserver/pod_configuration_linux_test.go index 67b5440088a..cd50c2a8c8a 100644 --- a/pkg/agent/cniserver/pod_configuration_linux_test.go +++ b/pkg/agent/cniserver/pod_configuration_linux_test.go @@ -25,6 +25,8 @@ import ( "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" + "antrea.io/antrea/pkg/agent/cniserver/ipam" + ipamtest "antrea.io/antrea/pkg/agent/cniserver/ipam/testing" "antrea.io/antrea/pkg/agent/interfacestore" openflowtest "antrea.io/antrea/pkg/agent/openflow/testing" routetest "antrea.io/antrea/pkg/agent/route/testing" @@ -387,6 +389,73 @@ func TestParseOVSPortInterfaceConfig(t *testing.T) { }, }, }, + { + name: "secondary", + portData: &ovsconfig.OVSPortData{ + Name: portName, + ExternalIDs: map[string]string{ + ovsExternalIDContainerID: containerID, + ovsExternalIDIP: containerIPs, + ovsExternalIDMAC: containerMACStr, + ovsExternalIDPodName: podName, + ovsExternalIDPodNamespace: testPodNamespace, + ovsExternalIDIFDev: "eth1", + }, + }, + portConfig: &interfacestore.OVSPortConfig{ + PortUUID: portUUID, + OFPort: ofPort, + }, + ifaceConfig: &interfacestore.InterfaceConfig{ + Type: interfacestore.ContainerInterface, + InterfaceName: portName, + IPs: parsedIPs, + MAC: containerMAC, + OVSPortConfig: &interfacestore.OVSPortConfig{ + PortUUID: portUUID, + OFPort: ofPort, + }, + ContainerInterfaceConfig: &interfacestore.ContainerInterfaceConfig{ + ContainerID: containerID, + PodName: podName, + PodNamespace: testPodNamespace, + IFDev: "eth1", + }, + }, + }, + { + name: "secondary-no-ip", + portData: &ovsconfig.OVSPortData{ + Name: portName, + ExternalIDs: map[string]string{ + ovsExternalIDContainerID: containerID, + ovsExternalIDIP: "", + ovsExternalIDMAC: containerMACStr, + ovsExternalIDPodName: podName, + ovsExternalIDPodNamespace: testPodNamespace, + ovsExternalIDIFDev: "eth1", + }, + }, + portConfig: &interfacestore.OVSPortConfig{ + PortUUID: portUUID, + OFPort: ofPort, + }, + ifaceConfig: &interfacestore.InterfaceConfig{ + Type: interfacestore.ContainerInterface, + InterfaceName: portName, + MAC: containerMAC, + OVSPortConfig: &interfacestore.OVSPortConfig{ + PortUUID: portUUID, + OFPort: ofPort, + }, + ContainerInterfaceConfig: &interfacestore.ContainerInterfaceConfig{ + ContainerID: containerID, + PodName: podName, + PodNamespace: testPodNamespace, + IFDev: "eth1", + }, + }, + }, } { t.Run(tc.name, func(t *testing.T) { iface := ParseOVSPortInterfaceConfig(tc.portData, tc.portConfig) @@ -403,7 +472,8 @@ func TestCheckHostInterface(t *testing.T) { interfaces := []*current.Interface{containerIntf, {Name: hostIfaceName}} containeIPs := ipamResult.IPs ifaceMAC, _ := net.ParseMAC("01:02:03:04:05:06") - containerInterface := interfacestore.NewContainerInterface(hostIfaceName, containerID, "pod1", testPodNamespace, ifaceMAC, []net.IP{containerIP}, 1) + containerInterface := interfacestore.NewContainerInterface(hostIfaceName, containerID, + "pod1", testPodNamespace, "eth0", ifaceMAC, []net.IP{containerIP}, 1) containerInterface.OVSPortConfig = &interfacestore.OVSPortConfig{ PortUUID: generateUUID(), OFPort: int32(10), @@ -453,7 +523,6 @@ func TestCheckHostInterface(t *testing.T) { } func TestConfigureSriovSecondaryInterface(t *testing.T) { - controller := gomock.NewController(t) containerID := generateUUID() containerNS := "containerNS" @@ -463,6 +532,7 @@ func TestConfigureSriovSecondaryInterface(t *testing.T) { configureLinkErr error advertiseErr error expectedErr error + intfConfig *interfacestore.InterfaceConfig }{ { name: "sriov-vf-not-set", @@ -478,23 +548,142 @@ func TestConfigureSriovSecondaryInterface(t *testing.T) { advertiseErr: fmt.Errorf("unable to advertise on the sriov link"), // When advertiseContainerAddr returns an error, it is logged, but does not // cause ConfigureSriovSecondaryInterface to also return an error. - + intfConfig: &interfacestore.InterfaceConfig{ + Type: interfacestore.ContainerInterface, + InterfaceName: "vf2", + ContainerInterfaceConfig: &interfacestore.ContainerInterfaceConfig{ + ContainerID: containerID, + PodName: podName, + PodNamespace: testPodNamespace, + IFDev: "eth0", + }, + }, }, { name: "success", podSriovVFDeviceID: "vf3", + intfConfig: &interfacestore.InterfaceConfig{ + Type: interfacestore.ContainerInterface, + InterfaceName: "vf3", + ContainerInterfaceConfig: &interfacestore.ContainerInterfaceConfig{ + ContainerID: containerID, + PodName: podName, + PodNamespace: testPodNamespace, + IFDev: "eth0", + }, + }, }, } { t.Run(tc.name, func(t *testing.T) { ifaceConfigurator := newTestInterfaceConfigurator() ifaceConfigurator.configureContainerLinkError = tc.configureLinkErr ifaceConfigurator.advertiseContainerAddrError = tc.advertiseErr - podConfigurator := createPodConfigurator(controller, ifaceConfigurator) + podConfigurator, _ := NewSecondaryInterfaceConfigurator(nil, interfacestore.NewInterfaceStore()) + podConfigurator.ifConfigurator = ifaceConfigurator err := podConfigurator.ConfigureSriovSecondaryInterface(podName, testPodNamespace, containerID, containerNS, containerIfaceName, mtu, tc.podSriovVFDeviceID, ¤t.Result{}) assert.Equal(t, tc.expectedErr, err) + if tc.expectedErr != nil { + assert.Equal(t, 0, podConfigurator.ifaceStore.Len()) + return + } + + assert.Equal(t, 1, podConfigurator.ifaceStore.Len()) + intfConfig, _ := podConfigurator.ifaceStore.GetContainerInterface(containerID) + assert.Equal(t, tc.intfConfig, intfConfig) + assert.NoError(t, podConfigurator.DeleteSriovSecondaryInterface(intfConfig)) + assert.Equal(t, 0, podConfigurator.ifaceStore.Len()) }) } } +func newTestContainerInterfaceConfig(podName, containerID, ifDev string, vlan int) *interfacestore.InterfaceConfig { + hostIfaceName := util.GenerateContainerHostVethName(podName, testPodNamespace, containerID, ifDev) + fakePortUUID := generateUUID() + podMAC, _ := net.ParseMAC("01:02:03:04:05:06") + podIP := net.ParseIP("1.1.1.11") + podIPv6 := net.ParseIP("3ffe:ffff:10:1ff::111") + containerConfig := interfacestore.NewContainerInterface( + hostIfaceName, containerID, podName, testPodNamespace, + ifDev, podMAC, []net.IP{podIP, podIPv6}, uint16(vlan)) + containerConfig.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: fakePortUUID, OFPort: 0} + return containerConfig +} + +func TestConfigureVLANSecondaryInterface(t *testing.T) { + controller := gomock.NewController(t) + mockOVSBridgeClient := ovsconfigtest.NewMockOVSBridgeClient(controller) + ifaceStore := interfacestore.NewInterfaceStore() + pc, err := NewSecondaryInterfaceConfigurator(mockOVSBridgeClient, ifaceStore) + require.Nil(t, err, "No error expected in podConfigurator constructor") + testIfaceConfigurator := newTestInterfaceConfigurator() + pc.ifConfigurator = testIfaceConfigurator + + containerNS := "containerNS" + podName := "pod1" + containerID := generateUUID() + containerCfg1 := newTestContainerInterfaceConfig(podName, containerID, "eth1", 100) + containerCfg2 := newTestContainerInterfaceConfig(podName, containerID, "eth2", 0) + // Secondary interface with no IP. + containerCfg2.IPs = nil + + result := ipamtest.GenerateIPAMResult([]string{"1.1.1.11/24,1.1.1.1,4", "3ffe:ffff:10:1ff::111/64,3ffe:ffff:10::1,6"}, routes, dns) + ipamResult := &ipam.IPAMResult{Result: *result, VLANID: 100} + testIfaceConfigurator.hostIfaceName = containerCfg1.InterfaceName + testIfaceConfigurator.containerMAC = containerCfg1.MAC.String() + + mockOVSBridgeClient.EXPECT().CreateAccessPort( + containerCfg1.InterfaceName, containerCfg1.InterfaceName, + gomock.Any(), uint16(100)).Return(containerCfg1.PortUUID, nil).Times(1) + assert.NoError(t, pc.ConfigureVLANSecondaryInterface(podName, testPodNamespace, containerID, containerNS, "eth1", 1500, ipamResult)) + assert.Equal(t, 1, ifaceStore.Len()) + intfConfig, _ := ifaceStore.GetContainerInterface(containerID) + assert.Equal(t, containerCfg1, intfConfig) + + testIfaceConfigurator.hostIfaceName = containerCfg2.InterfaceName + testIfaceConfigurator.containerMAC = containerCfg2.MAC.String() + ipamResult = &ipam.IPAMResult{} + mockOVSBridgeClient.EXPECT().CreatePort( + containerCfg2.InterfaceName, containerCfg2.InterfaceName, + gomock.Any()).Return(containerCfg2.PortUUID, nil).Times(1) + 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)) + for _, intfConfig := range intfConfigs { + if intfConfig.InterfaceName == containerCfg1.InterfaceName { + assert.Equal(t, containerCfg1, intfConfig) + } else { + assert.Equal(t, containerCfg2, intfConfig) + } + } +} + +func TestDeleteVLANSecondaryInterface(t *testing.T) { + controller := gomock.NewController(t) + mockOVSBridgeClient := ovsconfigtest.NewMockOVSBridgeClient(controller) + ifaceStore := interfacestore.NewInterfaceStore() + pc, err := NewSecondaryInterfaceConfigurator(mockOVSBridgeClient, ifaceStore) + require.Nil(t, err, "No error expected in podConfigurator constructor") + + podName := "pod1" + containerID := generateUUID() + containerCfg1 := newTestContainerInterfaceConfig(podName, containerID, "eth1", 100) + containerCfg2 := newTestContainerInterfaceConfig(podName, containerID, "eth2", 0) + // Secondary interface with no IP. + containerCfg2.IPs = nil + ifaceStore.AddInterface(containerCfg1) + ifaceStore.AddInterface(containerCfg2) + + mockOVSBridgeClient.EXPECT().DeletePort(containerCfg1.PortUUID).Return(nil) + assert.NoError(t, pc.DeleteVLANSecondaryInterface(containerCfg1)) + intfConfig, _ := ifaceStore.GetContainerInterface(containerID) + assert.Equal(t, containerCfg2, intfConfig) + + mockOVSBridgeClient.EXPECT().DeletePort(containerCfg2.PortUUID).Return(nil) + assert.NoError(t, pc.DeleteVLANSecondaryInterface(containerCfg2)) + _, found := ifaceStore.GetContainerInterface(containerID) + assert.False(t, found, "Interface should not be in the local cache anymore") +} + func createPodConfigurator(controller *gomock.Controller, testIfaceConfigurator *fakeInterfaceConfigurator) *podConfigurator { gwMAC, _ := net.ParseMAC("00:00:11:11:11:11") mockOVSBridgeClient = ovsconfigtest.NewMockOVSBridgeClient(controller) diff --git a/pkg/agent/cniserver/pod_configuration_windows.go b/pkg/agent/cniserver/pod_configuration_windows.go index 6c574ad0306..8842a6bf416 100644 --- a/pkg/agent/cniserver/pod_configuration_windows.go +++ b/pkg/agent/cniserver/pod_configuration_windows.go @@ -62,18 +62,13 @@ func (pc *podConfigurator) connectInterfaceToOVSAsync(ifConfig *interfacestore.I }) } -// connectInterfaceToOVS connects an existing interface to OVS br-int. +// connectInterfaceToOVS connects an existing interface to the OVS bridge. func (pc *podConfigurator) connectInterfaceToOVS( - podName string, - podNamespace string, - containerID string, - netNS string, - hostIface *current.Interface, - containerIface *current.Interface, + podName, podNamespace, containerID, netNS string, + hostIface, containerIface *current.Interface, ips []*current.IPConfig, vlanID uint16, - containerAccess *containerAccessArbitrator, -) (*interfacestore.InterfaceConfig, error) { + containerAccess *containerAccessArbitrator) (*interfacestore.InterfaceConfig, error) { // Use the outer veth interface name as the OVS port name. ovsPortName := hostIface.Name containerConfig := buildContainerConfig(ovsPortName, containerID, podName, podNamespace, containerIface, ips, vlanID) diff --git a/pkg/agent/cniserver/secondary.go b/pkg/agent/cniserver/secondary.go index bfe94751e99..a4176baba15 100644 --- a/pkg/agent/cniserver/secondary.go +++ b/pkg/agent/cniserver/secondary.go @@ -20,11 +20,17 @@ import ( current "github.com/containernetworking/cni/pkg/types/100" "k8s.io/klog/v2" + "antrea.io/antrea/pkg/agent/cniserver/ipam" + "antrea.io/antrea/pkg/agent/interfacestore" "antrea.io/antrea/pkg/ovs/ovsconfig" ) -func NewSecondaryInterfaceConfigurator(ovsBridgeClient ovsconfig.OVSBridgeClient) (*podConfigurator, error) { - return newPodConfigurator(ovsBridgeClient, nil, nil, nil, nil, ovsconfig.OVSDatapathSystem, false, false, nil) +func NewSecondaryInterfaceConfigurator(ovsBridgeClient ovsconfig.OVSBridgeClient, interfaceStore interfacestore.InterfaceStore) (*podConfigurator, error) { + pc, err := newPodConfigurator(ovsBridgeClient, nil, nil, interfaceStore, nil, ovsconfig.OVSDatapathSystem, false, false, nil) + if err == nil { + pc.isSecondaryNetwork = true + } + return pc, err } // ConfigureSriovSecondaryInterface configures a SR-IOV secondary interface for a Pod. @@ -42,9 +48,13 @@ func (pc *podConfigurator) ConfigureSriovSecondaryInterface( if err != nil { return err } - hostIface := result.Interfaces[0] containerIface := result.Interfaces[1] - klog.InfoS("Configured SR-IOV interface", "Pod", klog.KRef(podNamespace, podName), "interface", containerInterfaceName, "hostInterface", hostIface) + klog.InfoS("Configured SR-IOV interface", "Pod", klog.KRef(podNamespace, podName), "interface", containerInterfaceName) + + // Use podSriovVFDeviceID as the interface name in the interface store. + hostInterfaceName := podSriovVFDeviceID + containerConfig := buildContainerConfig(hostInterfaceName, containerID, podName, podNamespace, containerIface, result.IPs, 0) + pc.ifaceStore.AddInterface(containerConfig) if result.IPs != nil { if err = pc.ifConfigurator.advertiseContainerAddr(containerNetNS, containerIface.Name, result); err != nil { @@ -55,55 +65,35 @@ func (pc *podConfigurator) ConfigureSriovSecondaryInterface( return nil } +// DeleteSriovSecondaryInterface deletes a SRIOV secondary interface. +func (pc *podConfigurator) DeleteSriovSecondaryInterface(interfaceConfig *interfacestore.InterfaceConfig) error { + pc.ifaceStore.DeleteInterface(interfaceConfig) + klog.InfoS("Deleted SR-IOV interface", "Pod", klog.KRef(interfaceConfig.PodNamespace, interfaceConfig.PodName), + "interface", interfaceConfig.IFDev) + return nil + +} + // ConfigureVLANSecondaryInterface configures a VLAN secondary interface on the secondary network // OVS bridge, and returns the OVS port UUID. func (pc *podConfigurator) ConfigureVLANSecondaryInterface( podName, podNamespace string, containerID, containerNetNS, containerInterfaceName string, - mtu int, vlanID uint16, - result *current.Result) (string, error) { - // TODO: revisit the possibility of reusing configureInterfaces(), connectInterfaceToOVS() - // removeInterfaces() code, and using InterfaceStore to store secondary interface info. - if err := pc.ifConfigurator.configureContainerLink(podName, podNamespace, containerID, containerNetNS, containerInterfaceName, mtu, "", "", result, nil); err != nil { - return "", err - } - hostIface := result.Interfaces[0] - containerIface := result.Interfaces[1] - - success := false - defer func() { - if !success { - if err := pc.ifConfigurator.removeContainerLink(containerID, hostIface.Name); err != nil { - klog.ErrorS(err, "failed to roll back veth creation", "container", containerID, "interface", containerInterfaceName) - } - } - }() - - // Use the outer veth interface name as the OVS port name. - ovsPortName := hostIface.Name - ovsPortUUID, err := pc.createOVSPort(ovsPortName, nil, vlanID) - if err != nil { - return "", fmt.Errorf("failed to add OVS port for container %s: %v", containerID, err) - } - klog.InfoS("Configured VLAN interface", "Pod", klog.KRef(podNamespace, podName), "interface", containerInterfaceName, "hostInterface", hostIface) - - if result.IPs != nil { - if err := pc.ifConfigurator.advertiseContainerAddr(containerNetNS, containerIface.Name, result); err != nil { - klog.ErrorS(err, "Failed to advertise IP address for VLAN interface", - "container", containerID, "interface", containerInterfaceName) - } - } - success = true - return ovsPortUUID, nil + mtu int, ipamResult *ipam.IPAMResult) error { + return pc.configureInterfacesCommon(podName, podNamespace, containerID, containerNetNS, + containerInterfaceName, mtu, "", ipamResult, nil) } // DeleteVLANSecondaryInterface deletes a VLAN secondary interface. -func (pc *podConfigurator) DeleteVLANSecondaryInterface(containerID, hostInterfaceName, ovsPortUUID string) error { - if err := pc.ovsBridgeClient.DeletePort(ovsPortUUID); err != nil { - return fmt.Errorf("failed to delete OVS port for container %s: %v", containerID, err) - } - if err := pc.ifConfigurator.removeContainerLink(containerID, hostInterfaceName); err != nil { +func (pc *podConfigurator) DeleteVLANSecondaryInterface(interfaceConfig *interfacestore.InterfaceConfig) error { + if err := pc.disconnectInterfaceFromOVS(interfaceConfig); err != nil { return err } + if err := pc.ifConfigurator.removeContainerLink(interfaceConfig.ContainerID, interfaceConfig.InterfaceName); err != nil { + klog.ErrorS(err, "Failed to delete container interface link", + "Pod", klog.KRef(interfaceConfig.PodNamespace, interfaceConfig.PodName), + "interface", interfaceConfig.IFDev) + // No retry for interface deletion. + } return nil } diff --git a/pkg/agent/cniserver/server_linux_test.go b/pkg/agent/cniserver/server_linux_test.go index 52791d129b8..856024dd32c 100644 --- a/pkg/agent/cniserver/server_linux_test.go +++ b/pkg/agent/cniserver/server_linux_test.go @@ -141,6 +141,7 @@ func TestRemoveInterface(t *testing.T) { containerID, podName, testPodNamespace, + "eth0", containerMAC, []net.IP{containerIP}, 0) @@ -435,7 +436,7 @@ func TestCmdDel(t *testing.T) { response: &cnipb.CniCmdResponse{ Error: &cnipb.Error{ Code: cnipb.ErrorCode_CONFIG_INTERFACE_FAILURE, - Message: fmt.Sprintf("failed to delete OVS port for container %s: failed to delete port", testPodInfraContainerID), + Message: fmt.Sprintf("failed to delete OVS port for container %s interface A-1-b0d460: failed to delete port", testPodInfraContainerID), }, }, }, @@ -473,7 +474,9 @@ func TestCmdDel(t *testing.T) { cniserver := newMockCNIServer(t, controller, ipamMock, tc.ipamType, tc.enableSecondaryNetworkIPAM, tc.isChaining) requestMsg, hostInterfaceName := createCNIRequestAndInterfaceName(t, testPodNameA, tc.cniType, ipamResult, tc.ipamType, true) containerID := requestMsg.CniArgs.ContainerId - containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, testPodNameA, testPodNamespace, containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0) + containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, + testPodNameA, testPodNamespace, "eth0", + containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0) containerIfaceConfig.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: ovsPortID, OFPort: ovsPort} ifaceStore.AddInterface(containerIfaceConfig) testIfaceConfigurator := newTestInterfaceConfigurator() @@ -536,7 +539,9 @@ func TestCmdCheck(t *testing.T) { } podArgs := cniservertest.GenerateCNIArgs(name, testPodNamespace, testPodInfraContainerID) requestMsg, containerID := newRequest(podArgs, networkCfg, "", t) - containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, name, testPodNamespace, containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0) + containerIfaceConfig := interfacestore.NewContainerInterface(hostInterfaceName, containerID, + name, testPodNamespace, "eth0", + containerVethMac, []net.IP{net.ParseIP("10.1.2.100")}, 0) containerIfaceConfig.OVSPortConfig = &interfacestore.OVSPortConfig{PortUUID: ovsPortID, OFPort: ovsPort} ifaceStore.AddInterface(containerIfaceConfig) return requestMsg, hostInterfaceName diff --git a/pkg/agent/cniserver/server_test.go b/pkg/agent/cniserver/server_test.go index 97acdef7028..d1c3c75e5da 100644 --- a/pkg/agent/cniserver/server_test.go +++ b/pkg/agent/cniserver/server_test.go @@ -676,46 +676,69 @@ func TestBuildOVSPortExternalIDs(t *testing.T) { containerIP1 := net.ParseIP("10.1.2.100") containerIP2 := net.ParseIP("2001:fd1a::2") containerIPs := []net.IP{containerIP1, containerIP2} - containerConfig := interfacestore.NewContainerInterface("pod1-abcd", containerID, "test-1", "t1", containerMAC, containerIPs, 0) - externalIds := BuildOVSPortExternalIDs(containerConfig) - parsedIP, existed := externalIds[ovsExternalIDIP] + containerConfig := interfacestore.NewContainerInterface("pod1-abcd", containerID, + "test-1", "t1", "eth0", containerMAC, containerIPs, 0) + externalIDs := BuildOVSPortExternalIDs(containerConfig) + _, existed := externalIDs[ovsExternalIDIFDev] + assert.False(t, existed, "External IDs should not include interface name eth0") + parsedIP, existed := externalIDs[ovsExternalIDIP] parsedIPStr := parsedIP.(string) if !existed || !strings.Contains(parsedIPStr, "10.1.2.100") || !strings.Contains(parsedIPStr, "2001:fd1a::2") { - t.Errorf("Failed to parse container configuration") + t.Errorf("Failed to store IPs to external IDs") } - parsedMac, existed := externalIds[ovsExternalIDMAC] + parsedMac, existed := externalIDs[ovsExternalIDMAC] if !existed || parsedMac != containerMAC.String() { - t.Errorf("Failed to parse container configuration") + t.Errorf("Failed to store MAC to external IDs") } - parsedID, existed := externalIds[ovsExternalIDContainerID] + parsedID, existed := externalIDs[ovsExternalIDContainerID] if !existed || parsedID != containerID { - t.Errorf("Failed to parse container configuration") + t.Errorf("Failed to store container ID to external IDs") } - portExternalIDs := make(map[string]string) - for k, v := range externalIds { - val := v.(string) - portExternalIDs[k] = val - } - mockPort := &ovsconfig.OVSPortData{ - Name: "testPort", - ExternalIDs: portExternalIDs, - } - portConfig := &interfacestore.OVSPortConfig{ - PortUUID: "12345678", - OFPort: int32(1), - } - ifaceConfig := ParseOVSPortInterfaceConfig(mockPort, portConfig) - assert.Equal(t, len(containerIPs), len(ifaceConfig.IPs)) - for _, ip1 := range containerIPs { - existed := false - for _, ip2 := range ifaceConfig.IPs { - if ip2.Equal(ip1) { - existed = true - break + + testConfigParsingFn := func() { + portExternalIDs := make(map[string]string) + for k, v := range externalIDs { + val := v.(string) + portExternalIDs[k] = val + } + mockPort := &ovsconfig.OVSPortData{ + Name: "testPort", + ExternalIDs: portExternalIDs, + } + portConfig := &interfacestore.OVSPortConfig{ + PortUUID: "12345678", + OFPort: int32(1), + } + ifaceConfig := ParseOVSPortInterfaceConfig(mockPort, portConfig) + assert.Equal(t, len(containerIPs), len(ifaceConfig.IPs)) + for _, ip1 := range containerIPs { + existed := false + for _, ip2 := range ifaceConfig.IPs { + if ip2.Equal(ip1) { + existed = true + break + } } + assert.True(t, existed, fmt.Sprintf("IP %s should exist in the restored InterfaceConfig", ip1.String())) } - assert.True(t, existed, fmt.Sprintf("IP %s should exist in the restored InterfaceConfig", ip1.String())) } + testConfigParsingFn() + + // Secondary interface with no IP. + containerIPs = nil + containerConfig = interfacestore.NewContainerInterface("pod1-abcd", containerID, + "test-1", "t1", "eth1", containerMAC, containerIPs, 0) + externalIDs = BuildOVSPortExternalIDs(containerConfig) + parsedIFDev, existed := externalIDs[ovsExternalIDIFDev] + if !existed || parsedIFDev != "eth1" { + t.Errorf("Failed to store interface name to external IDs") + } + parsedIP, existed = externalIDs[ovsExternalIDIP] + if !existed || parsedIP.(string) != "" { + t.Errorf("Failed to store (none) IP to external IDs") + } + testConfigParsingFn() + } func translateRawPrevResult(prevResult *current.Result, cniVersion string) (map[string]interface{}, error) { diff --git a/pkg/agent/cniserver/server_windows_test.go b/pkg/agent/cniserver/server_windows_test.go index 600adb5bf8e..ba562bae762 100644 --- a/pkg/agent/cniserver/server_windows_test.go +++ b/pkg/agent/cniserver/server_windows_test.go @@ -624,7 +624,7 @@ func TestCmdDel(t *testing.T) { server.podConfigurator.ifConfigurator.(*ifConfigurator).addEndpoint(hnsEndpoint) } if tc.ifaceExists { - containerIface := interfacestore.NewContainerInterface(ovsPortName, containerID, testPodNameA, testPodNamespace, containerMAC, []net.IP{net.ParseIP("10.1.2.100")}, 0) + containerIface := interfacestore.NewContainerInterface(ovsPortName, containerID, testPodNameA, testPodNamespace, "", containerMAC, []net.IP{net.ParseIP("10.1.2.100")}, 0) containerIface.OVSPortConfig = &interfacestore.OVSPortConfig{ OFPort: 100, PortUUID: ovsPortID, @@ -687,7 +687,7 @@ func TestCmdCheck(t *testing.T) { return &result } wrapperContainerInterface := func(ifaceName, containerID, podName, ovsPortID string, mac net.HardwareAddr, containerIP net.IP) *interfacestore.InterfaceConfig { - containerIface := interfacestore.NewContainerInterface(ifaceName, containerID, podName, testPodNamespace, mac, []net.IP{containerIP}, 0) + containerIface := interfacestore.NewContainerInterface(ifaceName, containerID, podName, testPodNamespace, "", mac, []net.IP{containerIP}, 0) containerIface.OVSPortConfig = &interfacestore.OVSPortConfig{ PortUUID: ovsPortID, OFPort: 10, diff --git a/pkg/agent/interfacestore/interface_cache.go b/pkg/agent/interfacestore/interface_cache.go index 4eb4c49bf0e..bba8cf2068a 100644 --- a/pkg/agent/interfacestore/interface_cache.go +++ b/pkg/agent/interfacestore/interface_cache.go @@ -85,7 +85,7 @@ func getInterfaceKey(obj interface{}) (string, error) { interfaceConfig := obj.(*InterfaceConfig) var key string if interfaceConfig.Type == ContainerInterface { - key = util.GenerateContainerInterfaceKey(interfaceConfig.ContainerID) + key = util.GenerateContainerInterfaceKey(interfaceConfig.ContainerID, interfaceConfig.IFDev) } else if interfaceConfig.Type == IPSecTunnelInterface { // IPsec tunnel interface for a Node. key = util.GenerateNodeTunnelInterfaceKey(interfaceConfig.NodeName) @@ -266,7 +266,8 @@ func interfaceIPIndexFunc(obj interface{}) ([]string, error) { func interfaceOFPortIndexFunc(obj interface{}) ([]string, error) { interfaceConfig := obj.(*InterfaceConfig) - if interfaceConfig.OFPort < 0 { + // OVSPortConfig can be nil for a secondary SR-IOV interface. + if interfaceConfig.OVSPortConfig == nil || interfaceConfig.OFPort < 0 { // If interfaceConfig OFport is not valid, we return empty key. return []string{}, nil } diff --git a/pkg/agent/interfacestore/interface_cache_test.go b/pkg/agent/interfacestore/interface_cache_test.go index c338fb2fe09..36ed3c08005 100644 --- a/pkg/agent/interfacestore/interface_cache_test.go +++ b/pkg/agent/interfacestore/interface_cache_test.go @@ -28,6 +28,7 @@ import ( var ( podMAC, _ = net.ParseMAC("11:22:33:44:55:66") podIP = net.ParseIP("1.2.3.4") + podIPv6 = net.ParseIP("2001:db8::1") gwIP = net.ParseIP("1.2.3.1") hostIP = net.ParseIP("2.2.2.2") ipsecTunnelIP = net.ParseIP("2.2.2.3") @@ -37,6 +38,7 @@ var ( func TestNewInterfaceStore(t *testing.T) { t.Run("testContainerInterface", testContainerInterface) + t.Run("testSecondaryInterface", testSecondaryInterface) t.Run("testGatewayInterface", testGatewayInterface) t.Run("testTunnelInterface", testTunnelInterface) t.Run("testUplinkInterface", testUplinkInterface) @@ -45,17 +47,17 @@ func TestNewInterfaceStore(t *testing.T) { func testContainerInterface(t *testing.T) { store := NewInterfaceStore() - containerInterface := NewContainerInterface("ns0p0c0", "c0", "p0", "ns0", podMAC, []net.IP{podIP}, 2) + containerInterface := NewContainerInterface("ns0p0c0", "c0", "p0", "ns0", "eth0", podMAC, []net.IP{podIP, podIPv6}, 2) containerInterface.OVSPortConfig = &OVSPortConfig{ OFPort: 12, PortUUID: "1234567890", } - containerInterfaceKey := util.GenerateContainerInterfaceKey(containerInterface.ContainerID) + containerInterfaceKey := util.GenerateContainerInterfaceKey(containerInterface.ContainerID, containerInterface.IFDev) store.Initialize([]*InterfaceConfig{containerInterface}) assert.Equal(t, 1, store.Len()) storedIface, exists := store.GetInterface(containerInterfaceKey) assert.True(t, exists) - assert.True(t, reflect.DeepEqual(storedIface, containerInterface)) + assert.Equal(t, containerInterface, storedIface) // The name of Container InterfaceConfig is not the key in InterfaceStore _, exists = store.GetInterface(containerInterface.InterfaceName) assert.False(t, exists) @@ -65,21 +67,27 @@ func testContainerInterface(t *testing.T) { assert.True(t, exists) _, exists = store.GetInterfaceByIP(podIP.String()) assert.True(t, exists) + _, exists = store.GetInterfaceByIP(podIPv6.String()) + assert.True(t, exists) _, exists = store.GetInterfaceByOFPort(uint32(containerInterface.OVSPortConfig.OFPort)) assert.True(t, exists) ifaces := store.GetContainerInterfacesByPod(containerInterface.PodName, containerInterface.PodNamespace) assert.Equal(t, 1, len(ifaces)) - assert.True(t, reflect.DeepEqual(ifaces[0], containerInterface)) + assert.Equal(t, containerInterface, ifaces[0]) ifaceNames := store.GetInterfaceKeysByType(ContainerInterface) assert.Equal(t, 1, len(ifaceNames)) assert.Equal(t, containerInterfaceKey, ifaceNames[0]) assert.Equal(t, 1, store.GetContainerInterfaceNum()) + store.DeleteInterface(containerInterface) assert.Equal(t, 0, store.GetContainerInterfaceNum()) _, exists = store.GetContainerInterface(containerInterface.ContainerID) assert.False(t, exists) _, exists = store.GetInterfaceByIP(containerInterface.IPs[0].String()) assert.False(t, exists) + _, exists = store.GetInterfaceByIP(containerInterface.IPs[1].String()) + assert.False(t, exists) + containerInterface.IPs = nil store.AddInterface(containerInterface) assert.Equal(t, 1, store.GetContainerInterfaceNum()) @@ -87,6 +95,47 @@ func testContainerInterface(t *testing.T) { assert.False(t, exists) } +func testSecondaryInterface(t *testing.T) { + store := NewInterfaceStore() + // Seondary interface without an IP. + containerInterface1 := NewContainerInterface("c0-eth1", "c0", "p0", "ns0", "eth1", podMAC, nil, 2) + containerInterface2 := NewContainerInterface("c0-eth2", "c0", "p0", "ns0", "eth2", podMAC, []net.IP{podIP}, 0) + store.Initialize([]*InterfaceConfig{containerInterface1, containerInterface2}) + assert.Equal(t, 2, store.Len()) + + for _, containerInterface := range []*InterfaceConfig{containerInterface1, containerInterface2} { + interfaceKey := util.GenerateContainerInterfaceKey(containerInterface.ContainerID, containerInterface.IFDev) + storedIface, exists := store.GetInterface(interfaceKey) + assert.True(t, exists) + assert.Equal(t, containerInterface, storedIface) + _, exists = store.GetInterface(containerInterface.InterfaceName) + assert.False(t, exists) + _, exists = store.GetInterfaceByName(containerInterface.InterfaceName) + assert.True(t, exists) + _, exists = store.GetContainerInterface(containerInterface.ContainerID) + assert.True(t, exists) + if containerInterface.IPs != nil { + storedIface, exists = store.GetInterfaceByIP(podIP.String()) + assert.True(t, exists) + assert.Equal(t, containerInterface, storedIface) + } + ifaces := store.GetContainerInterfacesByPod(containerInterface.PodName, containerInterface.PodNamespace) + assert.Equal(t, 2, len(ifaces)) + ifaceNames := store.GetInterfaceKeysByType(ContainerInterface) + assert.Equal(t, 2, len(ifaceNames)) + assert.Equal(t, 2, store.GetContainerInterfaceNum()) + + store.DeleteInterface(containerInterface) + assert.Equal(t, 1, store.GetContainerInterfaceNum()) + if containerInterface.IPs != nil { + _, exists = store.GetInterfaceByIP(containerInterface.IPs[0].String()) + assert.False(t, exists) + } + store.AddInterface(containerInterface) + assert.Equal(t, 2, store.GetContainerInterfaceNum()) + } +} + func testGatewayInterface(t *testing.T) { gatewayInterface := NewGatewayInterface("antrea-gw0", util.GenerateRandomMAC()) gatewayInterface.IPs = []net.IP{gwIP} diff --git a/pkg/agent/interfacestore/types.go b/pkg/agent/interfacestore/types.go index 90715c1a2bc..9d0ca2afe57 100644 --- a/pkg/agent/interfacestore/types.go +++ b/pkg/agent/interfacestore/types.go @@ -64,6 +64,8 @@ type ContainerInterfaceConfig struct { ContainerID string PodName string PodNamespace string + // Interface name inside container. + IFDev string } type TunnelInterfaceConfig struct { @@ -133,13 +135,15 @@ func NewContainerInterface( containerID string, podName string, podNamespace string, + ifDev string, mac net.HardwareAddr, ips []net.IP, vlanID uint16) *InterfaceConfig { containerConfig := &ContainerInterfaceConfig{ ContainerID: containerID, PodName: podName, - PodNamespace: podNamespace} + PodNamespace: podNamespace, + IFDev: ifDev} return &InterfaceConfig{ InterfaceName: interfaceName, Type: ContainerInterface, diff --git a/pkg/agent/secondarynetwork/cnipodcache/cache.go b/pkg/agent/secondarynetwork/cnipodcache/cache.go deleted file mode 100644 index 0eba91af76e..00000000000 --- a/pkg/agent/secondarynetwork/cnipodcache/cache.go +++ /dev/null @@ -1,109 +0,0 @@ -// Copyright 2021 Antrea Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http:// www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package cnipodcache - -import ( - "sync" - - "k8s.io/client-go/tools/cache" - - "antrea.io/antrea/pkg/agent/util" - "antrea.io/antrea/pkg/util/k8s" -) - -const ( - podIndex = "pod" -) - -type CNIPodInfoCache struct { - // Mutex to protect CNIConfigInfo.PodCNIDeleted which is written by CNIServer, and read by - // the secondary network Pod controller. - sync.RWMutex - cache cache.Indexer -} - -// Add CNIPodInfo to local cache store. -func (c *CNIPodInfoCache) AddCNIConfigInfo(cniConfig *CNIConfigInfo) { - c.cache.Add(cniConfig) -} - -// Delete CNIPodInfo from local cache store. -func (c *CNIPodInfoCache) DeleteCNIConfigInfo(cniConfig *CNIConfigInfo) { - c.cache.Delete(cniConfig) -} - -func (c *CNIPodInfoCache) SetPodCNIDeleted(cniConfig *CNIConfigInfo) { - c.Lock() - defer c.Unlock() - cniConfig.PodCNIDeleted = true -} - -// Retrieve a valid CNI cache (PodCNIDeleted is not true) entry for the given Pod name and namespace. -func (c *CNIPodInfoCache) GetValidCNIConfigInfoPerPod(podName, podNamespace string) *CNIConfigInfo { - podObjs, _ := c.cache.ByIndex(podIndex, k8s.NamespacedName(podNamespace, podName)) - c.RLock() - defer c.RUnlock() - for i := range podObjs { - var cniPodConfig *CNIConfigInfo - cniPodConfig = podObjs[i].(*CNIConfigInfo) - if cniPodConfig.PodCNIDeleted != true { - return cniPodConfig - } - } - return nil -} - -// Retrieve all CNIConfigInfo from cacheStore for the given podName and its Namespace -// NOTE: In an ideal scenario, there should be one cache entry per Pod name and namespace. -func (c *CNIPodInfoCache) GetAllCNIConfigInfoPerPod(podName, podNamespace string) []*CNIConfigInfo { - podObjs, _ := c.cache.ByIndex(podIndex, k8s.NamespacedName(podNamespace, podName)) - CNIPodConfigs := make([]*CNIConfigInfo, len(podObjs)) - for i := range podObjs { - CNIPodConfigs[i] = podObjs[i].(*CNIConfigInfo) - } - return CNIPodConfigs -} - -func (c *CNIPodInfoCache) GetCNIConfigInfoByContainerID(podName, podNamespace, containerID string) *CNIConfigInfo { - podObjs, _ := c.cache.ByIndex(podIndex, k8s.NamespacedName(podNamespace, podName)) - for i := range podObjs { - var cniPodConfig *CNIConfigInfo - cniPodConfig = podObjs[i].(*CNIConfigInfo) - if cniPodConfig.ContainerID == containerID { - return cniPodConfig - } - } - return nil -} - -func podIndexFunc(obj interface{}) ([]string, error) { - podConfig := obj.(*CNIConfigInfo) - return []string{k8s.NamespacedName(podConfig.PodNamespace, podConfig.PodName)}, nil -} - -func getCNIPodInfoKey(obj interface{}) (string, error) { - podConfig := obj.(*CNIConfigInfo) - var key string - key = util.GenerateContainerInterfaceKey(podConfig.ContainerID) - return key, nil -} - -func NewCNIPodInfoStore() *CNIPodInfoCache { - return &CNIPodInfoCache{ - cache: cache.NewIndexer(getCNIPodInfoKey, cache.Indexers{ - podIndex: podIndexFunc, - }), - } -} diff --git a/pkg/agent/secondarynetwork/cnipodcache/types.go b/pkg/agent/secondarynetwork/cnipodcache/types.go deleted file mode 100644 index 657ce431c2d..00000000000 --- a/pkg/agent/secondarynetwork/cnipodcache/types.go +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright 2021 Antrea Authors -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -package cnipodcache - -type CNIConfigInfo struct { - PodName string - PodNamespace string - ContainerID string - ContainerNetNS string - PodCNIDeleted bool - // Interfaces is a map that stores the secondary interface information with interface - // name to be the key. - Interfaces map[string]*InterfaceInfo -} - -type NetworkType string - -type InterfaceInfo struct { - NetworkType NetworkType - HostInterfaceName string - // OVS port UUID for a VLAN interface. - OVSPortUUID string -} - -type CNIPodInfoStore interface { - AddCNIConfigInfo(cniConfig *CNIConfigInfo) - DeleteCNIConfigInfo(cniConfig *CNIConfigInfo) - GetValidCNIConfigInfoPerPod(podName, podNamespace string) *CNIConfigInfo - GetAllCNIConfigInfoPerPod(podName, podNamespace string) []*CNIConfigInfo - GetCNIConfigInfoByContainerID(podName, podNamespace, containerID string) *CNIConfigInfo - SetPodCNIDeleted(CNIConfig *CNIConfigInfo) -} diff --git a/pkg/agent/secondarynetwork/podwatch/controller.go b/pkg/agent/secondarynetwork/podwatch/controller.go index fd6d9f8c500..4fa884b9373 100644 --- a/pkg/agent/secondarynetwork/podwatch/controller.go +++ b/pkg/agent/secondarynetwork/podwatch/controller.go @@ -24,6 +24,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" clientset "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" @@ -38,7 +39,7 @@ import ( "antrea.io/antrea/pkg/agent/cniserver" "antrea.io/antrea/pkg/agent/cniserver/ipam" cnitypes "antrea.io/antrea/pkg/agent/cniserver/types" - cnipodcache "antrea.io/antrea/pkg/agent/secondarynetwork/cnipodcache" + "antrea.io/antrea/pkg/agent/interfacestore" "antrea.io/antrea/pkg/agent/types" crdv1a2 "antrea.io/antrea/pkg/apis/crd/v1alpha2" "antrea.io/antrea/pkg/ovs/ovsconfig" @@ -57,7 +58,6 @@ const ( const ( networkAttachDefAnnotationKey = "k8s.v1.cni.cncf.io/networks" cniPath = "/opt/cni/bin/" - defaultSecondaryInterfaceName = "eth1" startIfaceIndex = 1 endIfaceIndex = 101 @@ -67,8 +67,9 @@ const ( type InterfaceConfigurator interface { ConfigureSriovSecondaryInterface(podName, podNamespace, containerID, containerNetNS, containerInterfaceName string, mtu int, podSriovVFDeviceID string, result *current.Result) error - ConfigureVLANSecondaryInterface(podName, podNamespace, containerID, containerNetNS, containerInterfaceName string, mtu int, vlanID uint16, result *current.Result) (string, error) - DeleteVLANSecondaryInterface(containerID, hostInterfaceName, ovsPortUUID string) error + DeleteSriovSecondaryInterface(interfaceConfig *interfacestore.InterfaceConfig) error + ConfigureVLANSecondaryInterface(podName, podNamespace, containerID, containerNetNS, containerInterfaceName string, mtu int, ipamResult *ipam.IPAMResult) error + DeleteVLANSecondaryInterface(interfaceConfig *interfacestore.InterfaceConfig) error } type IPAMAllocator interface { @@ -76,18 +77,25 @@ type IPAMAllocator interface { SecondaryNetworkRelease(podOwner *crdv1a2.PodOwner) error } -type PodController struct { +type podCNIInfo struct { + containerID string + netNS string +} + +type podController struct { kubeClient clientset.Interface netAttachDefClient netdefclient.K8sCniCncfIoV1Interface queue workqueue.RateLimitingInterface podInformer cache.SharedIndexInformer nodeName string podUpdateSubscriber channel.Subscriber - podCache cnipodcache.CNIPodInfoStore ovsBridgeClient ovsconfig.OVSBridgeClient + interfaceStore interfacestore.InterfaceStore interfaceConfigurator InterfaceConfigurator ipamAllocator IPAMAllocator - vfDeviceIDUsageMap sync.Map + // Map from "namespace/pod" to podCNIInfo. + cniCache sync.Map + vfDeviceIDUsageMap sync.Map } func NewPodController( @@ -97,20 +105,22 @@ func NewPodController( nodeName string, podUpdateSubscriber channel.Subscriber, ovsBridgeClient ovsconfig.OVSBridgeClient, -) (*PodController, error) { - interfaceConfigurator, err := cniserver.NewSecondaryInterfaceConfigurator(ovsBridgeClient) +) (*podController, error) { + ifaceStore := interfacestore.NewInterfaceStore() + interfaceConfigurator, err := cniserver.NewSecondaryInterfaceConfigurator(ovsBridgeClient, ifaceStore) if err != nil { return nil, fmt.Errorf("failed to create SecondaryInterfaceConfigurator: %v", err) } - pc := PodController{ - kubeClient: kubeClient, - netAttachDefClient: netAttachDefClient, - queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "podcontroller"), + pc := podController{ + kubeClient: kubeClient, + netAttachDefClient: netAttachDefClient, + queue: workqueue.NewNamedRateLimitingQueue( + workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "podcontroller"), podInformer: podInformer, nodeName: nodeName, podUpdateSubscriber: podUpdateSubscriber, - podCache: cnipodcache.NewCNIPodInfoStore(), ovsBridgeClient: ovsBridgeClient, + interfaceStore: ifaceStore, interfaceConfigurator: interfaceConfigurator, ipamAllocator: ipam.GetSecondaryNetworkAllocator(), } @@ -134,54 +144,19 @@ func podKeyGet(podName, podNamespace string) string { return podNamespace + "/" + podName } -func generatePodSecondaryIfaceName(podCNIInfo *cnipodcache.CNIConfigInfo) (string, error) { - // Assign default interface name, if podCNIInfo.Interfaces is empty. - if len(podCNIInfo.Interfaces) == 0 { - return defaultSecondaryInterfaceName, nil - } else { - // 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.Interfaces[ifName] - if !exist { - return ifName, nil - } +func allocatePodSecondaryIfaceName(usedIFNames sets.Set[string]) (string, error) { + // Generate new interface name (eth1,eth2..eth100) and return to caller. + for ifaceIndex := startIfaceIndex; ifaceIndex < endIfaceIndex; ifaceIndex++ { + ifName := fmt.Sprintf("%s%d", "eth", ifaceIndex) + if !usedIFNames.Has(ifName) { + usedIFNames.Insert(ifName) + return ifName, nil } } return "", fmt.Errorf("no more interface names") } -func (pc *PodController) deletePodSecondaryNetwork(podCNIInfo *cnipodcache.CNIConfigInfo) error { - // Release IPAM allocation for all secondary networks of the Pod which is getting removed. - // NOTE: SR-IOV VF interface clean-up, upon Pod delete will be handled by SR-IOV device - // plugin. Not handled here. - for iface, interfaceInfo := range podCNIInfo.Interfaces { - klog.InfoS("Deleting secondary interface", - "Pod", klog.KRef(podCNIInfo.PodNamespace, podCNIInfo.PodName), "interface", iface) - if interfaceInfo.NetworkType == vlanNetworkType { - if err := pc.interfaceConfigurator.DeleteVLANSecondaryInterface(podCNIInfo.ContainerID, - interfaceInfo.HostInterfaceName, interfaceInfo.OVSPortUUID); err != nil { - return err - } - } - - podOwner := &crdv1a2.PodOwner{ - Name: podCNIInfo.PodName, - Namespace: podCNIInfo.PodNamespace, - ContainerID: podCNIInfo.ContainerID, - IFName: iface, - } - if err := pc.ipamAllocator.SecondaryNetworkRelease(podOwner); err != nil { - return fmt.Errorf("failed to clean up IPAM: %v", err) - } - - // Delete map entry for secNetInstIface, secNetInstConfig - delete(podCNIInfo.Interfaces, iface) - } - return nil -} - -func (pc *PodController) enqueuePod(obj interface{}) { +func (pc *podController) enqueuePod(obj interface{}) { var err error pod, isPod := obj.(*corev1.Pod) if !isPod { @@ -201,32 +176,32 @@ func (pc *PodController) enqueuePod(obj interface{}) { } // processCNIUpdate will be called when CNIServer publishes a Pod update event. -func (pc *PodController) processCNIUpdate(e interface{}) { +func (pc *podController) processCNIUpdate(e interface{}) { event := e.(types.PodUpdate) + podKey := podKeyGet(event.PodName, event.PodNamespace) if event.IsAdd { - // Go cache the CNI server info at CNIConfigInfo cache, for podWatch usage - cniInfo := &cnipodcache.CNIConfigInfo{PodName: event.PodName, PodNamespace: event.PodNamespace, - ContainerID: event.ContainerID, ContainerNetNS: event.NetNS, PodCNIDeleted: false} - pc.podCache.AddCNIConfigInfo(cniInfo) + pc.cniCache.Store(podKey, &podCNIInfo{containerID: event.ContainerID, netNS: event.NetNS}) } else { - containerInfo := pc.podCache.GetCNIConfigInfoByContainerID(event.PodName, event.PodNamespace, event.ContainerID) - if containerInfo != nil { - // Update PodCNIDeleted = true. - // This is to let Podwatch controller know that the CNI server cleaned up this Pod's primary network configuration. - pc.podCache.SetPodCNIDeleted(containerInfo) - } + pc.cniCache.Delete(podKey) } pc.queue.Add(podKeyGet(event.PodName, event.PodNamespace)) } // handleAddUpdatePod handles Pod Add, Update events and updates annotation if required. -func (pc *PodController) handleAddUpdatePod(obj interface{}) error { - var err error - var podCNIInfo *cnipodcache.CNIConfigInfo - pod := obj.(*corev1.Pod) +func (pc *podController) handleAddUpdatePod(pod *corev1.Pod, podCNIInfo *podCNIInfo, + storedInterfaces []*interfacestore.InterfaceConfig) error { + if len(storedInterfaces) > 0 { + // We do not support secondary network update at the moment. Return as long as one + // secondary interface has been created for the Pod. + klog.V(1).InfoS("Secondary network already configured on this Pod and update not supported, skipping update", + "Pod", klog.KObj(pod)) + return nil + } + if len(pod.Status.PodIPs) == 0 { - // Primary network configuration is not complete yet. - // Note: Return nil here to unqueue Pod add event. Secondary network configuration will be handled with Pod update event. + // Primary network configuration is not complete yet. Return nil here to unqueue the + // Pod event. Secondary network configuration will be handled with the following Pod + // update events. return nil } @@ -236,18 +211,6 @@ func (pc *PodController) handleAddUpdatePod(obj interface{}) error { klog.V(2).InfoS("Pod does not have a NetworkAttachmentDefinition", "Pod", klog.KObj(pod)) return nil } - // Retrieve Pod specific cache entry which has "PodCNIDeleted = false" - if podCNIInfo = pc.podCache.GetValidCNIConfigInfoPerPod(pod.Name, pod.Namespace); podCNIInfo == nil { - return nil - } - // Valid cache entry retrieved from cache and we received a Pod add or update event. - // Avoid processing Pod annotation, if we already have at least one secondary network successfully configured on this Pod. - // We do not support/handle Annotation updates yet. - if len(podCNIInfo.Interfaces) > 0 { - klog.V(1).InfoS("Secondary network already configured on this Pod and annotation update not supported, skipping update", "Pod", klog.KObj(pod)) - return nil - } - // Parse Pod annotation and proceed with the secondary network configuration. networklist, err := netdefutils.ParseNetworkAnnotation(secondaryNetwork, pod.Namespace) if err != nil { @@ -257,57 +220,93 @@ func (pc *PodController) handleAddUpdatePod(obj interface{}) error { return nil } - err = pc.configurePodSecondaryNetwork(pod, networklist, podCNIInfo) - if err != nil { - klog.ErrorS(err, "Error when configuring secondary network", "Pod", klog.KObj(pod)) - if len(podCNIInfo.Interfaces) == 0 { - // Return error to requeue and retry. - return err + return pc.configurePodSecondaryNetwork(pod, networklist, podCNIInfo) +} + +func (pc *podController) removeInterfaces(interfaces []*interfacestore.InterfaceConfig) error { + var savedErr error + for _, interfaceConfig := range interfaces { + podName := interfaceConfig.PodName + podNamespace := interfaceConfig.PodNamespace + klog.V(1).InfoS("Deleting secondary interface", + "Pod", klog.KRef(podNamespace, podName), "interface", interfaceConfig.IFDev) + + var err error + // Since only VLAN and SR-IOV interfaces are supported by now, we judge the + // interface type by checking interfaceConfig.OVSPortConfig is set or not. + if interfaceConfig.OVSPortConfig != nil { + err = pc.interfaceConfigurator.DeleteVLANSecondaryInterface(interfaceConfig) + } else { + err = pc.deleteSriovSecondaryInterface(interfaceConfig) + } + if err != nil { + klog.ErrorS(err, "Error when deleting secondary interface", + "Pod", klog.KRef(podNamespace, podName), "interface", interfaceConfig.IFDev) + savedErr = err + continue + } + + podOwner := &crdv1a2.PodOwner{ + Name: interfaceConfig.PodName, + Namespace: interfaceConfig.PodNamespace, + ContainerID: interfaceConfig.ContainerID, + IFName: interfaceConfig.IFDev} + if err = pc.ipamAllocator.SecondaryNetworkRelease(podOwner); err != nil { + klog.ErrorS(err, "Error when releasing IPAM allocation", + "Pod", klog.KRef(podNamespace, podName), "interface", interfaceConfig.IFDev) + savedErr = err } - // We do not return error to retry, if at least one secondary network is configured. } - return nil + return savedErr } -func (pc *PodController) handleRemovePod(key string) error { - var err error - pod := strings.Split(key, "/") - // Read the CNI info (stored during Pod creation by cniserver) from cache. - // Delete CNI info shared in cache for a specific Pod which is getting removed/deleted. - podCNIInfo := pc.podCache.GetAllCNIConfigInfoPerPod(pod[1], pod[0]) - for _, info := range podCNIInfo { - // Delete all secondary interfaces and release IPAM. - if err = pc.deletePodSecondaryNetwork(info); err != nil { - klog.ErrorS(err, "Error when deleting secondary network", "Pod", klog.KRef(info.PodNamespace, info.PodName)) - // Return error to requeue Pod delete. +func (pc *podController) syncPod(key string) error { + var pod *corev1.Pod + var cniInfo *podCNIInfo + podExists := false + + if cniObj, cniAdded := pc.cniCache.Load(key); cniAdded { + podObj, ok, err := pc.podInformer.GetIndexer().GetByKey(key) + if err != nil { return err - } else { - // Delete cache entry from podCNIInfo. - pc.podCache.DeleteCNIConfigInfo(info) - // Delete Pod specific VF cache (if one exists) - pc.deleteVFDeviceIDListPerPod(info.PodName, info.PodNamespace) + } + if ok { + pod = podObj.(*corev1.Pod) + cniInfo = cniObj.(*podCNIInfo) + podExists = true } } - return nil -} -func (pc *PodController) syncPod(key string) error { - obj, exists, err := pc.podInformer.GetIndexer().GetByKey(key) - if err != nil { - return err - } else if exists { - return pc.handleAddUpdatePod(obj) - } else { - return pc.handleRemovePod(key) + namespacePod := strings.Split(key, "/") + podNamespace := namespacePod[0] + podName := namespacePod[1] + storedInterfaces := pc.interfaceStore.GetContainerInterfacesByPod(podName, podNamespace) + if len(storedInterfaces) != 0 { + // Pod or its primary interface has been deleted. Remove secondary interfaces too. + if !podExists || + // Interfaces created for a previous Pod with the same Namespace/name are + // not deleted yet. First delete them before processing the new Pod's + // secondary networks. + storedInterfaces[0].ContainerID != cniInfo.containerID { + if err := pc.removeInterfaces(storedInterfaces); err != nil { + return err + } + } + } + + if !podExists { + pc.deleteVFDeviceIDListPerPod(podName, podNamespace) + return nil } + return pc.handleAddUpdatePod(pod, cniInfo, storedInterfaces) } -func (pc *PodController) Worker() { +func (pc *podController) Worker() { for pc.processNextWorkItem() { } } -func (pc *PodController) processNextWorkItem() bool { +func (pc *podController) processNextWorkItem() bool { obj, quit := pc.queue.Get() if quit { return false @@ -324,33 +323,22 @@ func (pc *PodController) processNextWorkItem() bool { } // Configure Secondary Network Interface. -func (pc *PodController) configureSecondaryInterface( +func (pc *podController) configureSecondaryInterface( pod *corev1.Pod, network *netdefv1.NetworkSelectionElement, - podCNIInfo *cnipodcache.CNIConfigInfo, + podCNIInfo *podCNIInfo, networkConfig *SecondaryNetworkConfig) error { - // Generate a new interface name, if the secondary interface name was not provided in the - // Pod annotation. - if len(network.InterfaceRequest) == 0 { - 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 - } - } - - var result *current.Result - var vlanID uint16 + var ipamResult *ipam.IPAMResult var ifConfigErr error if networkConfig.IPAM != nil { + var err error podOwner := &crdv1a2.PodOwner{ Name: pod.Name, Namespace: pod.Namespace, - ContainerID: podCNIInfo.ContainerID, + ContainerID: podCNIInfo.containerID, IFName: network.InterfaceRequest, } - ipamResult, err := pc.ipamAllocator.SecondaryNetworkAllocate(podOwner, &networkConfig.NetworkConfig) + ipamResult, err = pc.ipamAllocator.SecondaryNetworkAllocate(podOwner, &networkConfig.NetworkConfig) if err != nil { return fmt.Errorf("secondary network IPAM failed: %v", err) } @@ -362,84 +350,98 @@ func (pc *PodController) configureSecondaryInterface( } } }() - result = &ipamResult.Result - for _, ip := range result.IPs { + for _, ip := range ipamResult.IPs { ip.Interface = current.Int(1) } - vlanID = ipamResult.VLANID } else { - result = ¤t.Result{} + ipamResult = &ipam.IPAMResult{} } - var ovsPortUUID string switch networkConfig.NetworkType { case sriovNetworkType: - ifConfigErr = pc.configureSriovAsSecondaryInterface(pod, network, podCNIInfo, int(networkConfig.MTU), result) + ifConfigErr = pc.configureSriovAsSecondaryInterface(pod, network, podCNIInfo, int(networkConfig.MTU), &ipamResult.Result) case vlanNetworkType: if networkConfig.VLAN > 0 { // Let VLAN ID in the CNI network configuration override the IPPool subnet // VLAN. - vlanID = uint16(networkConfig.VLAN) + ipamResult.VLANID = uint16(networkConfig.VLAN) } - ovsPortUUID, ifConfigErr = pc.interfaceConfigurator.ConfigureVLANSecondaryInterface( - podCNIInfo.PodName, podCNIInfo.PodNamespace, - podCNIInfo.ContainerID, podCNIInfo.ContainerNetNS, network.InterfaceRequest, - int(networkConfig.MTU), vlanID, result) - } - if ifConfigErr != nil { - return ifConfigErr + ifConfigErr = pc.interfaceConfigurator.ConfigureVLANSecondaryInterface( + pod.Name, pod.Namespace, + podCNIInfo.containerID, podCNIInfo.netNS, network.InterfaceRequest, + int(networkConfig.MTU), ipamResult) } + return ifConfigErr +} - // Update Pod CNI cache with the network config which was successfully configured. - if podCNIInfo.Interfaces == nil { - podCNIInfo.Interfaces = make(map[string]*cnipodcache.InterfaceInfo) - } - hostInterfaceName := "" - if len(result.Interfaces) > 0 { - // In mock tests, result.Interfaces can be nil - hostInterfaceName = result.Interfaces[0].Name +func (pc *podController) configurePodSecondaryNetwork(pod *corev1.Pod, networkList []*netdefv1.NetworkSelectionElement, podCNIInfo *podCNIInfo) error { + usedIFNames := sets.New[string]() + for _, network := range networkList { + if network.InterfaceRequest != "" { + usedIFNames.Insert(network.InterfaceRequest) + } } - interfaceInfo := cnipodcache.InterfaceInfo{ - NetworkType: networkConfig.NetworkType, - HostInterfaceName: hostInterfaceName, - OVSPortUUID: ovsPortUUID} - podCNIInfo.Interfaces[network.InterfaceRequest] = &interfaceInfo - return nil -} -func (pc *PodController) configurePodSecondaryNetwork(pod *corev1.Pod, networklist []*netdefv1.NetworkSelectionElement, podCNIInfo *cnipodcache.CNIConfigInfo) error { - for _, network := range networklist { + var savedErr error + interfacesConfigured := 0 + for _, network := range networkList { klog.V(2).InfoS("Secondary Network attached to Pod", "network", network, "Pod", klog.KObj(pod)) netDefCRD, err := pc.netAttachDefClient.NetworkAttachmentDefinitions(network.Namespace).Get(context.TODO(), network.Name, metav1.GetOptions{}) if err != nil { - // NetworkAttachmentDefinition not found at this time. Return error to re-queue and re-try. - return fmt.Errorf("failed to get NetworkAttachmentDefinition: %v", err) + klog.ErrorS(err, "Failed to get NetworkAttachmentDefinition", + "network", network, "Pod", klog.KRef(pod.Namespace, pod.Name)) + savedErr = err + continue } + cniConfig, err := netdefutils.GetCNIConfig(netDefCRD, "") if err != nil { - // NetworkAttachmentDefinition Spec.Config parsing failed. Return error to re-queue and re-try. - return fmt.Errorf("NetworkAttachmentDefinition CNI config spec read error: %v", err) + klog.ErrorS(err, "Failed to parse NetworkAttachmentDefinition", + "network", network, "Pod", klog.KRef(pod.Namespace, pod.Name)) + // NetworkAttachmentDefinition Spec.Config parsing failed. Do not retry. + continue } + networkConfig, err := validateNetworkConfig(cniConfig) if err != nil { if networkConfig != nil && networkConfig.Type != cniserver.AntreaCNIType { // Ignore non-Antrea CNI type. klog.InfoS("Not Antrea CNI type in NetworkAttachmentDefinition, ignoring", "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KRef(pod.Namespace, pod.Name)) - continue + } else { + klog.ErrorS(err, "NetworkConfig validation failed", + "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KRef(pod.Namespace, pod.Name)) } - klog.ErrorS(err, "NetworkConfig validation failed", - "NetworkAttachmentDefinition", klog.KObj(netDefCRD), "Pod", klog.KRef(pod.Namespace, pod.Name)) continue } - // secondary network information retrieved from API server. Proceed to configure secondary interface now. + + // Generate a new interface name, if the secondary interface name was not provided in the + // Pod annotation. + if network.InterfaceRequest == "" { + var err error + if network.InterfaceRequest, err = allocatePodSecondaryIfaceName(usedIFNames); err != nil { + klog.ErrorS(err, "Cannot generate interface name", "Pod", klog.KRef(pod.Namespace, pod.Name)) + // Do not return error: no need to requeue. + continue + } + } + + // Secondary network information retrieved from API server. Proceed to configure secondary interface now. if err = pc.configureSecondaryInterface(pod, network, podCNIInfo, networkConfig); err != nil { klog.ErrorS(err, "Secondary interface configuration failed", - "Pod", klog.KRef(pod.Namespace, pod.Name), "networkType", networkConfig.NetworkType) - // Secondary interface configuration failed. return error to re-queue and re-try. - return fmt.Errorf("secondary interface configuration failed: %v", err) + "Pod", klog.KRef(pod.Namespace, pod.Name), "interface", network.InterfaceRequest, + "networkType", networkConfig.NetworkType) + savedErr = err + } else { + interfacesConfigured++ } } + + if savedErr != nil && interfacesConfigured == 0 { + // As we do not support secondary network update, do not return error to + // retry, if at least one secondary network is configured. + return savedErr + } return nil } @@ -479,7 +481,7 @@ func validateNetworkConfig(cniConfig []byte) (*SecondaryNetworkConfig, error) { return &networkConfig, nil } -func (pc *PodController) Run(stopCh <-chan struct{}) { +func (pc *podController) Run(stopCh <-chan struct{}) { defer func() { klog.InfoS("Shutting down", "controller", controllerName) pc.queue.ShutDown() diff --git a/pkg/agent/secondarynetwork/podwatch/controller_test.go b/pkg/agent/secondarynetwork/podwatch/controller_test.go index 63572d4353c..feb1b61154a 100644 --- a/pkg/agent/secondarynetwork/podwatch/controller_test.go +++ b/pkg/agent/secondarynetwork/podwatch/controller_test.go @@ -45,9 +45,10 @@ import ( "k8s.io/client-go/util/workqueue" "antrea.io/antrea/pkg/agent/cniserver/ipam" - "antrea.io/antrea/pkg/agent/cniserver/types" - "antrea.io/antrea/pkg/agent/secondarynetwork/cnipodcache" + cnitypes "antrea.io/antrea/pkg/agent/cniserver/types" + "antrea.io/antrea/pkg/agent/interfacestore" podwatchtesting "antrea.io/antrea/pkg/agent/secondarynetwork/podwatch/testing" + "antrea.io/antrea/pkg/agent/types" crdv1a2 "antrea.io/antrea/pkg/apis/crd/v1alpha2" ) @@ -86,10 +87,9 @@ const ( podIP = "1.2.3.4" networkName = "net" interfaceName = "eth2" - ovsPortUUID = "12345678-e29b-41d4-a716-446655440000" ) -func testNetwork(name string, networkType cnipodcache.NetworkType) *netdefv1.NetworkAttachmentDefinition { +func testNetwork(name string, networkType networkType) *netdefv1.NetworkAttachmentDefinition { return testNetworkExt(name, "", "", string(networkType), "", 0, 0, false) } @@ -134,7 +134,7 @@ func containerNetNs(container string) string { return fmt.Sprintf("/var/run/netns/%s", container) } -func testPod(name string, container string, podIP string, networks ...netdefv1.NetworkSelectionElement) (*corev1.Pod, *cnipodcache.CNIConfigInfo) { +func testPod(name string, container string, podIP string, networks ...netdefv1.NetworkSelectionElement) (*corev1.Pod, *podCNIInfo) { annotations := make(map[string]string) if len(networks) > 0 { annotation, _ := json.Marshal(networks) @@ -167,26 +167,25 @@ func testPod(name string, container string, podIP string, networks ...netdefv1.N }, } } - cniConfig := &cnipodcache.CNIConfigInfo{ - PodName: name, - PodNamespace: testNamespace, - ContainerID: container, - ContainerNetNS: containerNetNs(container), - PodCNIDeleted: false, + cniInfo := &podCNIInfo{ + containerID: container, + netNS: containerNetNs(container), } - return pod, cniConfig + return pod, cniInfo } -func testIPAMResult(cidr string) *ipam.IPAMResult { +func testIPAMResult(cidr string, vlan int) *ipam.IPAMResult { _, ipNet, _ := net.ParseCIDR(cidr) return &ipam.IPAMResult{ Result: current.Result{ IPs: []*current.IPConfig{ { - Address: *ipNet, + Address: *ipNet, + Interface: current.Int(1), }, }, }, + VLANID: uint16(vlan), } } @@ -201,7 +200,6 @@ func TestPodControllerRun(t *testing.T) { client := fake.NewSimpleClientset() netdefclient := netdefclientfake.NewSimpleClientset().K8sCniCncfIoV1() informerFactory := informers.NewSharedInformerFactory(client, resyncPeriod) - podCache := cnipodcache.NewCNIPodInfoStore() interfaceConfigurator := podwatchtesting.NewMockInterfaceConfigurator(ctrl) mockIPAM := podwatchtesting.NewMockIPAMAllocator(ctrl) podController, _ := NewPodController( @@ -210,9 +208,10 @@ func TestPodControllerRun(t *testing.T) { informerFactory.Core().V1().Pods().Informer(), testNode, nil, nil) - podController.podCache = podCache podController.interfaceConfigurator = interfaceConfigurator podController.ipamAllocator = mockIPAM + cniCache := &podController.cniCache + interfaceStore := podController.interfaceStore stopCh := make(chan struct{}) informerFactory.Start(stopCh) @@ -225,12 +224,33 @@ func TestPodControllerRun(t *testing.T) { podController.Run(stopCh) }() - pod, cniConfig := testPod(podName, containerID, podIP, netdefv1.NetworkSelectionElement{ + pod, cniInfo := testPod(podName, containerID, podIP, netdefv1.NetworkSelectionElement{ Name: networkName, InterfaceRequest: interfaceName, }) + podKey := podKeyGet(pod.Name, pod.Namespace) network := testNetwork(networkName, sriovNetworkType) - ipamResult := testIPAMResult("148.14.24.100/24") + ipamResult := testIPAMResult("148.14.24.100/24", 0) + podOwner := &crdv1a2.PodOwner{ + Name: pod.Name, + Namespace: pod.Namespace, + ContainerID: containerID, + IFName: interfaceName} + containerConfig := interfacestore.NewContainerInterface(interfaceName, containerID, + pod.Name, pod.Namespace, interfaceName, nil, nil, 0) + + // CNI Add event. + event := types.PodUpdate{ + IsAdd: true, + PodName: pod.Name, + PodNamespace: pod.Namespace, + ContainerID: containerID, + NetNS: cniInfo.netNS, + } + podController.processCNIUpdate(event) + cniObj, _ := cniCache.Load(podKey) + assert.NotNil(t, cniObj) + assert.Equal(t, cniInfo, cniObj.(*podCNIInfo)) var interfaceConfigured int32 interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( @@ -244,11 +264,11 @@ func TestPodControllerRun(t *testing.T) { &ipamResult.Result, ).Do(func(string, string, string, string, string, int, string, *current.Result) { atomic.AddInt32(&interfaceConfigured, 1) + interfaceStore.AddInterface(containerConfig) }) - mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(ipamResult, nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(ipamResult, nil) - podCache.AddCNIConfigInfo(cniConfig) - // the NetworkAttachmentDefinition must be created before the Pod: if handleAddUpdatePod + // The NetworkAttachmentDefinition must be created before the Pod: if handleAddUpdatePod // runs before the NetworkAttachmentDefinition has been created, it will return an // error. The Pod will then be requeued, but the Poll below will timeout before the Pod has // a chance to be processed again. Rather than increase the timeout or change the queue's @@ -259,21 +279,70 @@ func TestPodControllerRun(t *testing.T) { _, err = client.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{}) require.NoError(t, err, "error when creating test Pod") - // unfortunately, we cannot use the podcache being updated by the controller as a signal - // here: the podcache is not thread-safe and is only meant to be accessed by the controller - // event handlers (with the exception of the operations meant to be performed by the CNI server). + // Wait for ConfigureSriovSecondaryInterface is called. + assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { + return atomic.LoadInt32(&interfaceConfigured) == 1, nil + })) + _, exists := podController.vfDeviceIDUsageMap.Load(podKey) + assert.True(t, exists) + + podController.processCNIUpdate(event) + interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( + podName, + testNamespace, + containerID, + containerNetNs(containerID), + interfaceName, + defaultMTU, + "", + &ipamResult.Result, + ).Do(func(string, string, string, string, string, int, string, *current.Result) { + atomic.AddInt32(&interfaceConfigured, 1) + interfaceStore.AddInterface(containerConfig) + }) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(ipamResult, nil) + + interfaceStore.DeleteInterface(containerConfig) + // Since interface is not saved to the interface store, interface creation should be + // triggered again. + podController.processCNIUpdate(event) assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { - return atomic.LoadInt32(&interfaceConfigured) > 0, nil + return atomic.LoadInt32(&interfaceConfigured) == 2, nil })) - mockIPAM.EXPECT().SecondaryNetworkRelease(gomock.Any()) + interfaceConfigurator.EXPECT().DeleteSriovSecondaryInterface(containerConfig). + Do(func(*interfacestore.InterfaceConfig) { + atomic.AddInt32(&interfaceConfigured, -1) + }) + mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner) + require.NoError(t, client.CoreV1().Pods(testNamespace).Delete(context.Background(), + podName, metav1.DeleteOptions{}), "error when deleting test Pod") + + assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { + return atomic.LoadInt32(&interfaceConfigured) == 1, nil + })) + _, exists = podController.vfDeviceIDUsageMap.Load(podKey) + assert.False(t, exists) - require.NotNil(t, podCache.GetCNIConfigInfoByContainerID(podName, testNamespace, containerID) == nil) - require.NoError(t, client.CoreV1().Pods(testNamespace).Delete(context.Background(), podName, metav1.DeleteOptions{}), "error when deleting test Pod") + interfaceConfigurator.EXPECT().DeleteSriovSecondaryInterface(containerConfig). + Do(func(*interfacestore.InterfaceConfig) { + atomic.AddInt32(&interfaceConfigured, -1) + }) + mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner) + // CNI Del event. + event.IsAdd = false + // Interfac is not deleted from the interface store, so CNI Del should trigger interface + // deletion again. + podController.processCNIUpdate(event) + _, exists = cniCache.Load(podKey) + assert.False(t, exists) assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { - return podCache.GetCNIConfigInfoByContainerID(podName, testNamespace, containerID) == nil, nil + return atomic.LoadInt32(&interfaceConfigured) == 0, nil })) + interfaceStore.DeleteInterface(containerConfig) + podController.processCNIUpdate(event) + close(stopCh) wg.Wait() } @@ -297,24 +366,22 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { name string cniVersion string cniType string - networkType cnipodcache.NetworkType + networkType networkType ipamType string mtu int vlan int noIPAM bool doNotCreateNetwork bool - interfaceCreated bool expectedErr string expectedCalls func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) }{ { - name: "VLAN network", - networkType: vlanNetworkType, - mtu: 1600, - vlan: 101, - interfaceCreated: true, + name: "VLAN network", + networkType: vlanNetworkType, + mtu: 1600, + vlan: 101, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), nil) mockIC.EXPECT().ConfigureVLANSecondaryInterface( podName, testNamespace, @@ -322,17 +389,17 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { containerNetNs(containerID), interfaceName, 1600, - uint16(101), - gomock.Any(), - ).Return(ovsPortUUID, nil) + testIPAMResult("148.14.24.100/24", 101), + ) }, }, { - name: "default MTU", - networkType: vlanNetworkType, - interfaceCreated: true, + name: "VLAN in IPPool", + networkType: vlanNetworkType, + vlan: 0, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + // IPAM returns the VLAN ID in the IPPool subnet. + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 101), nil) mockIC.EXPECT().ConfigureVLANSecondaryInterface( podName, testNamespace, @@ -340,17 +407,16 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { containerNetNs(containerID), interfaceName, 1500, - uint16(0), - gomock.Any(), - ).Return(ovsPortUUID, nil) + testIPAMResult("148.14.24.100/24", 101), + ) }, }, { - name: "no IPAM", - networkType: vlanNetworkType, - noIPAM: true, - interfaceCreated: true, + name: "network VLAN overrides IPPool VLAN", + networkType: vlanNetworkType, + vlan: 101, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 102), nil) mockIC.EXPECT().ConfigureVLANSecondaryInterface( podName, testNamespace, @@ -358,18 +424,32 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { containerNetNs(containerID), interfaceName, 1500, - uint16(0), - gomock.Any(), - ).Return(ovsPortUUID, nil) + testIPAMResult("148.14.24.100/24", 101), + ) }, }, { - name: "SRIOV network", - networkType: sriovNetworkType, - mtu: 1500, - interfaceCreated: true, + name: "no IPAM", + networkType: vlanNetworkType, + noIPAM: true, + expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { + mockIC.EXPECT().ConfigureVLANSecondaryInterface( + podName, + testNamespace, + containerID, + containerNetNs(containerID), + interfaceName, + 1500, + &ipam.IPAMResult{}, + ) + }, + }, + { + name: "SRIOV network", + networkType: sriovNetworkType, + mtu: 1500, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), nil) mockIC.EXPECT().ConfigureSriovSecondaryInterface( podName, testNamespace, @@ -378,8 +458,8 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { interfaceName, 1500, sriovDeviceID, - gomock.Any(), - ).Return(nil) + &testIPAMResult("148.14.24.100/24", 0).Result, + ) }, }, { @@ -388,7 +468,7 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { mtu: 1500, vlan: 100, doNotCreateNetwork: true, - expectedErr: "failed to get NetworkAttachmentDefinition:", + expectedErr: "\"net\" not found", }, { name: "unsupported CNI version", @@ -433,7 +513,7 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { networkType: sriovNetworkType, mtu: 1500, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), errors.New("failure")) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), errors.New("failure")) }, expectedErr: "secondary network IPAM failed", }, @@ -443,7 +523,7 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { mtu: 1600, vlan: 101, expectedCalls: func(mockIPAM *podwatchtesting.MockIPAMAllocator, mockIC *podwatchtesting.MockInterfaceConfigurator) { - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner, gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), nil) mockIC.EXPECT().ConfigureVLANSecondaryInterface( podName, testNamespace, @@ -451,10 +531,9 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { containerNetNs(containerID), interfaceName, 1600, - uint16(101), - gomock.Any(), - ).Return("", errors.New("interface creation failure")) - mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner).Return(nil) + testIPAMResult("148.14.24.100/24", 101), + ).Return(errors.New("interface creation failure")) + mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner) }, expectedErr: "interface creation failure", }, @@ -470,9 +549,8 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { containerNetNs(containerID), interfaceName, 1500, - uint16(0), - gomock.Any(), - ).Return("", errors.New("interface creation failure")) + &ipam.IPAMResult{}, + ).Return(errors.New("interface creation failure")) }, expectedErr: "interface creation failure", }, @@ -480,58 +558,58 @@ func TestConfigurePodSecondaryNetwork(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - pod, cniConfigInfo := testPod(podName, containerID, podIP, element1) - pc, mockIPAM, interfaceConfigurator := testPodController(ctrl) - savedCNIConfig := *cniConfigInfo + pod, cniInfo := testPod(podName, containerID, podIP, element1) + pc, mockIPAM, interfaceConfigurator := testPodControllerStart(ctrl) - network1 := testNetworkExt(networkName, tc.cniVersion, tc.cniType, string(tc.networkType), tc.ipamType, tc.mtu, tc.vlan, tc.noIPAM) if !tc.doNotCreateNetwork { - pc.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), network1, metav1.CreateOptions{}) + network1 := testNetworkExt(networkName, tc.cniVersion, tc.cniType, + string(tc.networkType), tc.ipamType, tc.mtu, tc.vlan, tc.noIPAM) + pc.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), + network1, metav1.CreateOptions{}) } if tc.expectedCalls != nil { tc.expectedCalls(mockIPAM, interfaceConfigurator) } - err := pc.configurePodSecondaryNetwork(pod, []*netdefv1.NetworkSelectionElement{&element1}, cniConfigInfo) + err := pc.configurePodSecondaryNetwork(pod, []*netdefv1.NetworkSelectionElement{&element1}, cniInfo) if tc.expectedErr == "" { assert.Nil(t, err) } else { assert.True(t, strings.Contains(err.Error(), tc.expectedErr)) } - - if tc.interfaceCreated { - info := cnipodcache.InterfaceInfo{ - NetworkType: tc.networkType, - } - if tc.networkType == vlanNetworkType { - info.OVSPortUUID = ovsPortUUID - } - savedCNIConfig.Interfaces = map[string]*cnipodcache.InterfaceInfo{interfaceName: &info} - } - assert.Equal(t, &savedCNIConfig, cniConfigInfo) }) } } func TestPodControllerAddPod(t *testing.T) { - pod, cniConfig := testPod(podName, containerID, podIP, netdefv1.NetworkSelectionElement{ + pod, _ := testPod(podName, containerID, podIP, netdefv1.NetworkSelectionElement{ Name: networkName, InterfaceRequest: interfaceName, }) + podKey := podKeyGet(podName, testNamespace) - t.Run("missing network", func(t *testing.T) { - ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - podController.podCache.AddCNIConfigInfo(cniConfig) - _, err := podController.kubeClient.CoreV1().Pods(testNamespace).Create(context.Background(), pod, metav1.CreateOptions{}) + // Create Pod and wait for Informer cache updated. + createPodFn := func(pc *podController, pod *corev1.Pod) { + _, err := pc.kubeClient.CoreV1().Pods(testNamespace).Create(context.Background(), + pod, metav1.CreateOptions{}) require.NoError(t, err, "error when creating test Pod") - assert.Error(t, podController.handleAddUpdatePod(pod)) - }) + assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { + _, ok, err := pc.podInformer.GetIndexer().GetByKey(podKey) + return ok, err + })) + } + deletePodFn := func(pc *podController, podName string) { + require.NoError(t, pc.kubeClient.CoreV1().Pods(testNamespace).Delete(context.Background(), + podName, metav1.DeleteOptions{}), "error when deleting test Pod") + assert.NoError(t, wait.Poll(10*time.Millisecond, 1*time.Second, func() (bool, error) { + _, ok, err := pc.podInformer.GetIndexer().GetByKey(podKey) + return !ok, err + })) + } t.Run("multiple network interfaces", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, mockIPAM, interfaceConfigurator := testPodController(ctrl) - + podController, mockIPAM, interfaceConfigurator := testPodControllerStart(ctrl) pod, cniConfig := testPod( podName, containerID, @@ -545,27 +623,38 @@ func TestPodControllerAddPod(t *testing.T) { InterfaceRequest: "eth11", }, ) - savedCNIConfig := *cniConfig network1 := testNetwork("net1", sriovNetworkType) testVLAN := 100 network2 := testNetworkExt("net2", "", "", string(vlanNetworkType), "", defaultMTU, testVLAN, false) - podOwner1 := &crdv1a2.PodOwner{ - Name: podName, - Namespace: testNamespace, - ContainerID: containerID, - IFName: "eth10"} - podOwner2 := &crdv1a2.PodOwner{ - Name: podName, - Namespace: testNamespace, - ContainerID: containerID, - IFName: "eth11"} - networkConfig1 := types.NetworkConfig{ + podOwner1 := &crdv1a2.PodOwner{Name: podName, Namespace: testNamespace, + ContainerID: containerID, IFName: "eth10"} + podOwner2 := &crdv1a2.PodOwner{Name: podName, Namespace: testNamespace, + ContainerID: containerID, IFName: "eth11"} + containerConfig1 := interfacestore.NewContainerInterface("interface1", containerID, + pod.Name, pod.Namespace, "eth10", nil, nil, 0) + containerConfig2 := interfacestore.NewContainerInterface("interface2", containerID, + pod.Name, pod.Namespace, "eth11", nil, nil, 0) + // VLAN interface should have OVSPortConfig. + containerConfig2.OVSPortConfig = &interfacestore.OVSPortConfig{} + + staleContainerID := containerID + "-stale" + stalePodOwner1 := &crdv1a2.PodOwner{Name: podName, Namespace: testNamespace, + ContainerID: staleContainerID, IFName: "eth1"} + stalePodOwner2 := &crdv1a2.PodOwner{Name: podName, Namespace: testNamespace, + ContainerID: staleContainerID, IFName: "eth2"} + staleConfig1 := interfacestore.NewContainerInterface("interface1", staleContainerID, + pod.Name, pod.Namespace, "eth1", nil, nil, 0) + staleConfig2 := interfacestore.NewContainerInterface("interface2", staleContainerID, + pod.Name, pod.Namespace, "eth2", nil, nil, 0) + staleConfig1.OVSPortConfig = &interfacestore.OVSPortConfig{} + + networkConfig1 := cnitypes.NetworkConfig{ CNIVersion: "0.3.0", Name: "net1", Type: "antrea", MTU: 1500, - IPAM: &types.IPAMConfig{ + IPAM: &cnitypes.IPAMConfig{ Type: "antrea", IPPools: []string{"ipv4-pool-1", "ipv6-pool-1"}, }, @@ -573,6 +662,20 @@ func TestPodControllerAddPod(t *testing.T) { networkConfig2 := networkConfig1 networkConfig2.Name = "net2" + podController.interfaceStore.AddInterface(staleConfig1) + podController.interfaceStore.AddInterface(staleConfig2) + // Stale interfaces in the interface store should be deleted first. + mockIPAM.EXPECT().SecondaryNetworkRelease(stalePodOwner1) + mockIPAM.EXPECT().SecondaryNetworkRelease(stalePodOwner2) + interfaceConfigurator.EXPECT().DeleteVLANSecondaryInterface(staleConfig1) + interfaceConfigurator.EXPECT().DeleteSriovSecondaryInterface(staleConfig2) + + podController.cniCache.Store(podKey, cniConfig) + createPodFn(podController, pod) + assert.NoError(t, podController.syncPod(podKey)) + podController.interfaceStore.DeleteInterface(staleConfig1) + podController.interfaceStore.DeleteInterface(staleConfig2) + interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( podName, testNamespace, @@ -581,7 +684,7 @@ func TestPodControllerAddPod(t *testing.T) { "eth10", interfaceDefaultMTU, gomock.Any(), - gomock.Any(), + &testIPAMResult("148.14.24.100/24", 0).Result, ) interfaceConfigurator.EXPECT().ConfigureVLANSecondaryInterface( podName, @@ -590,90 +693,69 @@ func TestPodControllerAddPod(t *testing.T) { containerNetNs(containerID), "eth11", defaultMTU, - uint16(testVLAN), - gomock.Any(), - ).Return(ovsPortUUID, nil) - - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner1, &networkConfig1).Return(testIPAMResult("148.14.24.100/24"), nil) - mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner2, &networkConfig2).Return(testIPAMResult("148.14.24.101/24"), nil) + testIPAMResult("148.14.24.101/24", 100), + ) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner1, &networkConfig1).Return(testIPAMResult("148.14.24.100/24", 0), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(podOwner2, &networkConfig2).Return(testIPAMResult("148.14.24.101/24", 0), 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(), network1, metav1.CreateOptions{}) + _, err := podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), + network1, metav1.CreateOptions{}) require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") - _, err = podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), network2, metav1.CreateOptions{}) + _, err = podController.netAttachDefClient.NetworkAttachmentDefinitions(testNamespace).Create(context.Background(), + network2, metav1.CreateOptions{}) require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") - assert.NoError(t, podController.handleAddUpdatePod(pod)) + assert.NoError(t, podController.syncPod(podKey)) - infos := podController.podCache.GetAllCNIConfigInfoPerPod(podName, testNamespace) - assert.Equal(t, 1, len(infos)) - savedCNIConfig.Interfaces = map[string]*cnipodcache.InterfaceInfo{ - "eth10": { - NetworkType: sriovNetworkType, - }, - "eth11": { - OVSPortUUID: ovsPortUUID, - NetworkType: vlanNetworkType, - }, - } - assert.Equal(t, &savedCNIConfig, infos[0]) + podController.interfaceStore.AddInterface(containerConfig1) + podController.interfaceStore.AddInterface(containerConfig2) + mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner1) + mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner2) + interfaceConfigurator.EXPECT().DeleteSriovSecondaryInterface(containerConfig1) + interfaceConfigurator.EXPECT().DeleteVLANSecondaryInterface(containerConfig2) - mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner1).Return(nil) - mockIPAM.EXPECT().SecondaryNetworkRelease(podOwner2).Return(nil) - interfaceConfigurator.EXPECT().DeleteVLANSecondaryInterface( - containerID, - gomock.Any(), - ovsPortUUID).Return(nil) - assert.NoError(t, podController.handleRemovePod(testNamespace+"/"+podName)) - infos = podController.podCache.GetAllCNIConfigInfoPerPod(podName, testNamespace) - assert.Equal(t, 0, len(infos)) + deletePodFn(podController, pod.Name) + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("no network interfaces", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - + podController, _, _ := testPodControllerStart(ctrl) pod, cniConfig := testPod(podName, containerID, podIP) - 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") - assert.NoError(t, podController.handleAddUpdatePod(pod)) + podController.cniCache.Store(podKey, cniConfig) + createPodFn(podController, pod) + assert.NoError(t, podController.syncPod(podKey)) }) - t.Run("missing podcache entry", func(t *testing.T) { + t.Run("missing CNI cache entry", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - + podController, _, _ := testPodControllerStart(ctrl) network := testNetwork(networkName, sriovNetworkType) - _, 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{}) + _, 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)) + createPodFn(podController, pod) + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("missing Status.PodIPs", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - + podController, _, _ := testPodControllerStart(ctrl) pod, cniConfig := testPod(podName, containerID, "") network := testNetwork(networkName, sriovNetworkType) - 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{}) + _, 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)) + createPodFn(podController, pod) + podController.cniCache.Store(podKey, cniConfig) + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("different Namespace for Pod and NetworkAttachmentDefinition", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, mockIPAM, interfaceConfigurator := testPodController(ctrl) - + podController, mockIPAM, interfaceConfigurator := testPodControllerStart(ctrl) networkNamespace := "nsB" network := testNetwork(networkName, sriovNetworkType) network.Namespace = networkNamespace @@ -694,20 +776,19 @@ func TestPodControllerAddPod(t *testing.T) { sriovDeviceID, gomock.Any(), ) - mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), 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(networkNamespace).Create(context.Background(), network, metav1.CreateOptions{}) + _, err := podController.netAttachDefClient.NetworkAttachmentDefinitions(networkNamespace).Create(context.Background(), + network, metav1.CreateOptions{}) require.NoError(t, err, "error when creating test NetworkAttachmentDefinition") - assert.NoError(t, podController.handleAddUpdatePod(pod)) + createPodFn(podController, pod) + podController.cniCache.Store(podKey, cniConfig) + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("no interface name", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, mockIPAM, interfaceConfigurator := testPodController(ctrl) - + podController, mockIPAM, interfaceConfigurator := testPodControllerStart(ctrl) pod, cniConfig := testPod( podName, containerID, @@ -744,49 +825,20 @@ func TestPodControllerAddPod(t *testing.T) { gomock.Any(), ) - mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) - mockIPAM.EXPECT().SecondaryNetworkAllocate(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) - podController, mockIPAM, interfaceConfigurator := testPodController(ctrl) - - network := testNetwork(networkName, sriovNetworkType) - - interfaceConfigurator.EXPECT().ConfigureSriovSecondaryInterface( - podName, - testNamespace, - containerID, - containerNetNs(containerID), - interfaceName, - defaultMTU, - gomock.Any(), - gomock.Any(), - ).Return(fmt.Errorf("error when creating interface")) - - mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24"), nil) - mockIPAM.EXPECT().SecondaryNetworkRelease(gomock.Any()) + mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.100/24", 0), nil) + mockIPAM.EXPECT().SecondaryNetworkAllocate(gomock.Any(), gomock.Any()).Return(testIPAMResult("148.14.24.101/24", 0), 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{}) + _, 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)) + createPodFn(podController, pod) + podController.cniCache.Store(podKey, cniConfig) + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("invalid CNI config", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - + podController, _, _ := testPodControllerStart(ctrl) pod, cniConfig := testPod( podName, containerID, @@ -805,69 +857,82 @@ func TestPodControllerAddPod(t *testing.T) { }, } - 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{}) + _, 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.Error(t, podController.handleAddUpdatePod(pod)) + createPodFn(podController, pod) + podController.cniCache.Store(podKey, cniConfig) + // We don't expect an error here, no requeueing. + assert.NoError(t, podController.syncPod(podKey)) }) t.Run("invalid networks annotation", func(t *testing.T) { ctrl := gomock.NewController(t) - podController, _, _ := testPodController(ctrl) - + podController, _, _ := testPodControllerStart(ctrl) pod, cniConfig := testPod(podName, containerID, podIP) pod.Annotations = map[string]string{ networkAttachDefAnnotationKey: "", } network := testNetwork(networkName, sriovNetworkType) - 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{}) + _, 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)) + createPodFn(podController, pod) + podController.cniCache.Store(podKey, cniConfig) + // We don't expect an error here, no requeueing. + assert.NoError(t, podController.syncPod(podKey)) }) - t.Run("Error when adding VF deviceID cache per Pod", func(t *testing.T) { + t.Run("updating deviceID cache per Pod", func(t *testing.T) { ctrl := gomock.NewController(t) - network := testNetwork(networkName, sriovNetworkType) podController, _, _ := testPodController(ctrl) - 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") - - _, err = podController.assignUnusedSriovVFDeviceIDPerPod(podName, testNamespace, interfaceName) + _, err := podController.assignUnusedSriovVFDeviceID(podName, testNamespace, interfaceName) + _, exists := podController.vfDeviceIDUsageMap.Load(podKey) + assert.True(t, exists) require.NoError(t, err, "error while assigning unused VfDevice ID") - + podController.releaseSriovVFDeviceID(podName, testNamespace, interfaceName) + _, exists = podController.vfDeviceIDUsageMap.Load(podKey) + assert.True(t, exists) podController.deleteVFDeviceIDListPerPod(podName, testNamespace) - require.NoError(t, err, "error deleting cache") - + _, exists = podController.vfDeviceIDUsageMap.Load(podKey) + assert.False(t, exists) }) } -func testPodController(ctrl *gomock.Controller) (*PodController, *podwatchtesting.MockIPAMAllocator, *podwatchtesting.MockInterfaceConfigurator) { +func testPodController(ctrl *gomock.Controller) ( + *podController, *podwatchtesting.MockIPAMAllocator, + *podwatchtesting.MockInterfaceConfigurator) { client := fake.NewSimpleClientset() netdefclient := netdefclientfake.NewSimpleClientset().K8sCniCncfIoV1() informerFactory := informers.NewSharedInformerFactory(client, resyncPeriod) - podCache := cnipodcache.NewCNIPodInfoStore() interfaceConfigurator := podwatchtesting.NewMockInterfaceConfigurator(ctrl) mockIPAM := podwatchtesting.NewMockIPAMAllocator(ctrl) - // PodController object without event handlers - return &PodController{ - kubeClient: client, - netAttachDefClient: netdefclient, - queue: workqueue.NewNamedRateLimitingQueue(workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), "podcontroller"), + + // podController without event handlers. + return &podController{ + kubeClient: client, + netAttachDefClient: netdefclient, + queue: workqueue.NewNamedRateLimitingQueue( + workqueue.NewItemExponentialFailureRateLimiter(minRetryDelay, maxRetryDelay), + "podcontroller"), podInformer: informerFactory.Core().V1().Pods().Informer(), nodeName: testNode, - podCache: podCache, interfaceConfigurator: interfaceConfigurator, ipamAllocator: mockIPAM, + interfaceStore: interfacestore.NewInterfaceStore(), }, mockIPAM, interfaceConfigurator } + +// Create a test podController and start informerFactory. +func testPodControllerStart(ctrl *gomock.Controller) ( + *podController, *podwatchtesting.MockIPAMAllocator, + *podwatchtesting.MockInterfaceConfigurator) { + podController, mockIPAM, interfaceConfigurator := testPodController(ctrl) + informerFactory := informers.NewSharedInformerFactory(podController.kubeClient, resyncPeriod) + podController.podInformer = informerFactory.Core().V1().Pods().Informer() + stopCh := make(chan struct{}) + informerFactory.Start(stopCh) + informerFactory.WaitForCacheSync(stopCh) + return podController, mockIPAM, interfaceConfigurator +} diff --git a/pkg/agent/secondarynetwork/podwatch/sriov.go b/pkg/agent/secondarynetwork/podwatch/sriov.go index ba7c3490218..8e63551a973 100644 --- a/pkg/agent/secondarynetwork/podwatch/sriov.go +++ b/pkg/agent/secondarynetwork/podwatch/sriov.go @@ -32,7 +32,7 @@ import ( grpcinsecure "google.golang.org/grpc/credentials/insecure" podresourcesv1alpha1 "k8s.io/kubelet/pkg/apis/podresources/v1alpha1" - cnipodcache "antrea.io/antrea/pkg/agent/secondarynetwork/cnipodcache" + "antrea.io/antrea/pkg/agent/interfacestore" ) const ( @@ -47,7 +47,7 @@ var ( getPodContainerDeviceIDsFn = getPodContainerDeviceIDs ) -type KubeletPodResources struct { +type kubeletPodResources struct { resources []*podresourcesv1alpha1.PodResources } @@ -73,7 +73,6 @@ func getPodContainerDeviceIDs(podName string, podNamespace string) ([]string, er if err != nil { return []string{}, fmt.Errorf("error getting the gRPC client for Pod resources: %v", err) } - defer conn.Close() client := podresourcesv1alpha1.NewPodResourcesListerClient(conn) @@ -87,7 +86,7 @@ func getPodContainerDeviceIDs(podName string, podNamespace string) ([]string, er } var podDeviceIDs []string - var kpr KubeletPodResources + var kpr kubeletPodResources kpr.resources = podResources.GetPodResources() for _, pr := range kpr.resources { if pr.Name == podName && pr.Namespace == podNamespace { @@ -109,8 +108,8 @@ func getPodContainerDeviceIDs(podName string, podNamespace string) ([]string, er // which is still not associated with a network device name. // NOTE: buildVFDeviceIDListPerPod is called only if a Pod specific VF to Interface mapping cache // was not build earlier. Sample initial entry per Pod: "{18:01.1,""},{18:01.2,""},{18:01.3,""}" -func (pc *PodController) buildVFDeviceIDListPerPod(podName, podNamespace string) ([]podSriovVFDeviceIDInfo, error) { - podKey := podNamespace + "/" + podName +func (pc *podController) buildVFDeviceIDListPerPod(podName, podNamespace string) ([]podSriovVFDeviceIDInfo, error) { + podKey := podKeyGet(podName, podNamespace) deviceCache, cacheFound := pc.vfDeviceIDUsageMap.Load(podKey) if cacheFound { return deviceCache.([]podSriovVFDeviceIDInfo), nil @@ -129,8 +128,8 @@ func (pc *PodController) buildVFDeviceIDListPerPod(podName, podNamespace string) return vfDeviceIDInfoCache, nil } -func (pc *PodController) deleteVFDeviceIDListPerPod(podName, podNamespace string) { - podKey := podNamespace + "/" + podName +func (pc *podController) deleteVFDeviceIDListPerPod(podName, podNamespace string) { + podKey := podKeyGet(podName, podNamespace) _, cacheFound := pc.vfDeviceIDUsageMap.Load(podKey) if cacheFound { pc.vfDeviceIDUsageMap.Delete(podKey) @@ -139,7 +138,21 @@ func (pc *PodController) deleteVFDeviceIDListPerPod(podName, podNamespace string return } -func (pc *PodController) assignUnusedSriovVFDeviceIDPerPod(podName, podNamespace, interfaceName string) (string, error) { +func (pc *podController) releaseSriovVFDeviceID(podName, podNamespace, interfaceName string) { + podKey := podKeyGet(podName, podNamespace) + obj, cacheFound := pc.vfDeviceIDUsageMap.Load(podKey) + if !cacheFound { + return + } + cache := obj.([]podSriovVFDeviceIDInfo) + for idx := 0; idx < len(cache); idx++ { + if cache[idx].ifName == interfaceName { + cache[idx].ifName = "" + } + } +} + +func (pc *podController) assignUnusedSriovVFDeviceID(podName, podNamespace, interfaceName string) (string, error) { var cache []podSriovVFDeviceIDInfo cache, err := pc.buildVFDeviceIDListPerPod(podName, podNamespace) if err != nil { @@ -156,17 +169,25 @@ func (pc *PodController) assignUnusedSriovVFDeviceIDPerPod(podName, podNamespace } // Configure SRIOV VF as a Secondary Network Interface. -func (pc *PodController) configureSriovAsSecondaryInterface(pod *corev1.Pod, network *netdefv1.NetworkSelectionElement, containerInfo *cnipodcache.CNIConfigInfo, mtu int, result *current.Result) error { - podSriovVFDeviceID, err := pc.assignUnusedSriovVFDeviceIDPerPod(pod.Name, pod.Namespace, network.InterfaceRequest) +func (pc *podController) configureSriovAsSecondaryInterface(pod *corev1.Pod, network *netdefv1.NetworkSelectionElement, podCNIInfo *podCNIInfo, mtu int, result *current.Result) error { + podSriovVFDeviceID, err := pc.assignUnusedSriovVFDeviceID(pod.Name, pod.Namespace, network.InterfaceRequest) if err != nil { return err } - if err = pc.interfaceConfigurator.ConfigureSriovSecondaryInterface( - containerInfo.PodName, containerInfo.PodNamespace, containerInfo.ContainerID, - containerInfo.ContainerNetNS, network.InterfaceRequest, - mtu, podSriovVFDeviceID, result); err != nil { + pod.Name, pod.Namespace, podCNIInfo.containerID, podCNIInfo.netNS, + network.InterfaceRequest, mtu, podSriovVFDeviceID, result); err != nil { return fmt.Errorf("SRIOV Interface creation failed: %v", err) } return nil } + +func (pc *podController) deleteSriovSecondaryInterface(interfaceConfig *interfacestore.InterfaceConfig) error { + // NOTE: SR-IOV VF interface clean-up will be handled by SR-IOV device plugin. The interface + // is not deleted here. + if err := pc.interfaceConfigurator.DeleteSriovSecondaryInterface(interfaceConfig); err != nil { + return err + } + pc.releaseSriovVFDeviceID(interfaceConfig.PodName, interfaceConfig.PodNamespace, interfaceConfig.IFDev) + return nil +} diff --git a/pkg/agent/secondarynetwork/podwatch/testing/mock_podwatch.go b/pkg/agent/secondarynetwork/podwatch/testing/mock_podwatch.go index 149efe2dff6..b831261bc4f 100644 --- a/pkg/agent/secondarynetwork/podwatch/testing/mock_podwatch.go +++ b/pkg/agent/secondarynetwork/podwatch/testing/mock_podwatch.go @@ -1,4 +1,4 @@ -// Copyright 2023 Antrea Authors +// Copyright 2024 Antrea Authors // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -28,6 +28,7 @@ import ( ipam "antrea.io/antrea/pkg/agent/cniserver/ipam" types "antrea.io/antrea/pkg/agent/cniserver/types" + interfacestore "antrea.io/antrea/pkg/agent/interfacestore" v1alpha2 "antrea.io/antrea/pkg/apis/crd/v1alpha2" types100 "github.com/containernetworking/cni/pkg/types/100" gomock "go.uber.org/mock/gomock" @@ -71,32 +72,45 @@ func (mr *MockInterfaceConfiguratorMockRecorder) ConfigureSriovSecondaryInterfac } // ConfigureVLANSecondaryInterface mocks base method. -func (m *MockInterfaceConfigurator) ConfigureVLANSecondaryInterface(arg0, arg1, arg2, arg3, arg4 string, arg5 int, arg6 uint16, arg7 *types100.Result) (string, error) { +func (m *MockInterfaceConfigurator) ConfigureVLANSecondaryInterface(arg0, arg1, arg2, arg3, arg4 string, arg5 int, arg6 *ipam.IPAMResult) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "ConfigureVLANSecondaryInterface", arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7) - ret0, _ := ret[0].(string) - ret1, _ := ret[1].(error) - return ret0, ret1 + ret := m.ctrl.Call(m, "ConfigureVLANSecondaryInterface", arg0, arg1, arg2, arg3, arg4, arg5, arg6) + ret0, _ := ret[0].(error) + return ret0 } // ConfigureVLANSecondaryInterface indicates an expected call of ConfigureVLANSecondaryInterface. -func (mr *MockInterfaceConfiguratorMockRecorder) ConfigureVLANSecondaryInterface(arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7 any) *gomock.Call { +func (mr *MockInterfaceConfiguratorMockRecorder) ConfigureVLANSecondaryInterface(arg0, arg1, arg2, arg3, arg4, arg5, arg6 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConfigureVLANSecondaryInterface", reflect.TypeOf((*MockInterfaceConfigurator)(nil).ConfigureVLANSecondaryInterface), arg0, arg1, arg2, arg3, arg4, arg5, arg6) +} + +// DeleteSriovSecondaryInterface mocks base method. +func (m *MockInterfaceConfigurator) DeleteSriovSecondaryInterface(arg0 *interfacestore.InterfaceConfig) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "DeleteSriovSecondaryInterface", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// DeleteSriovSecondaryInterface indicates an expected call of DeleteSriovSecondaryInterface. +func (mr *MockInterfaceConfiguratorMockRecorder) DeleteSriovSecondaryInterface(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ConfigureVLANSecondaryInterface", reflect.TypeOf((*MockInterfaceConfigurator)(nil).ConfigureVLANSecondaryInterface), arg0, arg1, arg2, arg3, arg4, arg5, arg6, arg7) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteSriovSecondaryInterface", reflect.TypeOf((*MockInterfaceConfigurator)(nil).DeleteSriovSecondaryInterface), arg0) } // DeleteVLANSecondaryInterface mocks base method. -func (m *MockInterfaceConfigurator) DeleteVLANSecondaryInterface(arg0, arg1, arg2 string) error { +func (m *MockInterfaceConfigurator) DeleteVLANSecondaryInterface(arg0 *interfacestore.InterfaceConfig) error { m.ctrl.T.Helper() - ret := m.ctrl.Call(m, "DeleteVLANSecondaryInterface", arg0, arg1, arg2) + ret := m.ctrl.Call(m, "DeleteVLANSecondaryInterface", arg0) ret0, _ := ret[0].(error) return ret0 } // DeleteVLANSecondaryInterface indicates an expected call of DeleteVLANSecondaryInterface. -func (mr *MockInterfaceConfiguratorMockRecorder) DeleteVLANSecondaryInterface(arg0, arg1, arg2 any) *gomock.Call { +func (mr *MockInterfaceConfiguratorMockRecorder) DeleteVLANSecondaryInterface(arg0 any) *gomock.Call { mr.mock.ctrl.T.Helper() - return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteVLANSecondaryInterface", reflect.TypeOf((*MockInterfaceConfigurator)(nil).DeleteVLANSecondaryInterface), arg0, arg1, arg2) + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "DeleteVLANSecondaryInterface", reflect.TypeOf((*MockInterfaceConfigurator)(nil).DeleteVLANSecondaryInterface), arg0) } // MockIPAMAllocator is a mock of IPAMAllocator interface. diff --git a/pkg/agent/secondarynetwork/podwatch/types.go b/pkg/agent/secondarynetwork/podwatch/types.go index 329c6706e3e..3be1c226308 100644 --- a/pkg/agent/secondarynetwork/podwatch/types.go +++ b/pkg/agent/secondarynetwork/podwatch/types.go @@ -16,21 +16,18 @@ package podwatch import ( "antrea.io/antrea/pkg/agent/cniserver/types" - "antrea.io/antrea/pkg/agent/secondarynetwork/cnipodcache" ) -type RouteInfo struct { - Dst string `json:"dst,omitempty"` -} +type networkType string const ( - sriovNetworkType cnipodcache.NetworkType = "sriov" - vlanNetworkType cnipodcache.NetworkType = "vlan" + sriovNetworkType networkType = "sriov" + vlanNetworkType networkType = "vlan" ) type SecondaryNetworkConfig struct { types.NetworkConfig - NetworkType cnipodcache.NetworkType `json:"networkType,omitempty"` + NetworkType networkType `json:"networkType,omitempty"` // VLAN ID of the OVS port. Applicable only to the VLAN network type. If a // non-zero VLAN is specified, it will override the VLAN in the Antrea // IPAM IPPool subnet. diff --git a/pkg/agent/util/net.go b/pkg/agent/util/net.go index 099242d3cfa..c830c0a4aa1 100644 --- a/pkg/agent/util/net.go +++ b/pkg/agent/util/net.go @@ -72,15 +72,15 @@ func generateInterfaceName(key string, name string, useHead bool) string { } // GenerateContainerInterfaceKey generates a unique string for a Pod's -// interface as: container/. +// interface as: "c//". // We must use ContainerID instead of PodNamespace + PodName because there could // be more than one container associated with the same Pod at some point. // For example, when deleting a StatefulSet Pod with 0 second grace period, the // Pod will be removed from the Kubernetes API very quickly and a new Pod will // be created immediately, and kubelet may process the deletion of the previous // Pod and the addition of the new Pod simultaneously. -func GenerateContainerInterfaceKey(containerID string) string { - return fmt.Sprintf("container/%s", containerID) +func GenerateContainerInterfaceKey(containerID, ifDev string) string { + return fmt.Sprintf("c/%s/%s", containerID, ifDev) } // GenerateNodeTunnelInterfaceKey generates a unique string for a Node's