create empty decision when no cluster selected (#95)

* create empty decision when no cluster selected

Signed-off-by: haoqing0110 <qhao@redhat.com>

* create placementdecision when misconfigured

Signed-off-by: haoqing0110 <qhao@redhat.com>

Signed-off-by: haoqing0110 <qhao@redhat.com>

pkg/controllers/framework/interface.go

@@ -93,7 +93,7 @@ func NewStatus(plugin string, code Code, reasons ...string) *Status {
 		reasons: reasons,
 		plugin:  plugin,
 	}
-	if code == Error {
+	if s.IsError() {
 		s.err = errors.New(s.Message())
 	}
 	return s

pkg/controllers/scheduling/scheduling_controller.go

@@ -238,14 +238,6 @@ func (c *schedulingController) syncPlacement(ctx context.Context, syncCtx factor
 		status,
 	)
 
-	// update status and return error if schedule() returns error
-	if status.IsError() {
-		if err := c.updateStatus(ctx, placement, int32(len(scheduleResult.Decisions())), misconfiguredCondition, satisfiedCondition); err != nil {
-			return err
-		}
-		return status.AsError()
-	}
-
 	// requeue placement if requeueAfter is defined in scheduleResult
 	if syncCtx != nil && scheduleResult.RequeueAfter() != nil {
 		key, _ := cache.MetaNamespaceKeyFunc(placement)
@@ -254,13 +246,16 @@ func (c *schedulingController) syncPlacement(ctx context.Context, syncCtx factor
 		syncCtx.Queue().AddAfter(key, *t)
 	}
 
-	err = c.bind(ctx, placement, scheduleResult.Decisions(), scheduleResult.PrioritizerScores(), status)
-	if err != nil {
+	if err := c.bind(ctx, placement, scheduleResult.Decisions(), scheduleResult.PrioritizerScores(), status); err != nil {
 		return err
 	}
 
 	// update placement status if necessary to signal no bindings
-	return c.updateStatus(ctx, placement, int32(len(scheduleResult.Decisions())), misconfiguredCondition, satisfiedCondition)
+	if err := c.updateStatus(ctx, placement, int32(len(scheduleResult.Decisions())), misconfiguredCondition, satisfiedCondition); err != nil {
+		return err
+	}
+
+	return status.AsError()
 }
 
 // getManagedClusterSetBindings returns all bindings found in the placement namespace.
@@ -464,6 +459,11 @@ func (c *schedulingController) bind(
 		}
 		decisionSlices = append(decisionSlices, decisionSlice)
 	}
+	// if decisionSlices is empty, append one empty slice.
+	// so that can create a PlacementDecision with empty decisions in status.
+	if len(decisionSlices) == 0 {
+		decisionSlices = append(decisionSlices, []clusterapiv1beta1.ClusterDecision{})
+	}
 
 	// bind cluster decision slices to placementdecisions.
 	errs := []error{}

pkg/controllers/scheduling/scheduling_controller_test.go

@@ -133,9 +133,19 @@ func TestSchedulingController_sync(t *testing.T) {
 			},
 			validateActions: func(t *testing.T, actions []clienttesting.Action) {
 				// check if PlacementDecision has been updated
-				testinghelpers.AssertActions(t, actions, "update")
+				testinghelpers.AssertActions(t, actions, "create", "update")
+				// check if emtpy PlacementDecision has been created
+				actual := actions[0].(clienttesting.CreateActionImpl).Object
+				placementDecision, ok := actual.(*clusterapiv1beta1.PlacementDecision)
+				if !ok {
+					t.Errorf("expected PlacementDecision was created")
+				}
+
+				if len(placementDecision.Status.Decisions) != 0 {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, len(placementDecision.Status.Decisions))
+				}
 				// check if Placement has been updated
-				actual := actions[0].(clienttesting.UpdateActionImpl).Object
+				actual = actions[1].(clienttesting.UpdateActionImpl).Object
 				placement, ok := actual.(*clusterapiv1beta1.Placement)
 				if !ok {
 					t.Errorf("expected Placement was updated")
@@ -152,6 +162,95 @@ func TestSchedulingController_sync(t *testing.T) {
 				)
 			},
 		},
+		{
+			name:      "placement all managedclustersets empty ",
+			placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(),
+			initObjs: []runtime.Object{
+				testinghelpers.NewClusterSet("clusterset1").Build(),
+				testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"),
+			},
+			scheduleResult: &scheduleResult{
+				feasibleClusters:     []*clusterapiv1.ManagedCluster{},
+				scheduledDecisions:   []clusterapiv1beta1.ClusterDecision{},
+				unscheduledDecisions: 0,
+			},
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				// check if PlacementDecision has been updated
+				testinghelpers.AssertActions(t, actions, "create", "update")
+				// check if emtpy PlacementDecision has been created
+				actual := actions[0].(clienttesting.CreateActionImpl).Object
+				placementDecision, ok := actual.(*clusterapiv1beta1.PlacementDecision)
+				if !ok {
+					t.Errorf("expected PlacementDecision was created")
+				}
+
+				if len(placementDecision.Status.Decisions) != 0 {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, len(placementDecision.Status.Decisions))
+				}
+				// check if Placement has been updated
+				actual = actions[1].(clienttesting.UpdateActionImpl).Object
+				placement, ok := actual.(*clusterapiv1beta1.Placement)
+				if !ok {
+					t.Errorf("expected Placement was updated")
+				}
+
+				if placement.Status.NumberOfSelectedClusters != int32(0) {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, placement.Status.NumberOfSelectedClusters)
+				}
+				testinghelpers.HasCondition(
+					placement.Status.Conditions,
+					clusterapiv1beta1.PlacementConditionSatisfied,
+					"AllManagedClusterSetsEmpty",
+					metav1.ConditionFalse,
+				)
+			},
+		},
+		{
+			name:      "placement no cluster matches",
+			placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(),
+			initObjs: []runtime.Object{
+				testinghelpers.NewClusterSet("clusterset1").Build(),
+				testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"),
+				testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(),
+			},
+			scheduleResult: &scheduleResult{
+				feasibleClusters: []*clusterapiv1.ManagedCluster{
+					testinghelpers.NewManagedCluster("cluster1").Build(),
+				},
+				scheduledDecisions:   []clusterapiv1beta1.ClusterDecision{},
+				unscheduledDecisions: 0,
+			},
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				// check if PlacementDecision has been updated
+				testinghelpers.AssertActions(t, actions, "create", "update")
+				// check if emtpy PlacementDecision has been created
+				actual := actions[0].(clienttesting.CreateActionImpl).Object
+				placementDecision, ok := actual.(*clusterapiv1beta1.PlacementDecision)
+				if !ok {
+					t.Errorf("expected PlacementDecision was created")
+				}
+
+				if len(placementDecision.Status.Decisions) != 0 {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, len(placementDecision.Status.Decisions))
+				}
+				// check if Placement has been updated
+				actual = actions[1].(clienttesting.UpdateActionImpl).Object
+				placement, ok := actual.(*clusterapiv1beta1.Placement)
+				if !ok {
+					t.Errorf("expected Placement was updated")
+				}
+
+				if placement.Status.NumberOfSelectedClusters != int32(0) {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, placement.Status.NumberOfSelectedClusters)
+				}
+				testinghelpers.HasCondition(
+					placement.Status.Conditions,
+					clusterapiv1beta1.PlacementConditionSatisfied,
+					"NoManagedClusterMatched",
+					metav1.ConditionFalse,
+				)
+			},
+		},
 		{
 			name: "placement status not changed",
 			placement: testinghelpers.NewPlacement(placementNamespace, placementName).
@@ -668,6 +767,21 @@ func TestBind(t *testing.T) {
 				assertClustersSelected(t, placementDecision.Status.Decisions, selectedClusters[100:]...)
 			},
 		},
