From 08b26501ccfa926bd2069aed4fbf90cb2bdf4059 Mon Sep 17 00:00:00 2001 From: Yang Le Date: Thu, 10 Jun 2021 17:26:36 +0800 Subject: [PATCH] add more condition reasons Signed-off-by: Yang Le --- pkg/controllers/scheduling/schedule.go | 10 +- pkg/controllers/scheduling/schedule_test.go | 32 ++- .../scheduling/scheduling_controller.go | 108 ++++++--- .../scheduling/scheduling_controller_test.go | 210 ++++++++++++++---- 4 files changed, 268 insertions(+), 92 deletions(-) diff --git a/pkg/controllers/scheduling/schedule.go b/pkg/controllers/scheduling/schedule.go index e48cf4ad2..58e9a68b4 100644 --- a/pkg/controllers/scheduling/schedule.go +++ b/pkg/controllers/scheduling/schedule.go @@ -25,8 +25,9 @@ type scheduleFunc func( ) (*scheduleResult, error) type scheduleResult struct { - scheduled int - unscheduled int + feasibleClusters int + scheduledDecisions int + unscheduledDecisions int } func schedule( @@ -57,8 +58,9 @@ func schedule( } return &scheduleResult{ - scheduled: scheduled, - unscheduled: unscheduled, + feasibleClusters: len(feasibleClusters), + scheduledDecisions: scheduled, + unscheduledDecisions: unscheduled, }, nil } diff --git a/pkg/controllers/scheduling/schedule_test.go b/pkg/controllers/scheduling/schedule_test.go index fa5b149d5..5291a4e04 100644 --- a/pkg/controllers/scheduling/schedule_test.go +++ b/pkg/controllers/scheduling/schedule_test.go @@ -40,7 +40,8 @@ func TestSchedule(t *testing.T) { testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, clusterSetName).Build(), }, scheduleResult: scheduleResult{ - scheduled: 1, + feasibleClusters: 1, + scheduledDecisions: 1, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been created @@ -64,8 +65,9 @@ func TestSchedule(t *testing.T) { testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, clusterSetName).Build(), }, scheduleResult: scheduleResult{ - scheduled: 1, - unscheduled: 2, + feasibleClusters: 1, + scheduledDecisions: 1, + unscheduledDecisions: 2, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been updated @@ -93,7 +95,8 @@ func TestSchedule(t *testing.T) { testinghelpers.NewManagedCluster("cluster2").WithLabel(clusterSetLabel, clusterSetName).Build(), }, scheduleResult: scheduleResult{ - scheduled: 2, + feasibleClusters: 2, + scheduledDecisions: 2, }, validateActions: testinghelpers.AssertNoActions, }, @@ -111,8 +114,9 @@ func TestSchedule(t *testing.T) { testinghelpers.NewManagedCluster("cluster2").WithLabel(clusterSetLabel, clusterSetName).Build(), }, scheduleResult: scheduleResult{ - scheduled: 2, - unscheduled: 2, + feasibleClusters: 2, + scheduledDecisions: 2, + unscheduledDecisions: 2, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been updated @@ -138,8 +142,9 @@ func TestSchedule(t *testing.T) { testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, clusterSetName).Build(), }, scheduleResult: scheduleResult{ - scheduled: 1, - unscheduled: 3, + feasibleClusters: 1, + scheduledDecisions: 1, + unscheduledDecisions: 3, }, validateActions: testinghelpers.AssertNoActions, }, @@ -160,11 +165,14 @@ func TestSchedule(t *testing.T) { if err != nil { t.Errorf("unexpected err: %v", err) } - if result.scheduled != c.scheduleResult.scheduled { - t.Errorf("expected %d scheduled, but got %d", c.scheduleResult.scheduled, result.scheduled) + if result.feasibleClusters != c.scheduleResult.feasibleClusters { + t.Errorf("expected %d feasible clusters, but got %d", c.scheduleResult.feasibleClusters, result.feasibleClusters) } - if result.unscheduled != c.scheduleResult.unscheduled { - t.Errorf("expected %d unscheduled, but got %d", c.scheduleResult.unscheduled, result.unscheduled) + if result.scheduledDecisions != c.scheduleResult.scheduledDecisions { + t.Errorf("expected %d scheduled, but got %d", c.scheduleResult.scheduledDecisions, result.scheduledDecisions) + } + if result.unscheduledDecisions != c.scheduleResult.unscheduledDecisions { + t.Errorf("expected %d unscheduled, but got %d", c.scheduleResult.unscheduledDecisions, result.unscheduledDecisions) } c.validateActions(t, clusterClient.Actions()) }) diff --git a/pkg/controllers/scheduling/scheduling_controller.go b/pkg/controllers/scheduling/scheduling_controller.go index 609121632..fb5d9a1c8 100644 --- a/pkg/controllers/scheduling/scheduling_controller.go +++ b/pkg/controllers/scheduling/scheduling_controller.go @@ -4,6 +4,7 @@ import ( "context" "fmt" "reflect" + "strings" "github.com/openshift/library-go/pkg/controller/factory" "github.com/openshift/library-go/pkg/operator/events" @@ -33,13 +34,6 @@ const ( schedulingControllerName = "SchedulingController" ) -var noBindingCondition = metav1.Condition{ - Type: clusterapiv1alpha1.PlacementConditionSatisfied, - Status: metav1.ConditionFalse, - Reason: "NoManagedClusterSetBindings", - Message: "No ManagedClusterSetBindings found in placement namespace", -} - type enqueuePlacementFunc func(namespace, name string) // schedulingController schedules cluster decisions for Placements @@ -169,13 +163,17 @@ func (c *schedulingController) sync(ctx context.Context, syncCtx factory.SyncCon return nil } - // get available clusters for this placement - bindings, err := c.getManagedClusterSetBindings(placement) + // get all valid clustersetbindings in the placement namespace + bindings, err := c.getValidManagedClusterSetBindings(placement.Namespace) if err != nil { return err } - clusters, err := c.getAvailableClusters(placement, bindings) + // get eligible clustersets for the placement + clusterSetNames := c.getEligibleClusterSets(placement, bindings) + + // get available clusters for the placement + clusters, err := c.getAvailableClusters(clusterSetNames) if err != nil { return err } @@ -187,31 +185,23 @@ func (c *schedulingController) sync(ctx context.Context, syncCtx factory.SyncCon } // update placement status if necessary to signal no bindings - return c.updateStatus(ctx, placement, len(bindings) > 0, scheduleResult.scheduled, scheduleResult.unscheduled) + return c.updateStatus(ctx, placement, clusterSetNames, len(bindings), len(clusters), scheduleResult) } // getManagedClusterSetBindings returns all bindings found in the placement namespace. -func (c *schedulingController) getManagedClusterSetBindings(placement *clusterapiv1alpha1.Placement) ([]*clusterapiv1alpha1.ManagedClusterSetBinding, error) { +func (c *schedulingController) getValidManagedClusterSetBindings(placementNamespace string) ([]*clusterapiv1alpha1.ManagedClusterSetBinding, error) { // get all clusterset bindings under the placement namespace - bindings, err := c.clusterSetBindingLister.ManagedClusterSetBindings(placement.Namespace).List(labels.Everything()) + bindings, err := c.clusterSetBindingLister.ManagedClusterSetBindings(placementNamespace).List(labels.Everything()) if err != nil { return nil, err } if len(bindings) == 0 { bindings = nil } - return bindings, nil -} -// getAvailableClusters returns available clusters for the given placement. The clusters must -// 1) Be from clustersets bound to the placement namespace; -// 2) Belong to one of particular clustersets if .spec.clusterSets is specified; -func (c *schedulingController) getAvailableClusters(placement *clusterapiv1alpha1.Placement, bindings []*clusterapiv1alpha1.ManagedClusterSetBinding) ([]*clusterapiv1.ManagedCluster, error) { - - // filter out invaid clustersetbindings - clusterSetNames := sets.NewString() + validBindings := []*clusterapiv1alpha1.ManagedClusterSetBinding{} for _, binding := range bindings { - // ignore clusterset does not exist + // ignore clustersetbinding refers to a non-existent clusterset _, err := c.clusterSetLister.Get(binding.Name) if errors.IsNotFound(err) { continue @@ -219,7 +209,17 @@ func (c *schedulingController) getAvailableClusters(placement *clusterapiv1alpha if err != nil { return nil, err } + validBindings = append(validBindings, binding) + } + return validBindings, nil +} + +// getEligibleClusterSets returns the names of clusterset that eligible for the placement +func (c *schedulingController) getEligibleClusterSets(placement *clusterapiv1alpha1.Placement, bindings []*clusterapiv1alpha1.ManagedClusterSetBinding) []string { + // filter out invaid clustersetbindings + clusterSetNames := sets.NewString() + for _, binding := range bindings { clusterSetNames.Insert(binding.Name) } @@ -229,12 +229,18 @@ func (c *schedulingController) getAvailableClusters(placement *clusterapiv1alpha clusterSetNames = clusterSetNames.Intersection(sets.NewString(placement.Spec.ClusterSets...)) } + return clusterSetNames.List() +} + +// getAvailableClusters returns available clusters for the given placement. The clusters must +// 1) Be from clustersets bound to the placement namespace; +// 2) Belong to one of particular clustersets if .spec.clusterSets is specified; +func (c *schedulingController) getAvailableClusters(clusterSetNames []string) ([]*clusterapiv1.ManagedCluster, error) { if len(clusterSetNames) == 0 { return nil, nil } - - // list clusters from particular clustersets - requirement, err := labels.NewRequirement(clusterSetLabel, selection.In, clusterSetNames.List()) + // list clusters from those clustersets + requirement, err := labels.NewRequirement(clusterSetLabel, selection.In, clusterSetNames) if err != nil { return nil, err } @@ -242,15 +248,26 @@ func (c *schedulingController) getAvailableClusters(placement *clusterapiv1alpha return c.clusterLister.List(labelSelector) } -// updateStatus updates the status of the placement according to schedule result. -func (c *schedulingController) updateStatus(ctx context.Context, placement *clusterapiv1alpha1.Placement, bindings bool, numOfScheduledDecisions, numOfUnscheduledDecisions int) error { +// updateStatus updates the status of the placement according to intermediate scheduling data. +func (c *schedulingController) updateStatus( + ctx context.Context, + placement *clusterapiv1alpha1.Placement, + eligibleClusterSetNames []string, + numOfBindings, + numOfAvailableClusters int, + scheduleResult *scheduleResult, +) error { newPlacement := placement.DeepCopy() - newPlacement.Status.NumberOfSelectedClusters = int32(numOfScheduledDecisions) - satisfiedCondition := newSatisfiedCondition(numOfUnscheduledDecisions) + newPlacement.Status.NumberOfSelectedClusters = int32(scheduleResult.scheduledDecisions) - if !bindings { - satisfiedCondition = noBindingCondition - } + satisfiedCondition := newSatisfiedCondition( + placement.Spec.ClusterSets, + eligibleClusterSetNames, + numOfBindings, + numOfAvailableClusters, + scheduleResult.feasibleClusters, + scheduleResult.unscheduledDecisions, + ) meta.SetStatusCondition(&newPlacement.Status.Conditions, satisfiedCondition) if reflect.DeepEqual(newPlacement.Status, placement.Status) { @@ -261,11 +278,34 @@ func (c *schedulingController) updateStatus(ctx context.Context, placement *clus } // newSatisfiedCondition returns a new condition with type PlacementConditionSatisfied -func newSatisfiedCondition(numOfUnscheduledDecisions int) metav1.Condition { +func newSatisfiedCondition( + clusterSetsInSpec []string, + eligibleClusterSets []string, + numOfBindings, + numOfAvailableClusters, + numOfFeasibleClusters, + numOfUnscheduledDecisions int, +) metav1.Condition { condition := metav1.Condition{ Type: clusterapiv1alpha1.PlacementConditionSatisfied, } switch { + case numOfBindings == 0: + condition.Status = metav1.ConditionFalse + condition.Reason = "NoManagedClusterSetBindings" + condition.Message = "No valid ManagedClusterSetBindings found in placement namespace" + case len(eligibleClusterSets) == 0: + condition.Status = metav1.ConditionFalse + condition.Reason = "NoIntersection" + condition.Message = fmt.Sprintf("None of ManagedClusterSets [%s] is bound to placement namespace", strings.Join(clusterSetsInSpec, ",")) + case numOfAvailableClusters == 0: + condition.Status = metav1.ConditionFalse + condition.Reason = "AllManagedClusterSetsEmpty" + condition.Message = fmt.Sprintf("All ManagedClusterSets [%s] have no member ManagedCluster", strings.Join(eligibleClusterSets, ",")) + case numOfFeasibleClusters == 0: + condition.Status = metav1.ConditionFalse + condition.Reason = "NoManagedClusterMatched" + condition.Message = "No ManagedCluster matches any of the cluster predicate" case numOfUnscheduledDecisions == 0: condition.Status = metav1.ConditionTrue condition.Reason = "AllDecisionsScheduled" diff --git a/pkg/controllers/scheduling/scheduling_controller_test.go b/pkg/controllers/scheduling/scheduling_controller_test.go index 95f49cb83..d80d98117 100644 --- a/pkg/controllers/scheduling/scheduling_controller_test.go +++ b/pkg/controllers/scheduling/scheduling_controller_test.go @@ -33,8 +33,9 @@ func TestSchedulingController_sync(t *testing.T) { name: "placement satisfied", placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(), scheduleResult: &scheduleResult{ - scheduled: 3, - unscheduled: 0, + feasibleClusters: 3, + scheduledDecisions: 3, + unscheduledDecisions: 0, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been updated @@ -66,8 +67,9 @@ func TestSchedulingController_sync(t *testing.T) { testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(), }, scheduleResult: &scheduleResult{ - scheduled: 3, - unscheduled: 1, + feasibleClusters: 3, + scheduledDecisions: 3, + unscheduledDecisions: 1, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been updated @@ -94,8 +96,9 @@ func TestSchedulingController_sync(t *testing.T) { name: "placement missing managedclustersetbindings", placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(), scheduleResult: &scheduleResult{ - scheduled: 0, - unscheduled: 0, + feasibleClusters: 0, + scheduledDecisions: 0, + unscheduledDecisions: 0, }, validateActions: func(t *testing.T, actions []clienttesting.Action) { // check if PlacementDecision has been updated @@ -128,8 +131,9 @@ func TestSchedulingController_sync(t *testing.T) { testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(), }, scheduleResult: &scheduleResult{ - scheduled: 3, - unscheduled: 0, + feasibleClusters: 3, + scheduledDecisions: 3, + unscheduledDecisions: 0, }, validateActions: testinghelpers.AssertNoActions, }, @@ -170,70 +174,100 @@ func TestSchedulingController_sync(t *testing.T) { } } -func TestGetAvailableClusters(t *testing.T) { +func TestGetValidManagedClusterSetBindings(t *testing.T) { placementNamespace := "ns1" - placementName := "placement1" - cases := []struct { - name string - placement *clusterapiv1alpha1.Placement - initObjs []runtime.Object - expectedClusterNames []string + name string + initObjs []runtime.Object + expectedClusterSetBindingNames []string }{ { - name: "no bound clusterset", - placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(), + name: "no bound clusterset", initObjs: []runtime.Object{ testinghelpers.NewClusterSet("clusterset1"), + }, + }, + { + name: "invalid binding", + initObjs: []runtime.Object{ testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"), }, }, { - name: "select all clusters from bound clustersets", - placement: testinghelpers.NewPlacement(placementNamespace, placementName).Build(), + name: "valid binding", initObjs: []runtime.Object{ testinghelpers.NewClusterSet("clusterset1"), - testinghelpers.NewClusterSet("clusterset2"), testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"), - testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset2"), - testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(), - testinghelpers.NewManagedCluster("cluster2").WithLabel(clusterSetLabel, "clusterset2").Build(), }, - expectedClusterNames: []string{"cluster1", "cluster2"}, - }, - { - name: "select clusters from a bound clusterset", - placement: testinghelpers.NewPlacement(placementNamespace, placementName). - WithClusterSets("clusterset1").Build(), - initObjs: []runtime.Object{ - testinghelpers.NewClusterSet("clusterset1"), - testinghelpers.NewClusterSet("clusterset2"), - testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"), - testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset2"), - testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(), - testinghelpers.NewManagedCluster("cluster2").WithLabel(clusterSetLabel, "clusterset2").Build(), - }, - expectedClusterNames: []string{"cluster1"}, + expectedClusterSetBindingNames: []string{"clusterset1"}, }, } for _, c := range cases { t.Run(c.name, func(t *testing.T) { - c.initObjs = append(c.initObjs, c.placement) clusterClient := clusterfake.NewSimpleClientset(c.initObjs...) clusterInformerFactory := testinghelpers.NewClusterInformerFactory(clusterClient, c.initObjs...) ctrl := &schedulingController{ - clusterLister: clusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), clusterSetLister: clusterInformerFactory.Cluster().V1alpha1().ManagedClusterSets().Lister(), clusterSetBindingLister: clusterInformerFactory.Cluster().V1alpha1().ManagedClusterSetBindings().Lister(), } - bindings, err := ctrl.getManagedClusterSetBindings(c.placement) + bindings, err := ctrl.getValidManagedClusterSetBindings(placementNamespace) if err != nil { t.Errorf("unexpected err: %v", err) } - clusters, err := ctrl.getAvailableClusters(c.placement, bindings) + 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(), ",")) + } + }) + } +} + +func TestGetAvailableClusters(t *testing.T) { + placementNamespace := "ns1" + + cases := []struct { + name string + clusterSetNames []string + initObjs []runtime.Object + expectedClusterNames []string + }{ + { + name: "no clusterset", + initObjs: []runtime.Object{ + testinghelpers.NewClusterSet("clusterset1"), + testinghelpers.NewClusterSetBinding(placementNamespace, "clusterset1"), + }, + }, + { + name: "select clusters from clustersets", + clusterSetNames: []string{"clusterset1", "clusterset2"}, + initObjs: []runtime.Object{ + testinghelpers.NewManagedCluster("cluster1").WithLabel(clusterSetLabel, "clusterset1").Build(), + testinghelpers.NewManagedCluster("cluster2").WithLabel(clusterSetLabel, "clusterset2").Build(), + }, + expectedClusterNames: []string{"cluster1", "cluster2"}, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + clusterClient := clusterfake.NewSimpleClientset(c.initObjs...) + clusterInformerFactory := testinghelpers.NewClusterInformerFactory(clusterClient, c.initObjs...) + + ctrl := &schedulingController{ + clusterLister: clusterInformerFactory.Cluster().V1().ManagedClusters().Lister(), + } + + clusters, err := ctrl.getAvailableClusters(c.clusterSetNames) if err != nil { t.Errorf("unexpected err: %v", err) } @@ -251,3 +285,95 @@ func TestGetAvailableClusters(t *testing.T) { }) } } + +func TestNewSatisfiedCondition(t *testing.T) { + cases := []struct { + name string + clusterSetsInSpec []string + eligibleClusterSets []string + numOfBindings int + numOfAvailableClusters int + numOfFeasibleClusters int + numOfUnscheduledDecisions int + expectedStatus metav1.ConditionStatus + expectedReason string + }{ + { + name: "NoManagedClusterSetBindings", + numOfBindings: 0, + numOfUnscheduledDecisions: 5, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NoManagedClusterSetBindings", + }, + { + name: "NoIntersection", + clusterSetsInSpec: []string{"clusterset1"}, + numOfBindings: 1, + numOfAvailableClusters: 0, + numOfFeasibleClusters: 0, + numOfUnscheduledDecisions: 0, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NoIntersection", + }, + { + name: "AllManagedClusterSetsEmpty", + eligibleClusterSets: []string{"clusterset1"}, + numOfBindings: 1, + numOfAvailableClusters: 0, + numOfFeasibleClusters: 0, + numOfUnscheduledDecisions: 0, + expectedStatus: metav1.ConditionFalse, + expectedReason: "AllManagedClusterSetsEmpty", + }, + { + name: "NoManagedClusterMatched", + eligibleClusterSets: []string{"clusterset1"}, + numOfBindings: 1, + numOfAvailableClusters: 1, + numOfFeasibleClusters: 0, + numOfUnscheduledDecisions: 0, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NoManagedClusterMatched", + }, + { + name: "AllDecisionsScheduled", + eligibleClusterSets: []string{"clusterset1"}, + numOfBindings: 1, + numOfAvailableClusters: 1, + numOfFeasibleClusters: 1, + numOfUnscheduledDecisions: 0, + expectedStatus: metav1.ConditionTrue, + expectedReason: "AllDecisionsScheduled", + }, + { + name: "NotAllDecisionsScheduled", + eligibleClusterSets: []string{"clusterset1"}, + numOfBindings: 1, + numOfAvailableClusters: 1, + numOfFeasibleClusters: 1, + numOfUnscheduledDecisions: 1, + expectedStatus: metav1.ConditionFalse, + expectedReason: "NotAllDecisionsScheduled", + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + condition := newSatisfiedCondition( + c.clusterSetsInSpec, + c.eligibleClusterSets, + c.numOfBindings, + c.numOfAvailableClusters, + c.numOfFeasibleClusters, + c.numOfUnscheduledDecisions, + ) + + if condition.Status != c.expectedStatus { + t.Errorf("expected status %q but got %q", c.expectedStatus, condition.Status) + } + if condition.Reason != c.expectedReason { + t.Errorf("expected reason %q but got %q", c.expectedReason, condition.Reason) + } + }) + } +}