From 75b8f2ee57284dd6bc31f6657d8b2ea60908d4c5 Mon Sep 17 00:00:00 2001 From: Qing Hao Date: Tue, 3 Jan 2023 09:03:53 +0800 Subject: [PATCH] create empty decision when no cluster selected (#95) * create empty decision when no cluster selected Signed-off-by: haoqing0110 * create placementdecision when misconfigured Signed-off-by: haoqing0110 Signed-off-by: haoqing0110 --- pkg/controllers/framework/interface.go | 2 +- .../scheduling/scheduling_controller.go | 22 +-- .../scheduling/scheduling_controller_test.go | 130 +++++++++++++++++- test/e2e/placement_test.go | 22 ++- test/integration/assertion_test.go | 6 +- test/integration/placement_test.go | 7 + 6 files changed, 155 insertions(+), 34 deletions(-) diff --git a/pkg/controllers/framework/interface.go b/pkg/controllers/framework/interface.go index dd45ad28a..48b25339d 100644 --- a/pkg/controllers/framework/interface.go +++ b/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 diff --git a/pkg/controllers/scheduling/scheduling_controller.go b/pkg/controllers/scheduling/scheduling_controller.go index f173ea087..37dd7818c 100644 --- a/pkg/controllers/scheduling/scheduling_controller.go +++ b/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{} diff --git a/pkg/controllers/scheduling/scheduling_controller_test.go b/pkg/controllers/scheduling/scheduling_controller_test.go index 18778bb11..a1b7475d4 100644 --- a/pkg/controllers/scheduling/scheduling_controller_test.go +++ b/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)) + } }, }, } diff --git a/test/e2e/placement_test.go b/test/e2e/placement_test.go index a568ebdd3..f3022a51f 100644 --- a/test/e2e/placement_test.go +++ b/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()) diff --git a/test/integration/assertion_test.go b/test/integration/assertion_test.go index 16f909951..2e055ae75 100644 --- a/test/integration/assertion_test.go +++ b/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, diff --git a/test/integration/placement_test.go b/test/integration/placement_test.go index a33127877..d7a83d647 100644 --- a/test/integration/placement_test.go +++ b/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{})