From b915d90cfcfc1550e9192ea8dbe313b297f139fd Mon Sep 17 00:00:00 2001
From: Jonathan Innis <jonathan.innis.ji@gmail.com>
Date: Fri, 14 Apr 2023 10:42:46 -0700
Subject: [PATCH] Fix node requeue when node is nominated (#294)

---
 pkg/controllers/node/emptiness.go  | 11 +++++------
 pkg/controllers/node/suite_test.go | 22 +++++++++++++++++++++-
 2 files changed, 26 insertions(+), 7 deletions(-)

diff --git a/pkg/controllers/node/emptiness.go b/pkg/controllers/node/emptiness.go
index 8d5504bfce..cb4ed46b7d 100644
--- a/pkg/controllers/node/emptiness.go
+++ b/pkg/controllers/node/emptiness.go
@@ -56,10 +56,11 @@ func (r *Emptiness) Reconcile(ctx context.Context, provisioner *v1alpha5.Provisi
 	if err != nil {
 		return reconcile.Result{}, err
 	}
-
-	// node is empty, but it is in-use per the last scheduling round so we don't consider it empty
+	// Node is empty, but it is in-use per the last scheduling round, so we don't consider it empty
+	// We perform a short requeue if the node is nominated, so we can check the node for emptiness when the node
+	// nomination time ends since we don't watch node nomination events
 	if r.cluster.IsNodeNominated(n.Name) {
-		return reconcile.Result{}, nil
+		return reconcile.Result{RequeueAfter: time.Second * 30}, nil
 	}
 
 	_, hasEmptinessTimestamp := n.Annotations[v1alpha5.EmptinessTimestampAnnotationKey]
@@ -72,9 +73,7 @@ func (r *Emptiness) Reconcile(ctx context.Context, provisioner *v1alpha5.Provisi
 		})
 		logging.FromContext(ctx).Debugf("added TTL to empty node")
 	}
-
-	// Short requeue result so that we requeue to check for emptiness when the node nomination time ends
-	return reconcile.Result{RequeueAfter: time.Minute}, nil
+	return reconcile.Result{}, nil
 }
 
 func (r *Emptiness) isEmpty(ctx context.Context, n *v1.Node) (bool, error) {
diff --git a/pkg/controllers/node/suite_test.go b/pkg/controllers/node/suite_test.go
index 460802bf42..51077b3d4d 100644
--- a/pkg/controllers/node/suite_test.go
+++ b/pkg/controllers/node/suite_test.go
@@ -50,6 +50,7 @@ var ctx context.Context
 var nodeController controller.Controller
 var env *test.Environment
 var fakeClock *clock.FakeClock
+var cluster *state.Cluster
 var cp *fake.CloudProvider
 
 func TestAPIs(t *testing.T) {
@@ -63,7 +64,7 @@ var _ = BeforeSuite(func() {
 	env = test.NewEnvironment(scheme.Scheme, test.WithCRDs(apis.CRDs...))
 	ctx = settings.ToContext(ctx, test.Settings())
 	cp = fake.NewCloudProvider()
-	cluster := state.NewCluster(fakeClock, env.Client, cp)
+	cluster = state.NewCluster(fakeClock, env.Client, cp)
 	nodeController = node.NewController(fakeClock, env.Client, cp, cluster)
 })
 
@@ -382,6 +383,25 @@ var _ = Describe("Controller", func() {
 			node = ExpectNodeExists(ctx, env.Client, node.Name)
 			Expect(node.Annotations).To(HaveKey(v1alpha5.EmptinessTimestampAnnotationKey))
 		})
+		It("should return a requeue polling interval when the node is underutilized and nominated", func() {
+			provisioner.Spec.TTLSecondsAfterEmpty = ptr.Int64(30)
+			node := test.Node(test.NodeOptions{ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					v1alpha5.ProvisionerNameLabelKey: provisioner.Name,
+					v1alpha5.LabelNodeInitialized:    "true",
+					v1.LabelInstanceTypeStable:       "default-instance-type", // need the instance type for the cluster state update
+				},
+			}})
+			ExpectApplied(ctx, env.Client, provisioner, node)
+
+			// Add the node to the cluster state and nominate it in the internal cluster state
+			Expect(cluster.UpdateNode(ctx, node)).To(Succeed())
+			cluster.NominateNodeForPod(ctx, node.Name)
+
+			result := ExpectReconcileSucceeded(ctx, nodeController, client.ObjectKeyFromObject(node))
+			Expect(result.RequeueAfter).To(Equal(time.Second * 30))
+			Expect(node.Labels).ToNot(HaveKey(v1alpha5.EmptinessTimestampAnnotationKey))
+		})
 		It("should remove labels from non-empty nodes", func() {
 			provisioner.Spec.TTLSecondsAfterEmpty = ptr.Int64(30)
 			node := test.Node(test.NodeOptions{ObjectMeta: metav1.ObjectMeta{