Merge pull request kubernetes#333 from gujingit/bugfix/route-controll…

…er-patch-node-status fix route controller patches node condition based on old node info
Lyt99 · Oct 12, 2022 · 17ba5a9 · 17ba5a9
2 parents 1604f30 + e329623
commit 17ba5a9
Show file tree

Hide file tree

Showing 5 changed files with 140 additions and 31 deletions.
diff --git a/pkg/controller/helper/node_utils.go b/pkg/controller/helper/node_utils.go
@@ -89,13 +89,20 @@ func PatchM(mclient client.Client, target client.Object, getter func(runtime.Obj
 }
 
 func FindCondition(conds []v1.NodeCondition, conditionType v1.NodeConditionType) (*v1.NodeCondition, bool) {
+	var retCon *v1.NodeCondition
 	for i := range conds {
 		if conds[i].Type == conditionType {
-			return &conds[i], true
+			if retCon == nil || retCon.LastHeartbeatTime.Before(&conds[i].LastHeartbeatTime) {
+				retCon = &conds[i]
+			}
 		}
 	}
-	// condition not found, do not trigger repair
-	return &v1.NodeCondition{}, false
+
+	if retCon == nil {
+		return &v1.NodeCondition{}, false
+	} else {
+		return retCon, true
+	}
 }
 
 func HasExcludeLabel(node *v1.Node) bool {

diff --git a/pkg/controller/helper/node_utils_test.go b/pkg/controller/helper/node_utils_test.go
@@ -0,0 +1,89 @@
+package helper
+
+import (
+	"context"
+	"github.com/stretchr/testify/assert"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/runtime"
+	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"testing"
+)
+
+func TestPatchM(t *testing.T) {
+	diff := func(copy runtime.Object) (client.Object, error) {
+		nins := copy.(*v1.Node)
+		condition, ok := FindCondition(nins.Status.Conditions, v1.NodeNetworkUnavailable)
+		condition.Type = v1.NodeNetworkUnavailable
+		condition.Status = v1.ConditionFalse
+		condition.Reason = "RouteCreated"
+		condition.Message = "RouteController created a route"
+
+		if !ok {
+			nins.Status.Conditions = append(nins.Status.Conditions, *condition)
+		}
+		return nins, nil
+	}
+
+	mclient := getFakeKubeClient()
+	node1 := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:   "node-1",
+			Labels: map[string]string{"app": "nginx"},
+		},
+	}
+
+	err := PatchM(mclient, node1, diff, PatchStatus)
+	if err != nil {
+		t.Error(err)
+	}
+	node1Patched := &v1.Node{}
+	_ = mclient.Get(context.TODO(), client.ObjectKey{
+		Name: node1.Name,
+	}, node1Patched)
+
+	for _, con := range node1Patched.Status.Conditions {
+		if con.Type == v1.NodeNetworkUnavailable {
+			assert.Equal(t, v1.ConditionFalse, con.Status)
+			return
+		}
+	}
+	t.Error("Fail to patch node network status")
+
+}
+
+func getFakeKubeClient() client.Client {
+	// Node
+	nodeList := &v1.NodeList{
+		Items: []v1.Node{
+			{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:   "node-1",
+					Labels: map[string]string{"app": "nginx"},
+				},
+				Spec: v1.NodeSpec{
+					PodCIDR:    "10.96.0.64/26",
+					ProviderID: "cn-hangzhou.ecs-id",
+				},
+				Status: v1.NodeStatus{
+					Conditions: []v1.NodeCondition{
+						{
+							Type:   v1.NodeReady,
+							Reason: string(v1.NodeReady),
+							Status: v1.ConditionTrue,
+						},
+						{
+							Type:   v1.NodeNetworkUnavailable,
+							Reason: "No Route Created",
+							Status: v1.ConditionTrue,
+						},
+					},
+				},
+			},
+		},
+	}
+
+	objs := []runtime.Object{nodeList}
+	return fake.NewClientBuilder().WithRuntimeObjects(objs...).Build()
+}
diff --git a/pkg/controller/route/manager.go b/pkg/controller/route/manager.go
@@ -140,11 +140,8 @@ func (r *ReconcileRoute) syncTableRoutes(ctx context.Context, table string, node
 			continue
 		}
 
-		networkCondition, ok := helper.FindCondition(node.Status.Conditions, v1.NodeNetworkUnavailable)
-		if !ok || networkCondition.Status != v1.ConditionFalse {
-			if err := r.updateNetworkingCondition(ctx, &node, true); err != nil {
-				klog.Errorf("update node %s network condition err: %s", node.Name, err.Error())
-			}
+		if err := r.updateNetworkingCondition(ctx, &node, true); err != nil {
+			klog.Errorf("update node %s network condition err: %s", node.Name, err.Error())
 		}
 	}
 	return nil

diff --git a/pkg/controller/route/route_controller.go b/pkg/controller/route/route_controller.go
@@ -201,11 +201,6 @@ func (r *ReconcileRoute) syncCloudRoute(ctx context.Context, node *corev1.Node)
 		}
 		return utilerrors.NewAggregate(tablesErr)
 	} else {
-		networkCondition, ok := helper.FindCondition(node.Status.Conditions, corev1.NodeNetworkUnavailable)
-		if ok && networkCondition.Status == corev1.ConditionFalse {
-			// Update condition only if it doesn't reflect the current state.
-			return nil
-		}
 		return r.updateNetworkingCondition(ctx, node, true)
 	}
 }
