diff --git a/cni/network/network.go b/cni/network/network.go index 00a569bca9..111503be30 100644 --- a/cni/network/network.go +++ b/cni/network/network.go @@ -941,77 +941,89 @@ func (plugin *NetPlugin) Delete(args *cniSkel.CmdArgs) error { } } - // Initialize values from network config. - networkID, err = plugin.getNetworkName(args.Netns, nil, nwCfg) - if err != nil { - log.Printf("[cni-net] Failed to extract network name from network config. error: %v", err) - // If error is not found error, then we ignore it, to comply with CNI SPEC. - if !network.IsNetworkNotFoundError(err) { + // Loop through all the networks that are created for the given Netns. In case of multi-nic scenario ( currently supported + // scenario is dual-nic ), single container may have endpoints created in multiple networks. As all the endpoints are + // deleted, getNetworkName will return error of the type NetworkNotFoundError which will result in nil error as compliance + // with CNI SPEC as mentioned below. + + numEndpointsToDelete := 1 + if nwCfg.MultiTenancy { + numEndpointsToDelete = plugin.nm.GetNumEndpointsInNetNs(args.Netns) + } + + log.Printf("[cni-net] number of endpoints to be deleted %d", numEndpointsToDelete) + for i := 0; i < numEndpointsToDelete; i++ { + // Initialize values from network config. + networkID, err = plugin.getNetworkName(args.Netns, nil, nwCfg) + if err != nil { + // If error is not found error, then we ignore it, to comply with CNI SPEC. + if network.IsNetworkNotFoundError(err) { + err = nil + return err + } + + log.Printf("[cni-net] Failed to extract network name from network config. error: %v", err) err = plugin.Errorf("Failed to extract network name from network config. error: %v", err) return err } - } + // Query the network. + if nwInfo, err = plugin.nm.GetNetworkInfo(networkID); err != nil { + if !nwCfg.MultiTenancy { + log.Printf("[cni-net] Failed to query network:%s: %v", networkID, err) + // Log the error but return success if the network is not found. + // if cni hits this, mostly state file would be missing and it can be reboot scenario where + // container runtime tries to delete and create pods which existed before reboot. + err = nil + return err + } + } - // Query the network. - if nwInfo, err = plugin.nm.GetNetworkInfo(networkID); err != nil { - if !nwCfg.MultiTenancy { - log.Printf("[cni-net] Failed to query network:%s: %v", networkID, err) - // Log the error but return success if the network is not found. - // if cni hits this, mostly state file would be missing and it can be reboot scenario where - // container runtime tries to delete and create pods which existed before reboot. + endpointID := GetEndpointID(args) + // Query the endpoint. + if epInfo, err = plugin.nm.GetEndpointInfo(networkID, endpointID); err != nil { + if !nwCfg.MultiTenancy { + // attempt to release address associated with this Endpoint id + // This is to ensure clean up is done even in failure cases + log.Printf("[cni-net] Failed to query endpoint %s: %v", endpointID, err) + logAndSendEvent(plugin, fmt.Sprintf("Release ip by ContainerID (endpoint not found):%v", args.ContainerID)) + if err = plugin.ipamInvoker.Delete(nil, nwCfg, args, nwInfo.Options); err != nil { + return plugin.RetriableError(fmt.Errorf("failed to release address(no endpoint): %w", err)) + } + } + // Log the error but return success if the endpoint being deleted is not found. err = nil return err } - } - endpointID := GetEndpointID(args) - // Query the endpoint. - if epInfo, err = plugin.nm.GetEndpointInfo(networkID, endpointID); err != nil { + // schedule send metric before attempting delete + defer sendMetricFunc() //nolint:gocritic + logAndSendEvent(plugin, fmt.Sprintf("Deleting endpoint:%v", endpointID)) + // Delete the endpoint. + if err = plugin.nm.DeleteEndpoint(networkID, endpointID); err != nil { + // return a retriable error so the container runtime will retry this DEL later + // the implementation of this function returns nil if the endpoint doens't exist, so + // we don't have to check that here + return plugin.RetriableError(fmt.Errorf("failed to delete endpoint: %w", err)) + } if !nwCfg.MultiTenancy { - // attempt to release address associated with this Endpoint id - // This is to ensure clean up is done even in failure cases - log.Printf("[cni-net] Failed to query endpoint %s: %v", endpointID, err) - logAndSendEvent(plugin, fmt.Sprintf("Release ip by ContainerID (endpoint not found):%v", args.ContainerID)) - if err = plugin.ipamInvoker.Delete(nil, nwCfg, args, nwInfo.Options); err != nil { - return plugin.RetriableError(fmt.Errorf("failed to release address(no endpoint): %w", err)) + // Call into IPAM plugin to release the endpoint's addresses. + for i := range epInfo.IPAddresses { + logAndSendEvent(plugin, fmt.Sprintf("Release ip:%s", epInfo.IPAddresses[i].IP.String())) + err = plugin.ipamInvoker.Delete(&epInfo.IPAddresses[i], nwCfg, args, nwInfo.Options) + if err != nil { + return plugin.RetriableError(fmt.Errorf("failed to release address: %w", err)) + } } - } - - // Log the error but return success if the endpoint being deleted is not found. - err = nil - return err - } - - // schedule send metric before attempting delete - defer sendMetricFunc() - logAndSendEvent(plugin, fmt.Sprintf("Deleting endpoint:%v", endpointID)) - // Delete the endpoint. - if err = plugin.nm.DeleteEndpoint(networkID, endpointID); err != nil { - // return a retriable error so the container runtime will retry this DEL later - // the implementation of this function returns nil if the endpoint doens't exist, so - // we don't have to check that here - return plugin.RetriableError(fmt.Errorf("failed to delete endpoint: %w", err)) - } - - if !nwCfg.MultiTenancy { - // Call into IPAM plugin to release the endpoint's addresses. - for _, address := range epInfo.IPAddresses { - logAndSendEvent(plugin, fmt.Sprintf("Release ip:%s", address.IP.String())) - err = plugin.ipamInvoker.Delete(&address, nwCfg, args, nwInfo.Options) + } else if epInfo.EnableInfraVnet { + nwCfg.IPAM.Subnet = nwInfo.Subnets[0].Prefix.String() + nwCfg.IPAM.Address = epInfo.InfraVnetIP.IP.String() + err = plugin.ipamInvoker.Delete(nil, nwCfg, args, nwInfo.Options) if err != nil { return plugin.RetriableError(fmt.Errorf("failed to release address: %w", err)) } } - } else if epInfo.EnableInfraVnet { - nwCfg.IPAM.Subnet = nwInfo.Subnets[0].Prefix.String() - nwCfg.IPAM.Address = epInfo.InfraVnetIP.IP.String() - err = plugin.ipamInvoker.Delete(nil, nwCfg, args, nwInfo.Options) - if err != nil { - return plugin.RetriableError(fmt.Errorf("failed to release address: %w", err)) - } } - sendEvent(plugin, fmt.Sprintf("CNI DEL succeeded : Released ip %+v podname %v namespace %v", nwCfg.IPAM.Address, k8sPodName, k8sNamespace)) return err diff --git a/network/manager.go b/network/manager.go index 99042ed18c..0fbeb2d0a9 100644 --- a/network/manager.go +++ b/network/manager.go @@ -76,6 +76,7 @@ type NetworkManager interface { GetNetworkInfo(networkID string) (NetworkInfo, error) // FindNetworkIDFromNetNs returns the network name that contains an endpoint created for this netNS, errNetworkNotFound if no network is found FindNetworkIDFromNetNs(netNs string) (string, error) + GetNumEndpointsInNetNs(netNs string) int CreateEndpoint(client apipaClient, networkID string, epInfo *EndpointInfo) error DeleteEndpoint(networkID string, endpointID string) error diff --git a/network/manager_mock.go b/network/manager_mock.go index 83d79b37b1..524a64672e 100644 --- a/network/manager_mock.go +++ b/network/manager_mock.go @@ -114,3 +114,18 @@ func (nm *MockNetworkManager) FindNetworkIDFromNetNs(netNs string) (string, erro return "", errNetworkNotFound } + +// GetNumEndpointsInNetNs mock +func (nm *MockNetworkManager) GetNumEndpointsInNetNs(netNs string) int { + // based on the GetAllEndpoints func above, it seems that this mock is only intended to be used with + // one network, so just return the number of endpoints if network exists + numEndpoints := 0 + + for _, network := range nm.TestNetworkInfoMap { + if _, err := nm.GetAllEndpoints(network.Id); err == nil { + numEndpoints++ + } + } + + return numEndpoints +} diff --git a/network/network.go b/network/network.go index bdcb80612b..9a9dfac69a 100644 --- a/network/network.go +++ b/network/network.go @@ -274,3 +274,24 @@ func (nm *networkManager) FindNetworkIDFromNetNs(netNs string) (string, error) { return "", errNetworkNotFound } + +// GetNumEndpointsInNetNs returns number of endpoints +func (nm *networkManager) GetNumEndpointsInNetNs(netNs string) int { + numEndpoints := 0 + // Look through the external interfaces + for _, iface := range nm.ExternalInterfaces { + // Look through the networks + for _, network := range iface.Networks { + // Network may have multiple endpoints, so look through all of them + for _, endpoint := range network.Endpoints { + // If the netNs matches for this endpoint, return the network ID (which is the name) + if endpoint.NetNs == netNs { + log.Printf("Found endpoint [%s] for NetNS [%s]", endpoint.Id, netNs) + numEndpoints++ + } + } + } + } + + return numEndpoints +} diff --git a/network/network_test.go b/network/network_test.go index 5fdb4c14b2..c804aa2f2f 100644 --- a/network/network_test.go +++ b/network/network_test.go @@ -257,4 +257,68 @@ var _ = Describe("Test Network", func() { }) }) }) + + Describe("Test GetNumEndpointsInNetNs", func() { + Context("When one network has one endpoint and another network has two endpoints", func() { + It("Should return three endpoints", func() { + netNs := "989c079b-45a6-485f-8f9e-88b05d6c55c5" + networkOneID := "byovnetbridge-vlan1-10-128-8-0_23" + networkTwoID := "byovnetbridge-vlan2-20-128-8-0_23" + + nm := &networkManager{ + ExternalInterfaces: map[string]*externalInterface{ + networkOneID: { + Name: networkOneID, + Networks: map[string]*network{ + "byovnetbridge-vlan1-10-128-8-0_23": { + Id: "byovnetbridge-vlan1-10-128-8-0_23", + Endpoints: map[string]*endpoint{ + "a591be2a-eth0": { + Id: "a591be2a-eth0", + NetNs: netNs, + }, + "a691be2b-eth0": { + Id: "a691be2b-eth0", + NetNs: netNs, + }, + }, + NetNs: "aaac079b-45a6-485f-8f9e-88b05d6c55c5", + }, + }, + }, + networkTwoID: { + Name: networkTwoID, + Networks: map[string]*network{ + "byovnetbridge-vlan2-20-128-8-0_23": { + Id: "byovnetbridge-vlan2-20-128-8-0_23", + Endpoints: map[string]*endpoint{ + "a591be2b-eth0": { + Id: "a591be2b-eth0", + NetNs: netNs, + }, + }, + NetNs: "aaac079b-45a6-485f-8f9e-88b05d6c55c5", + }, + }, + }, + }, + } + + got := nm.GetNumEndpointsInNetNs(netNs) + Expect(got).To(Equal(3)) + }) + }) + + Context("When network does not exist", func() { + It("Should return zero endpoints", func() { + netNs := "989c079b-45a6-485f-8f9e-88b05d6c55c9" + nm := &networkManager{ + ExternalInterfaces: make(map[string]*externalInterface), + } + + got := nm.GetNumEndpointsInNetNs(netNs) + Expect(got).To(Equal(0)) + }) + }) + }) })