Repository: tilt-dev/tilt
Pull request #5141 (2021-11-05 16:38:53)

features: flip live_update_v2 back off by default
Hello @milas,

Please review the following commits I made in branch nicks/lu2:

722764e506f3ba7ba4412b36d7294316eb9f2fa7 (2021-11-05 12:38:46 -0400)
features: flip live_update_v2 back off by default

Code review reminders, by giving a LGTM you attest that:

* Commits are adequately tested
* Code is easy to understand and conforms to style guides
* Incomplete code is marked with TODOs
* Code is suitably instrumented with logging and metrics


Patch:
diff --git a/integration/Tiltfile b/integration/Tiltfile
index 3c9f16e8e..692ce3a82 100644
--- a/integration/Tiltfile
+++ b/integration/Tiltfile
@@ -1,5 +1,7 @@
 # -*- mode: Python -*-
 
+enable_feature('live_update_v2')
+
 # HACK: load namespaces on `tilt up` but not on `tilt down`
 load_namespace = not os.environ.get('SKIP_NAMESPACE', '')
 if load_namespace:
diff --git a/integration/live_update_only/Tiltfile b/integration/live_update_only/Tiltfile
index c551e9758..61eae8ee9 100644
--- a/integration/live_update_only/Tiltfile
+++ b/integration/live_update_only/Tiltfile
@@ -1,4 +1,6 @@
 
+include('../Tiltfile')
+
 # TODO(milas): this test is failing with live_update_v2 because the LU Reconciler
 #   stops after seeing a path that it can't sync, but a full BaD never happens,
 #   and the resource gets stuck in update Pending
