Repository: tilt-dev/tilt
Pull request #6246 (2023-10-10 16:27:45)

test: use a mutex to prevent simultaneous reconcilation
i think this will fix a bunch of weird race conditions and test flakiness


Patch:
diff --git a/internal/controllers/fake/fixture.go b/internal/controllers/fake/fixture.go
index 60498d4ea..7e84026e4 100644
--- a/internal/controllers/fake/fixture.go
+++ b/internal/controllers/fake/fixture.go
@@ -5,6 +5,7 @@ import (
 	"io"
 	"os"
 	"strings"
+	"sync"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -17,8 +18,10 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/cache"
 	ctrlclient "sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/controller-runtime/pkg/source"
 
 	"github.com/tilt-dev/tilt-apiserver/pkg/server/builder/resource"
+	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/internal/store"
 	"github.com/tilt-dev/tilt/internal/testutils"
 	"github.com/tilt-dev/tilt/internal/testutils/bufsync"
@@ -44,20 +47,22 @@ type ControllerFixture struct {
 	out        *bufsync.ThreadSafeBuffer
 	ctx        context.Context
 	cancel     context.CancelFunc
-	controller controller
+	controller reconcile.Reconciler
 	Store      *testStore
 	Scheme     *runtime.Scheme
 	Client     ctrlclient.Client
 }
 
 type ControllerFixtureBuilder struct {
-	t      testing.TB
-	ctx    context.Context
-	cancel context.CancelFunc
-	out    *bufsync.ThreadSafeBuffer
-	ma     *analytics.MemoryAnalytics
-	Client ctrlclient.Client
-	Store  *testStore
+	t                  testing.TB
+	ctx                context.Context
+	cancel             context.CancelFunc
+	out                *bufsync.ThreadSafeBuffer
+	ma                 *analytics.MemoryAnalytics
+	Client             ctrlclient.Client
+	Store              *testStore
+	requeuer           source.Source
+	requeuerResultChan chan indexer.RequeueForTestResult
 }
 
 func NewControllerFixtureBuilder(t testing.TB) *ControllerFixtureBuilder {
@@ -80,15 +85,25 @@ func NewControllerFixtureBuilder(t testing.TB) *ControllerFixtureBuilder {
 	}
 }
 
-func (b ControllerFixtureBuilder) Scheme() *runtime.Scheme {
+func (b *ControllerFixtureBuilder) WithRequeuer(r source.Source) *ControllerFixtureBuilder {
+	b.requeuer = r
+	return b
+}
+
+func (b *ControllerFixtureBuilder) WithRequeuerResultChan(ch chan indexer.RequeueForTestResult) *ControllerFixtureBuilder {
+	b.requeuerResultChan = ch
+	return b
+}
+
+func (b *ControllerFixtureBuilder) Scheme() *runtime.Scheme {
 	return b.Client.Scheme()
 }
 
-func (b ControllerFixtureBuilder) Analytics() *analytics.MemoryAnalytics {
+func (b *ControllerFixtureBuilder) Analytics() *analytics.MemoryAnalytics {
 	return b.ma
 }
 
-func (b ControllerFixtureBuilder) Build(c controller) *ControllerFixture {
+func (b *ControllerFixtureBuilder) Build(c controller) *ControllerFixture {
 	b.t.Helper()
 
 	// apiserver controller initialization is awkward and some parts are done via the builder,
@@ -99,6 +114,16 @@ func (b ControllerFixtureBuilder) Build(c controller) *ControllerFixture {
 	_, err := c.CreateBuilder(&FakeManager{})
 	require.NoError(b.t, err, "Error in controller CreateBuilder()")
 
+	// In a normal controller, there's a central reconciliation loop
+	// that ensures we never have two reconcile() calls running simultaneously.
+	//
+	// In our test code, we want to people to invoke Reconcile() directly and in
+	// the background.  So instead, we wrap the Reconcile() call in mutex.
+	lc := NewLockedController(c)
+	if b.requeuer != nil {
+		indexer.StartSourceForTesting(b.Context(), b.requeuer, lc, b.requeuerResultChan)
+	}
+
 	return &ControllerFixture{
 		t:          b.t,
 		out:        b.out,
@@ -107,23 +132,23 @@ func (b ControllerFixtureBuilder) Build(c controller) *ControllerFixture {
 		Scheme:     b.Client.Scheme(),
 		Client:     b.Client,
 		Store:      b.Store,
-		controller: c,
+		controller: lc,
 	}
 }
 
-func (b ControllerFixtureBuilder) OutWriter() io.Writer {
+func (b *ControllerFixtureBuilder) OutWriter() io.Writer {
 	return b.out
 }
 
-func (b ControllerFixtureBuilder) Context() context.Context {
+func (b *ControllerFixtureBuilder) Context() context.Context {
 	return b.ctx
 }
 
-func (b ControllerFixture) Stdout() string {
+func (b *ControllerFixture) Stdout() string {
 	return b.out.String()
 }
 
-func (f ControllerFixture) T() testing.TB {
+func (f *ControllerFixture) T() testing.TB {
 	return f.t
 }
 
@@ -132,7 +157,7 @@ func (f ControllerFixture) T() testing.TB {
 // Normally, it's not necessary to call this - the fixture will automatically cancel the context as part of test
 // cleanup to avoid leaking resources. However, if you want to explicitly test how a controller reacts to context
 // cancellation, this method can be used.
-func (f ControllerFixture) Cancel() {
+func (f *ControllerFixture) Cancel() {
 	f.cancel()
 }
 
@@ -269,3 +294,24 @@ type FakeManager struct {
 func (m *FakeManager) GetCache() cache.Cache {
 	return nil
 }
+
+type LockedController struct {
+	mu         sync.Mutex
+	controller controller
+}
+
+func NewLockedController(c controller) *LockedController {
+	return &LockedController{controller: c}
+}
+
+func (c *LockedController) Reconcile(ctx context.Context, req reconcile.Request) (ctrl.Result, error) {
+	c.mu.Lock()
+	defer c.mu.Unlock()
+	return c.controller.Reconcile(ctx, req)
+}
+
+func (c *LockedController) CreateBuilder(mgr ctrl.Manager) (*builder.Builder, error) {
+	return c.controller.CreateBuilder(mgr)
+}
+
+var _ controller = &LockedController{}
diff --git a/pkg/webview/view.pb.go b/pkg/webview/view.pb.go
index f5a6ff609..5761f919e 100644
--- a/pkg/webview/view.pb.go
+++ b/pkg/webview/view.pb.go
@@ -741,12 +741,12 @@ func (x *VersionSettings) GetCheckUpdates() bool {
 
 // Our websocket service has two kinds of View messages:
 //
-// 1) On initialization, we send down the complete view state
-//    (TiltStartTime, UISession, UIResources, and LogList)
+//  1. On initialization, we send down the complete view state
+//     (TiltStartTime, UISession, UIResources, and LogList)
 //
-// 2) On every change, we send down the resources that have
-//    changed since the last send().
-//    (new logs and any updated UISession/UIResource objects).
+//  2. On every change, we send down the resources that have
+//     changed since the last send().
+//     (new logs and any updated UISession/UIResource objects).
 //
 // All other fields are obsolete, but are needed for deserializing
 // old snapshots.
@@ -1268,9 +1268,9 @@ func (x *UploadSnapshotResponse) GetUrl() string {
 // NOTE(nick): This is obsolete.
 //
 // Our websocket service has two kinds of messages:
-// 1) On initialization, we send down the complete view state
-// 2) On every change, we send down the resources that have
-//    changed since the last send().
+//  1. On initialization, we send down the complete view state
+//  2. On every change, we send down the resources that have
+//     changed since the last send().
 type AckWebsocketRequest struct {
 	state         protoimpl.MessageState
 	sizeCache     protoimpl.SizeCache


Test patch:
diff --git a/internal/controllers/core/cluster/reconciler_test.go b/internal/controllers/core/cluster/reconciler_test.go
index c29710da6..a4d30b283 100644
--- a/internal/controllers/core/cluster/reconciler_test.go
+++ b/internal/controllers/core/cluster/reconciler_test.go
@@ -291,9 +291,8 @@ func newFixture(t *testing.T) *fixture {
 		base,
 		"tilt-default")
 	requeueChan := make(chan indexer.RequeueForTestResult, 1)
-	indexer.StartSourceForTesting(cfb.Context(), r.requeuer, r, requeueChan)
 	return &fixture{
-		ControllerFixture: cfb.Build(r),
+		ControllerFixture: cfb.WithRequeuer(r.requeuer).WithRequeuerResultChan(requeueChan).Build(r),
 		r:                 r,
 		ma:                cfb.Analytics(),
 		clock:             clock,
diff --git a/internal/controllers/core/cmd/controller_test.go b/internal/controllers/core/cmd/controller_test.go
index 1aeafa35b..c04ec6c31 100644
--- a/internal/controllers/core/cmd/controller_test.go
+++ b/internal/controllers/core/cmd/controller_test.go
@@ -20,7 +20,6 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
-	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/internal/engine/local"
 	"github.com/tilt-dev/tilt/internal/store"
 	"github.com/tilt-dev/tilt/internal/testutils/configmap"
@@ -946,10 +945,9 @@ func newFixture(t *testing.T) *fixture {
 	sc := local.NewServerController(f.Client)
 	clock := clockwork.NewFakeClock()
 	c := NewController(f.Context(), fe, fpm, f.Client, st, clock, v1alpha1.NewScheme())
-	indexer.StartSourceForTesting(f.Context(), c.requeuer, c, nil)
 
 	return &fixture{
-		ControllerFixture: f.Build(c),
+		ControllerFixture: f.WithRequeuer(c.requeuer).Build(c),
 		st:                st,
 		fe:                fe,
 		fpm:               fpm,
diff --git a/internal/controllers/core/dockercomposelogstream/reconciler_test.go b/internal/controllers/core/dockercomposelogstream/reconciler_test.go
index cbc2126d0..401a16bc5 100644
--- a/internal/controllers/core/dockercomposelogstream/reconciler_test.go
+++ b/internal/controllers/core/dockercomposelogstream/reconciler_test.go
@@ -11,7 +11,6 @@ import (
 
 	"github.com/tilt-dev/tilt/internal/container"
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
-	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/internal/docker"
 	"github.com/tilt-dev/tilt/internal/dockercompose"
 	"github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
@@ -173,10 +172,9 @@ func newFixture(t *testing.T) *fixture {
 	dcCli := dockercompose.NewFakeDockerComposeClient(t, cfb.Context())
 	dCli := docker.NewFakeClient()
 	r := NewReconciler(cfb.Client, cfb.Store, dcCli, dCli)
-	indexer.StartSourceForTesting(cfb.Context(), r.requeuer, r, nil)
 
 	return &fixture{
-		ControllerFixture: cfb.Build(r),
+		ControllerFixture: cfb.WithRequeuer(r.requeuer).Build(r),
 		r:                 r,
 		dc:                dCli,
 		dcc:               dcCli,
diff --git a/internal/controllers/core/filewatch/controller_test.go b/internal/controllers/core/filewatch/controller_test.go
index 946f2b7ff..ba392f531 100644
--- a/internal/controllers/core/filewatch/controller_test.go
+++ b/internal/controllers/core/filewatch/controller_test.go
@@ -19,7 +19,6 @@ import (
 
 	"github.com/tilt-dev/tilt/internal/controllers/core/filewatch/fsevent"
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
-	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/internal/store"
 	"github.com/tilt-dev/tilt/internal/testutils/configmap"
 	"github.com/tilt-dev/tilt/internal/testutils/tempdir"
@@ -75,10 +74,8 @@ func newFixture(t *testing.T) *fixture {
 	clock := clockwork.NewFakeClock()
 	controller := NewController(cfb.Client, testingStore, fakeMultiWatcher.NewSub, timerMaker.Maker(), filewatches.NewScheme(), clock)
 
-	indexer.StartSourceForTesting(cfb.Context(), controller.requeuer, controller, nil)
-
 	return &fixture{
-		ControllerFixture: cfb.Build(controller),
+		ControllerFixture: cfb.WithRequeuer(controller.requeuer).Build(controller),
 		t:                 t,
 		tmpdir:            tmpdir,
 		controller:        controller,
diff --git a/internal/controllers/core/kubernetesdiscovery/reconciler_test.go b/internal/controllers/core/kubernetesdiscovery/reconciler_test.go
index 4baa012c7..a79cc5ed3 100644
--- a/internal/controllers/core/kubernetesdiscovery/reconciler_test.go
+++ b/internal/controllers/core/kubernetesdiscovery/reconciler_test.go
@@ -23,7 +23,6 @@ import (
 	"github.com/tilt-dev/tilt/internal/container"
 	"github.com/tilt-dev/tilt/internal/controllers/apis/cluster"
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
-	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/internal/k8s"
 	"github.com/tilt-dev/tilt/internal/timecmp"
 	"github.com/tilt-dev/tilt/pkg/apis"
@@ -722,10 +721,9 @@ func newFixture(t *testing.T) *fixture {
 	cfb := fake.NewControllerFixtureBuilder(t)
 	clients := cluster.NewFakeClientProvider(t, cfb.Client)
 	pw := NewReconciler(cfb.Client, cfb.Scheme(), clients, rd, cfb.Store)
-	indexer.StartSourceForTesting(cfb.Context(), pw.requeuer, pw, nil)
 
 	ret := &fixture{
-		ControllerFixture: cfb.Build(pw),
+		ControllerFixture: cfb.WithRequeuer(pw.requeuer).Build(pw),
 		r:                 pw,
 		ctx:               cfb.Context(),
 		t:                 t,
diff --git a/internal/controllers/core/podlogstream/podlogstreamcontroller_test.go b/internal/controllers/core/podlogstream/podlogstreamcontroller_test.go
index e7232ca5a..b6f52988d 100644
--- a/internal/controllers/core/podlogstream/podlogstreamcontroller_test.go
+++ b/internal/controllers/core/podlogstream/podlogstreamcontroller_test.go
@@ -558,11 +558,10 @@ func newPLMFixture(t testing.TB) *plmFixture {
 	st := newPLMStore(t, out)
 	podSource := NewPodSource(ctx, kClient, cfb.Client.Scheme(), clock)
 	plsc := NewController(ctx, cfb.Client, cfb.Scheme(), st, kClient, podSource, clock)
-	indexer.StartSourceForTesting(cfb.Context(), plsc.podSource, plsc, nil)
 
 	return &plmFixture{
 		t:                 t,
-		ControllerFixture: cfb.Build(plsc),
+		ControllerFixture: cfb.WithRequeuer(plsc.podSource).Build(plsc),
 		kClient:           kClient,
 		plsc:              plsc,
 		ctx:               ctx,
diff --git a/internal/controllers/core/portforward/reconciler_test.go b/internal/controllers/core/portforward/reconciler_test.go
index 079253f65..bf4801524 100644
--- a/internal/controllers/core/portforward/reconciler_test.go
+++ b/internal/controllers/core/portforward/reconciler_test.go
@@ -18,7 +18,6 @@ import (
 
 	"github.com/tilt-dev/tilt/internal/controllers/apis/cluster"
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
-	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/pkg/apis"
 
 	"github.com/tilt-dev/tilt/pkg/model"
@@ -353,10 +352,9 @@ func newPFRFixture(t *testing.T) *pfrFixture {
 	cfb := fake.NewControllerFixtureBuilder(t)
 	clients := cluster.NewFakeClientProvider(t, cfb.Client)
 	r := NewReconciler(cfb.Client, cfb.Scheme(), cfb.Store, clients)
-	indexer.StartSourceForTesting(cfb.Context(), r.requeuer, r, nil)
 
 	return &pfrFixture{
-		ControllerFixture: cfb.Build(r),
+		ControllerFixture: cfb.WithRequeuer(r.requeuer).Build(r),
 		t:                 t,
 		st:                cfb.Store,
 		r:                 r,
diff --git a/internal/controllers/indexer/list_test.go b/internal/controllers/indexer/list_test.go
index b5a05ee37..a548252f0 100644
--- a/internal/controllers/indexer/list_test.go
+++ b/internal/controllers/indexer/list_test.go
@@ -1,4 +1,4 @@
-package indexer
+package indexer_test
 
 import (
 	"context"
@@ -11,6 +11,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/tilt-dev/tilt/internal/controllers/fake"
+	"github.com/tilt-dev/tilt/internal/controllers/indexer"
 	"github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
 	"github.com/tilt-dev/tilt/pkg/logger"
 )
@@ -57,10 +58,10 @@ func TestListOwnedBy(t *testing.T) {
 	assert.NoError(t, c.Create(ctx, pls2a))
 
 	var plsList1 v1alpha1.PodLogStreamList
-	assert.NoError(t, ListOwnedBy(ctx, c, &plsList1, types.NamespacedName{Name: kd1.Name}, kd1.TypeMeta))
+	assert.NoError(t, indexer.ListOwnedBy(ctx, c, &plsList1, types.NamespacedName{Name: kd1.Name}, kd1.TypeMeta))
 	assert.ElementsMatch(t, []v1alpha1.PodLogStream{*pls1a, *pls1b}, plsList1.Items)
 
 	var plsList2 v1alpha1.PodLogStreamList
-	assert.NoError(t, ListOwnedBy(ctx, c, &plsList2, types.NamespacedName{Name: kd2.Name}, kd2.TypeMeta))
+	assert.NoError(t, indexer.ListOwnedBy(ctx, c, &plsList2, types.NamespacedName{Name: kd2.Name}, kd2.TypeMeta))
 	assert.ElementsMatch(t, []v1alpha1.PodLogStream{*pls2a}, plsList2.Items)
 }