cherrypick(v0.32): feat: Add Versioned for NodePool Hash to Prevent Drift on NodePool CRD Upgrade (#1131)

Author: engedaam

pkg/apis/v1beta1/labels.go

@@ -44,6 +44,7 @@ const (
 	ProviderCompatabilityAnnotationKey = CompatabilityGroup + "/provider"
 	ManagedByAnnotationKey             = Group + "/managed-by"
 	NodePoolHashAnnotationKey          = Group + "/nodepool-hash"
+	NodePoolHashVersionAnnotationKey   = Group + "/nodepool-hash-version"
 )
 
 // Karpenter specific finalizers

pkg/apis/v1beta1/nodepool.go

@@ -144,6 +144,12 @@ type NodePool struct {
 	IsProvisioner bool `json:"-"`
 }
 
+// We need to bump the NodePoolHashVersion when we make an update to the NodePool CRD under these conditions:
+// 1. A field changes its default value for an existing field that is already hashed
+// 2. A field is added to the hash calculation with an already-set value
+// 3. A field is removed from the hash calculations
+const NodePoolHashVersion = "v1"
+
 func (in *NodePool) Hash() string {
 	return fmt.Sprint(lo.Must(hashstructure.Hash(in.Spec.Template, hashstructure.FormatV2, &hashstructure.HashOptions{
 		SlicesAsSets:    true,

pkg/controllers/nodeclaim/disruption/drift.go

@@ -109,7 +109,7 @@ func (d *Drift) isDrifted(ctx context.Context, nodePool *v1beta1.NodePool, nodeC
 	return driftedReason, nil
 }
 
-// Eligible fields for static drift are described in the docs
+// Eligible fields for drift are described in the docs
 // https://karpenter.sh/docs/concepts/deprovisioning/#drift
 func areStaticFieldsDrifted(nodePool *v1beta1.NodePool, nodeClaim *v1beta1.NodeClaim) cloudprovider.DriftReason {
 	var ownerHashKey string
@@ -123,6 +123,19 @@ func areStaticFieldsDrifted(nodePool *v1beta1.NodePool, nodeClaim *v1beta1.NodeC
 	if !foundHashNodePool || !foundHashNodeClaim {
 		return ""
 	}
+
+	if !nodePool.IsProvisioner {
+		nodeClaimVersionHash, foundVersionHashNodeClaim := nodeClaim.Annotations[v1beta1.NodePoolHashVersionAnnotationKey]
+		nodePoolVersionHash, foundVersionHashNodePool := nodePool.Annotations[v1beta1.NodePoolHashVersionAnnotationKey]
+		if !foundVersionHashNodePool || !foundVersionHashNodeClaim {
+			return ""
+		}
+		// validate that the version of the crd is the same
+		if nodePoolVersionHash != nodeClaimVersionHash {
+			return ""
+		}
+	}
+
 	if nodePoolHash != nodeClaimHash {
 		if nodeClaim.IsMachine {
 			return ProvisionerDrifted

pkg/controllers/nodeclaim/disruption/nodeclaim_drift_test.go

@@ -120,7 +120,12 @@ var _ = Describe("NodeClaim/Drift", func() {
 	It("should detect static drift before cloud provider drift", func() {
 		cp.Drifted = "drifted"
 		nodePool.Annotations = lo.Assign(nodePool.Annotations, map[string]string{
-			v1beta1.NodePoolHashAnnotationKey: "123456789",
+			v1beta1.NodePoolHashAnnotationKey:        "test-123456789",
+			v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
+		})
+		nodeClaim.Annotations = lo.Assign(nodeClaim.Annotations, map[string]string{
+			v1beta1.NodePoolHashAnnotationKey:        "test-123",
+			v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
 		})
 		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
 		ExpectReconcileSucceeded(ctx, nodeClaimDisruptionController, client.ObjectKeyFromObject(nodeClaim))
@@ -425,8 +430,9 @@ var _ = Describe("NodeClaim/Drift", func() {
 					Template: v1beta1.NodeClaimTemplate{
 						ObjectMeta: v1beta1.ObjectMeta{
 							Annotations: map[string]string{
-								"keyAnnotation":  "valueAnnotation",
-								"keyAnnotation2": "valueAnnotation2",
+								"keyAnnotation":                          "valueAnnotation",
+								"keyAnnotation2":                         "valueAnnotation2",
+								v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
 							},
 							Labels: map[string]string{
 								"keyLabel":  "valueLabel",
@@ -493,5 +499,19 @@ var _ = Describe("NodeClaim/Drift", func() {
 			nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 			Expect(nodeClaim.StatusConditions().GetCondition(v1beta1.Drifted)).To(BeNil())
 		})
+		It("should not return drifted if the NodeClaim's karpenter.sh/nodepool-hash-version annotation does not match the NodePool's", func() {
+			nodePool.ObjectMeta.Annotations = map[string]string{
+				v1beta1.NodePoolHashAnnotationKey:        "test-hash-1",
+				v1beta1.NodePoolHashVersionAnnotationKey: "test-version-1",
+			}
+			nodeClaim.ObjectMeta.Annotations = map[string]string{
+				v1beta1.NodePoolHashAnnotationKey:        "test-hash-2",
+				v1beta1.NodePoolHashVersionAnnotationKey: "test-version-2",
+			}
+			ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+			ExpectReconcileSucceeded(ctx, nodeClaimDisruptionController, client.ObjectKeyFromObject(nodeClaim))
+			nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+			Expect(nodeClaim.StatusConditions().GetCondition(v1beta1.Drifted)).To(BeNil())
+		})
 	})
 })

pkg/controllers/nodepool/hash/controller.go

@@ -18,6 +18,7 @@ import (
 	"context"
 
 	"github.com/samber/lo"
+	"go.uber.org/multierr"
 	"k8s.io/apimachinery/pkg/api/equality"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
@@ -47,6 +48,12 @@ func NewController(kubeClient client.Client) *Controller {
 // Reconcile the resource
 func (c *Controller) Reconcile(ctx context.Context, np *v1beta1.NodePool) (reconcile.Result, error) {
 	stored := np.DeepCopy()
+
+	if np.Annotations[v1beta1.NodePoolHashVersionAnnotationKey] != v1beta1.NodePoolHashVersion {
+		if err := c.updateNodeClaimHash(ctx, np); err != nil {
+			return reconcile.Result{}, err
+		}
+	}
 	np.Annotations = lo.Assign(np.Annotations, nodepoolutil.HashAnnotation(np))
 
 	if !equality.Semantic.DeepEqual(stored, np) {
@@ -107,3 +114,45 @@ func (c *NodePoolController) Builder(_ context.Context, m manager.Manager) corec
 		WithOptions(controller.Options{MaxConcurrentReconciles: 10}),
 	)
 }
+
+// Updating `nodepool-hash-version` annotation inside the controller means a breaking change has made to the hash function calculating
+// `nodepool-hash` on both the NodePool and NodeClaim, automatically making the `nodepool-hash` on the NodeClaim different from
+// NodePool. Since we can not rely on the hash on the NodeClaims, we will need to re-calculate the hash and update the annotation.
+// Look at designs/drift-hash-version.md for more information.
+func (c *Controller) updateNodeClaimHash(ctx context.Context, np *v1beta1.NodePool) error {
+	if np.IsProvisioner {
+		return nil
+	}
+	ncList := &v1beta1.NodeClaimList{}
+	if err := c.kubeClient.List(ctx, ncList, client.MatchingLabels(map[string]string{v1beta1.NodePoolLabelKey: np.Name})); err != nil {
+		return err
+	}
+
+	errs := make([]error, len(ncList.Items))
+	for i := range ncList.Items {
+		nc := ncList.Items[i]
+		stored := nc.DeepCopy()
+
+		if nc.Annotations[v1beta1.NodePoolHashVersionAnnotationKey] != v1beta1.NodePoolHashVersion {
+			nc.Annotations = lo.Assign(nc.Annotations, map[string]string{
+				v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
+			})
+
+			// Any NodeClaim that is already drifted will remain drifted if the karpenter.sh/nodepool-hash-version doesn't match
+			// Since the hashing mechanism has changed we will not be able to determine if the drifted status of the node has changed
+			if nc.StatusConditions().GetCondition(v1beta1.Drifted) == nil {
+				nc.Annotations = lo.Assign(nc.Annotations, map[string]string{
+					v1beta1.NodePoolHashAnnotationKey: np.Hash(),
+				})
+			}
+
+			if !equality.Semantic.DeepEqual(stored, nc) {
+				if err := c.kubeClient.Patch(ctx, &nc, client.MergeFrom(stored)); err != nil {
+					errs[i] = client.IgnoreNotFound(err)
+				}
+			}
+		}
+	}
+
+	return multierr.Combine(errs...)
+}

pkg/controllers/nodepool/hash/nodepool_test.go

@@ -28,6 +28,8 @@ import (
 	"github.com/aws/karpenter-core/pkg/apis/v1beta1"
 	"github.com/aws/karpenter-core/pkg/test"
 	. "github.com/aws/karpenter-core/pkg/test/expectations"
+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
 var _ = Describe("NodePool Static Drift Hash", func() {
@@ -105,4 +107,107 @@ var _ = Describe("NodePool Static Drift Hash", func() {
 
 		Expect(nodePool.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, expectedHash))
 	})
+	It("should update nodepool hash version when the nodepool hash version is out of sync with the controller hash version", func() {
+		nodePool.Annotations = map[string]string{
+			v1beta1.NodePoolHashAnnotationKey:        "abceduefed",
+			v1beta1.NodePoolHashVersionAnnotationKey: "test",
+		}
+		ExpectApplied(ctx, env.Client, nodePool)
+
+		ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool))
+		nodePool = ExpectExists(ctx, env.Client, nodePool)
+
+		expectedHash := nodePool.Hash()
+		Expect(nodePool.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, expectedHash))
+		Expect(nodePool.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+	})
+	It("should update nodepool hash versions on all nodeclaims when the hash versions don't match the controller hash version", func() {
+		nodePool.Annotations = map[string]string{
+			v1beta1.NodePoolHashAnnotationKey:        "abceduefed",
+			v1beta1.NodePoolHashVersionAnnotationKey: "test",
+		}
+		nodeClaimOne := test.NodeClaim(v1beta1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{v1beta1.NodePoolLabelKey: nodePool.Name},
+				Annotations: map[string]string{
+					v1beta1.NodePoolHashAnnotationKey:        "123456",
+					v1beta1.NodePoolHashVersionAnnotationKey: "test",
+				},
+			},
+		})
+		nodeClaimTwo := test.NodeClaim(v1beta1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{v1beta1.NodePoolLabelKey: nodePool.Name},
+				Annotations: map[string]string{
+					v1beta1.NodePoolHashAnnotationKey:        "123456",
+					v1beta1.NodePoolHashVersionAnnotationKey: "test",
+				},
+			},
+		})
+
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaimOne, nodeClaimTwo)
+
+		ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool))
+		nodePool = ExpectExists(ctx, env.Client, nodePool)
+		nodeClaimOne = ExpectExists(ctx, env.Client, nodeClaimOne)
+		nodeClaimTwo = ExpectExists(ctx, env.Client, nodeClaimTwo)
+
+		expectedHash := nodePool.Hash()
+		Expect(nodeClaimOne.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, expectedHash))
+		Expect(nodeClaimOne.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+		Expect(nodeClaimTwo.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, expectedHash))
+		Expect(nodeClaimTwo.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+	})
+	It("should not update nodepool hash on all nodeclaims when the hash versions match the controller hash version", func() {
+		nodePool.Annotations = map[string]string{
+			v1beta1.NodePoolHashAnnotationKey:        "abceduefed",
+			v1beta1.NodePoolHashVersionAnnotationKey: "test-version",
+		}
+		nodeClaim := test.NodeClaim(v1beta1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{v1beta1.NodePoolLabelKey: nodePool.Name},
+				Annotations: map[string]string{
+					v1beta1.NodePoolHashAnnotationKey:        "1234564654",
+					v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
+				},
+			},
+		})
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+
+		ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool))
+		nodePool = ExpectExists(ctx, env.Client, nodePool)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+
+		expectedHash := nodePool.Hash()
+
+		// Expect NodeClaims to have been updated to the original hash
+		Expect(nodePool.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, expectedHash))
+		Expect(nodePool.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, "1234564654"))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+	})
+	It("should not update nodepool hash on the nodeclaim if it's drifted", func() {
+		nodePool.Annotations = map[string]string{
+			v1beta1.NodePoolHashAnnotationKey:        "abceduefed",
+			v1beta1.NodePoolHashVersionAnnotationKey: "test",
+		}
+		nodeClaim := test.NodeClaim(v1beta1.NodeClaim{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{v1beta1.NodePoolLabelKey: nodePool.Name},
+				Annotations: map[string]string{
+					v1beta1.NodePoolHashAnnotationKey:        "123456",
+					v1beta1.NodePoolHashVersionAnnotationKey: "test",
+				},
+			},
+		})
+		nodeClaim.StatusConditions().MarkTrue(v1beta1.Drifted)
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim)
+
+		ExpectReconcileSucceeded(ctx, nodePoolController, client.ObjectKeyFromObject(nodePool))
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+
+		// Expect NodeClaims hash to not have been updated
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashAnnotationKey, "123456"))
+		Expect(nodeClaim.Annotations).To(HaveKeyWithValue(v1beta1.NodePoolHashVersionAnnotationKey, v1beta1.NodePoolHashVersion))
+	})
 })

