diff --git a/pkg/placement/controllers/scheduling/scheduling_controller.go b/pkg/placement/controllers/scheduling/scheduling_controller.go index afa924e7f..29d85f7cf 100644 --- a/pkg/placement/controllers/scheduling/scheduling_controller.go +++ b/pkg/placement/controllers/scheduling/scheduling_controller.go @@ -712,46 +712,60 @@ func (c *schedulingController) createOrUpdatePlacementDecision( // If status has been updated, just return, this is to avoid conflict when updating the label later. // Labels and annotations will still be updated in next reconcile. if updated { + // Record events when status is updated + c.recordDecisionEvents(placement, existPlacementDecision, clusterScores, status) return err } + // Do not record events when label is updated _, err = placementDecisionPatcher.PatchLabelAnnotations(ctx, newPlacementDecision, newPlacementDecision.ObjectMeta, existPlacementDecision.ObjectMeta) - if err != nil { - return err - } + return err +} - // update the event with warning +// recordDecisionEvents records DecisionUpdate and ScoreUpdate events for placement decision +func (c *schedulingController) recordDecisionEvents( + placement *clusterapiv1beta1.Placement, + placementDecision *clusterapiv1beta1.PlacementDecision, + clusterScores PrioritizerScore, + status *framework.Status, +) { + // Record decision update event with warning or normal type if status.Code() == framework.Warning { c.eventsRecorder.Eventf( - placement, existPlacementDecision, corev1.EventTypeWarning, + placement, placementDecision, corev1.EventTypeWarning, "DecisionUpdate", "DecisionUpdated", - "Decision %s is updated with placement %s in namespace %s: %s in plugin %s", existPlacementDecision.Name, placement.Name, placement.Namespace, + "Decision %s is updated with placement %s in namespace %s: %s in plugin %s", + placementDecision.Name, placement.Name, placement.Namespace, status.Message(), status.Plugin()) } else { c.eventsRecorder.Eventf( - placement, existPlacementDecision, corev1.EventTypeNormal, + placement, placementDecision, corev1.EventTypeNormal, "DecisionUpdate", "DecisionUpdated", - "Decision %s is updated with placement %s in namespace %s", existPlacementDecision.Name, placement.Name, placement.Namespace) + "Decision %s is updated with placement %s in namespace %s", + placementDecision.Name, placement.Name, placement.Namespace) } - // update the event with prioritizer score. + // Record score update event only if there are scores + sortedClusterNames := sets.List(sets.KeySet(clusterScores)) + if len(sortedClusterNames) == 0 { + return + } + + // Build score string with sorted cluster names for deterministic event messages scoreStr := "" - for k, v := range clusterScores { - tmpScore := fmt.Sprintf("%s:%d ", k, v) + for _, name := range sortedClusterNames { + tmpScore := fmt.Sprintf("%s:%d ", name, clusterScores[name]) if len(scoreStr)+len(tmpScore) > maxEventMessageLength { scoreStr += "......" break - } else { - scoreStr += tmpScore } + scoreStr += tmpScore } c.eventsRecorder.Eventf( - placement, existPlacementDecision, corev1.EventTypeNormal, + placement, placementDecision, corev1.EventTypeNormal, "ScoreUpdate", "ScoreUpdated", scoreStr) - - return nil } func calculateLength(intOrStr *intstr.IntOrString, total int) (int, *framework.Status) { diff --git a/pkg/placement/controllers/scheduling/scheduling_controller_event_test.go b/pkg/placement/controllers/scheduling/scheduling_controller_event_test.go new file mode 100644 index 000000000..ce1a2b3ad --- /dev/null +++ b/pkg/placement/controllers/scheduling/scheduling_controller_event_test.go @@ -0,0 +1,199 @@ +package scheduling + +import ( + "context" + "strings" + "testing" + "time" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + kevents "k8s.io/client-go/tools/events" + + clusterapiv1beta1 "open-cluster-management.io/api/cluster/v1beta1" + + "open-cluster-management.io/ocm/pkg/placement/controllers/framework" + testinghelpers "open-cluster-management.io/ocm/pkg/placement/helpers/testing" +) + +func TestCreateOrUpdatePlacementDecision_EventRecording(t *testing.T) { + cases := []struct { + name string + existingPD *clusterapiv1beta1.PlacementDecision + newDecisions []clusterapiv1beta1.ClusterDecision + newLabels map[string]string + status *framework.Status + clusterScores PrioritizerScore + expectEventCount int + expectDecisionCreate bool + expectDecisionUpdate bool + expectScoreUpdate bool + expectWarning bool + }{ + { + name: "new placement decision created", + existingPD: nil, + newDecisions: []clusterapiv1beta1.ClusterDecision{{ClusterName: "cluster1"}}, + newLabels: map[string]string{"test": "label"}, + status: framework.NewStatus("", framework.Success, ""), + clusterScores: PrioritizerScore{"cluster1": 85}, + expectEventCount: 1, + expectDecisionCreate: true, + expectDecisionUpdate: false, + expectScoreUpdate: false, + }, + { + name: "decisions changed", + existingPD: testinghelpers.NewPlacementDecision(placementNamespace, "placement1-decision-1"). + WithDecisions("cluster1"). + Build(), + newDecisions: []clusterapiv1beta1.ClusterDecision{ + {ClusterName: "cluster1"}, + {ClusterName: "cluster2"}, + }, + status: framework.NewStatus("", framework.Success, ""), + clusterScores: PrioritizerScore{"cluster1": 85, "cluster2": 92}, + expectEventCount: 2, + expectDecisionCreate: false, + expectDecisionUpdate: true, + expectScoreUpdate: true, + }, + { + name: "labels changed only", + existingPD: testinghelpers.NewPlacementDecision(placementNamespace, "placement1-decision-1"). + WithDecisions("cluster1"). + WithLabel(clusterapiv1beta1.DecisionGroupIndexLabel, "0"). + Build(), + newDecisions: []clusterapiv1beta1.ClusterDecision{{ClusterName: "cluster1"}}, + newLabels: map[string]string{ + clusterapiv1beta1.PlacementLabel: placementName, + clusterapiv1beta1.DecisionGroupIndexLabel: "1", + }, + status: framework.NewStatus("", framework.Success, ""), + clusterScores: PrioritizerScore{"cluster1": 85}, + expectEventCount: 0, + expectDecisionCreate: false, + expectDecisionUpdate: false, + expectScoreUpdate: false, + }, + { + name: "no changes", + existingPD: testinghelpers.NewPlacementDecision(placementNamespace, "placement1-decision-1"). + WithDecisions("cluster1"). + WithLabel(clusterapiv1beta1.PlacementLabel, placementName). + WithLabel(clusterapiv1beta1.DecisionGroupIndexLabel, "0"). + Build(), + newDecisions: []clusterapiv1beta1.ClusterDecision{{ClusterName: "cluster1"}}, + newLabels: map[string]string{ + clusterapiv1beta1.PlacementLabel: placementName, + clusterapiv1beta1.DecisionGroupIndexLabel: "0", + }, + status: framework.NewStatus("", framework.Success, ""), + clusterScores: PrioritizerScore{"cluster1": 85}, + expectEventCount: 0, + expectDecisionCreate: false, + expectDecisionUpdate: false, + expectScoreUpdate: false, + }, + { + name: "warning status event", + existingPD: testinghelpers.NewPlacementDecision(placementNamespace, "placement1-decision-1"). + WithDecisions("cluster1"). + Build(), + newDecisions: []clusterapiv1beta1.ClusterDecision{ + {ClusterName: "cluster1"}, + {ClusterName: "cluster2"}, + }, + status: framework.NewStatus("TestPlugin", framework.Warning, "test warning"), + clusterScores: PrioritizerScore{"cluster1": 85}, + expectEventCount: 2, + expectDecisionCreate: false, + expectDecisionUpdate: true, + expectScoreUpdate: true, + expectWarning: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var initObjs []runtime.Object + if c.existingPD != nil { + initObjs = append(initObjs, c.existingPD) + } + + ctrl, _, fakeRecorder := newTestSchedulingController(t, initObjs) + + placement := testinghelpers.NewPlacement(placementNamespace, placementName).Build() + placementDecision := &clusterapiv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "placement1-decision-1", + Namespace: placementNamespace, + Labels: c.newLabels, + }, + Status: clusterapiv1beta1.PlacementDecisionStatus{ + Decisions: c.newDecisions, + }, + } + + err := ctrl.createOrUpdatePlacementDecision(context.TODO(), placement, placementDecision, c.clusterScores, c.status) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // Collect and verify events + events := collectEvents(fakeRecorder, c.expectEventCount, 500*time.Millisecond) + verifyEvents(t, events, c.expectEventCount, c.expectDecisionCreate, c.expectDecisionUpdate, c.expectScoreUpdate, c.expectWarning) + }) + } +} + +// collectEvents collects events from FakeRecorder with a timeout +func collectEvents(recorder *kevents.FakeRecorder, expectedCount int, timeout time.Duration) []string { + var events []string + deadline := time.After(timeout) + + for i := 0; i < expectedCount; i++ { + select { + case event := <-recorder.Events: + events = append(events, event) + case <-deadline: + return events + } + } + + return events +} + +// verifyEvents verifies the collected events match expectations +func verifyEvents(t *testing.T, events []string, expectEventCount int, expectDecisionCreate, expectDecisionUpdate, expectScoreUpdate, expectWarning bool) { + if len(events) != expectEventCount { + t.Errorf("expected %d events, got %d events: %v", expectEventCount, len(events), events) + } + + eventTypes := map[string]bool{ + "DecisionCreate": expectDecisionCreate, + "DecisionUpdate": expectDecisionUpdate, + "ScoreUpdate": expectScoreUpdate, + } + + for eventType, expected := range eventTypes { + found := containsEventType(events, eventType) + if found != expected { + t.Errorf("expected %s=%v, got %v", eventType, expected, found) + } + } + + if expectWarning && !containsEventType(events, "Warning") { + t.Errorf("expected Warning event, got events: %v", events) + } +} + +// containsEventType checks if any event contains the specified type +func containsEventType(events []string, eventType string) bool { + for _, event := range events { + if strings.Contains(event, eventType) { + return true + } + } + return false +} diff --git a/pkg/placement/controllers/scheduling/scheduling_controller_test.go b/pkg/placement/controllers/scheduling/scheduling_controller_test.go index 962a3f551..826218cd0 100644 --- a/pkg/placement/controllers/scheduling/scheduling_controller_test.go +++ b/pkg/placement/controllers/scheduling/scheduling_controller_test.go @@ -613,28 +613,13 @@ func TestGetValidManagedClusterSetBindings(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - clusterClient := clusterfake.NewSimpleClientset(c.initObjs...) - clusterInformerFactory := newClusterInformerFactory(t, clusterClient, c.initObjs...) - - ctrl := &schedulingController{ - clusterSetLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSets().Lister(), - clusterSetBindingLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSetBindings().Lister(), - } + ctrl, _, _ := newTestSchedulingController(t, c.initObjs) bindings, err := ctrl.getValidManagedClusterSetBindings(placementNamespace) if err != nil { t.Errorf("unexpected err: %v", err) } - expectedBindingNames := sets.NewString(c.expectedClusterSetBindingNames...) - if len(bindings) != expectedBindingNames.Len() { - t.Errorf("expected %d bindings but got %d", expectedBindingNames.Len(), len(bindings)) - } - for _, binding := range bindings { - expectedBindingNames.Delete(binding.Name) - } - if expectedBindingNames.Len() > 0 { - t.Errorf("expected bindings: %s", strings.Join(expectedBindingNames.List(), ",")) - } + assertClusterSetBindingNames(t, bindings, c.expectedClusterSetBindingNames) }) } } @@ -688,25 +673,9 @@ func TestGetValidManagedClusterSets(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - clusterClient := clusterfake.NewSimpleClientset(c.initObjs...) - clusterInformerFactory := newClusterInformerFactory(t, clusterClient, c.initObjs...) - - ctrl := &schedulingController{ - clusterSetLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSets().Lister(), - clusterSetBindingLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSetBindings().Lister(), - } + ctrl, _, _ := newTestSchedulingController(t, c.initObjs) actualClusterSetNames := ctrl.getEligibleClusterSets(c.placement, c.bindings) - - expectedClusterSetNames := sets.NewString(c.expectedClusterSetNames...) - if len(actualClusterSetNames) != expectedClusterSetNames.Len() { - t.Errorf("expected %d bindings but got %d", expectedClusterSetNames.Len(), len(actualClusterSetNames)) - } - for _, name := range actualClusterSetNames { - expectedClusterSetNames.Delete(name) - } - if expectedClusterSetNames.Len() > 0 { - t.Errorf("expected names: %s", strings.Join(expectedClusterSetNames.List(), ",")) - } + assertClusterSetNames(t, actualClusterSetNames, c.expectedClusterSetNames) }) } } @@ -829,26 +798,9 @@ func TestGetAvailableClusters(t *testing.T) { for _, c := range cases { t.Run(c.name, func(t *testing.T) { - clusterClient := clusterfake.NewSimpleClientset(c.initObjs...) - clusterInformerFactory := newClusterInformerFactory(t, clusterClient, c.initObjs...) - - ctrl := &schedulingController{ - clusterLister: clusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), - clusterSetLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSets().Lister(), - } - + ctrl, _, _ := newTestSchedulingController(t, c.initObjs) clusters, _ := ctrl.getAvailableClusters(c.clusterSetNames) - - expectedClusterNames := sets.NewString(c.expectedClusterNames...) - if len(clusters) != expectedClusterNames.Len() { - t.Errorf("expected %d clusters but got %d", expectedClusterNames.Len(), len(clusters)) - } - for _, cluster := range clusters { - expectedClusterNames.Delete(cluster.Name) - } - if expectedClusterNames.Len() > 0 { - t.Errorf("expected clusters not selected: %s", strings.Join(expectedClusterNames.List(), ",")) - } + assertClusterNames(t, clusters, c.expectedClusterNames) }) } } @@ -1468,3 +1420,60 @@ func newSelectedClusters(num int) (clusters []string) { return clusters } + +// newTestSchedulingController creates a test scheduling controller with the given objects. +// This helper reduces boilerplate code in tests. +// Returns the controller, cluster client, and fake events recorder. +func newTestSchedulingController(t *testing.T, objs []runtime.Object) (*schedulingController, *clusterfake.Clientset, *kevents.FakeRecorder) { + clusterClient := clusterfake.NewSimpleClientset(objs...) + clusterInformerFactory := newClusterInformerFactory(t, clusterClient, objs...) + fakeRecorder := kevents.NewFakeRecorder(100) + + ctrl := &schedulingController{ + clusterClient: clusterClient, + clusterLister: clusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), + clusterSetLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSets().Lister(), + clusterSetBindingLister: clusterInformerFactory.Cluster().V1beta2().ManagedClusterSetBindings().Lister(), + placementLister: clusterInformerFactory.Cluster().V1beta1().Placements().Lister(), + placementDecisionLister: clusterInformerFactory.Cluster().V1beta1().PlacementDecisions().Lister(), + eventsRecorder: fakeRecorder, + metricsRecorder: metrics.NewScheduleMetrics(clock.RealClock{}), + } + + return ctrl, clusterClient, fakeRecorder +} + +// assertClusterSetBindingNames validates that the bindings match expected names. +func assertClusterSetBindingNames(t *testing.T, bindings []*clusterapiv1beta2.ManagedClusterSetBinding, expectedNames []string) { + expectedBindingNames := sets.NewString(expectedNames...) + if len(bindings) != expectedBindingNames.Len() { + t.Errorf("expected %d bindings but got %d", expectedBindingNames.Len(), len(bindings)) + } + for _, binding := range bindings { + expectedBindingNames.Delete(binding.Name) + } + if expectedBindingNames.Len() > 0 { + t.Errorf("expected bindings: %s", strings.Join(expectedBindingNames.List(), ",")) + } +} + +// assertClusterSetNames validates that the cluster set names match expected names. +func assertClusterSetNames(t *testing.T, clusterSetNames []string, expectedNames []string) { + actual := sets.NewString(clusterSetNames...) + expected := sets.NewString(expectedNames...) + if !actual.Equal(expected) { + t.Errorf("expected cluster sets %v, but got %v", expected.List(), actual.List()) + } +} + +// assertClusterNames validates that the clusters match expected cluster names. +func assertClusterNames(t *testing.T, clusters []*clusterapiv1.ManagedCluster, expectedNames []string) { + actual := sets.NewString() + for _, cluster := range clusters { + actual.Insert(cluster.Name) + } + expected := sets.NewString(expectedNames...) + if !actual.Equal(expected) { + t.Errorf("expected clusters %v, but got %v", expected.List(), actual.List()) + } +}