engine: improve behavior prior to DisableStatus reconciliation (#5411)

internal/controllers/apis/configmap/disable.go

@@ -13,33 +13,41 @@ import (
 	"github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
 )
 
-func DisableStatus(getCM func(name string) (v1alpha1.ConfigMap, error), disableSource *v1alpha1.DisableSource) (isDisabled bool, reason string, err error) {
+type DisableResult int
+
+func DisableStatus(getCM func(name string) (v1alpha1.ConfigMap, error), disableSource *v1alpha1.DisableSource) (result v1alpha1.DisableState, reason string, err error) {
 	if disableSource == nil {
-		return false, "object does not specify a DisableSource", nil
+		// if there is no source, assume the object has opted out of being disabled and is always eanbled
+		return v1alpha1.DisableStateEnabled, "object does not specify a DisableSource", nil
 	}
 
 	if disableSource.ConfigMap == nil {
-		return false, "DisableSource specifies no ConfigMap", nil
+		return v1alpha1.DisableStateError, "DisableSource specifies no ConfigMap", nil
 	}
 
 	cm, err := getCM(disableSource.ConfigMap.Name)
 	if err != nil {
 		if apierrors.IsNotFound(err) {
-			return false, fmt.Sprintf("ConfigMap %q does not exist", disableSource.ConfigMap.Name), nil
+			return v1alpha1.DisableStatePending, fmt.Sprintf("ConfigMap %q does not exist", disableSource.ConfigMap.Name), nil
 		}
-		return false, fmt.Sprintf("error reading ConfigMap %q", disableSource.ConfigMap.Name), err
+		return v1alpha1.DisableStatePending, fmt.Sprintf("error reading ConfigMap %q", disableSource.ConfigMap.Name), err
 	}
 
 	cmVal, ok := cm.Data[disableSource.ConfigMap.Key]
 	if !ok {
-		return false, fmt.Sprintf("ConfigMap %q has no key %q", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key), nil
+		return v1alpha1.DisableStateError, fmt.Sprintf("ConfigMap %q has no key %q", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key), nil
 	}
-	isDisabled, err = strconv.ParseBool(cmVal)
+	isDisabled, err := strconv.ParseBool(cmVal)
 	if err != nil {
-		return false, fmt.Sprintf("error parsing ConfigMap/key %q/%q value %q as a bool: %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, cmVal, err.Error()), nil
+		return v1alpha1.DisableStateError, fmt.Sprintf("error parsing ConfigMap/key %q/%q value %q as a bool: %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, cmVal, err.Error()), nil
 	}
 
-	return isDisabled, fmt.Sprintf("ConfigMap/key %q/%q is %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, isDisabled), nil
+	if isDisabled {
+		result = v1alpha1.DisableStateDisabled
+	} else {
+		result = v1alpha1.DisableStateEnabled
+	}
+	return result, fmt.Sprintf("ConfigMap/key %q/%q is %v", disableSource.ConfigMap.Name, disableSource.ConfigMap.Key, isDisabled), nil
 }
 
 // Returns a new DisableStatus if the disable status has changed, or the prev status if it hasn't.
@@ -50,16 +58,22 @@ func MaybeNewDisableStatus(ctx context.Context, client client.Client, disableSou
 		return cm, err
 	}
 
-	isDisabled, reason, err := DisableStatus(getCM, disableSource)
+	result, reason, err := DisableStatus(getCM, disableSource)
 	if err != nil {
 		return nil, err
 	}
-	statusDiffers := prevStatus == nil || prevStatus.Disabled != isDisabled || prevStatus.Reason != reason
+	// we treat pending as disabled
+	// eventually we should probably represent isDisabled by an enum in the API, but for now
+	// we treat pending as disabled, with the understanding that it's better to momentarily delay the start of an
+	// object than to spin it up and quickly kill it, as the latter might generate undesired side effects / logs
+	isDisabled := result == v1alpha1.DisableStateDisabled || result == v1alpha1.DisableStatePending || result == v1alpha1.DisableStateError
+	statusDiffers := prevStatus == nil || prevStatus.State != result || prevStatus.Reason != reason
 	if statusDiffers {
 		return &v1alpha1.DisableStatus{
 			Disabled:       isDisabled,
 			LastUpdateTime: apis.Now(),
 			Reason:         reason,
+			State:          result,
 		}, nil
 	}
 	return prevStatus, nil

internal/controllers/apis/configmap/disable_test.go

@@ -23,6 +23,7 @@ func TestMaybeNewDisableStatusNoSource(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
 	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateEnabled, newStatus.State)
 	require.Contains(t, newStatus.Reason, "does not specify a DisableSource")
 }
 
@@ -31,7 +32,8 @@ func TestMaybeNewDisableStatusNoConfigMapDisableSource(t *testing.T) {
 	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, &v1alpha1.DisableSource{}, nil)
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
-	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateError, newStatus.State)
 	require.Contains(t, newStatus.Reason, "specifies no ConfigMap")
 }
 
@@ -40,7 +42,8 @@ func TestMaybeNewDisableStatusNoConfigMap(t *testing.T) {
 	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, disableSource(), nil)
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
-	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStatePending, newStatus.State)
 	require.Contains(t, newStatus.Reason, "ConfigMap \"fe-disable\" does not exist")
 }
 
