[Windows] CNI Server installs OpenFlow entries by PortStatus #6763
Conversation
wenyingd
force-pushed
the
async_ofport
branch
from
9c74fdb
to
370e578
Compare
…ge is received Signed-off-by: Wenying Dong <[email protected]>
wenyingd
force-pushed
the
async_ofport
branch
from
370e578
to
4fcbe6f
Compare
| createTime time.Time | ||
| } | ||
|
|
||
| type podIfaceMonitor struct { |
There was a problem hiding this comment.
when I look at all this new code, it seems that there is nothing platform-specific about it: it could build and run on Linux just as well as on Windows (unless I am missing something). Given this, could we move the podInfaceMonitor code to a separate file or sub-package, that will not be platform-specific. It will make it much easier to update that code and unit test it locally on macOS laptops or Linux dev servers. I think this code deserved to be unit-tested independently of the rest of the pod configuration code.
On Windows, we can use a pointer to an actual instance of podIfaceMonitor, and on Linux, we can use a nil pointer.
| } | ||
| } | ||
|
|
||
| func (m *podIfaceMonitor) monitorUnReadyInterface(stopCh <-chan struct{}) { |
There was a problem hiding this comment.
typically by convention we name a function like this Run, and we call Run in its own goroutine
| func (m *podIfaceMonitor) updatePodFlows(ifName string, ofPort int32) error { | ||
| ifConfig, found := m.ifaceStore.GetInterfaceByName(ifName) | ||
| if !found { | ||
| klog.Info("Interface config is not found", "name", ifName) |
There was a problem hiding this comment.
you probably meant to use klog.InfoS here. Please check other occurrences as well.
| m.ifaceStore.UpdateInterface(ifConfig) | ||
|
|
||
| // Install OpenFlow entries for the Pod. | ||
| klog.V(2).Infof("Setting up Openflow entries for container %s", containerID) |
There was a problem hiding this comment.
Please make sure you use structured logging (klog.InfoS / klog.ErrorS) in all new code
| return nil | ||
| } | ||
|
|
||
| func (m *podIfaceMonitor) updatePodUnreadyAnnotation(podNamespace, podName string, addAnnotation bool) { |
There was a problem hiding this comment.
I think this function would benefit from having an actual context.Context as a parameter
It's true that when we started the Antrea codebase, contexts were not as much of a thing, so we have a lot of "placeholders" (context.TODO() or context.Background()). For this new function, we should add ctx context.Context, and the caller can choose what to do.
| "annotations": map[string]interface{}{types.PodNotReadyAnnotationKey: ""}, | ||
| }, | ||
| }) | ||
| m.kubeClient.CoreV1().Pods(podNamespace).Patch(context.Background(), podName, apitypes.MergePatchType, patch, metav1.PatchOptions{}) |
There was a problem hiding this comment.
should we check the error?
| } | ||
|
|
||
| func (m *podIfaceMonitor) updatePodUnreadyAnnotation(podNamespace, podName string, addAnnotation bool) { | ||
| pod, err := m.kubeClient.CoreV1().Pods(podNamespace).Get(context.Background(), podName, metav1.GetOptions{}) |
There was a problem hiding this comment.
usually we avoid reading directly from the K8s API. Is using an informer possible here?
| podInfo := value.(*unReadyPodInfo) | ||
| if !podInfo.annotated && time.Now().Sub(podInfo.createTime).Seconds() > podNotReadyTimeInSeconds { | ||
| m.updatePodUnreadyAnnotation(podInfo.podNamespace, podInfo.podName, true) | ||
| podInfo.annotated = true |
There was a problem hiding this comment.
I think this should be conditional: patching the Pod can fail and we want to be able to try again at the next tick.
| } | ||
|
|
||
| func (m *podIfaceMonitor) updateUnReadyPod(status *openflow15.PortStatus) { | ||
| ovsPort := string(bytes.Trim(status.Desc.Name, "\x00")) |
There was a problem hiding this comment.
It feels like this should be handled in libOpenflow?
| if err != nil { | ||
| klog.ErrorS(err, "Failed to get Pod when trying to update 'unready' annotations", "Namespace", podNamespace, "Name", podName) | ||
| return | ||
| } |
There was a problem hiding this comment.
Is it possible for a Pod to no longer exist and still be in the map (because of some failure)? In this case we would never delete it from the map.
This change has introduced
podIfaceMonitorin CNI Server module, which listens for the OpenFlow PortStatus message when a new OpenFlow port is allocated in OVS. After receiving the message, antrea-agent Windows will install Pod related OpenFlow entries. If the OpenFlow port is not allocated within 30s after the CmdAdd request is responded, an annotation is added on the Pod with key "pod.antrea.io/not-ready". The annotation is removed if the Pod network is ready.Fix: #6721