feat: require and then remove karpenter startup taint

Makefile

@@ -69,7 +69,7 @@ delete: ## Delete the controller from your ~/.kube/config cluster
 test: ## Run tests
 	go test ./pkg/... \
 		-race \
-		-timeout 15m \
+		-timeout 20m \
 		--ginkgo.focus="${FOCUS}" \
 		--ginkgo.randomize-all \
 		--ginkgo.v \

go.mod

@@ -15,7 +15,7 @@ require (
 	github.com/patrickmn/go-cache v2.1.0+incompatible
 	github.com/prometheus/client_golang v1.19.1
 	github.com/prometheus/client_model v0.6.1
-	github.com/samber/lo v1.43.0
+	github.com/samber/lo v1.44.0
 	go.uber.org/multierr v1.11.0
 	go.uber.org/zap v1.27.0
 	golang.org/x/sync v0.7.0

go.sum

@@ -287,8 +287,8 @@ github.com/rogpeppe/go-internal v1.3.0/go.mod h1:M8bDsm7K2OlrFYOpmOWEs/qY81heoFR
 github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ=
 github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog=
 github.com/russross/blackfriday/v2 v2.1.0/go.mod h1:+Rmxgy9KzJVeS9/2gXHxylqXiyQDYRxCVz55jmeOWTM=
-github.com/samber/lo v1.43.0 h1:ts0VhPi8+ZQZFVLv/2Vkgt2Cds05FM2v3Enmv+YMBtg=
-github.com/samber/lo v1.43.0/go.mod h1:w7R6fO7h2lrnx/s0bWcZ55vXJI89p5UPM6+kyDL373E=
+github.com/samber/lo v1.44.0 h1:5il56KxRE+GHsm1IR+sZ/6J42NODigFiqCWpSc2dybA=
+github.com/samber/lo v1.44.0/go.mod h1:RmDH9Ct32Qy3gduHQuKJ3gW1fMHAnE/fAzQuf6He5cU=
 github.com/sirupsen/logrus v1.2.0/go.mod h1:LxeOpSwHxABJmUn/MG1IvRgCAasNZTLOkJPxbbu5VWo=
 github.com/sirupsen/logrus v1.4.2/go.mod h1:tLMulIdttU9McNUspp0xgXVQah82FyeX6MwdIuYE2rE=
 github.com/sirupsen/logrus v1.6.0/go.mod h1:7uNnSEd1DgxDLC74fIahvMZmmYsHGZGEOFrfsX/uA88=

pkg/apis/v1beta1/taints.go

@@ -26,6 +26,7 @@ import (
 const (
 	DisruptionTaintKey             = apis.Group + "/disruption"
 	DisruptingNoScheduleTaintValue = "disrupting"
+	UnregisteredTaintKey           = apis.Group + "/unregistered"
 )
 
 var (
@@ -36,6 +37,10 @@ var (
 		Effect: v1.TaintEffectNoSchedule,
 		Value:  DisruptingNoScheduleTaintValue,
 	}
+	UnregisteredNoExecuteTaint = v1.Taint{
+		Key:    UnregisteredTaintKey,
+		Effect: v1.TaintEffectNoExecute,
+	}
 )
 
 func IsDisruptingTaint(taint v1.Taint) bool {

pkg/cloudprovider/types.go

@@ -199,7 +199,7 @@ func (its InstanceTypes) SatisfiesMinValues(requirements scheduling.Requirements
 // Truncate truncates the InstanceTypes based on the passed-in requirements
 // It returns an error if it isn't possible to truncate the instance types on maxItems without violating minValues
 func (its InstanceTypes) Truncate(requirements scheduling.Requirements, maxItems int) (InstanceTypes, error) {
-	truncatedInstanceTypes := InstanceTypes(lo.Slice(its.OrderByPrice(requirements), 0, maxItems))
+	truncatedInstanceTypes := lo.Slice(its.OrderByPrice(requirements), 0, maxItems)
 	// Only check for a validity of NodeClaim if its requirement has minValues in it.
 	if requirements.HasMinValues() {
 		if _, err := truncatedInstanceTypes.SatisfiesMinValues(requirements); err != nil {

pkg/controllers/nodeclaim/disruption/suite_test.go

@@ -113,6 +113,7 @@ var _ = Describe("Disruption", func() {
 		Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty).IsTrue()).To(BeTrue())
 	})
 	It("should remove multiple disruption conditions simultaneously", func() {
+		cp.Drifted = ""
 		nodePool.Spec.Disruption.ExpireAfter.Duration = nil
 		nodePool.Spec.Disruption.ConsolidateAfter = &v1beta1.NillableDuration{Duration: nil}
 
@@ -124,7 +125,7 @@ var _ = Describe("Disruption", func() {
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimDisruptionController, nodeClaim)
 
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted).IsTrue()).To(BeTrue())
+		Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeDrifted)).To(BeNil())
 		Expect(nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeEmpty)).To(BeNil())
 	})
 })