@@ -50,7 +53,8 @@ func TestMaybeNewDisableStatusNoKey(t *testing.T) {
 	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, disableSource(), nil)
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
-	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateError, newStatus.State)
 	require.Contains(t, newStatus.Reason, "has no key")
 }
 
@@ -61,6 +65,7 @@ func TestMaybeNewDisableStatusTrue(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
 	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateDisabled, newStatus.State)
 	require.Contains(t, newStatus.Reason, "is true")
 }
 
@@ -71,6 +76,7 @@ func TestMaybeNewDisableStatusFalse(t *testing.T) {
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
 	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateEnabled, newStatus.State)
 	require.Contains(t, newStatus.Reason, "is false")
 }
 
@@ -80,7 +86,8 @@ func TestMaybeNewDisableStatusGobbledygookValue(t *testing.T) {
 	newStatus, err := MaybeNewDisableStatus(f.ctx, f.fc, disableSource(), nil)
 	require.NoError(t, err)
 	require.NotNil(t, newStatus)
-	require.Equal(t, false, newStatus.Disabled)
+	require.Equal(t, true, newStatus.Disabled)
+	require.Equal(t, v1alpha1.DisableStateError, newStatus.State)
 	require.Contains(t, newStatus.Reason, "strconv.ParseBool: parsing \"asdf\"")
 }
 

internal/controllers/core/cmd/controller.go

@@ -169,7 +169,7 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		status.DisableStatus = disableStatus
 	})
 
