From 2df78f9a25f152af11ce0e2d4c36467d9de5f285 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/BUILD | 1 + pkg/scheduler/util/utils.go | 18 +++++--- pkg/scheduler/util/utils_test.go | 79 ++++++++++++++++++++++++++++++++ 4 files changed, 95 insertions(+), 11 deletions(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 720882e5d37..bd1b40a4d3d 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -357,17 +357,17 @@ func truncateMessage(message string) string { func updatePod(client clientset.Interface, pod *v1.Pod, condition *v1.PodCondition, nominatedNode string) error { klog.V(3).Infof("Updating pod condition for %s/%s to (%s==%s, Reason=%s)", pod.Namespace, pod.Name, condition.Type, condition.Status, 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/BUILD b/pkg/scheduler/util/BUILD index 0b292065e0e..4d5de820ab5 100644 --- a/pkg/scheduler/util/BUILD +++ b/pkg/scheduler/util/BUILD @@ -25,6 +25,7 @@ go_test( "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library", "//staging/src/k8s.io/client-go/testing:go_default_library", "//staging/src/k8s.io/kube-scheduler/extender/v1:go_default_library", + "//vendor/github.com/google/go-cmp/cmp:go_default_library", "//vendor/github.com/stretchr/testify/assert:go_default_library", ], ) diff --git a/pkg/scheduler/util/utils.go b/pkg/scheduler/util/utils.go index 05e822e67d9..579ad66c670 100644 --- a/pkg/scheduler/util/utils.go +++ b/pkg/scheduler/util/utils.go @@ -117,15 +117,19 @@ func GetPodAntiAffinityTerms(affinity *v1.Affinity) (terms []v1.PodAffinityTerm) return terms } -// 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 } @@ -155,9 +159,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 0a7e6f5967a..f9cd9b2f7e1 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" @@ -171,3 +173,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) + } + }) + } +}