pkg/controllers/nodeclaim/lifecycle/initialization_test.go

@@ -61,6 +61,7 @@ var _ = Describe("Initialization", func() {
 
 		node := test.Node(test.NodeOptions{
 			ProviderID: nodeClaim.Status.ProviderID,
+			Taints:     []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -122,6 +123,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourceMemory: resource.MustParse("80Mi"),
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -165,6 +167,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
 			ReadyStatus: v1.ConditionFalse,
+			Taints:      []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
@@ -213,6 +216,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourceMemory: resource.MustParse("80Mi"),
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -263,6 +267,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourceMemory: resource.MustParse("80Mi"),
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -330,6 +335,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourceMemory: resource.MustParse("80Mi"),
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -401,6 +407,7 @@ var _ = Describe("Initialization", func() {
 				v1.ResourceMemory: resource.MustParse("80Mi"),
 				v1.ResourcePods:   resource.MustParse("110"),
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectMakeNodesReady(ctx, env.Client, node) // Remove the not-ready taint
@@ -480,6 +487,7 @@ var _ = Describe("Initialization", func() {
 					Effect: v1.TaintEffectNoSchedule,
 					Value:  "true",
 				},
+				v1beta1.UnregisteredNoExecuteTaint,
 			},
 		})
 		ExpectApplied(ctx, env.Client, node)
@@ -549,6 +557,7 @@ var _ = Describe("Initialization", func() {
 					Effect: v1.TaintEffectNoSchedule,
 					Value:  "true",
 				},
+				v1beta1.UnregisteredNoExecuteTaint,
 			},
 		})
 		ExpectApplied(ctx, env.Client, node)

pkg/controllers/nodeclaim/lifecycle/registration.go

@@ -61,9 +61,14 @@ func (r *Registration) Reconcile(ctx context.Context, nodeClaim *v1beta1.NodeCla
 		}
 		return reconcile.Result{}, fmt.Errorf("getting node for nodeclaim, %w", err)
 	}
+	if _, hasStartupTaint := lo.Find(node.Spec.Taints, func(t v1.Taint) bool {
+		return t.MatchTaint(&v1beta1.UnregisteredNoExecuteTaint)
+	}); !hasStartupTaint {
+		return reconcile.Result{}, fmt.Errorf("missing required startup taint, %s", v1beta1.UnregisteredTaintKey)
+	}
 	ctx = log.IntoContext(ctx, log.FromContext(ctx).WithValues("Node", klog.KRef("", node.Name)))
 	if err = r.syncNode(ctx, nodeClaim, node); err != nil {
-		return reconcile.Result{}, fmt.Errorf("syncing node, %w", err)
+		return reconcile.Result{Requeue: true}, err
 	}
 	log.FromContext(ctx).Info("registered nodeclaim")
 	nodeClaim.StatusConditions().SetTrue(v1beta1.ConditionTypeRegistered)
@@ -88,12 +93,17 @@ func (r *Registration) syncNode(ctx context.Context, nodeClaim *v1beta1.NodeClai
 	// Sync all taints inside NodeClaim into the Node taints
 	node.Spec.Taints = scheduling.Taints(node.Spec.Taints).Merge(nodeClaim.Spec.Taints)
 	node.Spec.Taints = scheduling.Taints(node.Spec.Taints).Merge(nodeClaim.Spec.StartupTaints)
+	// Remove karpenter.sh/unregistered taint
+	taints := lo.Reject(node.Spec.Taints, func(t v1.Taint, _ int) bool {
+		return t.MatchTaint(&v1beta1.UnregisteredNoExecuteTaint)
+	})
 	node.Labels = lo.Assign(node.Labels, nodeClaim.Labels, map[string]string{
 		v1beta1.NodeRegisteredLabelKey: "true",
 	})
+	node.Spec.Taints = taints
 	if !equality.Semantic.DeepEqual(stored, node) {
-		if err := r.kubeClient.Patch(ctx, node, client.StrategicMergeFrom(stored)); err != nil {
-			return fmt.Errorf("syncing node labels, %w", err)
+		if err := r.kubeClient.Update(ctx, node); err != nil {
+			return fmt.Errorf("syncing node, %w", err)
 		}
 	}
 	return nil

pkg/controllers/nodeclaim/lifecycle/registration_test.go

@@ -46,7 +46,7 @@ var _ = Describe("Registration", func() {
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 
@@ -66,7 +66,7 @@ var _ = Describe("Registration", func() {
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 
@@ -85,12 +85,32 @@ var _ = Describe("Registration", func() {
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)
 		Expect(node.Labels).To(HaveKeyWithValue(v1beta1.NodeRegisteredLabelKey, "true"))
 	})
+	It("should remove the karpenter.sh/unregistered taint from the Node when the Node comes online", func() {
+		nodeClaim := test.NodeClaim(v1beta1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					v1beta1.NodePoolLabelKey: nodePool.Name,
+				},
+			},
+		})
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
+		ExpectApplied(ctx, env.Client, node)
+		Expect(node.Spec.Taints).To(ContainElement(v1beta1.UnregisteredNoExecuteTaint))
+		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
+		node = ExpectExists(ctx, env.Client, node)
+		Expect(node.Labels).To(HaveKeyWithValue(v1beta1.NodeRegisteredLabelKey, "true"))
+		Expect(node.Spec.Taints).To(Not(ContainElement(v1beta1.UnregisteredNoExecuteTaint)))
+	})
 	It("should sync the labels to the Node when the Node comes online", func() {
 		nodeClaim := test.NodeClaim(v1beta1.NodeClaim{
 			ObjectMeta: metav1.ObjectMeta{
@@ -107,7 +127,7 @@ var _ = Describe("Registration", func() {
 		Expect(nodeClaim.Labels).To(HaveKeyWithValue("custom-label", "custom-value"))
 		Expect(nodeClaim.Labels).To(HaveKeyWithValue("other-custom-label", "other-custom-value"))
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)
@@ -135,7 +155,7 @@ var _ = Describe("Registration", func() {
 		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1beta1.DoNotDisruptAnnotationKey, "true"))
 		Expect(nodeClaim.Annotations).To(HaveKeyWithValue("my-custom-annotation", "my-custom-value"))
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)
@@ -183,7 +203,7 @@ var _ = Describe("Registration", func() {
 			},
 		))
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)
@@ -251,7 +271,7 @@ var _ = Describe("Registration", func() {
 			},
 		))
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)
@@ -305,7 +325,7 @@ var _ = Describe("Registration", func() {
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 
-		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID})
+		node := test.Node(test.NodeOptions{ProviderID: nodeClaim.Status.ProviderID, Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint}})
 		ExpectApplied(ctx, env.Client, node)
 		ExpectObjectReconciled(ctx, env.Client, nodeClaimController, nodeClaim)
 		node = ExpectExists(ctx, env.Client, node)

pkg/controllers/provisioning/scheduling/suite_test.go

@@ -2069,6 +2069,7 @@ var _ = Context("Scheduling", func() {
 				// delete the pod so that the node is empty
 				ExpectDeleted(ctx, env.Client, initialPod)
 				// startup taint + node not ready taint = 2
+				// note that this node is not post registration so the unregistered taint is present
 				Expect(node1.Spec.Taints).To(HaveLen(2))
 				Expect(node1.Spec.Taints).To(ContainElement(v1.Taint{
 					Key:    "foo.com/taint",

pkg/controllers/provisioning/suite_test.go

@@ -259,6 +259,7 @@ var _ = Describe("Provisioning", func() {
 				},
 				Finalizers: []string{v1beta1.TerminationFinalizer},
 			},
+			Taints: []v1.Taint{v1beta1.UnregisteredNoExecuteTaint},
 		},
 		)
 		ExpectApplied(ctx, env.Client, node, nodePool)

pkg/scheduling/taints.go

@@ -23,6 +23,8 @@ import (
 	"go.uber.org/multierr"
 	v1 "k8s.io/api/core/v1"
 	cloudproviderapi "k8s.io/cloud-provider/api"
+
+	"sigs.k8s.io/karpenter/pkg/apis/v1beta1"
 )
 
 // KnownEphemeralTaints are taints that are expected to be added to a node while it's initializing
@@ -32,6 +34,7 @@ var KnownEphemeralTaints = []v1.Taint{
 	{Key: v1.TaintNodeNotReady, Effect: v1.TaintEffectNoSchedule},
 	{Key: v1.TaintNodeUnreachable, Effect: v1.TaintEffectNoSchedule},
 	{Key: cloudproviderapi.TaintExternalCloudProvider, Effect: v1.TaintEffectNoSchedule, Value: "true"},
+	v1beta1.UnregisteredNoExecuteTaint,
 }
 
 // Taints is a decorated alias type for []v1.Taint

pkg/test/expectations/expectations.go

@@ -453,7 +453,7 @@ func ExpectMakeNodesReady(ctx context.Context, c client.Client, nodes ...*v1.Nod
 		// Remove any of the known ephemeral taints to make the Node ready
 		nodes[i].Spec.Taints = lo.Reject(nodes[i].Spec.Taints, func(taint v1.Taint, _ int) bool {
 			_, found := lo.Find(pscheduling.KnownEphemeralTaints, func(t v1.Taint) bool {
-				return t.MatchTaint(&taint)
+				return t.MatchTaint(&taint) && !t.MatchTaint(&v1beta1.UnregisteredNoExecuteTaint)
 			})
 			return found
 		})

pkg/test/nodes.go

@@ -20,6 +20,7 @@ import (
 	"fmt"
 
 	"github.com/imdario/mergo"
+	"github.com/samber/lo"
 	v1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 
@@ -67,15 +68,23 @@ func Node(overrides ...NodeOptions) *v1.Node {
 	}
 }
 
+// this check is done for tests that assume the nodeclaim is past registration but will not go through reconciliation
 func NodeClaimLinkedNode(nodeClaim *v1beta1.NodeClaim) *v1.Node {
+	taints := lo.Reject(nodeClaim.Spec.StartupTaints, func(t v1.Taint, _ int) bool {
+		return t.MatchTaint(&v1beta1.UnregisteredNoExecuteTaint)
+	})
+	if nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeRegistered).IsFalse() &&
+		nodeClaim.StatusConditions().Get(v1beta1.ConditionTypeInitialized).IsFalse() {
+		taints = append(taints, v1beta1.UnregisteredNoExecuteTaint)
+	}
 	return Node(
 		NodeOptions{
 			ObjectMeta: metav1.ObjectMeta{
 				Labels:      nodeClaim.Labels,
 				Annotations: nodeClaim.Annotations,
 				Finalizers:  nodeClaim.Finalizers,
 			},
-			Taints:      append(nodeClaim.Spec.Taints, nodeClaim.Spec.StartupTaints...),
+			Taints:      append(nodeClaim.Spec.Taints, taints...),
 			Capacity:    nodeClaim.Status.Capacity,
 			Allocatable: nodeClaim.Status.Allocatable,
 			ProviderID:  nodeClaim.Status.ProviderID,