From 48dfa2a55420fc5078e993ac863d566962873927 Mon Sep 17 00:00:00 2001 From: Marwan Ahmed Date: Thu, 24 Jun 2021 16:30:48 -0700 Subject: [PATCH] generate scheduler merge patches on the pod status instead of the full pod --- pkg/scheduler/scheduler.go | 8 ++-- pkg/scheduler/util/utils.go | 18 +++++--- pkg/scheduler/util/utils_test.go | 79 ++++++++++++++++++++++++++++++++ 3 files changed, 94 insertions(+), 11 deletions(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 672dd537b24..e07850cd899 100644 --- a/pkg/scheduler/scheduler.go +++ b/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. diff --git a/pkg/scheduler/util/utils.go b/pkg/scheduler/util/utils.go index 6836a2150b4..7f8c8dde3d6 100644 --- a/pkg/scheduler/util/utils.go +++ b/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 to , +// PatchPodStatus calculates the delta bytes change from to , // 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) } } diff --git a/pkg/scheduler/util/utils_test.go b/pkg/scheduler/util/utils_test.go index c48e6d7336a..ef0c77360da 100644 --- a/pkg/scheduler/util/utils_test.go +++ b/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) + } + }) + } +}