Improve BGP controller unit tests to remove dependency on informer goroutine timing

[antoninbas]

Background

The BGP controller unit tests in pkg/agent/controller/bgp/controller_test.go use a pattern where syncBGPPolicy is driven manually while live informer goroutines run concurrently. The central synchronization primitive is waitAndGetDummyEvent:

func waitAndGetDummyEvent(t *testing.T, c *fakeController) {
    require.Eventually(t, func() bool {
        return c.queue.Len() == 1
    }, 5*time.Second, 10*time.Millisecond)
    c.queue.Get()
}

This polls the work queue length and dequeues one item. The tests assume that when c.queue.Len() == 1, the item was enqueued by a specific triggering action (e.g., a BGPPolicy DELETE). This assumption has never been sound.

The problem

startInformers calls WaitForCacheSync on both informer factories. WaitForCacheSync only guarantees that the informer store is populated from the initial list — it does not guarantee that event handlers have been called for those objects. The event-handler processor runs in separate goroutines and may still be delivering initial-list ADD notifications when startInformers returns.

In practice this means: after startInformers and the first waitAndGetDummyEvent/doneDummyEvent cycle, pending ADD notifications for the remaining initial BGPPolicy objects may still be in the processor's ring buffer. When doneDummyEvent marks the in-flight item as done, the rate-limiting queue re-queues dummyKey immediately. The subsequent waitAndGetDummyEvent (intended to wait for e.g. a DELETE event) fires on one of these stale re-queued items instead, while the lister may still have the deleted policy present. syncBGPPolicy then returns nil instead of the expected error.

This was a pre-existing latent race that became consistently observable after the upgrade to client-go v0.36, which introduced the watchSynced() goroutine per processor listener (an additional goroutine hop in the event delivery chain), making the window between WaitForCacheSync returning and event handlers being called larger and reliably visible on loaded CI runners.

A short-term fix was applied in #8057 (register a no-op handler on bgpPolicyInformer in startInformers and wait for its HasSynced, which only returns true once all initial-list ADD notifications have been delivered). This reduces the race window but does not eliminate the fundamental fragility.

The fundamental issue

The tests mix two concerns that are hard to compose correctly:

1. Live informer goroutines delivering events asynchronously
2. Manual invocation of syncBGPPolicy as if the controller were running synchronously

waitAndGetDummyEvent is an inherently fragile bridge: it checks a queue length snapshot with no guarantee about which event caused the enqueue, and no mechanism to drain stale events.

Proposed improvements

Option A: Don't run live informers at all (preferred)

Pre-populate the fake lister with the desired objects and call syncBGPPolicy directly, without starting any informers. This is the standard approach for controller unit tests:

• Set up the initial world state in the fake indexer
• Call syncBGPPolicy, assert on mock expectations and state
• Mutate the fake indexer to simulate ADD/UPDATE/DELETE
• Call syncBGPPolicy again, assert

No goroutines, no timing dependencies, no waitAndGetDummyEvent. This is how most well-written controller tests in the Kubernetes ecosystem work.

Option B: Use testing/synctest (Go 1.24)

Wrap the tests in synctest.Test so all goroutines run on a deterministic fake scheduler. waitAndGetDummyEvent could remain but all goroutine scheduling would be deterministic. This is less invasive but requires understanding the synctest model.

Option C: Separate event handler tests from sync tests

Unit-test addBGPPolicy/deleteBGPPolicy/updateBGPPolicy in isolation to verify they enqueue correctly. Test syncBGPPolicy with a fake lister and no live informers. This is the right long-term architecture and avoids the integration-style setup.

Affected tests

All tests that call startInformers followed by waitAndGetDummyEvent:

• TestSyncBGPPolicy
• TestSyncBGPPolicyFailures
• TestBGPPolicyAdd
• TestBGPPolicyUpdate
• TestBGPPolicyDelete
• Several sub-tests in table-driven test functions

References

• Upgrade K8s dependencies to 1.36.1 and regenerate code #8057 — K8s 1.36 upgrade that surfaced this issue
• client-go v0.36 change: introduction of watchSynced() goroutine per processor listener in sharedProcessor.run

[pyd-07]

Hi, I've read through controller_test.go and would like to implement Option A.

From my analysis:

• startInformers is called in 8 places across TestSyncBGPPolicy, TestSyncBGPPolicyFailures, TestBGPPolicyAdd, TestBGPPolicyUpdate, TestBGPPolicyDelete, and the lifecycle tests.
• The Controller struct stores each informer as a cache.SharedIndexInformer, so I can call .GetIndexer().Add/Update/Delete() directly instead of going through the fake clientset and informer pipeline.
• The queue only ever holds one logical key (dummyKey), and syncBGPPolicy reads exclusively from the listers, so removing the queue interaction appears safe based on my reading.
• TestSecretLifecycle (around line 1884) is a special case because the Secret handler updates an in-memory map rather than the Indexer. I plan to keep the Secret informer goroutine there but remove waitAndGetDummyEvent for the main queue interactions. Does that match your intent, or would you prefer that test to be fully decoupled as well?

I'd also update newFakeController to avoid requiring objects at construction time, with tests populating the indexers directly instead.

If you're happy with this plan, could I be assigned the issue? I'll start with a small proof-of-concept refactor and share it for feedback before tackling the remaining tests.

[OmAmbole009]

Hi @antoninbas,

I noticed there hasn't been any follow-up on this issue for a couple of weeks. I'd be interested in working on it and implementing Option A (removing the dependency on live informers in the unit tests).

If the issue is still available, I'd be happy to put together a PR.

Thanks!

[pyd-07]

Update: Proof-of-concept complete. Refactored TestServiceLifecycle using Option A.

To quantify the flakiness: on main, go test -count=100 consistently produces at least 1 failure per run, always WithdrawRoutes not being called because syncBGPPolicy runs before the DELETE propagates through the informer goroutine. Exact failure count varies run to run, but it never survives 100 iterations cleanly.
On the fix branch: go test -count=100 produces 100/100 pass, fully deterministic. No goroutines, no timing window.

Draft: https://github.com/pyd-07/antrea/blob/fix/bgp-controller-tests-remove-informer-timing/pkg/agent/controller/bgp/controller_test.go#L1651

Failure on main

controller.go:97: missing call(s) to *testing.MockInterface.WithdrawRoutes(is anything, is equal to [{192.168.77.101/32}] ([]bgp.Route))
controller.go:97: missing call(s) to *testing.MockInterface.WithdrawRoutes(is anything, is equal to [{192.168.77.151/32}] ([]bgp.Route))
controller.go:97: aborting test due to missing call(s)
--- FAIL: TestServiceLifecycle (0.15s)