fix(podevents): check the lastPodEventTime have been set, dedupe timeout or is max

Signed-off-by: flavono123 <[email protected]>

Author: flavono123

pkg/controllers/nodeclaim/disruption/consolidation.go

@@ -60,7 +60,7 @@ func (c *Consolidation) Reconcile(ctx context.Context, nodePool *v1.NodePool, no
 	timeToCheck := lo.Ternary(!nodeClaim.Status.LastPodEventTime.IsZero(), nodeClaim.Status.LastPodEventTime.Time, initialized.LastTransitionTime.Time)
 
 	// Consider a node consolidatable by looking at the lastPodEvent status field on the nodeclaim.
-	// This time is now based on the PodScheduled condition's lastTransitionTime or pod is being removed(terminal or terminating)
+	// This time is based on the PodScheduled condition's lastTransitionTime or pod is being removed(terminal or terminating)
 	if c.clock.Since(timeToCheck) < lo.FromPtr(nodePool.Spec.Disruption.ConsolidateAfter.Duration) {
 		if hasConsolidatableCondition {
 			_ = nodeClaim.StatusConditions().Clear(v1.ConditionTypeConsolidatable)

pkg/controllers/nodeclaim/hydration/suite_test.go

@@ -35,12 +35,10 @@ import (
 	. "sigs.k8s.io/karpenter/pkg/utils/testing"
 )
 
-var (
-	ctx                 context.Context
-	hydrationController *hydration.Controller
-	env                 *test.Environment
-	cloudProvider       *fake.CloudProvider
-)
+var ctx context.Context
+var hydrationController *hydration.Controller
+var env *test.Environment
+var cloudProvider *fake.CloudProvider
 
 func TestAPIs(t *testing.T) {
 	ctx = TestContextWithLogger(t)

pkg/controllers/nodeclaim/podevents/controller.go

@@ -40,6 +40,11 @@ import (
 	podutils "sigs.k8s.io/karpenter/pkg/utils/pod"
 )
 
+// dedupeTimeout is 10 seconds to reduce the number of writes to the APIServer, since pod scheduling and deletion events are very frequent.
+// The smaller this value is, the more writes Karpenter will make in a busy cluster. This timeout is intentionally smaller than the consolidation
+// 15 second validation period, so that we can ensure that we invalidate consolidation commands that are decided while we're de-duping pod events.
+const dedupeTimeout = 10 * time.Second
+
 // Podevents is a nodeclaim controller that updates the lastPodEvent status based on PodScheduled condition
 type Controller struct {
 	clock         clock.Clock
@@ -96,9 +101,16 @@ func (c *Controller) Reconcile(ctx context.Context, pod *corev1.Pod) (reconcile.
 		return reconcile.Result{}, nil
 	}
 
-	// otherwise, set the pod event time to now
-	stored := nc.DeepCopy()
+	// If we've set the lastPodEvent before
+	// and the stored lastPodEventTime is after(max) eventTime
+	// and it hasn't been before the timeout, don't do anything
+	if !nc.Status.LastPodEventTime.Time.IsZero() &&
+		nc.Status.LastPodEventTime.Time.After(eventTime) &&
+		c.clock.Since(nc.Status.LastPodEventTime.Time) < dedupeTimeout {
+		return reconcile.Result{}, nil
+	}
 
+	stored := nc.DeepCopy()
 	nc.Status.LastPodEventTime.Time = eventTime
 	if !equality.Semantic.DeepEqual(stored, nc) {
 		if err = c.kubeClient.Status().Patch(ctx, nc, client.MergeFrom(stored)); err != nil {

pkg/controllers/nodeclaim/podevents/suite_test.go

@@ -38,13 +38,11 @@ import (
 	. "sigs.k8s.io/karpenter/pkg/utils/testing"
 )
 
-var (
-	ctx                 context.Context
-	podEventsController *podevents.Controller
-	env                 *test.Environment
-	fakeClock           *clock.FakeClock
-	cp                  *fake.CloudProvider
-)
+var ctx context.Context
+var podEventsController *podevents.Controller
+var env *test.Environment
+var fakeClock *clock.FakeClock
+var cp *fake.CloudProvider
 
 func TestAPIs(t *testing.T) {
 	ctx = TestContextWithLogger(t)
@@ -77,7 +75,6 @@ var _ = AfterEach(func() {
 	cp.Reset()
 	ExpectCleanedUp(ctx, env.Client)
 })
-
 var _ = Describe("PodEvents", func() {
 	var nodePool *v1.NodePool
 	var nodeClaim *v1.NodeClaim
@@ -101,45 +98,109 @@ var _ = Describe("PodEvents", func() {
 			NodeName: node.Name,
 		})
 	})
-	It("should set the nodeclaim lastPodEvent based on PodScheduled condition", func() {
-		scheduledTime := fakeClock.Now().Truncate(time.Second)
+	It("should set the nodeclaim lastPodEvent", func() {
+		timeToCheck := fakeClock.Now().Truncate(time.Second)
 		pod.Status.Conditions = []corev1.PodCondition{{
 			Type:               corev1.PodScheduled,
 			Status:             corev1.ConditionTrue,
-			LastTransitionTime: metav1.Time{Time: scheduledTime},
+			LastTransitionTime: metav1.Time{Time: timeToCheck},
 		}}
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim, node, pod)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
+	})
+	It("should not set the nodeclaim lastPodEvent when the node does not exist", func() {
+		ExpectApplied(ctx, env.Client, nodePool, nodeClaim, pod)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeZero())
+	})
+	It("should not set the nodeclaim lastPodEvent when pod has no PodScheduled condition", func() {
 		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
 		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
 
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(scheduledTime))
+		Expect(nodeClaim.Status.LastPodEventTime.Time.IsZero()).To(BeTrue())
+	})
+	It("should not set the nodeclaim lastPodEvent when PodScheduled condition is not True", func() {
+		pod.Status.Conditions = []corev1.PodCondition{{
+			Type:               corev1.PodScheduled,
+			Status:             corev1.ConditionFalse, // PodScheduled false
+			LastTransitionTime: metav1.Time{Time: fakeClock.Now()},
+		}}
+		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
 
-		// Update the PodScheduled condition's lastTransitionTime
-		newScheduledTime := fakeClock.Now().Add(time.Minute).Truncate(time.Second)
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time.IsZero()).To(BeTrue())
+	})
+	It("should succeed when the nodeclaim lastPodEvent when the nodeclaim does not exist", func() {
+		ExpectApplied(ctx, env.Client, nodePool, node, pod)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+	})
+	It("should set the nodeclaim lastPodEvent when it's been set before", func() {
+		nodeClaim.Status.LastPodEventTime.Time = fakeClock.Now().Add(-5 * time.Minute)
+		timeToCheck := fakeClock.Now().Truncate(time.Second)
 		pod.Status.Conditions = []corev1.PodCondition{{
 			Type:               corev1.PodScheduled,
 			Status:             corev1.ConditionTrue,
-			LastTransitionTime: metav1.Time{Time: newScheduledTime},
+			LastTransitionTime: metav1.Time{Time: timeToCheck},
 		}}
-		ExpectApplied(ctx, env.Client, pod)
+		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
 		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
 
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(newScheduledTime))
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
 	})
-	It("should not set the nodeclaim lastPodEvent when the node does not exist", func() {
-		ExpectApplied(ctx, env.Client, nodePool, nodeClaim, pod)
+	It("should only set the nodeclaim lastPodEventTime when the event time is after than", func() {
+		timeToCheck := fakeClock.Now().Truncate(time.Second)
+		nodeClaim.Status.LastPodEventTime.Time = timeToCheck
+		scheduledTimeBeforeLastPodEventTime := timeToCheck.Add(-5 * time.Minute)
+		pod.Status.Conditions = []corev1.PodCondition{{
+			Type:               corev1.PodScheduled,
+			Status:             corev1.ConditionTrue,
+			LastTransitionTime: metav1.Time{Time: scheduledTimeBeforeLastPodEventTime},
+		}}
+		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
 		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
 
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeZero())
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
 	})
-	It("should succeed when the nodeclaim lastPodEvent when the nodeclaim does not exist", func() {
-		ExpectApplied(ctx, env.Client, nodePool, node, pod)
+	It("should only set the nodeclaim lastPodEvent within the dedupe timeframe", func() {
+		timeToCheck := fakeClock.Now().Truncate(time.Second)
+		pod.Status.Conditions = []corev1.PodCondition{{
+			Type:               corev1.PodScheduled,
+			Status:             corev1.ConditionTrue,
+			LastTransitionTime: metav1.Time{Time: timeToCheck},
+		}}
+		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+
+		// Expect that the lastPodEventTime is set
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
+
+		// step through half of the dedupe timeout, and re-reconcile,
+		// expecting the status to not change
+		fakeClock.Step(5 * time.Second)
 		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
+
+		// step through rest of the dedupe timeout, and re-reconcile,
+		// expecting the status to not change since the podscheduled condition has not changed or terminal or terminating
+		fakeClock.Step(5 * time.Second)
+		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
+
+		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
+		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
 	})
 	It("should set the nodeclaim lastPodEvent when pod becomes terminal", func() {
-		// First set up a regular pod
 		scheduledTime := fakeClock.Now().Truncate(time.Second)
 		pod.Status.Conditions = []corev1.PodCondition{{
 			Type:               corev1.PodScheduled,
@@ -172,26 +233,6 @@ var _ = Describe("PodEvents", func() {
 		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
 		Expect(nodeClaim.Status.LastPodEventTime.Time).To(BeEquivalentTo(timeToCheck))
 	})
-	It("should not update lastPodEvent when pod has no PodScheduled condition", func() {
-		// Pod with no conditions
-		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
-		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
-
-		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.Status.LastPodEventTime.Time.IsZero()).To(BeTrue())
-	})
-	It("should not update lastPodEvent when PodScheduled condition is not True", func() {
-		pod.Status.Conditions = []corev1.PodCondition{{
-			Type:               corev1.PodScheduled,
-			Status:             corev1.ConditionFalse,
-			LastTransitionTime: metav1.Time{Time: fakeClock.Now()},
-		}}
-		ExpectApplied(ctx, env.Client, nodePool, node, nodeClaim, pod)
-		ExpectObjectReconciled(ctx, env.Client, podEventsController, pod)
-
-		nodeClaim = ExpectExists(ctx, env.Client, nodeClaim)
-		Expect(nodeClaim.Status.LastPodEventTime.Time.IsZero()).To(BeTrue())
-	})
 	It("should set the nodeclaim lastPodEvent when pod is already in a terminal state", func() {
 		// Setup time
 		timeToCheck := fakeClock.Now().Truncate(time.Second)