bgpv1: avoid object tracker vs informer race

[bimmlerd]

The fake k8s testing infrastructure has an unfortunate interaction between real informers and the object tracker used in fake clientsets: the informer uses ListAndWatch to subscribe to the api server, but ListAndWatch relies on the ResourceVersion field to bridge the gap between the List and the Watch. The simple object tracker does not provide the resource versioning, nor does its Watch implementation attempt to replay existing state.

The race window, hence, allows for creation of objects after the initial list, but before the establishment of Watch. These objects are not observed by the informer and thus tests are likely to fail.

As a workaround, we ensure that the object tracker has registered a watcher before we start the test in earnest. It is only important that we do not create/update/delete objects in the window between starting the hive (and hence running of the informer) and having ensured that the watcher is in place. Creation prior to starting the hive is okay, as well as after ensuring the watcher exists.

Fixes: #31006

[bimmlerd]

/test

[harsimran-pabla]

Thanks @bimmlerd for thorough explanation. I think we have this pattern repeated across tests in Cilium which uses fake client set. For eg BGP component tests

Does it require wider discussion to address all these test cases ?

[YutaroHayakawa]

I found ControllerRuntime has a versionedTracker that implements testing.ObjectTracker with resource versioning, which "looks like" what we want, but I'm not sure if we can reuse it for our use case.

https://github.com/cilium/cilium/blob/main/vendor/sigs.k8s.io/controller-runtime/pkg/client/fake/client.go#L61-L65

Anyways, thanks for working on this David!

[bimmlerd]

I think we have this pattern repeated across tests in Cilium which uses fake client set. For eg BGP component tests

Does it require wider discussion to address all these test cases ?

Yep, see also #30906 and #30885. I'll cc you on a slack thread, also.

I found ControllerRuntime has a versionedTracker that implements testing.ObjectTracker with resource versioning, which "looks like" what we want, but I'm not sure if we can reuse it for our use case.

Nice find. I took a brief peek, and unfortunately it doesn't seem like this will be enough - the versionedTracker's Watch is delegated to the underlying, standard ObjectTracker, as far as I can tell. This would then not solve the problem, even if the resources are versioned: the missing part is replaying the right information to a watcher which subscribes starting at a specific version.

[bimmlerd]

CI triage:

• ci-aks: hit CI: Conformance AKS - check-log-errors - EndpointPolicyVisibilityEvent event failed #31009 in https://github.com/cilium/cilium/actions/runs/8067653433/job/22038619283#step:28:195.
• ci-eks: https://github.com/cilium/cilium/actions/runs/8067653691/job/22038621674#step:21:135 and all following tests failed with pods "client2-5668f9f59b-5f9sq" not found - this is very likely due to the fact that during the test, a node was removed by EKS. We don't seem to be very resilient against this.
• ci-gateway-api: hit a persistent failure in https://github.com/cilium/cilium/actions/runs/8067653750/job/22038610100#step:16:52 - fixed in gateway: Sync up the experimental conformance test #31017, will need a rebase 😢
• ci-ginkgo failed with evel=warning msg="github.com/cilium/cilium/pkg/k8s/resource/resource.go:808: watch of *v2.CiliumNode ended with: an error on the server (\"unable to decode an event from the watch stream: http2: client connection lost\") has prevented the request from succeeding" subsys=klog for many resources, which seems odd, probably a network blip?

[bimmlerd]

/test (rebased to get the fix for the gateway conformance test)

[bimmlerd]

CI triage (WIP)

• ci-ingress: https://github.com/cilium/cilium/actions/runs/8078278780/job/22070247578#step:19:306 failed with CI: Ingress Conformance Test: An Ingress with mixed path rules should send traffic to the matching backend service where Exact is preferred #30281

[bimmlerd]

/test

[bimmlerd]

Fixed a race in my fix, PTAL 😓 Specifically, I was closing the channel before actually calling Watch on the underlying tracker (delegating it to the default reaction). Fixed by calling Watch in our reaction and only then closing the channel.

Quantified using the deflaking tips from https://gist.github.com/liggitt/6a3a2217fa5f846b52519acfc0ffece0#running-unit-tests-to-reproduce-flakes I'm now confident that it's less broken than before :)

before: 21m30s: 3069 runs so far, 49 failures (1.60%)
after: 21m30s: 3033 runs so far, 0 failures

[YutaroHayakawa]

Marking as needs-backport to v1.15. I hit this in #31354.