pkg/controllers/provisioning/scheduling/nodeclaimtemplate.go

@@ -70,8 +70,11 @@ func (i *NodeClaimTemplate) ToNodeClaim(nodePool *v1beta1.NodePool) *v1beta1.Nod
 	nc := &v1beta1.NodeClaim{
 		ObjectMeta: metav1.ObjectMeta{
 			GenerateName: fmt.Sprintf("%s-", i.OwnerKey.Name),
-			Annotations:  lo.Assign(i.Annotations, map[string]string{v1beta1.NodePoolHashAnnotationKey: nodePool.Hash()}),
-			Labels:       i.Labels,
+			Annotations: lo.Assign(i.Annotations, map[string]string{
+				v1beta1.NodePoolHashAnnotationKey:        nodePool.Hash(),
+				v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
+			}),
+			Labels: i.Labels,
 			OwnerReferences: []metav1.OwnerReference{
 				{
 					APIVersion:         v1beta1.SchemeGroupVersion.String(),

pkg/utils/nodepool/nodepool.go

@@ -159,5 +159,8 @@ func HashAnnotation(nodePool *v1beta1.NodePool) map[string]string {
 		provisioner := provisionerutil.New(nodePool)
 		return map[string]string{v1alpha5.ProvisionerHashAnnotationKey: provisioner.Hash()}
 	}
-	return map[string]string{v1beta1.NodePoolHashAnnotationKey: nodePool.Hash()}
+	return map[string]string{
+		v1beta1.NodePoolHashAnnotationKey:        nodePool.Hash(),
+		v1beta1.NodePoolHashVersionAnnotationKey: v1beta1.NodePoolHashVersion,
+	}
 }