-
Notifications
You must be signed in to change notification settings - Fork 359
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
Change secondary network Pod controller to subscribe to CNIServer events #5767
Conversation
059dc34
to
24ece85
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.
LGTM, nice cleanup
in the PR title / description: s/subscribe CNIServer events/subscribe to CNIServer events
pc.podCache.SetPodCNIDeleted(containerInfo) | ||
} | ||
} | ||
pc.queue.Add(podKeyGet(event.PodNamespace, event.PodName)) |
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 guess this is a key code difference in this patch: with CNI events being handled asynchronously, we need to ensure we enqueue the Pods
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.
Yes. Actually I am thinking to change the Pod event handling part in the next PR, so we can create/delete secondary interfaces right after CNI add/delete.
24ece85
to
06bc160
Compare
cmd/antrea-agent/agent.go
Outdated
if err != nil { | ||
return fmt.Errorf("error initializing CNI server with cniPodInfoStore cache: %v", err) | ||
} | ||
} else { | ||
err = cniServer.Initialize(ovsBridgeClient, ofClient, ifaceStore, podUpdateChannel, nil) | ||
err = cniServer.Initialize(ovsBridgeClient, ofClient, ifaceStore, podUpdateChannel) |
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 should merge the two branches since cniServer is decoupled with SecondaryNetwork
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.
Good catch. Fixed.
ContainerID: containerConfig.ContainerID, | ||
NetNS: netNS, |
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 wonder how it handles restart case if it relies on the netNS which is not persistent anywhere. There may be two cases it can't handle:
- If agent is restarted after CNI event is handled but before PodController handles it, it can't create no longer create the secondary interface.
- After agent is restarted, it can't clean up the secondary interface as the cache is always empty?
Should it cache netNS in containerConfig and use containerConfig to initialize podCache?
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.
Right. Today we do not persist any secondary interface configuration, and I plan to implement that in the next PR. We can definitely save net NS with the primary interface OVS port and I thought about that too. But I want to mention that only address the case antrea-agent restarts after Pod's primary interfaces are created but before any secondary interface is created, as only secondary interface creation (not deletion) will require net NS. I am still thinking is it worthwhile to cover that corner case or not.
The commit moves CNI Pod information store from CNIServer to secondary network PodController, and let PodController subscribe CNIServer Pod events to maintain the Pod CNI information in the store. This way simplifies CNIServer and removes secondary network logic from it. Signed-off-by: Jianjun Shen <shenj@vmware.com>
06bc160
to
33942ac
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.
LGTM
/test-all |
The commit moves CNI Pod information store from CNIServer to secondary network PodController, and let PodController subscribe to CNIServer Pod events to maintain the Pod CNI information in the store. This way simplifies CNIServer and removes secondary network logic from it.
Issue: #5278