-	disabled := disableStatus.Disabled
+	disabled := disableStatus.State == v1alpha1.DisableStateDisabled
 	if disabled {
 		// Disabling should both stop the process, and make it look like
 		// it didn't previously run.

internal/controllers/core/cmd/controller_test.go

@@ -23,6 +23,7 @@ import (
 	"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"
 	"github.com/tilt-dev/tilt/pkg/apis"
 	"github.com/tilt-dev/tilt/pkg/apis/core/v1alpha1"
 	"github.com/tilt-dev/tilt/pkg/model"
@@ -529,23 +530,23 @@ func TestDisableCmd(t *testing.T) {
 	f.requireCmdMatchesInAPI(cmd.Name, func(cmd *Cmd) bool {
 		return cmd.Status.Running != nil &&
 			cmd.Status.DisableStatus != nil &&
-			!cmd.Status.DisableStatus.Disabled
+			cmd.Status.DisableStatus.State == v1alpha1.DisableStateEnabled
 	})
 
 	f.setDisabled(cmd.Name, true)
 
 	f.requireCmdMatchesInAPI(cmd.Name, func(cmd *Cmd) bool {
 		return cmd.Status.Terminated != nil &&
 			cmd.Status.DisableStatus != nil &&
-			cmd.Status.DisableStatus.Disabled
+			cmd.Status.DisableStatus.State == v1alpha1.DisableStateDisabled
 	})
 
 	f.setDisabled(cmd.Name, false)
 
 	f.requireCmdMatchesInAPI(cmd.Name, func(cmd *Cmd) bool {
 		return cmd.Status.Running != nil &&
 			cmd.Status.DisableStatus != nil &&
-			!cmd.Status.DisableStatus.Disabled
+			cmd.Status.DisableStatus.State == v1alpha1.DisableStateEnabled
 	})
 }
 
@@ -574,15 +575,15 @@ func TestReenable(t *testing.T) {
 	f.requireCmdMatchesInAPI(cmd.Name, func(cmd *Cmd) bool {
 		return cmd.Status.Running == nil &&
 			cmd.Status.DisableStatus != nil &&
-			cmd.Status.DisableStatus.Disabled
+			cmd.Status.DisableStatus.State == v1alpha1.DisableStateDisabled
 	})
 
 	f.setDisabled(cmd.Name, false)
 
 	f.requireCmdMatchesInAPI(cmd.Name, func(cmd *Cmd) bool {
 		return cmd.Status.Running != nil &&
 			cmd.Status.DisableStatus != nil &&
-			!cmd.Status.DisableStatus.Disabled
+			cmd.Status.DisableStatus.State == v1alpha1.DisableStateEnabled
 	})
 }
 
@@ -593,20 +594,17 @@ func TestDisableServeCmd(t *testing.T) {
 	t1 := time.Unix(1, 0)
 	localTarget := model.NewLocalTarget("foo", model.Cmd{}, model.ToHostCmd("."), nil)
 	localTarget.ServeCmdDisableSource = &ds
+	err := configmap.UpsertDisableConfigMap(f.Context(), f.Client, ds.ConfigMap.Name, ds.ConfigMap.Key, false)
+	require.NoError(t, err)
+
 	f.resourceFromTarget("foo", localTarget, t1)
 
 	f.step()
 	f.requireCmdMatchesInAPI("foo-serve-1", func(cmd *Cmd) bool {
 		return cmd != nil && cmd.Status.Running != nil
 	})
 
-	cm := ConfigMap{
-		ObjectMeta: ObjectMeta{Name: ds.ConfigMap.Name},
-		Data: map[string]string{
-			ds.ConfigMap.Key: "true",
-		},
-	}
-	err := f.Client.Create(f.Context(), &cm)
+	err = configmap.UpsertDisableConfigMap(f.Context(), f.Client, ds.ConfigMap.Name, ds.ConfigMap.Key, true)
 	require.NoError(t, err)
 
 	f.step()
@@ -617,13 +615,7 @@ func TestEnableServeCmd(t *testing.T) {
 	f := newFixture(t)
 
 	ds := v1alpha1.DisableSource{ConfigMap: &v1alpha1.ConfigMapDisableSource{Name: "disable-foo", Key: "isDisabled"}}
-	cm := ConfigMap{
-		ObjectMeta: ObjectMeta{Name: ds.ConfigMap.Name},
-		Data: map[string]string{
-			ds.ConfigMap.Key: "true",
-		},
-	}
-	err := f.Client.Create(f.Context(), &cm)
+	err := configmap.UpsertDisableConfigMap(f.Context(), f.Client, ds.ConfigMap.Name, ds.ConfigMap.Key, true)
 	require.NoError(t, err)
 
 	t1 := time.Unix(1, 0)
@@ -633,8 +625,7 @@ func TestEnableServeCmd(t *testing.T) {
 
 	f.step()
 	f.assertCmdCount(0)
-	cm.Data[ds.ConfigMap.Key] = "false"
-	err = f.Client.Update(f.Context(), &cm)
+	err = configmap.UpsertDisableConfigMap(f.Context(), f.Client, ds.ConfigMap.Name, ds.ConfigMap.Key, false)
 	require.NoError(t, err)
 
 	f.step()
@@ -1002,10 +993,17 @@ func (f *fixture) setDisabled(cmdName string, isDisabled bool) {
 
 	f.reconcileCmd(cmdName)
 
+	var expectedDisableState v1alpha1.DisableState
+	if isDisabled {
+		expectedDisableState = v1alpha1.DisableStateDisabled
+	} else {
+		expectedDisableState = v1alpha1.DisableStateEnabled
+	}
+
 	// block until the change has been processed
 	f.requireCmdMatchesInAPI(cmdName, func(cmd *Cmd) bool {
 		return cmd.Status.DisableStatus != nil &&
-			cmd.Status.DisableStatus.Disabled == isDisabled
+			cmd.Status.DisableStatus.State == expectedDisableState
 	})
 }
 

internal/controllers/core/filewatch/controller.go

@@ -109,7 +109,7 @@ func (c *Controller) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 
 	// Clean up existing filewatches if it's disabled
 	result := ctrl.Result{}
-	if disableStatus.Disabled {
+	if disableStatus.State == v1alpha1.DisableStateDisabled {
 		if hasExisting {
 			existing.cleanupWatch(ctx)
 			c.removeWatch(existing)

internal/controllers/core/filewatch/controller_test.go

@@ -13,7 +13,6 @@ import (
 	"github.com/jonboulle/clockwork"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
@@ -22,6 +21,7 @@ import (
 	"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"
 	"github.com/tilt-dev/tilt/internal/watch"
 	"github.com/tilt-dev/tilt/pkg/apis"
@@ -151,6 +151,8 @@ func (f *fixture) CreateSimpleFileWatch() (types.NamespacedName, *filewatches.Fi
 		},
 	}
 	f.Create(fw)
+
+	f.setDisabled(types.NamespacedName{Namespace: fw.Namespace, Name: fw.Name}, false)
 	return f.KeyForObject(fw), fw
 }
 
@@ -168,22 +170,9 @@ func (f *fixture) setDisabled(key types.NamespacedName, isDisabled bool) {
 	require.NotNil(f.T(), fw.Spec.DisableSource)
 	require.NotNil(f.T(), fw.Spec.DisableSource.ConfigMap)
 
-	// Make sure that the configmap exists
-	configmap := &filewatches.ConfigMap{}
-	err = f.Client.Get(f.Context(), types.NamespacedName{Name: fw.Spec.DisableSource.ConfigMap.Name}, configmap)
-	// If the configmap doesn't exist, create it
-	if apierrors.IsNotFound(err) {
-		configmap.ObjectMeta.Name = fw.Spec.DisableSource.ConfigMap.Name
-		configmap.Data = map[string]string{fw.Spec.DisableSource.ConfigMap.Key: strconv.FormatBool(isDisabled)}
-		err = f.Client.Create(f.Context(), configmap)
-		require.NoError(f.T(), err)
-	} else {
-		// Otherwise, update the existing configmap
-		require.Nil(f.T(), err)
-		configmap.Data[fw.Spec.DisableSource.ConfigMap.Key] = strconv.FormatBool(isDisabled)
-		err = f.Client.Update(f.Context(), configmap)
-		require.NoError(f.T(), err)
-	}
+	ds := fw.Spec.DisableSource.ConfigMap
+	err = configmap.UpsertDisableConfigMap(f.Context(), f.Client, ds.Name, ds.Key, isDisabled)
+	require.NoError(f.T(), err)
 
 	f.reconcileFw(key)
 
@@ -355,13 +344,13 @@ func TestController_Disable_By_Configmap(t *testing.T) {
 	key, _ := f.CreateSimpleFileWatch()
 
 	// when enabling the configmap, the filewatch object is enabled
-	f.setDisabled(key, true)
+	f.setDisabled(key, false)
 
 	// when disabling the configmap, the filewatch object is disabled
-	f.setDisabled(key, false)
+	f.setDisabled(key, true)
 
 	// when enabling the configmap, the filewatch object is enabled
-	f.setDisabled(key, true)
+	f.setDisabled(key, false)
 }
 
 func TestController_Disable_Ignores_File_Changes(t *testing.T) {

internal/controllers/core/kubernetesapply/reconciler.go

@@ -144,7 +144,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (
 	}
 
 	// Delete kubernetesapply if it's disabled
-	if disableStatus.Disabled {
+	if disableStatus.State == v1alpha1.DisableStateDisabled {
 		err := r.deleteCreatedObjects(ctx, nn, "deleting disabled Kubernetes objects")
 		if err != nil {
 			return ctrl.Result{}, err