fix: fixing Stateless CNI delete in SwiftV2 scenario #3967

behzad-mir · 2025-08-27T07:19:37Z

A number of changes is made to stateless CNI to fully support SWiftV2 in Linux:

For Stateless CNI, the EndpointID should include ifName when the NicType is Delegated. This ensures it can be distinguished from the InfraNIC endpoint. The reason for this behavior is that Stateless CNI uses only the ContainerID as the endpoint ID when saving the state file in CNS, to maintain consistency with Cilium. However, within CNI’s in-memory representation, the EndpointID must uniquely identify each NIC. As a result:
- We always pass the ContainerID to CNS as the endpoint state key.
- Internally, CNI uses EndpointID (which includes ifName for delegated NICs) to differentiate endpoints.
Delete flow has been revised and NetNSPath has been added to the statefile since it is needed by the TransparentClient for Frontend NIC.
The Transparent mode used by statefull CNI for SWiftV1 and V2 and stateless CNI should follow the same. TransparentVlan which is the original value seems to be a mistake.
Async delete file creation in Stateless CNI has been moved to CNS_invoker to be consistent with Stateful CNI.
Removing secondary interface from pod namespace in SwiftV2 Linux in case of asynchronous delete. This is needed to be covered for stateless CNI since if CNS is down, CNI does not have the secondary interface name. A dedicated function has been added to list the interface in pod namespace and remove the secondary.

For validating the scenario ADD/Delete calls have been issues on SwiftV2 cluster and and logs and satefile has been analyzed to make sure it is consistent with Stateful CNI and also nothing gets leaked.

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

Copilot

Pull Request Overview

This PR fixes issues with Stateless CNI delete operations in SwiftV2 scenarios by modifying endpoint ID generation and improving the delete flow. The changes ensure proper distinction between different NIC types and provide necessary context for transparent client operations.

Modifies GetEndpointID to accept a NICType parameter and append interface name for delegated NICs
Updates delete flow to use proper network manager clients and adds NetNsPath to state information
Adds NetworkNameSpace field to CNS REST server structures for frontend NIC support

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
network/manager.go	Core logic changes for endpoint ID generation and delete flow improvements
network/manager_mock.go	Mock implementation updated to match new GetEndpointID signature
cns/restserver/restserver.go	Added NetworkNameSpace field to IPInfo struct
cns/restserver/ipam.go	Updated validation and state management for NetworkNameSpace field
cni/network/network.go	Updated callers to pass NICType parameter to GetEndpointID

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

network/manager.go

cns/restserver/ipam.go

network/manager.go

santhoshmprabhu

I don't have a lot of context on the need for these changes. The changes look fine, but we should improve the PR a bit -

Update the description to cover why the opMode change
Address other copilot comments (one about NICType in particular seems serious)
Add a description of what validation steps have been carried out. Include screenshots/logs if necessary.
Add tests.

cns/restserver/ipam.go

network/manager.go

cni/network/network.go

cns/restserver/restserver.go

network/manager.go

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

cni/network/network.go

network/manager.go

QxBytes

preliminary suggestions-- could you also add how you tested and what happens without each of these changes?

network/secondary_endpoint_client_linux.go

QxBytes · 2025-09-18T20:09:26Z

network/manager.go

 	UpdateEndpoint(networkID string, existingEpInfo *EndpointInfo, targetEpInfo *EndpointInfo) error
 	GetNumberOfEndpoints(ifName string, networkID string) int
 	GetEndpointID(containerID, ifName string) string
+	GetEndpointIDByNicType(containerID, ifName string, nicType cns.NICType) string


nit: GetEndpointIDByNICType as nic is an acronym

cni/network/invoker_cns.go

cni/network/network.go

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

network/secondary_endpoint_client_linux.go

cni/network/network.go

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

network/manager.go

network/secondary_endpoint_client_linux.go

cni/network/network.go

network/secondary_endpoint_client_linux.go

Copilot

Pull Request Overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

tamilmani1989

@behzad-mir lets discuss offline on these changes. i feel it can be done in another way

tamilmani1989 · 2025-09-30T21:25:06Z

cni/network/network.go

