Merge pull request #103133 from marwanad/allow-scheduler-to-patch-conditions

switch scheduler to generate the merge patch on pod status instead of the full pod

pkg/scheduler/scheduler.go

@@ -413,17 +413,17 @@ func truncateMessage(message string) string {
 
 func updatePod(client clientset.Interface, pod *v1.Pod, condition *v1.PodCondition, nominatedNode string) error {
 	klog.V(3).InfoS("Updating pod condition", "pod", klog.KObj(pod), "conditionType", condition.Type, "conditionStatus", condition.Status, "conditionReason", condition.Reason)
-	podCopy := pod.DeepCopy()
+	podStatusCopy := pod.Status.DeepCopy()
 	// NominatedNodeName is updated only if we are trying to set it, and the value is
 	// different from the existing one.
-	if !podutil.UpdatePodCondition(&podCopy.Status, condition) &&
+	if !podutil.UpdatePodCondition(podStatusCopy, condition) &&
 		(len(nominatedNode) == 0 || pod.Status.NominatedNodeName == nominatedNode) {
 		return nil
 	}
 	if nominatedNode != "" {
-		podCopy.Status.NominatedNodeName = nominatedNode
+		podStatusCopy.NominatedNodeName = nominatedNode
 	}
-	return util.PatchPod(client, pod, podCopy)
+	return util.PatchPodStatus(client, pod, podStatusCopy)
 }
 
 // assume signals to the cache that a pod is already in the cache, so that binding can be asynchronous.

pkg/scheduler/util/utils.go

@@ -90,15 +90,19 @@ func MoreImportantPod(pod1, pod2 *v1.Pod) bool {
 	return GetPodStartTime(pod1).Before(GetPodStartTime(pod2))
 }
 
-// PatchPod calculates the delta bytes change from <old> to <new>,
+// PatchPodStatus calculates the delta bytes change from <old.Status> to <newStatus>,
 // and then submit a request to API server to patch the pod changes.
-func PatchPod(cs kubernetes.Interface, old *v1.Pod, new *v1.Pod) error {
-	oldData, err := json.Marshal(old)
+func PatchPodStatus(cs kubernetes.Interface, old *v1.Pod, newStatus *v1.PodStatus) error {
+	if newStatus == nil {
+		return nil
+	}
+
+	oldData, err := json.Marshal(v1.Pod{Status: old.Status})
 	if err != nil {
 		return err
 	}
 
-	newData, err := json.Marshal(new)
+	newData, err := json.Marshal(v1.Pod{Status: *newStatus})
 	if err != nil {
 		return err
 	}
@@ -128,9 +132,9 @@ func ClearNominatedNodeName(cs kubernetes.Interface, pods ...*v1.Pod) utilerrors
 		if len(p.Status.NominatedNodeName) == 0 {
 			continue
 		}
-		podCopy := p.DeepCopy()
-		podCopy.Status.NominatedNodeName = ""
-		if err := PatchPod(cs, p, podCopy); err != nil {
+		podStatusCopy := p.Status.DeepCopy()
+		podStatusCopy.NominatedNodeName = ""
+		if err := PatchPodStatus(cs, p, podStatusCopy); err != nil {
 			errs = append(errs, err)
 		}
 	}

pkg/scheduler/util/utils_test.go

@@ -17,7 +17,9 @@ limitations under the License.
 package util
 
 import (
+	"context"
 	"fmt"
+	"github.com/google/go-cmp/cmp"
 	"testing"
 	"time"
 
@@ -173,3 +175,80 @@ func TestRemoveNominatedNodeName(t *testing.T) {
 		})
 	}
 }
+
+func TestPatchPodStatus(t *testing.T) {
+	tests := []struct {
+		name           string
+		pod            v1.Pod
+		statusToUpdate v1.PodStatus
+	}{
+		{
+			name: "Should update pod conditions successfully",
+			pod: v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns",
+					Name:      "pod1",
+				},
+				Spec: v1.PodSpec{
+					ImagePullSecrets: []v1.LocalObjectReference{{Name: "foo"}},
+				},
+			},
+			statusToUpdate: v1.PodStatus{
+				Conditions: []v1.PodCondition{
+					{
+						Type:   v1.PodScheduled,
+						Status: v1.ConditionFalse,
+					},
+				},
+			},
+		},
+		{
+			// ref: #101697, #94626 - ImagePullSecrets are allowed to have empty secret names
+			// which would fail the 2-way merge patch generation on Pod patches
+			// due to the mergeKey being the name field
+			name: "Should update pod conditions successfully on a pod Spec with secrets with empty name",
+			pod: v1.Pod{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "ns",
+					Name:      "pod2",
+				},
+				Spec: v1.PodSpec{
+					// this will serialize to imagePullSecrets:[{}]
+					ImagePullSecrets: make([]v1.LocalObjectReference, 1),
+				},
+			},
+			statusToUpdate: v1.PodStatus{
+				Conditions: []v1.PodCondition{
+					{
+						Type:   v1.PodScheduled,
+						Status: v1.ConditionFalse,
+					},
+				},
+			},
+		},
+	}
+
+	client := clientsetfake.NewSimpleClientset()
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			_, err := client.CoreV1().Pods(tc.pod.Namespace).Create(context.TODO(), &tc.pod, metav1.CreateOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			err = PatchPodStatus(client, &tc.pod, &tc.statusToUpdate)
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			retrievedPod, err := client.CoreV1().Pods(tc.pod.Namespace).Get(context.TODO(), tc.pod.Name, metav1.GetOptions{})
+			if err != nil {
+				t.Fatal(err)
+			}
+
+			if diff := cmp.Diff(tc.statusToUpdate, retrievedPod.Status); diff != "" {
+				t.Errorf("unexpected pod status (-want,+got):\n%s", diff)
+			}
+		})
+	}
+}