improve event recording logic and test maintainability (#1376)

🤖 Generated with [Claude Code](https://claude.ai/code)

Signed-off-by: Qing Hao <qhao@redhat.com>
Co-authored-by: Claude Sonnet 4.5 <noreply@anthropic.com>

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) {

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
+}

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())
+	}
+}