+				// The delegated NIC (SR-IOV VF) used by SwiftV2 for multitenant pods remains tied to the pod namespace,
+				// triggering hot-unplug/re-register events and leaving the node in an unhealthy state.
+				// This workaround mitigates the issue by removing the secondary NIC from the pod netns when CNS is unreachable during DEL to provide the endpoint state.
+				if err = plugin.nm.RemoveSecondaryEndpointFromPodNetNS(args.IfName, args.Netns); err != nil {


it should still create file for cns to release ip allocated for pod

Addressed in the new commit

QxBytes

As I'm reading this it seems like a lot of the code is geared around accessing FetchInterfacesFromNetnsPath in secondary_endpoint_client_linux.go, requiring us to drill down through cni/network/network.go > network/manager.go > network/endpoint_linux.go > secondary_endpoint_client_linux.go, adding methods to interfaces which are only specific to swiftv2/secondary endpoint client.

What is it from secondary endpoint client that we actually need? At GetEndpoint() > nm.generateEndpointLocally, there is no guarantee we are even in swiftv2 right? Why can't we just make a helper function (doesn't even need to be part of a client) where generateEndpointLocally is called that does the following:

Enters the pod's netns
Searches for any swiftv2/secondary interfaces if they exist
Creates endpoint infos and returns them if found
Instead of going through the trouble of creating a secondary endpoint client etc.

QxBytes · 2025-09-30T00:33:46Z

cni/network/network.go

-	// for when the endpoint is not created, but the ips are already allocated (only works if single network, single infra)
-	// this block is not applied to stateless CNI
+	// for Stateful CNI when the endpoint is not created, but the ips are already allocated (only works if single network, single infra)
+	// this block is applied to stateless CNI only if there was a connection failure in previous block and asynchronous delete by CNS will remover the endpoint from state file


No description provided.

can you clarify the comment (1109-1110) to:
the following block applies to:

stateful when ...

stateless when ...

network/secondary_endpoint_client_linux.go

QxBytes · 2025-10-06T22:35:47Z

cni/network/network.go

