remove unnecessary patch requests (#3380)

the suit seems failed due to a flaky test timeout, will manually rerun the suit
kubernetes-sigs · Sep 11, 2023 · 5a5885b · 5a5885b
1 parent db7416e
commit 5a5885b
Show file tree

Hide file tree

Showing 2 changed files with 26 additions and 85 deletions.
diff --git a/pkg/targetgroupbinding/resource_manager.go b/pkg/targetgroupbinding/resource_manager.go
@@ -2,7 +2,6 @@ package targetgroupbinding
 
 import (
 	"context"
-	"encoding/json"
 	"fmt"
 	"net/netip"
 	"time"
@@ -17,9 +16,7 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/apimachinery/pkg/util/sets"
-	"k8s.io/apimachinery/pkg/util/strategicpatch"
 	elbv2api "sigs.k8s.io/aws-load-balancer-controller/apis/elbv2/v1beta1"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/aws/services"
 	"sigs.k8s.io/aws-load-balancer-controller/pkg/backend"
@@ -290,9 +287,9 @@ func (m *defaultResourceManager) updateTargetHealthPodConditionForPod(ctx contex
 	}
 	needFurtherProbe := targetHealthCondStatus != corev1.ConditionTrue
 
-	existingTargetHealthCond, exists := pod.GetPodCondition(targetHealthCondType)
+	existingTargetHealthCond, hasExistingTargetHealthCond := pod.GetPodCondition(targetHealthCondType)
 	// we skip patch pod if it matches current computed status/reason/message.
-	if exists &&
+	if hasExistingTargetHealthCond &&
 		existingTargetHealthCond.Status == targetHealthCondStatus &&
 		existingTargetHealthCond.Reason == reason &&
 		existingTargetHealthCond.Message == message {
@@ -305,22 +302,30 @@ func (m *defaultResourceManager) updateTargetHealthPodConditionForPod(ctx contex
 		Reason:  reason,
 		Message: message,
 	}
-	if !exists || existingTargetHealthCond.Status != targetHealthCondStatus {
+	if !hasExistingTargetHealthCond || existingTargetHealthCond.Status != targetHealthCondStatus {
 		newTargetHealthCond.LastTransitionTime = metav1.Now()
+	} else {
+		newTargetHealthCond.LastTransitionTime = existingTargetHealthCond.LastTransitionTime
 	}
 
-	patch, err := buildPodConditionPatch(pod, newTargetHealthCond)
-	if err != nil {
-		return false, err
-	}
-	k8sPod := &corev1.Pod{
+	podPatchSource := &corev1.Pod{
 		ObjectMeta: metav1.ObjectMeta{
 			Namespace: pod.Key.Namespace,
 			Name:      pod.Key.Name,
-			UID:       pod.UID,
+		},
+		Status: corev1.PodStatus{
+			Conditions: []corev1.PodCondition{},
 		},
 	}
-	if err := m.k8sClient.Status().Patch(ctx, k8sPod, patch); err != nil {
+	if hasExistingTargetHealthCond {
+		podPatchSource.Status.Conditions = []corev1.PodCondition{existingTargetHealthCond}
+	}
+
+	podPatchTarget := podPatchSource.DeepCopy()
+	podPatchTarget.UID = pod.UID // only put the uid in the new object to ensure it appears in the patch as a precondition
+	podPatchTarget.Status.Conditions = []corev1.PodCondition{newTargetHealthCond}
+
+	if err := m.k8sClient.Status().Patch(ctx, podPatchTarget, client.StrategicMergeFrom(podPatchSource)); err != nil {
 		if apierrors.IsNotFound(err) {
 			return false, nil
 		}
@@ -512,31 +517,6 @@ func matchNodePortEndpointWithTargets(endpoints []backend.NodePortEndpoint, targ
 	return matchedEndpointAndTargets, unmatchedEndpoints, unmatchedTargets
 }
 
-func buildPodConditionPatch(pod k8s.PodInfo, condition corev1.PodCondition) (client.Patch, error) {
-	oldData, err := json.Marshal(corev1.Pod{
-		Status: corev1.PodStatus{
-			Conditions: nil,
-		},
-	})
-	if err != nil {
-		return nil, err
-	}
-	newData, err := json.Marshal(corev1.Pod{
-		ObjectMeta: metav1.ObjectMeta{UID: pod.UID}, // only put the uid in the new object to ensure it appears in the patch as a precondition
-		Status: corev1.PodStatus{
-			Conditions: []corev1.PodCondition{condition},
-		},
-	})
-	if err != nil {
-		return nil, err
-	}
-	patchBytes, err := strategicpatch.CreateTwoWayMergePatch(oldData, newData, corev1.Pod{})
-	if err != nil {
-		return nil, err
-	}
-	return client.RawPatch(types.StrategicMergePatchType, patchBytes), nil
-}
-
 func isELBV2TargetGroupNotFoundError(err error) bool {
 	var awsErr awserr.Error
 	if errors.As(err, &awsErr) {

diff --git a/pkg/targetgroupbinding/resource_manager_test.go b/pkg/targetgroupbinding/resource_manager_test.go
@@ -137,8 +137,10 @@ func Test_defaultResourceManager_updateTargetHealthPodConditionForPod(t *testing
 						Status: corev1.PodStatus{
 							Conditions: []corev1.PodCondition{
 								{
-									Type:   "target-health.elbv2.k8s.aws/my-tgb",
-									Status: corev1.ConditionFalse,
+									Type:    "target-health.elbv2.k8s.aws/my-tgb",
+									Message: elbv2sdk.TargetHealthReasonEnumElbRegistrationInProgress,
+									Reason:  "Elb.RegistrationInProgress",
+									Status:  corev1.ConditionFalse,
 								},
 								{
 									Type:   corev1.ContainersReady,
@@ -160,8 +162,10 @@ func Test_defaultResourceManager_updateTargetHealthPodConditionForPod(t *testing
 					},
 					Conditions: []corev1.PodCondition{
 						{
-							Type:   "target-health.elbv2.k8s.aws/my-tgb",
-							Status: corev1.ConditionFalse,
+							Type:    "target-health.elbv2.k8s.aws/my-tgb",
+							Message: elbv2sdk.TargetHealthReasonEnumElbRegistrationInProgress,
+							Reason:  "Elb.RegistrationInProgress",
+							Status:  corev1.ConditionFalse,
 						},
 						{
 							Type:   corev1.ContainersReady,
@@ -459,46 +463,3 @@ func Test_containsTargetsInInitialState(t *testing.T) {
 		})
 	}
 }
-
-func Test_buildPodConditionPatch(t *testing.T) {
-	type args struct {
-		pod       k8s.PodInfo
-		condition corev1.PodCondition
-	}
-	tests := []struct {
-		name      string
-		args      args
-		wantPatch []byte
-		wantErr   error
-	}{
-		{
-			name: "standard case",
-			args: args{
-				pod: k8s.PodInfo{
-					Key: types.NamespacedName{Namespace: "ns-1", Name: "pod-1"},
-					UID: "pod-uuid",
-				},
-				condition: corev1.PodCondition{
-					Type:    "custom-condition",
-					Status:  corev1.ConditionTrue,
-					Reason:  "some-reason",
-					Message: "some-msg",
-				},
-			},
-			wantPatch: []byte(`{"metadata":{"uid":"pod-uuid"},"status":{"conditions":[{"lastProbeTime":null,"lastTransitionTime":null,"message":"some-msg","reason":"some-reason","status":"True","type":"custom-condition"}]}}`),
-		},
-	}
-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			got, err := buildPodConditionPatch(tt.args.pod, tt.args.condition)
-			if tt.wantErr != nil {
-				assert.EqualError(t, err, tt.wantErr.Error())
-			} else {
-				assert.NoError(t, err)
-				gotPatch, _ := got.Data(nil)
-				assert.Equal(t, tt.wantPatch, gotPatch)
-				assert.Equal(t, types.StrategicMergePatchType, got.Type())
-			}
-		})
-	}
-}