+		{
+			name:             "create empty placementdecision",
+			clusterDecisions: newClusterDecisions(0),
+			validateActions: func(t *testing.T, actions []clienttesting.Action) {
+				testinghelpers.AssertActions(t, actions, "create")
+				actual := actions[0].(clienttesting.CreateActionImpl).Object
+				placementDecision, ok := actual.(*clusterapiv1beta1.PlacementDecision)
+				if !ok {
+					t.Errorf("expected PlacementDecision was created")
+				}
+				if len(placementDecision.Status.Decisions) != 0 {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, len(placementDecision.Status.Decisions))
+				}
+			},
+		},
 		{
 			name:             "no change",
 			clusterDecisions: newClusterDecisions(128),
@@ -722,7 +836,7 @@ func TestBind(t *testing.T) {
 			},
 		},
 		{
-			name:             "delete all placementdecisions",
+			name:             "delete all placementdecisions and leave one empty placementdecision",
 			clusterDecisions: newClusterDecisions(0),
 			initObjs: []runtime.Object{
 				testinghelpers.NewPlacementDecision(placementNamespace, placementDecisionName(placementName, 1)).
@@ -733,7 +847,15 @@ func TestBind(t *testing.T) {
 					WithDecisions(newSelectedClusters(128)[100:]...).Build(),
 			},
 			validateActions: func(t *testing.T, actions []clienttesting.Action) {
-				testinghelpers.AssertActions(t, actions, "delete", "delete")
+				testinghelpers.AssertActions(t, actions, "update", "delete")
+				actual := actions[0].(clienttesting.UpdateActionImpl).Object
+				placementDecision, ok := actual.(*clusterapiv1beta1.PlacementDecision)
+				if !ok {
+					t.Errorf("expected PlacementDecision was updated")
+				}
+				if len(placementDecision.Status.Decisions) != 0 {
+					t.Errorf("expecte %d cluster selected, but got %d", 0, len(placementDecision.Status.Decisions))
+				}
 			},
 		},
 	}

