-
Notifications
You must be signed in to change notification settings - Fork 240
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: Dev stateless cni #2276
feat: Dev stateless cni #2276
Conversation
427ed8d
to
6c04d2d
Compare
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
1c40e0b
to
bfebb47
Compare
bf03466
to
8f2cedf
Compare
87d6ab0
to
85b217a
Compare
err = nil | ||
return err | ||
if !plugin.nm.IsStatelessCNIMode() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this check if it doesn't apply to stateless cni?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a comment on top of it :
// this condition will not apply to stateless CNI since the network struct will be crated on each call
var logger = zapLog.CNILogger.With(zap.String("component", "cni-main")) | ||
|
||
const ( | ||
hostNetAgentURL = "http://168.63.129.16/machine/plugins?comp=netagent&type=cnireport" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some of these look common across regular cni
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will be corrected on next commit
network/manager.go
Outdated
NetworkContainerID: epInfo.Id, | ||
} | ||
logger.Info("Deleting endpoint with", zap.String("Endpoint Info: ", epInfo.PrettyString()), zap.String("HNISID : ", ep.HnsId)) | ||
return nw.deleteEndpointImpl(netlink.NewNetlink(), platform.NewExecClient(logger), nil, nil, ep) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we'd need to pass endpoint with SecondaryInterfaceInfo for deletion as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only going to cover stateless CNI for single Tenancy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not covering Swift 2.0 at this point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we address swift 2.0, then we dont need to care about migration for swift2.0 later.. Not in this PR, probably a different PR
54aacbd
to
53969b9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
few minor comments. it would be easier to review if we scoped the changes to smaller PRs container only CNS and only CNI
endpointStoreLocationLinux = "/var/run/azure-cns/" | ||
endpointStoreLocationWindows = "/k/azurecns/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/var/run/azure-cns
is a legal file path (in Go) on Windows, do we need to differentiate this? maybe it should just be required that it comes from the config? If we must do this though, we shouldn't put multiplatform variables in main/the same file - they need to be in _linux.go
, _windows.go
etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used that path at first but store package showed error and couldn't create the endpoint file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
then it needs to come from config instead of being hardcoded. we could have config_linux.go
and config_windows.go
to populate defaults if it is not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We also already made the AgentBaker changes according to k/azurecns directory since it was provided int he CNS Configmap for Windows. I simply applied the value form the configmap
#2102) * Adding getEndpoint and UpdateEndpoint API to CNS with the respective clients in support of stateless CNI. * Updating the unit tests and address the comments. * Addressing the comments. * Addressing the coments regarding CNS support for Stateless CNI * Adddressing the PR comments
feat: stateless cni
…package. (#2197) * Apllying stateless CNI mode in network package. * Addresing the commetns.
ae573a2
to
ae68d79
Compare
ae68d79
to
9344500
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unblocking but let's revisit the windows/linux state file path (and how it comes from config). the hardcoded paths can't stay
/azp run Azure Container Networking PR |
Azure Pipelines successfully started running 1 pipeline(s). |
* feat: 🌈 StatelessCNI: Adding getEndpoint and UpdateEndpoint API to CNS (#2102) * Adding getEndpoint and UpdateEndpoint API to CNS with the respective clients in support of stateless CNI. * Updating the unit tests and address the comments. * Addressing the comments. * Addressing the coments regarding CNS support for Stateless CNI * Adddressing the PR comments * 🌈 feat: adding flags for stateless cni (#2103) feat: stateless cni * 🌈 feat: StatelessCNI: Applying stateless CNI mode changes in network package. (#2197) * Apllying stateless CNI mode in network package. * Addresing the commetns. * feat: create stateless cni binary for swift (#2275) * enabling CNS telemetry * Master rebase changes * CNI Telemetry enabled on CNS * Stateless CNI changes. * making change to CNSendpointStorePath * Updating makefile to avoid creating statless CNI release. --------- Co-authored-by: Vipul Singh <vipul21sept@gmail.com>
* feat: 🌈 StatelessCNI: Adding getEndpoint and UpdateEndpoint API to CNS (#2102) * Adding getEndpoint and UpdateEndpoint API to CNS with the respective clients in support of stateless CNI. * Updating the unit tests and address the comments. * Addressing the comments. * Addressing the coments regarding CNS support for Stateless CNI * Adddressing the PR comments * 🌈 feat: adding flags for stateless cni (#2103) feat: stateless cni * 🌈 feat: StatelessCNI: Applying stateless CNI mode changes in network package. (#2197) * Apllying stateless CNI mode in network package. * Addresing the commetns. * feat: create stateless cni binary for swift (#2275) * enabling CNS telemetry * Master rebase changes * CNI Telemetry enabled on CNS * Stateless CNI changes. * making change to CNSendpointStorePath * Updating makefile to avoid creating statless CNI release. --------- Co-authored-by: Vipul Singh <vipul21sept@gmail.com>
This PR includes all changes related to Stateless CNI in which CNS will manage and maintain the endpoint states.
There are changes related to CNS to add two new APIs with respect of endpoint state management.
GetEndpoint() is used to return the state of a given endpointId. UpdateEndpoint() is used to update the HNSId and VethName field within the endpoint state.
The other changes are related to CNI and network package so the ADD/Del calls can be done without the use of statefile on Store package and instead use CNS and aformenetioned APIs.
uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added