From f718096b744b32b068c484e057c6b69ff1c11d99 Mon Sep 17 00:00:00 2001 From: Quan Tian Date: Thu, 21 Sep 2023 23:32:51 +0800 Subject: [PATCH] NoExecute taint should be added when a Node's ready condition becomes Unknown After a Node has stopped posting heartbeats for nodeMonitorGracePeriod, it will be considered unreachable, its ready condition will be set to Unknown, NoSchedule taint will be added, all Pods on it will be set to NotReady, but there is always a delay of 5s before NoExecute taint is added to the Node, adding 5s to the recovery time of Pods which are supposed to be evicted by the taint and recreated on other Nodes sooner. The delay is because processTaintBaseEviction() uses the last observed ready condition of the Node instead of the current one to determine whether it should add the Node to the taint queue. When a Node is set to unreachable due to missing heartbeats, the last observed ready condition is still true and the current ready condition is unknown, we should use the latter for processTaintBaseEviction(). Signed-off-by: Quan Tian --- .../node_lifecycle_controller.go | 8 +- .../node_lifecycle_controller_test.go | 185 ++++++++++++++++++ 2 files changed, 189 insertions(+), 4 deletions(-) diff --git a/pkg/controller/nodelifecycle/node_lifecycle_controller.go b/pkg/controller/nodelifecycle/node_lifecycle_controller.go index 24d68a8dfe6..3aff066e832 100644 --- a/pkg/controller/nodelifecycle/node_lifecycle_controller.go +++ b/pkg/controller/nodelifecycle/node_lifecycle_controller.go @@ -734,7 +734,7 @@ func (nc *Controller) monitorNodeHealth(ctx context.Context) error { } return } - nc.processTaintBaseEviction(ctx, node, &observedReadyCondition) + nc.processTaintBaseEviction(ctx, node, currentReadyCondition) _, needsRetry := nc.nodesToRetry.Load(node.Name) switch { @@ -765,11 +765,11 @@ func (nc *Controller) monitorNodeHealth(ctx context.Context) error { return nil } -func (nc *Controller) processTaintBaseEviction(ctx context.Context, node *v1.Node, observedReadyCondition *v1.NodeCondition) { +func (nc *Controller) processTaintBaseEviction(ctx context.Context, node *v1.Node, currentReadyCondition *v1.NodeCondition) { decisionTimestamp := nc.now() // Check eviction timeout against decisionTimestamp logger := klog.FromContext(ctx) - switch observedReadyCondition.Status { + switch currentReadyCondition.Status { case v1.ConditionFalse: // We want to update the taint straight away if Node is already tainted with the UnreachableTaint if taintutils.TaintExists(node.Spec.Taints, UnreachableTaintTemplate) { @@ -781,7 +781,7 @@ func (nc *Controller) processTaintBaseEviction(ctx context.Context, node *v1.Nod logger.V(2).Info("Node is NotReady. Adding it to the Taint queue", "node", klog.KObj(node), "timeStamp", decisionTimestamp) } case v1.ConditionUnknown: - // We want to update the taint straight away if Node is already tainted with the UnreachableTaint + // We want to update the taint straight away if Node is already tainted with the NotReadyTaintTemplate if taintutils.TaintExists(node.Spec.Taints, NotReadyTaintTemplate) { taintToAdd := *UnreachableTaintTemplate if !controllerutil.SwapNodeControllerTaint(ctx, nc.kubeClient, []*v1.Taint{&taintToAdd}, []*v1.Taint{NotReadyTaintTemplate}, node) { diff --git a/pkg/controller/nodelifecycle/node_lifecycle_controller_test.go b/pkg/controller/nodelifecycle/node_lifecycle_controller_test.go index 6b6e337f981..01c8d603bf4 100644 --- a/pkg/controller/nodelifecycle/node_lifecycle_controller_test.go +++ b/pkg/controller/nodelifecycle/node_lifecycle_controller_test.go @@ -2523,6 +2523,191 @@ func TestApplyNoExecuteTaints(t *testing.T) { } } +// TestApplyNoExecuteTaintsToUnreachableNode ensures a NoExecute taint is applied to node that hasn't posted the +// node status for a period greater than nodeMonitorGracePeriod. +func TestApplyNoExecuteTaintsToUnreachableNode(t *testing.T) { + // TODO: Remove skip once https://github.com/kubernetes/kubernetes/pull/114607 merges. + if goruntime.GOOS == "windows" { + t.Skip("Skipping test on Windows.") + } + time1 := metav1.Date(2017, 1, 1, 12, 0, 0, 0, time.UTC) + // time2 is set to NodeMonitorGracePeriod plus 1 second after time1. + time2 := metav1.Time{Time: time1.Add(testNodeMonitorGracePeriod + time.Second)} + + fakeNodeHandler := &testutil.FakeNodeHandler{ + Existing: []*v1.Node{ + // Unreachable Taint with effect 'NoExecute' should be applied to this node after 2015-01-01 12:00:40. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + Labels: map[string]string{ + v1.LabelTopologyRegion: "region1", + v1.LabelTopologyZone: "zone1", + v1.LabelFailureDomainBetaRegion: "region1", + v1.LabelFailureDomainBetaZone: "zone1", + }, + }, + Status: v1.NodeStatus{ + Conditions: []v1.NodeCondition{ + { + Type: v1.NodeReady, + Status: v1.ConditionTrue, + LastHeartbeatTime: time1, + LastTransitionTime: time1, + }, + }, + }, + }, + // Because of the logic that prevents NC from evicting anything when all Nodes are NotReady + // we need second healthy node in tests. + { + ObjectMeta: metav1.ObjectMeta{ + Name: "node1", + CreationTimestamp: metav1.Date(2012, 1, 1, 0, 0, 0, 0, time.UTC), + Labels: map[string]string{ + v1.LabelTopologyRegion: "region1", + v1.LabelTopologyZone: "zone1", + v1.LabelFailureDomainBetaRegion: "region1", + v1.LabelFailureDomainBetaZone: "zone1", + }, + }, + Status: v1.NodeStatus{ + Conditions: []v1.NodeCondition{ + { + Type: v1.NodeReady, + Status: v1.ConditionTrue, + LastHeartbeatTime: time1, + LastTransitionTime: time1, + }, + }, + }, + }, + }, + Clientset: fake.NewSimpleClientset(&v1.PodList{Items: []v1.Pod{*testutil.NewPod("pod0", "node0")}}), + } + _, ctx := ktesting.NewTestContext(t) + nodeController, _ := newNodeLifecycleControllerFromClient( + ctx, + fakeNodeHandler, + testRateLimiterQPS, + testRateLimiterQPS, + testLargeClusterThreshold, + testUnhealthyThreshold, + testNodeMonitorGracePeriod, + testNodeStartupGracePeriod, + testNodeMonitorPeriod, + ) + // No taint should be added when the NodeReady conditions were not set longer ago than NodeMonitorGracePeriod. + nodeController.now = func() metav1.Time { return time1 } + nodeController.recorder = testutil.NewFakeRecorder() + nodeController.getPodsAssignedToNode = fakeGetPodsAssignedToNode(fakeNodeHandler.Clientset) + if err := nodeController.syncNodeStore(fakeNodeHandler); err != nil { + t.Errorf("unexpected error: %v", err) + } + if err := nodeController.monitorNodeHealth(ctx); err != nil { + t.Errorf("unexpected error: %v", err) + } + nodeController.doNoExecuteTaintingPass(ctx) + node0, err := fakeNodeHandler.Get(ctx, "node0", metav1.GetOptions{}) + if err != nil { + t.Errorf("Can't get current node0...") + return + } + if len(node0.Spec.Taints) > 0 { + t.Errorf("Got unexpected taints %v", node0.Spec.Taints) + } + node1, err := fakeNodeHandler.Get(ctx, "node1", metav1.GetOptions{}) + if err != nil { + t.Errorf("Can't get current node1...") + return + } + if len(node1.Spec.Taints) > 0 { + t.Errorf("Got unexpected taints %v", node1.Spec.Taints) + } + + // Advance the clock to time2 to expire the previous NodeReady condition. + nodeController.now = func() metav1.Time { return time2 } + // Only node1 sends a new heartbeat at time2. + node1.Status = v1.NodeStatus{ + Conditions: []v1.NodeCondition{ + { + Type: v1.NodeReady, + Status: v1.ConditionTrue, + LastHeartbeatTime: time2, + LastTransitionTime: time1, + }, + }, + } + _, err = fakeNodeHandler.UpdateStatus(ctx, node1, metav1.UpdateOptions{}) + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + // Unreachable taint should be added to node0 and not node1 because the former's NodeReadyCondition was set longer ago than NodeMonitorGracePeriod. + if err := nodeController.syncNodeStore(fakeNodeHandler); err != nil { + t.Errorf("unexpected error: %v", err) + } + if err := nodeController.monitorNodeHealth(ctx); err != nil { + t.Errorf("unexpected error: %v", err) + } + if err := nodeController.syncNodeStore(fakeNodeHandler); err != nil { + t.Errorf("unexpected error: %v", err) + } + nodeController.doNoExecuteTaintingPass(ctx) + + node0, err = fakeNodeHandler.Get(ctx, "node0", metav1.GetOptions{}) + if err != nil { + t.Errorf("Can't get current node0...") + return + } + if !taintutils.TaintExists(node0.Spec.Taints, UnreachableTaintTemplate) { + t.Errorf("Can't find taint %v in %v", UnreachableTaintTemplate, node0.Spec.Taints) + } + node1, err = fakeNodeHandler.Get(ctx, "node1", metav1.GetOptions{}) + if err != nil { + t.Errorf("Can't get current node1...") + return + } + if len(node1.Spec.Taints) > 0 { + t.Errorf("Got unexpected taints %v", node1.Spec.Taints) + } + + // Unreachable taint should be deleted from node0 after it sends a new heartbeat. + node0.Status = v1.NodeStatus{ + Conditions: []v1.NodeCondition{ + { + Type: v1.NodeReady, + Status: v1.ConditionTrue, + LastHeartbeatTime: time2, + LastTransitionTime: time2, + }, + }, + } + _, err = fakeNodeHandler.UpdateStatus(ctx, node0, metav1.UpdateOptions{}) + if err != nil { + t.Errorf("unexpected error: %v", err) + return + } + if err := nodeController.syncNodeStore(fakeNodeHandler); err != nil { + t.Errorf("unexpected error: %v", err) + } + if err := nodeController.monitorNodeHealth(ctx); err != nil { + t.Errorf("unexpected error: %v", err) + } + nodeController.doNoExecuteTaintingPass(ctx) + + node0, err = fakeNodeHandler.Get(ctx, "node0", metav1.GetOptions{}) + if err != nil { + t.Errorf("Can't get current node0...") + return + } + // We should not see any taint on the node. + if len(node0.Spec.Taints) > 0 { + t.Errorf("Got unexpected taints %v", node0.Spec.Taints) + } +} + // TestApplyNoExecuteTaintsToNodesEnqueueTwice ensures we taint every node with NoExecute even if enqueued twice func TestApplyNoExecuteTaintsToNodesEnqueueTwice(t *testing.T) { // TODO: Remove skip once https://github.com/kubernetes/kubernetes/pull/114607 merges.