diff --git a/internal/controllers/core/cmd/controller.go b/internal/controllers/core/cmd/controller.go
index 61824d075..163d12e72 100644
--- a/internal/controllers/core/cmd/controller.go
+++ b/internal/controllers/core/cmd/controller.go
@@ -473,7 +473,11 @@ func (c *Controller) updateStatus(name types.NamespacedName, update func(status
 
 	err = c.client.Status().Update(c.globalCtx, &cmd)
 	if err != nil && !apierrors.IsNotFound(err) {
-		c.st.Dispatch(store.NewErrorAction(fmt.Errorf("syncing to apiserver: %v", err)))
+		if c.globalCtx.Err() == nil {
+			// if the global context has been canceled, the controller is being torn down,
+			// so don't propagate a store error
+			c.st.Dispatch(store.NewErrorAction(fmt.Errorf("syncing to apiserver: %v", err)))
+		}
 		return
 	}
 
diff --git a/internal/controllers/core/filewatch/controller.go b/internal/controllers/core/filewatch/controller.go
index 6ff9ea660..15de61ffd 100644
--- a/internal/controllers/core/filewatch/controller.go
+++ b/internal/controllers/core/filewatch/controller.go
@@ -224,13 +224,21 @@ func (c *Controller) dispatchFileChangesLoop(ctx context.Context, st store.RStor
 			}
 		case <-ctx.Done():
 			return
-
 		case fsEvents, ok := <-eventsCh:
 			if !ok {
 				return
 			}
 			if err := w.recordEvent(ctx, c.Client, st, fsEvents); err != nil {
-				st.Dispatch(store.NewErrorAction(err))
+				if ctx.Err() == nil {
+					// there's an unavoidable race here - the context might have
+					// been canceled while we were recording the event, which will
+					// cause a failure, so we just ignore _any_ errors in this case
+					// (even if it was a non-context related error, this watcher is
+					// being disposed of, so it's no longer relevant)
+					st.Dispatch(store.NewErrorAction(err))
+				} else {
+					logger.Get(ctx).Debugf("Ignored stale error for %q: %v", w.name, err)
+				}
 				return
 			}
 		}
diff --git a/internal/controllers/core/filewatch/fsevent/fake_watcher.go b/internal/controllers/core/filewatch/fsevent/fake_watcher.go
index 5b3a47d73..f5cd81b3c 100644
--- a/internal/controllers/core/filewatch/fsevent/fake_watcher.go
+++ b/internal/controllers/core/filewatch/fsevent/fake_watcher.go
@@ -3,6 +3,7 @@ package fsevent
 import (
 	"path/filepath"
 	"sync"
+	"sync/atomic"
 
 	"github.com/tilt-dev/tilt/internal/ospath"
 	"github.com/tilt-dev/tilt/internal/watch"
@@ -20,7 +21,10 @@ type FakeMultiWatcher struct {
 }
 
 func NewFakeMultiWatcher() *FakeMultiWatcher {
-	r := &FakeMultiWatcher{Events: make(chan watch.FileEvent, 20), Errors: make(chan error, 20)}
+	r := &FakeMultiWatcher{
+		Events: make(chan watch.FileEvent, 20),
+		Errors: make(chan error, 20),
+	}
 	go r.loop()
 	return r
 }
@@ -92,6 +96,8 @@ type FakeWatcher struct {
 	outboundCh chan watch.FileEvent
 	errorCh    chan error
 
+	eventCount uint64
+
 	paths  []string
 	ignore watch.PathMatcher
 }
@@ -141,6 +147,14 @@ func (w *FakeWatcher) Events() chan watch.FileEvent {
 	return w.outboundCh
 }
 
+func (w *FakeWatcher) TotalEventCount() uint64 {
+	return atomic.LoadUint64(&w.eventCount)
+}
+
+func (w *FakeWatcher) QueuedCount() int {
+	return len(w.outboundCh)
+}
+
 func (w *FakeWatcher) loop() {
 	var q []watch.FileEvent
 	for {
@@ -154,6 +168,7 @@ func (w *FakeWatcher) loop() {
 		} else {
 			e := q[0]
 			w.outboundCh <- e
+			atomic.AddUint64(&w.eventCount, 1)
 			q = q[1:]
 		}
 	}
diff --git a/internal/controllers/fake/client.go b/internal/controllers/fake/client.go
index ba101f7dd..82c128a18 100644
--- a/internal/controllers/fake/client.go
+++ b/internal/controllers/fake/client.go
@@ -18,6 +18,12 @@ type fakeTiltClient struct {
 //
 // We simulate this behavior to catch this class of bug.
 func (f fakeTiltClient) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	if ctxErr := ctx.Err(); ctxErr != nil {
+		// controller-runtime fake ignores context; check it here to allow controllers to test
+		// handling of cancellation related errors
+		return ctxErr
+	}
+
 	err := f.Client.Update(ctx, obj, opts...)
 	if err != nil {
 		return err
@@ -39,6 +45,12 @@ type fakeStatusWriter struct {
 }
 
 func (f fakeStatusWriter) Update(ctx context.Context, obj client.Object, opts ...client.UpdateOption) error {
+	if ctxErr := ctx.Err(); ctxErr != nil {
+		// controller-runtime fake ignores context; check it here to allow controllers to test
+		// handling of cancellation related errors
+		return ctxErr
+	}
+
 	err := f.StatusWriter.Update(ctx, obj, opts...)
 	if err != nil {
 		return err
diff --git a/internal/feature/flags.go b/internal/feature/flags.go
index 617db90bd..aeb0833cd 100644
--- a/internal/feature/flags.go
+++ b/internal/feature/flags.go
@@ -64,7 +64,7 @@ var MainDefaults = Defaults{
 		Status:  Active,
 	},
 	LiveUpdateV2: Value{
-		Enabled: true,
+		Enabled: false,
 		Status:  Active,
 	},
 	DisableResources: Value{


Test patch:
diff --git a/internal/controllers/core/filewatch/controller_test.go b/internal/controllers/core/filewatch/controller_test.go
index 1cfb4d91c..6a8cc1322 100644
--- a/internal/controllers/core/filewatch/controller_test.go
+++ b/internal/controllers/core/filewatch/controller_test.go
@@ -6,6 +6,7 @@ import (
 	"reflect"
 	"runtime"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -348,3 +349,42 @@ func TestController_Disable_Ignores_File_Changes(t *testing.T) {
 	f.MustGet(key, &fwAfterDisable)
 	require.Equal(t, 0, len(fwAfterDisable.Status.FileEvents))
 }
+
+func TestController_IgnoreErrorsOnCancel(t *testing.T) {
+	if testing.Short() {
+		t.Skip("skipping test in short mode")
+	}
+
+	f := newFixture(t)
+	key, _ := f.CreateSimpleFileWatch()
+
+	tw := f.controller.targetWatches[key]
+	require.NotNil(t, tw, "Watcher never created")
+
+	// HACK(milas): this is seriously misusing the internals of this controller to
+	// 	force the synchronization so we can cancel the context at the right time
+	tw.mu.Lock()
+
+	watcher := tw.notify.(*fsevent.FakeWatcher)
+	require.Zero(t, watcher.TotalEventCount(), "No events should have been seen yet")
+
+	f.ChangeFile("a", "1")
+
+	require.Eventually(t, func() bool {
+		return watcher.TotalEventCount() == 1
+	}, time.Second, 20*time.Millisecond, "Event never seen")
+
+	require.Eventually(t, func() bool {
+		return watcher.QueuedCount() == 0
+	}, time.Second, 20*time.Millisecond, "Event never consumed")
+
+	// at this point, the watcher is blocked trying to report the event, so
+	// we'll cancel the context on it, simulating a race, to force a failure
+	tw.cancel()
+	tw.mu.Unlock()
+
+	require.Eventually(t, func() bool {
+		return strings.Contains(f.Stdout(),
+			`Ignored stale error for "TestController_IgnoreErrorsOnCancel/test-file-watch": apiserver update status error: context canceled`)
+	}, time.Second, 10*time.Millisecond, "Error ignored log message never seen")
+}