test/e2e/placement_test.go

@@ -18,8 +18,9 @@ import (
 )
 
 const (
-	clusterSetLabel = "cluster.open-cluster-management.io/clusterset"
-	placementLabel  = "cluster.open-cluster-management.io/placement"
+	clusterSetLabel          = "cluster.open-cluster-management.io/clusterset"
+	placementLabel           = "cluster.open-cluster-management.io/placement"
+	maxNumOfClusterDecisions = 100
 )
 
 var _ = ginkgo.Describe("Placement", func() {
@@ -84,6 +85,7 @@ var _ = ginkgo.Describe("Placement", func() {
 
 	assertNumberOfDecisions := func(placementName string, desiredNOD int) {
 		ginkgo.By("Check the number of decisions in placementdecisions")
+		desiredNOPD := desiredNOD/maxNumOfClusterDecisions + 1
 		gomega.Eventually(func() bool {
 			pdl, err := clusterClient.ClusterV1beta1().PlacementDecisions(namespace).List(context.Background(), metav1.ListOptions{
 				LabelSelector: placementLabel + "=" + placementName,
@@ -91,6 +93,9 @@ var _ = ginkgo.Describe("Placement", func() {
 			if err != nil {
 				return false
 			}
+			if len(pdl.Items) != desiredNOPD {
+				return false
+			}
 			actualNOD := 0
 			for _, pd := range pdl.Items {
 				actualNOD += len(pd.Status.Decisions)
@@ -279,21 +284,10 @@ var _ = ginkgo.Describe("Placement", func() {
 			return err
 		}, eventuallyTimeout, eventuallyInterval).ShouldNot(gomega.HaveOccurred())
 
+		ginkgo.By("Create empty placement decision")
 		assertNumberOfDecisions(placementName, 0)
 		assertPlacementStatus(placementName, 0, false)
 
-		ginkgo.By("Check if placementdecisions are deleted as well")
-		gomega.Eventually(func() bool {
-			placementDecisions, err := clusterClient.ClusterV1beta1().PlacementDecisions(namespace).List(context.TODO(), metav1.ListOptions{
-				LabelSelector: fmt.Sprintf("%s=%s", placementLabel, placementName),
-			})
-			if err != nil {
-				return false
-			}
-
-			return len(placementDecisions.Items) == 0
-		}, eventuallyTimeout*5, eventuallyInterval*5).Should(gomega.BeTrue())
-
 		ginkgo.By("Delete placement")
 		err = clusterClient.ClusterV1beta1().Placements(namespace).Delete(context.TODO(), placementName, metav1.DeleteOptions{})
 		gomega.Expect(err).ToNot(gomega.HaveOccurred())

test/integration/assertion_test.go

@@ -79,10 +79,8 @@ func assertPlacementDeleted(placementName, namespace string) {
 
 func assertNumberOfDecisions(placementName, namespace string, desiredNOD int) {
 	ginkgo.By("Check the number of decisions in placementdecisions")
-	desiredNOPD := desiredNOD / maxNumOfClusterDecisions
-	if desiredNOD%maxNumOfClusterDecisions != 0 {
-		desiredNOPD++
-	}
+	// at least one decision for each placement
+	desiredNOPD := desiredNOD/maxNumOfClusterDecisions + 1
 	gomega.Eventually(func() bool {
 		pdl, err := clusterClient.ClusterV1beta1().PlacementDecisions(namespace).List(context.Background(), metav1.ListOptions{
 			LabelSelector: placementLabel + "=" + placementName,

test/integration/placement_test.go

@@ -101,6 +101,12 @@ var _ = ginkgo.Describe("Placement", func() {
 			assertNumberOfDecisions(placementName, namespace, 5)
 		})
 
+		ginkgo.It("Should create empty placementdecision when no cluster selected", func() {
+			placement := assertCreatingPlacement(placementName, namespace, nil, clusterapiv1beta1.PrioritizerPolicy{}, []clusterapiv1beta1.Toleration{})
+			assertPlacementDecisionCreated(placement)
+			assertNumberOfDecisions(placementName, namespace, 0)
+		})
+
 		ginkgo.It("Should create multiple placementdecisions once scheduled", func() {
 			assertBindingClusterSet(clusterSet1Name, namespace)
 			assertCreatingClusters(clusterSet1Name, 101)
@@ -273,6 +279,7 @@ var _ = ginkgo.Describe("Placement", func() {
 					Value:    "value1",
 				},
 			})
+			assertNumberOfDecisions(placementName, namespace, 0)
 			assertPlacementConditionMisconfigured(placementName, namespace, true)
 
 			assertUpdatingPlacement(placementName, namespace, noc(1), clusterapiv1beta1.PrioritizerPolicy{}, []clusterapiv1beta1.Toleration{})