From 194a448b6ff92e20ef757da7a8b9da004fb0d076 Mon Sep 17 00:00:00 2001 From: yy158775 <1584616775@qq.com> Date: Sun, 7 Aug 2022 16:49:40 +0800 Subject: [PATCH 1/2] patch ClusterResourceBinding instead of update and update observed generation. Signed-off-by: yy158775 <1584616775@qq.com> --- pkg/scheduler/scheduler.go | 53 +++++++++++++++++++------------------- 1 file changed, 27 insertions(+), 26 deletions(-) diff --git a/pkg/scheduler/scheduler.go b/pkg/scheduler/scheduler.go index 2b8c436ff330..cfcb6bbfb057 100644 --- a/pkg/scheduler/scheduler.go +++ b/pkg/scheduler/scheduler.go @@ -18,7 +18,6 @@ import ( "k8s.io/client-go/kubernetes" "k8s.io/client-go/tools/cache" "k8s.io/client-go/tools/record" - "k8s.io/client-go/util/retry" "k8s.io/client-go/util/workqueue" "k8s.io/klog/v2" @@ -417,10 +416,10 @@ func (s *Scheduler) doScheduleClusterBinding(name string) (err error) { } else { condition = util.NewCondition(workv1alpha2.Scheduled, scheduleFailedReason, err.Error(), metav1.ConditionFalse) } - if updateErr := s.updateClusterBindingScheduledConditionIfNeeded(crb, condition); updateErr != nil { - klog.Errorf("Failed update condition(%s) for ClusterResourceBinding(%s)", workv1alpha2.Scheduled, crb.Name) + if updateErr := s.patchClusterBindingScheduleStatus(crb, condition); updateErr != nil { + klog.Errorf("Failed to patch schedule status to ClusterResourceBinding(%s): %v", crb.Name, err) if err == nil { - // schedule succeed but update condition failed, return err in order to retry in next loop. + // schedule succeed but update status failed, return err in order to retry in next loop. err = updateErr } } @@ -662,36 +661,38 @@ func (s *Scheduler) patchBindingScheduleStatus(rb *workv1alpha2.ResourceBinding, return nil } -// updateClusterBindingScheduledConditionIfNeeded sets the scheduled condition of ClusterResourceBinding if needed -func (s *Scheduler) updateClusterBindingScheduledConditionIfNeeded(crb *workv1alpha2.ClusterResourceBinding, newScheduledCondition metav1.Condition) error { +// patchClusterBindingScheduleStatus patches schedule status of ClusterResourceBinding when necessary +func (s *Scheduler) patchClusterBindingScheduleStatus(crb *workv1alpha2.ClusterResourceBinding, newScheduledCondition metav1.Condition) error { if crb == nil { return nil } - oldScheduledCondition := meta.FindStatusCondition(crb.Status.Conditions, workv1alpha2.Scheduled) - if oldScheduledCondition != nil { - if util.IsConditionsEqual(newScheduledCondition, *oldScheduledCondition) { - klog.V(4).Infof("No need to update scheduled condition") - return nil - } + modifiedObj := crb.DeepCopy() + meta.SetStatusCondition(&modifiedObj.Status.Conditions, newScheduledCondition) + // Postpone setting observed generation until schedule succeed, assume scheduler will retry and + // will succeed eventually + if newScheduledCondition.Status == metav1.ConditionTrue { + modifiedObj.Status.SchedulerObservedGeneration = modifiedObj.Generation } - return retry.RetryOnConflict(retry.DefaultRetry, func() error { - meta.SetStatusCondition(&crb.Status.Conditions, newScheduledCondition) - _, updateErr := s.KarmadaClient.WorkV1alpha2().ClusterResourceBindings().UpdateStatus(context.TODO(), crb, metav1.UpdateOptions{}) - if updateErr == nil { - return nil - } + // Short path, ignore patch if no change. + if reflect.DeepEqual(crb.Status, modifiedObj.Status) { + return nil + } - if updated, err := s.KarmadaClient.WorkV1alpha2().ClusterResourceBindings().Get(context.TODO(), crb.Name, metav1.GetOptions{}); err == nil { - // make a copy so we don't mutate the shared cache - crb = updated.DeepCopy() - } else { - klog.Errorf("failed to get updated cluster resource binding %s: %v", crb.Name, err) - } + patchBytes, err := helper.GenMergePatch(crb, modifiedObj) + if err != nil { + return fmt.Errorf("failed to create a merge patch: %v", err) + } - return updateErr - }) + _, err = s.KarmadaClient.WorkV1alpha2().ClusterResourceBindings().Patch(context.TODO(), crb.Name, types.MergePatchType, patchBytes, metav1.PatchOptions{}, "status") + if err != nil { + klog.Errorf("Failed to patch schedule status to ClusterResourceBinding(%s): %v", crb.Name, err) + return err + } + + klog.V(4).Infof("Patch schedule status to ClusterResourceBinding(%s) succeed", crb.Name) + return nil } func (s *Scheduler) recordScheduleResultEventForResourceBinding(rb *workv1alpha2.ResourceBinding, schedulerErr error) { From 2d8d70202bfb7e5e29c17666ee72da54f9cc983f Mon Sep 17 00:00:00 2001 From: yy158775 <1584616775@qq.com> Date: Sun, 7 Aug 2022 16:53:20 +0800 Subject: [PATCH 2/2] add test for Scheduler's patchXXX Signed-off-by: yy158775 <1584616775@qq.com> --- pkg/scheduler/scheduler_test.go | 214 ++++++++++++++++++++++++++++++++ 1 file changed, 214 insertions(+) diff --git a/pkg/scheduler/scheduler_test.go b/pkg/scheduler/scheduler_test.go index 35a09a6cc2b6..364a2c08c3ea 100644 --- a/pkg/scheduler/scheduler_test.go +++ b/pkg/scheduler/scheduler_test.go @@ -1,13 +1,19 @@ package scheduler import ( + "context" + "reflect" "testing" + "time" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" dynamicfake "k8s.io/client-go/dynamic/fake" "k8s.io/client-go/kubernetes/fake" + workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" karmadafake "github.com/karmada-io/karmada/pkg/generated/clientset/versioned/fake" + "github.com/karmada-io/karmada/pkg/util" ) func TestCreateScheduler(t *testing.T) { @@ -63,3 +69,211 @@ func TestCreateScheduler(t *testing.T) { }) } } + +func Test_patchBindingScheduleStatus(t *testing.T) { + oneHourBefore := time.Now().Add(-1 * time.Hour).Round(time.Second) + oneHourAfter := time.Now().Add(1 * time.Hour).Round(time.Second) + + successCondition := util.NewCondition(workv1alpha2.Scheduled, scheduleSuccessReason, scheduleSuccessMessage, metav1.ConditionTrue) + failureCondition := util.NewCondition(workv1alpha2.Scheduled, scheduleFailedReason, "schedule error", metav1.ConditionFalse) + + successCondition.LastTransitionTime = metav1.Time{Time: oneHourBefore} + failureCondition.LastTransitionTime = metav1.Time{Time: oneHourAfter} + + dynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + karmadaClient := karmadafake.NewSimpleClientset() + kubeClient := fake.NewSimpleClientset() + + scheduler, err := NewScheduler(dynamicClient, karmadaClient, kubeClient) + if err != nil { + t.Error(err) + } + + tests := []struct { + name string + binding *workv1alpha2.ResourceBinding + newScheduledCondition metav1.Condition + expected *workv1alpha2.ResourceBinding + }{ + { + name: "add success condition", + binding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-1", Namespace: "default", Generation: 1}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{}, + }, + newScheduledCondition: successCondition, + expected: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-1", Namespace: "default", Generation: 1}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}, SchedulerObservedGeneration: 1}, + }, + }, + { + name: "add failure condition", + binding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-2", Namespace: "default"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{}, + }, + newScheduledCondition: failureCondition, + expected: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-2", Namespace: "default"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}}, + }, + }, + { + name: "replace to success condition", + binding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-3", Namespace: "default", Generation: 1}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}, SchedulerObservedGeneration: 2}, + }, + newScheduledCondition: successCondition, + expected: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-3", Namespace: "default"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}, SchedulerObservedGeneration: 1}, + }, + }, + { + name: "replace failure condition", + binding: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-4", Namespace: "default"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}}, + }, + newScheduledCondition: failureCondition, + expected: &workv1alpha2.ResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-4", Namespace: "default"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := karmadaClient.WorkV1alpha2().ResourceBindings(test.binding.Namespace).Create(context.TODO(), test.binding, metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + err = scheduler.patchBindingScheduleStatus(test.binding, test.newScheduledCondition) + if err != nil { + t.Error(err) + } + res, err := karmadaClient.WorkV1alpha2().ResourceBindings(test.binding.Namespace).Get(context.TODO(), test.binding.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(res.Status, test.expected.Status) { + t.Errorf("expected status: %v, but got: %v", test.expected.Status, res.Status) + } + }) + } +} + +func Test_patchClusterBindingScheduleStatus(t *testing.T) { + oneHourBefore := time.Now().Add(-1 * time.Hour).Round(time.Second) + oneHourAfter := time.Now().Add(1 * time.Hour).Round(time.Second) + + successCondition := util.NewCondition(workv1alpha2.Scheduled, scheduleSuccessReason, scheduleSuccessMessage, metav1.ConditionTrue) + failureCondition := util.NewCondition(workv1alpha2.Scheduled, scheduleFailedReason, "schedule error", metav1.ConditionFalse) + + successCondition.LastTransitionTime = metav1.Time{Time: oneHourBefore} + failureCondition.LastTransitionTime = metav1.Time{Time: oneHourAfter} + + dynamicClient := dynamicfake.NewSimpleDynamicClient(runtime.NewScheme()) + karmadaClient := karmadafake.NewSimpleClientset() + kubeClient := fake.NewSimpleClientset() + + scheduler, err := NewScheduler(dynamicClient, karmadaClient, kubeClient) + if err != nil { + t.Error(err) + } + + tests := []struct { + name string + binding *workv1alpha2.ClusterResourceBinding + newScheduledCondition metav1.Condition + expected *workv1alpha2.ClusterResourceBinding + }{ + { + name: "add success condition", + binding: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-1", Generation: 1}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{}, + }, + newScheduledCondition: successCondition, + expected: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-1"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}, SchedulerObservedGeneration: 1}, + }, + }, + { + name: "add failure condition", + binding: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-2"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{}, + }, + newScheduledCondition: failureCondition, + expected: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-2"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}}, + }, + }, + { + name: "replace to success condition", + binding: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-3", Generation: 1}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}, SchedulerObservedGeneration: 2}, + }, + newScheduledCondition: successCondition, + expected: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-3"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}, SchedulerObservedGeneration: 1}, + }, + }, + { + name: "replace failure condition", + binding: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-4"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{successCondition}}, + }, + newScheduledCondition: failureCondition, + expected: &workv1alpha2.ClusterResourceBinding{ + ObjectMeta: metav1.ObjectMeta{Name: "rb-4"}, + Spec: workv1alpha2.ResourceBindingSpec{}, + Status: workv1alpha2.ResourceBindingStatus{Conditions: []metav1.Condition{failureCondition}}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + _, err := karmadaClient.WorkV1alpha2().ClusterResourceBindings().Create(context.TODO(), test.binding, metav1.CreateOptions{}) + if err != nil { + t.Fatal(err) + } + err = scheduler.patchClusterBindingScheduleStatus(test.binding, test.newScheduledCondition) + if err != nil { + t.Error(err) + } + res, err := karmadaClient.WorkV1alpha2().ClusterResourceBindings().Get(context.TODO(), test.binding.Name, metav1.GetOptions{}) + if err != nil { + t.Fatal(err) + } + if !reflect.DeepEqual(res.Status, test.expected.Status) { + t.Errorf("expected status: %v, but got: %v", test.expected.Status, res.Status) + } + }) + } +}