-		}
-	} else {
-		epInfos = plugin.nm.GetEndpointInfosFromContainerID(args.ContainerID)
+	epInfos, err := plugin.nm.GetEndpoint(networkID, args)


Can you rename this-- it currently isn't getting an endpoint object-- it's getting a slice of endpoint infos. Also please add a comment to that function

QxBytes · 2025-10-06T22:44:30Z

network/manager.go

+		if err != nil {
+			switch {
+			case errors.Is(err, ErrConnectionFailure):
+				logger.Error("Failed to connect to CNS", zap.Error(err))


Can we combine these logs-- they are related

QxBytes · 2025-10-06T22:47:54Z

network/manager.go

+				// This workaround mitigates the issue by generating a minimal endpointInfo via containerd args and netlink APIs that can be then passed to DeleteEndpoint API.
+				epInfos, err = nm.generateEndpointLocally(args)
+				if err != nil {
+					logger.Error("Failed to fetch secondary endpoint from pod netns", zap.String("netns", args.Netns), zap.Error(err))


Would like to avoid logging and returning the error here since we should already log the error in the main Delete call

QxBytes · 2025-10-06T23:04:46Z

network/manager.go

+
+func (nm *networkManager) GetEndpoint(networkID string, args *cniSkel.CmdArgs) ([]*EndpointInfo, error) {
+	if nm.IsStatelessCNIMode() {
+		logger.Info("calling cns getEndpoint API")


capitalize log messages

QxBytes · 2025-10-06T23:17:44Z

network/manager.go

+		NetworkNameSpace: args.Netns,
+		IfName:           args.IfName, // TODO: For stateless cni linux populate IfName here to use in deletion in secondary endpoint client
+	}
+	epInfo, err := nm.getEndpointInfoByIfNameImpl(ep)


did you confirm what happens on windows? If the error is nil, I would prefer returning an empty slice over "nil" in the windows implementation. I believe in windows this leads to GetEndpoint returning empty/nil, we going into the len(epInfos)==0 block, calling ipamInvoker.Delete-- please confirm this is intended

Also can we make the function/method names more consistent? If it returns a slice of endpoint infos, we getEndpointInfos (plural)

The method name is also a bit confusing since we're using more than just the endpoint's ifname

QxBytes · 2025-10-06T23:20:32Z

network/endpoint_linux.go

+	}
+	ret := []*EndpointInfo{}
+	ret = append(ret, epInfo)
+	logger.Info("Fetching Secondary Endpoint from", zap.String("NetworkNameSpace: ", ep.NetworkNameSpace))


You don't need to put the colon in "NetworkNameSpace: "-- it should automatically format

QxBytes · 2025-10-06T23:29:37Z

network/secondary_endpoint_client_linux.go

 		if strings.Contains(err.Error(), errFileNotExist.Error()) {
-			// clear SecondaryInterfaces map since network namespace doesn't exist anymore
-			ep.SecondaryInterfaces = make(map[string]*InterfaceInfo)
+			// clear SecondaryInterfaces map since network namespace doesn't exist anymore, Specific to legacy code in stateful cni


could you explain these clearings of the secondary interface map? This function should only deal with moving into or out of a network namespace based on its naming-- this is a hidden side effect that modifies the endpoint client and should be moved out of this function if possible.

QxBytes · 2025-10-06T23:35:00Z

network/errors.go

+	errV6SnatRuleNotSet         = errors.New("ipv6 snat rule not set. Might be VM ipv6 address missing") // nolint
+	ErrEndpointStateNotFound    = errors.New("endpoint state could not be found in the statefile")
+	ErrConnectionFailure        = errors.New("couldn't connect to CNS")
+	ErrEndpointRemovalFailure   = errors.New("Failed to remove endpoint")


I know some of these are capitalized already but new error messages shouldn't be capitalized: https://go.dev/wiki/CodeReviewComments#error-strings

tamilmani1989 · 2025-10-07T02:58:56Z

cni/network/network.go

+			// This is an special case for stateless CNI when Asychronous DEL to CNS will take place
+			// At this point the endpoint is already deleted in previous block and CNS will release the IP whenever it is up
+			if epInfo.IPAddresses == nil && plugin.nm.IsStatelessCNIMode() {
+				logger.Warn("Release ip Asynchronously by CNS",


why releaseip only for stateless cni and not for stateful cni case? current code calling release for both cases

tamilmani1989 · 2025-10-07T03:03:15Z

network/manager.go

+	GetEndpoint(networkID string, args *cniSkel.CmdArgs) ([]*EndpointInfo, error)
 	GetEndpointInfosFromContainerID(containerID string) []*EndpointInfo
-	GetEndpointState(networkID, containerID string) ([]*EndpointInfo, error)
+	GetEndpointState(networkID, containerID, netns string) ([]*EndpointInfo, error)


why do we need 2 apis? can we merge to 1 api, otherwise this is confusing and not sure about purpose of 2 apis

tamilmani1989 · 2025-10-07T03:04:35Z

network/manager.go

+// GetEndpointIDByNicType returns a unique endpoint ID based on the CNI mode and NIC type.
+func (nm *networkManager) GetEndpointIDByNicType(containerID, ifName string, nicType cns.NICType) string {
+	// For stateless CNI, secondary NICs use containerID-ifName as endpointID.
+	if nm.IsStatelessCNIMode() && nicType != cns.InfraNIC {


for stateful cni, this is not an issue? what's the impact if we remove statelesscnimode check here?

tamilmani1989 · 2025-10-07T03:14:57Z

network/manager.go

+		}
+		return epInfos, nil
+	}
+	return nm.GetEndpointInfosFromContainerID(args.ContainerID), nil


if this api is called only for stateful cni, can we keep in else to avoid any error in future

Copilot AI review requested due to automatic review settings August 27, 2025 07:19

behzad-mir requested review from a team as code owners August 27, 2025 07:19

behzad-mir requested a review from tamilmani1989 August 27, 2025 07:19

Copilot AI reviewed Aug 27, 2025

View reviewed changes

network/manager.go Show resolved Hide resolved

cns/restserver/ipam.go Outdated Show resolved Hide resolved

MikeZappa87 reviewed Aug 27, 2025

View reviewed changes

cns/restserver/ipam.go Outdated Show resolved Hide resolved

MikeZappa87 reviewed Aug 27, 2025

View reviewed changes

cns/restserver/ipam.go Outdated Show resolved Hide resolved

MikeZappa87 reviewed Aug 27, 2025

View reviewed changes

network/manager.go Outdated Show resolved Hide resolved

santhoshmprabhu requested changes Aug 27, 2025

View reviewed changes

tamilmani1989 reviewed Aug 27, 2025

View reviewed changes

cns/restserver/ipam.go Outdated Show resolved Hide resolved

cns/restserver/ipam.go Outdated Show resolved Hide resolved

cns/restserver/ipam.go Outdated Show resolved Hide resolved

network/manager.go Outdated Show resolved Hide resolved

network/manager.go Show resolved Hide resolved

QxBytes reviewed Aug 27, 2025

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

cns/restserver/restserver.go Outdated Show resolved Hide resolved

network/manager.go Outdated Show resolved Hide resolved

network/manager.go Show resolved Hide resolved

behzad-mir force-pushed the swiftv2-stateless branch 6 times, most recently from 52b1ed1 to a6b378e Compare September 18, 2025 16:46

behzad-mir requested review from tamilmani1989, QxBytes, santhoshmprabhu, Copilot and MikeZappa87 September 18, 2025 16:46

Copilot AI reviewed Sep 18, 2025

View reviewed changes

cni/network/network.go Outdated Show resolved Hide resolved

network/manager.go Show resolved Hide resolved

QxBytes reviewed Sep 19, 2025

View reviewed changes

behzad-mir force-pushed the swiftv2-stateless branch 5 times, most recently from 4b7c752 to 5c01db3 Compare September 25, 2025 17:30

behzad-mir force-pushed the swiftv2-stateless branch 4 times, most recently from b342517 to 0118a49 Compare September 29, 2025 05:04

behzad-mir requested review from QxBytes and Copilot September 29, 2025 05:14

Copilot AI reviewed Sep 29, 2025

View reviewed changes

network/secondary_endpoint_client_linux.go Outdated Show resolved Hide resolved

network/secondary_endpoint_client_linux.go Outdated Show resolved Hide resolved

cni/network/network.go Outdated Show resolved Hide resolved

cni/network/network.go Show resolved Hide resolved

behzad-mir force-pushed the swiftv2-stateless branch from 0118a49 to 8bfda69 Compare September 29, 2025 05:58

behzad-mir requested a review from Copilot September 29, 2025 16:05

Copilot AI reviewed Sep 29, 2025

View reviewed changes

network/manager.go Show resolved Hide resolved

network/secondary_endpoint_client_linux.go Outdated Show resolved Hide resolved

cni/network/network.go Show resolved Hide resolved

network/secondary_endpoint_client_linux.go Outdated Show resolved Hide resolved

behzad-mir force-pushed the swiftv2-stateless branch from 8bfda69 to 3f3e4a5 Compare September 29, 2025 16:21

behzad-mir requested a review from Copilot September 29, 2025 16:21

Copilot AI reviewed Sep 29, 2025

View reviewed changes

behzad-mir force-pushed the swiftv2-stateless branch from 3f3e4a5 to ab9e3ce Compare September 30, 2025 19:15

Fix: adding support for SWiftV2 Linux stateless CNI mode

6fbcfd0

behzad-mir force-pushed the swiftv2-stateless branch from ab9e3ce to 6fbcfd0 Compare September 30, 2025 19:26

tamilmani1989 requested changes Sep 30, 2025

View reviewed changes

behzad-mir force-pushed the swiftv2-stateless branch 5 times, most recently from b364575 to b27f6af Compare October 3, 2025 01:51

behzad-mir requested a review from tamilmani1989 October 3, 2025 01:53

behzad-mir force-pushed the swiftv2-stateless branch 3 times, most recently from 1f5326e to 6b96a7c Compare October 6, 2025 17:56

fix: generating endpoint locally when CNS is not reachabe.

8584493

behzad-mir force-pushed the swiftv2-stateless branch from 6b96a7c to 8584493 Compare October 6, 2025 18:22

QxBytes requested changes Oct 6, 2025

View reviewed changes

tamilmani1989 reviewed Oct 7, 2025

View reviewed changes

fix: fixing Stateless CNI delete in SwiftV2 scenario #3967

Are you sure you want to change the base?

fix: fixing Stateless CNI delete in SwiftV2 scenario #3967

Uh oh!

Conversation

behzad-mir commented Aug 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

santhoshmprabhu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

QxBytes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

tamilmani1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QxBytes left a comment

Choose a reason for hiding this comment

Uh oh!

QxBytes Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behzad-mir commented Aug 27, 2025 •

edited

Loading

QxBytes Sep 30, 2025 •

edited

Loading