Skip to content

Commit

Permalink
bgpv1: avoid object tracker vs informer race
Browse files Browse the repository at this point in the history
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.

Signed-off-by: David Bimmler <david.bimmler@isovalent.com>
  • Loading branch information
bimmlerd authored and joestringer committed Mar 5, 2024
1 parent f77f3c3 commit cfd1790
Showing 1 changed file with 23 additions and 1 deletion.
24 changes: 23 additions & 1 deletion pkg/bgpv1/manager/store/diffstore_test.go
Expand Up @@ -5,9 +5,13 @@ package store

import (
"context"
"sync"
"testing"
"time"

"k8s.io/apimachinery/pkg/watch"
k8sTesting "k8s.io/client-go/testing"

"github.com/cilium/cilium/pkg/bgpv1/agent/signaler"
"github.com/cilium/cilium/pkg/hive"
"github.com/cilium/cilium/pkg/hive/cell"
Expand All @@ -25,14 +29,30 @@ type DiffStoreFixture struct {
signaler *signaler.BGPCPSignaler
slimCs *slim_fake.Clientset
hive *hive.Hive
watching chan struct{} // closed once we have ensured there is a service watcher registered
}

func newDiffStoreFixture() *DiffStoreFixture {
fixture := &DiffStoreFixture{}
fixture := &DiffStoreFixture{
watching: make(chan struct{}),
}

// Create a new faked CRD client set with the pools as initial objects
fixture.slimCs = slim_fake.NewSimpleClientset()

var once sync.Once
fixture.slimCs.PrependWatchReactor("*", func(action k8sTesting.Action) (handled bool, ret watch.Interface, err error) {
w := action.(k8sTesting.WatchAction)
gvr := w.GetResource()
ns := w.GetNamespace()
watch, err := fixture.slimCs.Tracker().Watch(gvr, ns)
if err != nil {
return false, nil, err
}
once.Do(func() { close(fixture.watching) })
return true, watch, nil
})

// Construct a new Hive with faked out dependency cells.
fixture.hive = hive.New(
cell.Provide(func(lc cell.Lifecycle, c k8sClient.Clientset) resource.Resource[*slimv1.Service] {
Expand Down Expand Up @@ -90,6 +110,7 @@ func TestDiffSignal(t *testing.T) {
if err != nil {
t.Fatal(err)
}
<-fixture.watching

timer := time.NewTimer(5 * time.Second)
select {
Expand Down Expand Up @@ -187,6 +208,7 @@ func TestDiffUpsertCoalesce(t *testing.T) {
if err != nil {
t.Fatal(err)
}
<-fixture.watching

// Add first object
err = tracker.Add(&slimv1.Service{
Expand Down

0 comments on commit cfd1790

Please sign in to comment.