@@ -264,28 +259,43 @@ func (r *ReconcileRoute) addRouteForNode(
 }
 
 func (r *ReconcileRoute) updateNetworkingCondition(ctx context.Context, node *corev1.Node, routeCreated bool) error {
+	networkCondition, ok := helper.FindCondition(node.Status.Conditions, corev1.NodeNetworkUnavailable)
+	if routeCreated && ok && networkCondition.Status == corev1.ConditionFalse {
+		klog.V(2).Infof("set node %v with NodeNetworkUnavailable=false was canceled because it is already set", node.Name)
+		return nil
+	}
+
+	if !routeCreated && ok && networkCondition.Status == corev1.ConditionTrue {
+		klog.V(2).Infof("set node %v with NodeNetworkUnavailable=true was canceled because it is already set", node.Name)
+		return nil
+	}
+
+	klog.Infof("Patching node status %v with %v previous condition was:%+v", node.Name, routeCreated, networkCondition)
 	var err error
 	for i := 0; i < updateNodeStatusMaxRetries; i++ {
 		// Patch could also fail, even though the chance is very slim. So we still do
 		// patch in the retry loop.
-		patch := client.MergeFrom(node.DeepCopy())
-		condition, ok := helper.FindCondition(node.Status.Conditions, corev1.NodeNetworkUnavailable)
-		condition.Type = corev1.NodeNetworkUnavailable
-		condition.LastHeartbeatTime = metav1.Now()
-		condition.LastTransitionTime = metav1.Now()
-		if routeCreated {
-			condition.Status = corev1.ConditionFalse
-			condition.Reason = "RouteCreated"
-			condition.Message = "RouteController created a route"
-		} else {
-			condition.Status = corev1.ConditionTrue
-			condition.Reason = "NoRouteCreated"
-			condition.Message = "RouteController failed to create a route"
-		}
-		if !ok {
-			node.Status.Conditions = append(node.Status.Conditions, *condition)
+		diff := func(copy runtime.Object) (client.Object, error) {
+			nins := copy.(*corev1.Node)
+			condition, ok := helper.FindCondition(nins.Status.Conditions, corev1.NodeNetworkUnavailable)
+			condition.Type = corev1.NodeNetworkUnavailable
+			condition.LastTransitionTime = metav1.Now()
+			condition.LastHeartbeatTime = metav1.Now()
+			if routeCreated {
+				condition.Status = corev1.ConditionFalse
+				condition.Reason = "RouteCreated"
+				condition.Message = "RouteController created a route"
+			} else {
+				condition.Status = corev1.ConditionTrue
+				condition.Reason = "NoRouteCreated"
+				condition.Message = "RouteController failed to create a route"
+			}
+			if !ok {
+				nins.Status.Conditions = append(nins.Status.Conditions, *condition)
+			}
+			return nins, nil
 		}
-		err = r.client.Status().Patch(ctx, node, patch)
+		err = helper.PatchM(r.client, node, diff, helper.PatchStatus)
 		if err == nil {
 			return nil
 		}

diff --git a/pkg/controller/route/route_controller_test.go b/pkg/controller/route/route_controller_test.go
@@ -33,10 +33,12 @@ func TestUpdateNetworkingCondition(t *testing.T) {
 		Status: v1.NodeStatus{
 			Conditions: []v1.NodeCondition{
 				{
+					Type:   v1.NodeReady,
 					Reason: string(v1.NodeReady),
 					Status: v1.ConditionTrue,
 				},
 				{
+					Type:   v1.NodeNetworkUnavailable,
 					Reason: string(v1.NodeNetworkUnavailable),
 					Status: v1.ConditionTrue,
 				},
@@ -55,7 +57,7 @@ func TestUpdateNetworkingCondition(t *testing.T) {
 		t.Error(err)
 	}
 
-	networkCondition, ok := helper.FindCondition(node.Status.Conditions, v1.NodeNetworkUnavailable)
+	networkCondition, ok := helper.FindCondition(updatedNode.Status.Conditions, v1.NodeNetworkUnavailable)
 	if !ok || networkCondition.Status != v1.ConditionFalse {
 		t.Error("node condition update failed")
 	}
@@ -97,10 +99,12 @@ func getFakeKubeClient() client.Client {
 				Status: v1.NodeStatus{
 					Conditions: []v1.NodeCondition{
 						{
+							Type:   v1.NodeReady,
 							Reason: string(v1.NodeReady),
 							Status: v1.ConditionTrue,
 						},
 						{
+							Type:   v1.NodeNetworkUnavailable,
 							Reason: string(v1.NodeNetworkUnavailable),
 							Status: v1.ConditionTrue,
 						},
@@ -118,10 +122,12 @@ func getFakeKubeClient() client.Client {
 				Status: v1.NodeStatus{
 					Conditions: []v1.NodeCondition{
 						{
+							Type:   v1.NodeReady,
 							Reason: string(v1.NodeReady),
 							Status: v1.ConditionTrue,
 						},
 						{
+							Type:   v1.NodeNetworkUnavailable,
 							Reason: string(v1.NodeNetworkUnavailable),
 							Status: v1.ConditionFalse,
 						},