k8s: send Pods and Jobs down the same update codepath as other resour…

…ces (#5552) We used to handle Pods and Jobs differently, because they were immutable. Now, the main codepath should handle immutability correctly.
tilt-dev · Mar 3, 2022 · fddce0b · fddce0b
1 parent a1cd93f
commit fddce0b
Show file tree

Hide file tree

Showing 4 changed files with 20 additions and 161 deletions.
diff --git a/internal/k8s/client.go b/internal/k8s/client.go
@@ -62,7 +62,11 @@ type KubeContextOverride string
 // NOTE(nick): This isn't right. DefaultNamespace is a function of your kubectl context.
 const DefaultNamespace = Namespace("default")
 
-var ForbiddenFieldsRe = regexp.MustCompile(`updates to .* are forbidden`)
+// Kubernetes uses "Forbidden" errors for a variety of field immutability errors.
+//
+// https://github.com/kubernetes/kubernetes/blob/5d6a793221370d890af6ea766d056af4e33f1118/pkg/apis/core/validation/validation.go#L4383
+// https://github.com/kubernetes/kubernetes/blob/5d6a793221370d890af6ea766d056af4e33f1118/pkg/apis/core/validation/validation.go#L4196
+var ForbiddenFieldsPrefix = "Forbidden:"
 
 func (pID PodID) Empty() bool    { return pID.String() == "" }
 func (pID PodID) String() string { return string(pID) }
@@ -320,10 +324,7 @@ func (k *K8sClient) ToRawKubeConfigLoader() clientcmd.ClientConfig {
 
 func (k *K8sClient) Upsert(ctx context.Context, entities []K8sEntity, timeout time.Duration) ([]K8sEntity, error) {
 	result := make([]K8sEntity, 0, len(entities))
-
-	mutable, immutable := MutableAndImmutableEntities(entities)
-
-	for _, e := range mutable {
+	for _, e := range entities {
 		innerCtx, cancel := context.WithTimeout(ctx, timeout)
 		defer cancel()
 
@@ -337,20 +338,6 @@ func (k *K8sClient) Upsert(ctx context.Context, entities []K8sEntity, timeout ti
 		result = append(result, newEntity...)
 	}
 
-	for _, e := range immutable {
-		innerCtx, cancel := context.WithTimeout(ctx, timeout)
-		defer cancel()
-
-		newEntities, err := k.deleteAndCreateEntity(innerCtx, e)
-		if err != nil {
-			if ctx.Err() == context.DeadlineExceeded {
-				return nil, timeoutError(timeout)
-			}
-			return nil, err
-		}
-		result = append(result, newEntities...)
-	}
-
 	return result, nil
 }
 
@@ -516,10 +503,11 @@ func (k *K8sClient) escalatingUpdate(ctx context.Context, entity K8sEntity) ([]K
 	}
 
 	if err != nil {
-		isImmutable := maybeImmutableFieldStderr(err.Error())
-		if isImmutable {
+		maybeImmutable := maybeImmutableFieldStderr(err.Error())
+		if maybeImmutable {
 			fallback = true
-			logger.Get(ctx).Infof("Updating %q failed: immutable field error", entity.Name())
+			logger.Get(ctx).Infof("Updating %q failed: %s", entity.Name(),
+				truncateErrorToOneLine(err.Error()))
 			logger.Get(ctx).Infof("Attempting to delete and re-create")
 			result, err = k.deleteAndCreateEntity(ctx, entity)
 		}
@@ -534,14 +522,23 @@ func (k *K8sClient) escalatingUpdate(ctx context.Context, entity K8sEntity) ([]K
 	return result, nil
 }
 
+func truncateErrorToOneLine(stderr string) string {
+	index := strings.Index(stderr, "\n")
+	if index != -1 {
+		return stderr[:index]
+	}
+	return stderr
+}
+
 // We're using kubectl, so we only get stderr, not structured errors.
 //
 // Take a wild guess if the update is failing due to immutable field errors.
 //
 // This should bias towards false positives (i.e., we think something is an
 // immutable field error when it's not).
 func maybeImmutableFieldStderr(stderr string) bool {
-	return strings.Contains(stderr, validation.FieldImmutableErrorMsg) || ForbiddenFieldsRe.Match([]byte(stderr))
+	return strings.Contains(stderr, validation.FieldImmutableErrorMsg) ||
+		strings.Contains(stderr, ForbiddenFieldsPrefix)
 }
 
 var MetadataAnnotationsTooLongRe = regexp.MustCompile(`metadata.annotations: Too long: must have at most \d+ bytes.*`)

diff --git a/internal/k8s/client_test.go b/internal/k8s/client_test.go
@@ -11,7 +11,6 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 	"helm.sh/helm/v3/pkg/kube"
 	v1 "k8s.io/api/core/v1"
 	"k8s.io/apimachinery/pkg/api/errors"
@@ -85,32 +84,6 @@ func TestDeleteMissingKind(t *testing.T) {
 		kinds)
 }
 
-func TestUpsertMutableAndImmutable(t *testing.T) {
-	f := newClientTestFixture(t)
-	eDeploy := MustParseYAMLFromString(t, testyaml.SanchoYAML)[0]
-	eJob := MustParseYAMLFromString(t, testyaml.JobYAML)[0]
-	eNamespace := MustParseYAMLFromString(t, testyaml.MyNamespaceYAML)[0]
-
-	_, err := f.k8sUpsert(f.ctx, []K8sEntity{eDeploy, eJob, eNamespace})
-	if !assert.Nil(t, err) {
-		t.FailNow()
-	}
-
-	require.Len(t, f.resourceClient.updates, 2)
-	require.Len(t, f.resourceClient.creates, 1)
-
-	// compare entities instead of strings because str > entity > string gets weird
-	call0Entity := NewK8sEntity(f.resourceClient.updates[0].Object)
-	call1Entity := NewK8sEntity(f.resourceClient.updates[1].Object)
-
-	// `apply` should preserve input order of entities (we sort them further upstream)
-	require.Equal(t, eDeploy, call0Entity, "expect call 0 to have applied deployment first (preserve input order)")
-	require.Equal(t, eNamespace, call1Entity, "expect call 0 to have applied namespace second (preserve input order)")
-
-	call2Entity := NewK8sEntity(f.resourceClient.creates[0].Object)
-	require.Equal(t, eJob, call2Entity, "expect create job")
-}
-
 func TestUpsertAnnotationTooLong(t *testing.T) {
 	f := newClientTestFixture(t)
 	postgres := MustParseYAMLFromString(t, testyaml.PostgresYAML)

diff --git a/internal/k8s/entity.go b/internal/k8s/entity.go
@@ -159,41 +159,6 @@ func CopyEntities(entities []K8sEntity) []K8sEntity {
 	return res
 }
 
-// MutableAndImmutableEntities returns two lists of k8s entities: mutable ones (that can simply be
-// `kubectl apply`'d), and immutable ones (such as jobs and pods, which will need to be `--force`'d).
-// (We assume input entities are already sorted in a safe order to apply -- see kustomize/ordering.go.)
-func MutableAndImmutableEntities(entities entityList) (mutable, immutable []K8sEntity) {
-	for _, e := range entities {
-		if e.ImmutableOnceCreated() {
-			immutable = append(immutable, e)
-			continue
-		}
-		mutable = append(mutable, e)
-	}
-
-	return mutable, immutable
-}
-
-func ImmutableEntities(entities []K8sEntity) []K8sEntity {
-	result := make([]K8sEntity, 0)
-	for _, e := range entities {
-		if e.ImmutableOnceCreated() {
-			result = append(result, e)
-		}
-	}
-	return result
-}
-
-func MutableEntities(entities []K8sEntity) []K8sEntity {
-	result := make([]K8sEntity, 0)
-	for _, e := range entities {
-		if !e.ImmutableOnceCreated() {
-			result = append(result, e)
-		}
-	}
-	return result
-}
-
 type LoadBalancerSpec struct {
 	Name      string
 	Namespace Namespace

diff --git a/internal/k8s/entity_test.go b/internal/k8s/entity_test.go
@@ -12,7 +12,6 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/tilt-dev/tilt/internal/k8s/testyaml"
-	"github.com/tilt-dev/tilt/internal/kustomize"
 )
 
 func TestTypedPodGVK(t *testing.T) {
@@ -50,43 +49,6 @@ func TestNamespace(t *testing.T) {
 	assert.Equal(t, "kube-system", string(entities[0].Namespace()))
 }
 
-func TestImmutableFilter(t *testing.T) {
-	yaml := fmt.Sprintf("%s\n---\n%s\n---\n%s", testyaml.JobYAML, testyaml.SanchoYAML, testyaml.PodYAML)
-	entities, err := ParseYAMLFromString(yaml)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	immEntities := ImmutableEntities(entities)
-	if len(immEntities) != 2 {
-		t.Fatalf("Expected 2 entities, actual: %d", len(immEntities))
-	}
-
-	if immEntities[0].GVK().Kind != "Job" {
-		t.Errorf("Expected Job entity, actual: %+v", immEntities)
-	}
-	if immEntities[1].GVK().Kind != "Pod" {
-		t.Errorf("Expected Pod entity, actual: %+v", immEntities)
-	}
-}
-
-func TestMutableFilter(t *testing.T) {
-	yaml := fmt.Sprintf("%s\n---\n%s", testyaml.JobYAML, testyaml.SanchoYAML)
-	entities, err := ParseYAMLFromString(yaml)
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	results := MutableEntities(entities)
-	if len(results) != 1 {
-		t.Fatalf("Expected 1 entity, actual: %d", len(results))
-	}
-
-	if results[0].GVK().Kind != "Deployment" {
-		t.Errorf("Expected Deployment entity, actual: %+v", results)
-	}
-}
-
 func TestLoadBalancerSpecs(t *testing.T) {
 	entities, err := ParseYAMLFromString(testyaml.BlorgBackendYAML)
 	if err != nil {
@@ -230,44 +192,6 @@ func TestSortEntities(t *testing.T) {
 	}
 }
 
-func TestMutableAndImmutableEntities(t *testing.T) {
-	for _, test := range []struct {
-		name                       string
-		inputKindOrder             []string
-		expectedMutableKindOrder   []string
-		expectedImmutableKindOrder []string
-	}{
-		{"only mutable",
-			[]string{"Deployment", "Namespace", "Service"},
-			[]string{"Deployment", "Namespace", "Service"},
-			[]string{},
-		},
-		{"only immutable",
-			[]string{"Job", "Pod"},
-			[]string{},
-			[]string{"Job", "Pod"},
-		},
-		{"mutable and immutable interspersed",
-			[]string{"Deployment", "Job", "Namespace", "Pod", "Service"},
-			[]string{"Deployment", "Namespace", "Service"},
-			[]string{"Job", "Pod"},
-		},
-		{"no explicitly sorted kinds are immutable",
-			// If any kinds in the explicit sort list are also immutable, things will get weird
-			kustomize.OrderFirst,
-			kustomize.OrderFirst,
-			[]string{},
-		},
-	} {
-		t.Run(test.name, func(t *testing.T) {
-			input := entitiesWithKinds(test.inputKindOrder)
-			mutable, immutable := MutableAndImmutableEntities(input)
-			assertKindOrder(t, test.expectedMutableKindOrder, mutable, "mutable entities")
-			assertKindOrder(t, test.expectedImmutableKindOrder, immutable, "immutable entities")
-		})
-	}
-}
-
 func TestClean(t *testing.T) {
 	yaml := `apiVersion: v1
 kind: Namespace