Skip to content
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

feat: adding stateless CNI support for ACI #3085

Merged
merged 3 commits into from
Nov 9, 2024
Merged

Conversation

behzad-mir
Copy link
Contributor

@behzad-mir behzad-mir commented Oct 24, 2024

feat: adding stateless CNI support for ACI>
New entry in CNS statefile will be added for a container upon updateEndpoint request if the entry does not already exist. This covers a special case for ACI.

In ACI the CNS path to allocate IP address and send goalstate is different than CNS IPAM for Cilium and azure-cni for AKS. As a result, the endpoint entry for statefile is not getting create upon IP allocation and this fix will create the whole entry when CNI send updateEndpoint API request to CNS.

Notes:

@behzad-mir behzad-mir requested a review from a team as a code owner October 24, 2024 20:57
@behzad-mir behzad-mir requested a review from rbtr October 24, 2024 20:57
@behzad-mir behzad-mir added cns Related to CNS. cni Related to CNI. labels Oct 24, 2024
@jackieluc
Copy link
Member

LGTM. we tested this in an integration environment with ACI and it solved the issue we faced

Copy link
Contributor

@QxBytes QxBytes left a comment

Choose a reason for hiding this comment

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

could you add the details of the special case in the pr description (like the timeline of events and why there is no entry in the cns state by the time we call update endpoint helper)-- I thought the endpoint state is usually populated in updateEndpointState in cns, unless we don't request ips? Is InfraContainerID (in update endpoint state) typically equal to endpointID (in update endpoint helper)?

@behzad-mir
Copy link
Contributor Author

could you add the details of the special case in the pr description (like the timeline of events and why there is no entry in the cns state by the time we call update endpoint helper)-- I thought the endpoint state is usually populated in updateEndpointState in cns, unless we don't request ips? Is InfraContainerID (in update endpoint state) typically equal to endpointID (in update endpoint helper)?

DOne

@behzad-mir behzad-mir closed this Nov 5, 2024
@behzad-mir behzad-mir reopened this Nov 5, 2024
QxBytes
QxBytes previously approved these changes Nov 5, 2024
Copy link
Member

@tamilmani1989 tamilmani1989 left a comment

Choose a reason for hiding this comment

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

can we write UT for this case

@behzad-mir
Copy link
Contributor Author

can we write UT for this case

Done

@behzad-mir behzad-mir force-pushed the statelessCNI-ACI branch 4 times, most recently from 152aeee to ee27d1a Compare November 8, 2024 16:25
cns/restserver/ipam_test.go Outdated Show resolved Hide resolved
cns/restserver/ipam_test.go Outdated Show resolved Hide resolved
cns/restserver/ipam_test.go Outdated Show resolved Hide resolved
@behzad-mir
Copy link
Contributor Author

/azp run Azure Container Networking PR

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@behzad-mir behzad-mir added this pull request to the merge queue Nov 8, 2024
Merged via the queue into master with commit c524294 Nov 9, 2024
14 checks passed
@behzad-mir behzad-mir deleted the statelessCNI-ACI branch November 9, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cni Related to CNI. cns Related to CNS.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants