Skip to content

Commit

Permalink
Fix node requeue when node is nominated
Browse files Browse the repository at this point in the history
  • Loading branch information
jonathan-innis committed Apr 14, 2023
1 parent ace3584 commit 423e954
Show file tree
Hide file tree
Showing 2 changed files with 26 additions and 7 deletions.
11 changes: 5 additions & 6 deletions pkg/controllers/node/emptiness.go
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand All @@ -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) {
Expand Down
22 changes: 21 additions & 1 deletion pkg/controllers/node/suite_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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)
})

Expand Down Expand Up @@ -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
cluster.UpdateNode(ctx, node)
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{
Expand Down

0 comments on commit 423e954

Please sign in to comment.