fix: Ensure all patch calls can conflict when resource version doesn'…

…t match (#6985)
aws · Sep 17, 2024 · f0dbb26 · f0dbb26
1 parent 9beb56f
commit f0dbb26
Show file tree

Hide file tree

Showing 8 changed files with 34 additions and 15 deletions.
diff --git a/charts/karpenter-crd/templates/karpenter.k8s.aws_ec2nodeclasses.yaml b/charts/karpenter-crd/templates/karpenter.k8s.aws_ec2nodeclasses.yaml
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.2
+    controller-gen.kubebuilder.io/version: v0.16.3
   name: ec2nodeclasses.karpenter.k8s.aws
 spec:
   group: karpenter.k8s.aws

diff --git a/charts/karpenter-crd/templates/karpenter.sh_nodepools.yaml b/charts/karpenter-crd/templates/karpenter.sh_nodepools.yaml
@@ -71,6 +71,8 @@ spec:
                 from a combination of nodepool and pod scheduling constraints.
               properties:
                 disruption:
+                  default:
+                    consolidateAfter: 0s
                   description: Disruption contains the parameters that relate to Karpenter's disruption logic
                   properties:
                     budgets:

diff --git a/go.mod b/go.mod
@@ -31,7 +31,7 @@ require (
 	k8s.io/klog/v2 v2.130.1
 	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	sigs.k8s.io/controller-runtime v0.19.0
-	sigs.k8s.io/karpenter v1.0.1-0.20240912184512-932d95819986
+	sigs.k8s.io/karpenter v1.0.1-0.20240917183044-71f7aef707c9
 	sigs.k8s.io/yaml v1.4.0
 )
 
@@ -93,9 +93,9 @@ require (
 	gopkg.in/inf.v0 v0.9.1 // indirect
 	gopkg.in/yaml.v2 v2.4.0 // indirect
 	gopkg.in/yaml.v3 v3.0.1 // indirect
-	k8s.io/cloud-provider v0.31.0 // indirect
+	k8s.io/cloud-provider v0.31.1 // indirect
 	k8s.io/component-base v0.31.1 // indirect
-	k8s.io/csi-translation-lib v0.31.0 // indirect
+	k8s.io/csi-translation-lib v0.31.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect

diff --git a/go.sum b/go.sum
@@ -260,12 +260,12 @@ k8s.io/apimachinery v0.31.1 h1:mhcUBbj7KUjaVhyXILglcVjuS4nYXiwC+KKFBgIVy7U=
 k8s.io/apimachinery v0.31.1/go.mod h1:rsPdaZJfTfLsNJSQzNHQvYoTmxhoOEofxtOsF3rtsMo=
 k8s.io/client-go v0.31.1 h1:f0ugtWSbWpxHR7sjVpQwuvw9a3ZKLXX0u0itkFXufb0=
 k8s.io/client-go v0.31.1/go.mod h1:sKI8871MJN2OyeqRlmA4W4KM9KBdBUpDLu/43eGemCg=
-k8s.io/cloud-provider v0.31.0 h1:qNOs78I2/7zQmyStfDtY2M7EdilUl9fCSYMcqBju/tA=
-k8s.io/cloud-provider v0.31.0/go.mod h1:QgUPqLoL6aXhLlrNg1U4IrJk/PvvxgeOnT2ixkgnqT0=
+k8s.io/cloud-provider v0.31.1 h1:40b6AgDizwm5eWratZbqubTHMob25VWr6NX2Ei5TwZA=
+k8s.io/cloud-provider v0.31.1/go.mod h1:xAdkE7fdZdu9rKLuOZUMBfagu7bM+bas3iPux/2nLGg=
 k8s.io/component-base v0.31.1 h1:UpOepcrX3rQ3ab5NB6g5iP0tvsgJWzxTyAo20sgYSy8=
 k8s.io/component-base v0.31.1/go.mod h1:WGeaw7t/kTsqpVTaCoVEtillbqAhF2/JgvO0LDOMa0w=
-k8s.io/csi-translation-lib v0.31.0 h1:5aCBPyFScdhfcWCUj0KDMAi/lDhxK99DF4XcfSnmH1A=
-k8s.io/csi-translation-lib v0.31.0/go.mod h1:CM3U0vDm4jfuQpjKkqlJdRDqmAEcLQPm7aoJFjYf668=
+k8s.io/csi-translation-lib v0.31.1 h1:ps9kya8+ih0CVL59JO2B4AYH8U/e3WLQxl9sx19NjjM=
+k8s.io/csi-translation-lib v0.31.1/go.mod h1:VeYSucPZJbAt6RT25AzfG7WjyxCcmqxtr4V/CaDdNZc=
 k8s.io/klog/v2 v2.130.1 h1:n9Xl7H1Xvksem4KFG4PYbdQCQxqc/tTUyrgXaOhHSzk=
 k8s.io/klog/v2 v2.130.1/go.mod h1:3Jpz1GvMt720eyJH1ckRHK1EDfpxISzJ7I9OYgaDtPE=
 k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 h1:BZqlfIlq5YbRMFko6/PM7FjZpUb45WallggurYhKGag=
@@ -276,8 +276,8 @@ sigs.k8s.io/controller-runtime v0.19.0 h1:nWVM7aq+Il2ABxwiCizrVDSlmDcshi9llbaFbC
 sigs.k8s.io/controller-runtime v0.19.0/go.mod h1:iRmWllt8IlaLjvTTDLhRBXIEtkCK6hwVBJJsYS9Ajf4=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd h1:EDPBXCAspyGV4jQlpZSudPeMmr1bNJefnuqLsRAsHZo=
 sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd/go.mod h1:B8JuhiUyNFVKdsE8h686QcCxMaH6HrOAZj4vswFpcB0=
-sigs.k8s.io/karpenter v1.0.1-0.20240912184512-932d95819986 h1:i9QLisyAbANPGq07i5HaJ70cjBtZUoK9YGBDaPXCEMA=
-sigs.k8s.io/karpenter v1.0.1-0.20240912184512-932d95819986/go.mod h1:bYlfeGdZBtTNmz0qlyi3VJ2n6/10HVv8dSmJ90zvyjs=
+sigs.k8s.io/karpenter v1.0.1-0.20240917183044-71f7aef707c9 h1:PJsVtGgJUTDqYWXa6xkrTDUNUlMyZj3yDsJTX2tJu5g=
+sigs.k8s.io/karpenter v1.0.1-0.20240917183044-71f7aef707c9/go.mod h1:U70Yuu2wiH1nBC5/nDYYpqYNadYnMgetmVQ5s2NouyU=
 sigs.k8s.io/structured-merge-diff/v4 v4.4.1 h1:150L+0vs/8DA78h1u02ooW1/fFq/Lwr+sGiqlzvrtq4=
 sigs.k8s.io/structured-merge-diff/v4 v4.4.1/go.mod h1:N8hJocpFajUSSeSJ9bOZ77VzejKZaXsTtZo4/u7Io08=
 sigs.k8s.io/yaml v1.4.0 h1:Mk1wCc2gy/F0THH0TAp1QYyJNzRm2KCLy3o5ASXVI5E=

diff --git a/pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml b/pkg/apis/crds/karpenter.k8s.aws_ec2nodeclasses.yaml
@@ -3,7 +3,7 @@ apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
   annotations:
-    controller-gen.kubebuilder.io/version: v0.16.2
+    controller-gen.kubebuilder.io/version: v0.16.3
   name: ec2nodeclasses.karpenter.k8s.aws
 spec:
   group: karpenter.k8s.aws

diff --git a/pkg/apis/crds/karpenter.sh_nodepools.yaml b/pkg/apis/crds/karpenter.sh_nodepools.yaml
@@ -71,6 +71,8 @@ spec:
                 from a combination of nodepool and pod scheduling constraints.
               properties:
                 disruption:
+                  default:
+                    consolidateAfter: 0s
                   description: Disruption contains the parameters that relate to Karpenter's disruption logic
                   properties:
                     budgets:

diff --git a/pkg/controllers/nodeclass/status/controller.go b/pkg/controllers/nodeclass/status/controller.go
@@ -19,6 +19,7 @@ import (
 
 	"go.uber.org/multierr"
 	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/errors"
 	controllerruntime "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
@@ -72,7 +73,14 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
 	if !controllerutil.ContainsFinalizer(nodeClass, v1.TerminationFinalizer) {
 		stored := nodeClass.DeepCopy()
 		controllerutil.AddFinalizer(nodeClass, v1.TerminationFinalizer)
-		if err := c.kubeClient.Patch(ctx, nodeClass, client.MergeFrom(stored)); err != nil {
+
+		// We use client.MergeFromWithOptimisticLock because patching a list with a JSON merge patch
+		// can cause races due to the fact that it fully replaces the list on a change
+		// Here, we are updating the finalizer list
+		if err := c.kubeClient.Patch(ctx, nodeClass, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
+			if errors.IsConflict(err) {
+				return reconcile.Result{Requeue: true}, nil
+			}
 			return reconcile.Result{}, err
 		}
 	}
@@ -93,7 +101,13 @@ func (c *Controller) Reconcile(ctx context.Context, nodeClass *v1.EC2NodeClass)
 	}
 
 	if !equality.Semantic.DeepEqual(stored, nodeClass) {
-		if err := c.kubeClient.Status().Patch(ctx, nodeClass, client.MergeFrom(stored)); err != nil {
+		// We use client.MergeFromWithOptimisticLock because patching a list with a JSON merge patch
+		// can cause races due to the fact that it fully replaces the list on a change
+		// Here, we are updating the status condition list
+		if err := c.kubeClient.Status().Patch(ctx, nodeClass, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
+			if errors.IsConflict(err) {
+				return reconcile.Result{Requeue: true}, nil
+			}
 			errs = multierr.Append(errs, client.IgnoreNotFound(err))
 		}
 	}

diff --git a/pkg/controllers/nodeclass/termination/controller.go b/pkg/controllers/nodeclass/termination/controller.go
@@ -96,10 +96,11 @@ func (c *Controller) finalize(ctx context.Context, nodeClass *v1.EC2NodeClass) (
 	}
 	controllerutil.RemoveFinalizer(nodeClass, v1.TerminationFinalizer)
 	if !equality.Semantic.DeepEqual(stored, nodeClass) {
-		// We call Update() here rather than Patch() because patching a list with a JSON merge patch
+		// We use client.MergeFromWithOptimisticLock because patching a list with a JSON merge patch
 		// can cause races due to the fact that it fully replaces the list on a change
+		// Here, we are updating the finalizer list
 		// https://github.com/kubernetes/kubernetes/issues/111643#issuecomment-2016489732
-		if err := c.kubeClient.Update(ctx, nodeClass); err != nil {
+		if err := c.kubeClient.Patch(ctx, nodeClass, client.MergeFromWithOptions(stored, client.MergeFromWithOptimisticLock{})); err != nil {
 			if errors.IsConflict(err) {
 				return reconcile.Result{Requeue: true}, nil
 			}