Skip to content

fix: endpointID for Stateless CNI should add ifName if the NiCType is… #3770

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

behzad-mir
Copy link
Contributor

EndpointID for Stateless CNI should add ifName if the NiCType is Delegated to distinguish that from the InfraNIC endpoint.
The reason for this behavior is that Stateless CNI is using only ContainerID for the endpoint ID.

Notes:

@Copilot Copilot AI review requested due to automatic review settings July 3, 2025 01:55
@behzad-mir behzad-mir requested review from a team as code owners July 3, 2025 01:55
@behzad-mir behzad-mir requested a review from ashvindeodhar July 3, 2025 01:55
@behzad-mir behzad-mir added cni Related to CNI. do-not-merge labels Jul 3, 2025
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances the endpoint ID generation for Stateless CNI by including the interface name when the NIC type is Delegated, preventing collisions with InfraNIC endpoints.
Key changes:

  • Extended the GetEndpointID signature to accept nicType and altered its behavior for DelegatedVMNIC.
  • Updated calls in createEpInfo and Delete to pass the appropriate NICType.

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
network/manager.go Added nicType parameter to GetEndpointID and added branching for DelegatedVMNIC.
cni/network/network.go Modified calls to GetEndpointID to pass NICType in endpoint creation and deletion.
Comments suppressed due to low confidence (3)

network/manager.go:118

  • Changing the GetEndpointID signature is a breaking API change. Ensure all implementations of NetworkManager are updated (including mocks/stubs) and consider bumping the major version or updating documentation accordingly.
	GetEndpointID(containerID, ifName string, nicType cns.NICType) string

network/manager.go:734

  • The doc comment should be updated to mention the new nicType parameter and describe its effect on the returned endpoint ID.
// GetEndpointID returns a unique endpoint ID based on the CNI mode.

cni/network/network.go:715

  • No unit tests were added to cover the new behavior when NICType is DelegatedVMNIC. Add tests to verify both stateless and non-stateless modes with different NIC types.
		endpointID = plugin.nm.GetEndpointID(opt.args.ContainerID, ifName, opt.ifInfo.NICType)

if nm.IsStatelessCNIMode() {
if nicType == cns.DelegatedVMNIC {
return containerID + "-" + ifName
Copy link
Preview

Copilot AI Jul 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] Consider using fmt.Sprintf("%s-%s", containerID, ifName) or defining a constant delimiter to improve readability and consistency.

Copilot uses AI. Check for mistakes.

@behzad-mir behzad-mir marked this pull request as draft July 3, 2025 01:57
@behzad-mir behzad-mir force-pushed the swiftv2-stateless branch from 7c5292d to cfc2492 Compare July 3, 2025 02:16
@behzad-mir behzad-mir force-pushed the swiftv2-stateless branch from cfc2492 to db2bd0b Compare July 3, 2025 02:27
Copy link

This pull request is stale because it has been open for 2 weeks with no activity. Remove stale label or comment or this will be closed in 7 days

@github-actions github-actions bot added the stale Stale due to inactivity. label Jul 18, 2025
Copy link

Pull request closed due to inactivity.

@github-actions github-actions bot closed this Jul 25, 2025
@github-actions github-actions bot deleted the swiftv2-stateless branch July 25, 2025 00:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. do-not-merge stale Stale due to inactivity.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant