Ignore pod allocation errors during sync if no node exists

[bpickard22]

During start up when existing pods run through the sync function, their IPs will attempt to be pre-allocated. This fails if the node does not exist, which can happen in kapi. Ignore those errors rather than causing ovnkube start up to fail.

[flavio-fernandes]

Question on this logic is. Do we need to return an error, or nil is the right value here ?
Assuming we are just going to ignore the allocation, then what you have is correct.

Otherwise, we may need to change that line to:

return expectedLogicalPortName, fmt.Errorf("pod: %s/%s is in a non-existing node: %s: %w", 
pod.Namespace, pod.Name, pod.Spec.NodeName, err)

[flavio-fernandes]

so if bnc.doesNetworkRequireIPAM() is true we want to continue with the allocation attempt and not simply return?
or should the new check be added in line 63 above?

[flavio-fernandes]

nit: undo space added (line 71)

[npinaeva]

talked to @trozet here is recap
returning an error from allocatePodIPsOnSwitch during initial sync will cause restart. we know there is a k8s bug that can give us Running pod without existing node. Currently, for all non-terminated pods waitForNodeLogicalSwitchSubnetsInCache will wait for 30 seconds. Now we agreed that in case waitForNodeLogicalSwitchSubnetsInCache returns error for running pods without a node, we should ignore it considering that the node actually doesn't exist and won't come up similar to https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/ovn/base_network_controller_pods.go#L59-L62 and return error in other cases.
So you could set some bool like podWithoutNode under https://github.com/ovn-org/ovn-kubernetes/blob/master/go-controller/pkg/ovn/base_network_controller_pods.go#L57-L58 and then conditionally return error from waitForNodeLogicalSwitchSubnetsInCache based on this value
hope this makes sense :) if not, please reach out to me

[trozet]

Maybe a more elegant way to fix it is to make the error be a specific instance of a new type, lets call it WaitForNodeSwitchError. Then in syncPods, when this code happens:

			expectedLogicalPortName, annotations, err = oc.allocateSyncPodsIPs(pod)
			if err != nil {
				return err
			}

We check here if the error is WatiForNodeSwitchError and then just continue? I guess continue may not be the best option by itself, since we will continue to try to add every pod and wait 30 seconds for the switch, that we already waited 30 seconds for and it never showed up. So might need to store notFoundSwitches map[string]bool var or something and compare each pod so you can short-circuit pods that we already know arent going find the switch.

[trozet]

you dont want this else statement. If a network does not require IPAM we wouldnt care if the node isnt found and cant allocate IP. What we are interested in is a few lines above in this block:

} else if bnc.doesNetworkRequireIPAM() {
			// unknown condition how we are getting a non-terminating pod without a node here
			klog.Errorf("Pod IP allocation found for a non-existent node in API with unknown "+
				"condition. Pod: %s/%s, node: %s", pod.Namespace, pod.Name, pod.Spec.NodeName)
		} 

Here we still want to communicate that we couldnt find the node for this pod, but we can change it to be a warning as we will not return out of the function and still proceed in the function to eventually call:

if err := bnc.waitForNodeLogicalSwitchSubnetsInCache(switchName); err != nil {

to see if the node (and switch) shows up within 30 seconds.

[trozet]

here you need to make a new error type, and return that new error type. This is so the caller can check if the error is this type and decide to ignore it or not (which will be used in the sync case).

[trozet]

this for statement doesnt work with overriding the pod.Spec.NodeName with the values in the map, then checking if that value exists in the map. It will always be true. Also you want to update the map when the error is returned, and check the map before you allocateSyncPodIPs. Also the error needs to be the new type indicating the switch never showed up, and not a kapi not found error. So you need something like this:

if switchesNotFound[pod.Spec.NodeName] {
    klog.Warningf("Cannot allocate IPs for %s/%s, node was not found after 30 seconds")
   continue
}

expectedLogicalPortName, annotations, err = oc.allocateSyncPodsIPs(pod)

if errors.Is(err, WaitForNodeSwitchError) {
   klog.Warningf("Cannot allocate IPs for %s/%s, node was not found after 30 seconds")
   switchesNotFound[pod.Spec.NodeName] = true
   continue
} else {
   return err
}

[tssurya]

poke @bpickard22 : PTAL review comments, this will help close https://issues.redhat.com/browse/OCPBUGS-15788

[trozet]

would be good to have a test for this, may be its as simple as creating a pod in the informer without the node and verifying sync does not error.

Also you need to squash your first and 2nd commit together.

[trozet]

you removed some logic here that shouldnt have been removed, like skipping non local zone pods, and checking if annotations is nil. This is causing the unit tests to crash. Also you need to check if there is an error other than nodeNotFoundError:

[trozet@fedora go-controller]$ git diff
diff --git a/go-controller/pkg/ovn/pods.go b/go-controller/pkg/ovn/pods.go
index 7c23fba85..7d53f2ecf 100644
--- a/go-controller/pkg/ovn/pods.go
+++ b/go-controller/pkg/ovn/pods.go
@@ -54,11 +54,20 @@ func (oc *DefaultNetworkController) syncPods(pods []interface{}) error {
                        }
                        expectedLogicalPortName, annotations, err = oc.allocateSyncPodsIPs(pod)
 
-                       if errors.Is(err, nodeNotFoundError) {
-                               klog.Warningf("Cannot allocate IPs for %s/%s, node was not found after 30 seconds", pod.Namespace, pod.Name)
-                               switchesNotFound[pod.Spec.NodeName] = true
-                               continue
+                       if err != nil {
+                               if errors.Is(err, nodeNotFoundError) {
+                                       klog.Warningf("Cannot allocate IPs for %s/%s, node was not found after 30 seconds", pod.Namespace, pod.Name)
+                                       switchesNotFound[pod.Spec.NodeName] = true
+                                       continue
+                               }
+                               return err
                        }
+               } else {
+                       continue
+               }
+
+               if annotations == nil {
+                       continue
                }

[coveralls]

coverage: 50.436% (-0.004%) from 50.44%
when pulling 94eeac6 on bpickard22:ignore-pod-on-no-node
into effbc31 on ovn-org:master.

[trozet]

can you add a test case as well?