Refactor getting placements and decisions

Previously, the `common` package had separate functions to get the decisions from a PlacementBinding depending on whether it was bound to a Placement or a PlacementRule. But every caller would need to use both, so that was unnecessary. Now, there is a unified `GetDecisions` func. Similarly, the `propagator` had separate functions to find the policy placements (not to be confused with Placements...), and the de- duplicated logic was brought to the `getPolicyPlacementDecisions` func more directly. Signed-off-by: Justin Kulikauskas <[email protected]> (cherry picked from commit 4178f22)
stolostron · Oct 11, 2023 · fb2837c · fb2837c
1 parent 9d459d4
commit fb2837c
Show file tree

Hide file tree

Showing 3 changed files with 159 additions and 322 deletions.
diff --git a/controllers/common/common.go b/controllers/common/common.go
@@ -10,7 +10,6 @@ import (
 	"fmt"
 	"strings"
 
-	"github.com/go-logr/logr"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	clusterv1 "open-cluster-management.io/api/cluster/v1"
@@ -150,80 +149,76 @@ func FindNonCompliantClustersForPolicy(plc *policiesv1.Policy) []string {
 	return clusterList
 }
 
-// GetClusterPlacementDecisions return the placement decisions from cluster
-// placement decisions
-func GetClusterPlacementDecisions(
-	c client.Client, pb policiesv1.PlacementBinding, instance *policiesv1.Policy, log logr.Logger,
-) ([]appsv1.PlacementDecision, error) {
-	log = log.WithValues("name", pb.PlacementRef.Name, "namespace", instance.GetNamespace())
-	pl := &clusterv1beta1.Placement{}
+func HasValidPlacementRef(pb *policiesv1.PlacementBinding) bool {
+	switch pb.PlacementRef.Kind {
+	case "PlacementRule":
+		return pb.PlacementRef.APIGroup == appsv1.SchemeGroupVersion.Group
+	case "Placement":
+		return pb.PlacementRef.APIGroup == clusterv1beta1.SchemeGroupVersion.Group
+	default:
+		return false
+	}
+}
 
-	err := c.Get(context.TODO(), types.NamespacedName{
-		Namespace: instance.GetNamespace(),
-		Name:      pb.PlacementRef.Name,
-	}, pl)
-	// no error when not found
-	if err != nil && !k8serrors.IsNotFound(err) {
-		log.Error(err, "Failed to get the Placement")
+// GetDecisions returns the placement decisions from the Placement or PlacementRule referred to by
+// the PlacementBinding
+func GetDecisions(c client.Client, pb *policiesv1.PlacementBinding) ([]appsv1.PlacementDecision, error) {
+	if !HasValidPlacementRef(pb) {
+		return nil, fmt.Errorf("placement binding %s/%s reference is not valid", pb.Name, pb.Namespace)
+	}
 
-		return nil, err
+	refNN := types.NamespacedName{
+		Namespace: pb.GetNamespace(),
+		Name:      pb.PlacementRef.Name,
 	}
 
-	list := &clusterv1beta1.PlacementDecisionList{}
-	lopts := &client.ListOptions{Namespace: instance.GetNamespace()}
+	switch pb.PlacementRef.Kind {
+	case "Placement":
+		pl := &clusterv1beta1.Placement{}
 
-	opts := client.MatchingLabels{"cluster.open-cluster-management.io/placement": pl.GetName()}
-	opts.ApplyToList(lopts)
-	err = c.List(context.TODO(), list, lopts)
+		err := c.Get(context.TODO(), refNN, pl)
+		if err != nil && !k8serrors.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to get Placement '%v': %w", pb.PlacementRef.Name, err)
+		}
 
-	// do not error out if not found
-	if err != nil && !k8serrors.IsNotFound(err) {
-		log.Error(err, "Failed to get the PlacementDecision")
+		if k8serrors.IsNotFound(err) {
+			return nil, nil
+		}
 
-		return nil, err
-	}
+		list := &clusterv1beta1.PlacementDecisionList{}
+		lopts := &client.ListOptions{Namespace: pb.GetNamespace()}
 
-	var decisions []appsv1.PlacementDecision
-	decisions = make([]appsv1.PlacementDecision, 0, len(list.Items))
+		opts := client.MatchingLabels{"cluster.open-cluster-management.io/placement": pl.GetName()}
+		opts.ApplyToList(lopts)
 
-	for _, item := range list.Items {
-		for _, cluster := range item.Status.Decisions {
-			decided := &appsv1.PlacementDecision{
-				ClusterName:      cluster.ClusterName,
-				ClusterNamespace: cluster.ClusterName,
-			}
-			decisions = append(decisions, *decided)
+		err = c.List(context.TODO(), list, lopts)
+		if err != nil && !k8serrors.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to list the PlacementDecisions for '%v', %w", pb.PlacementRef.Name, err)
 		}
-	}
 
-	return decisions, nil
-}
+		decisions := make([]appsv1.PlacementDecision, 0)
+
+		for _, item := range list.Items {
+			for _, cluster := range item.Status.Decisions {
+				decisions = append(decisions, appsv1.PlacementDecision{
+					ClusterName:      cluster.ClusterName,
+					ClusterNamespace: cluster.ClusterName,
+				})
+			}
+		}
 
-// GetApplicationPlacementDecisions return the placement decisions from an application
-// lifecycle placementrule
-func GetApplicationPlacementDecisions(
-	c client.Client, pb policiesv1.PlacementBinding, instance *policiesv1.Policy, log logr.Logger,
-) ([]appsv1.PlacementDecision, error) {
-	log = log.WithValues("name", pb.PlacementRef.Name, "namespace", instance.GetNamespace())
-	plr := &appsv1.PlacementRule{}
+		return decisions, nil
+	case "PlacementRule":
+		plr := &appsv1.PlacementRule{}
+		if err := c.Get(context.TODO(), refNN, plr); err != nil && !k8serrors.IsNotFound(err) {
+			return nil, fmt.Errorf("failed to get PlacementRule '%v': %w", pb.PlacementRef.Name, err)
+		}
 
-	err := c.Get(context.TODO(), types.NamespacedName{
-		Namespace: instance.GetNamespace(),
-		Name:      pb.PlacementRef.Name,
-	}, plr)
-	// no error when not found
-	if err != nil && !k8serrors.IsNotFound(err) {
-		log.Error(
-			err,
-			"Failed to get the PlacementRule",
-			"namespace", instance.GetNamespace(),
-			"name", pb.PlacementRef.Name,
-		)
-
-		return nil, err
+		// if the PlacementRule was not found, the decisions will be empty
+		return plr.Status.Decisions, nil
 	}
 
-	return plr.Status.Decisions, nil
+	return nil, fmt.Errorf("placement binding %s/%s reference is not valid", pb.Name, pb.Namespace)
 }
 
 // GetNumWorkers is a helper function to return the number of workers to handle concurrent tasks

diff --git a/controllers/policyset/policyset_controller.go b/controllers/policyset/policyset_controller.go
@@ -190,7 +190,7 @@ func (r *PolicySetReconciler) processPolicySet(ctx context.Context, plcSet *poli
 				}
 
 				var decisions []appsv1.PlacementDecision
-				decisions, err = getDecisions(r.Client, *pb, childPlc)
+				decisions, err = common.GetDecisions(r.Client, pb)
 				if err != nil {
 					log.Error(err, "Error getting placement decisions for binding "+pbName)
 				}
@@ -297,31 +297,6 @@ func showCompliance(compliancesFound []string, unknown []string, pending []strin
 	return false
 }
 
-// getDecisions gets the PlacementDecisions for a PlacementBinding
-func getDecisions(c client.Client, pb policyv1.PlacementBinding,
-	instance *policyv1.Policy,
-) ([]appsv1.PlacementDecision, error) {
-	if pb.PlacementRef.APIGroup == appsv1.SchemeGroupVersion.Group &&
-		pb.PlacementRef.Kind == "PlacementRule" {
-		d, err := common.GetApplicationPlacementDecisions(c, pb, instance, log)
-		if err != nil {
-			return nil, err
-		}
-
-		return d, nil
-	} else if pb.PlacementRef.APIGroup == clusterv1beta1.SchemeGroupVersion.Group &&
-		pb.PlacementRef.Kind == "Placement" {
-		d, err := common.GetClusterPlacementDecisions(c, pb, instance, log)
-		if err != nil {
-			return nil, err
-		}
-
-		return d, nil
-	}
-
-	return nil, fmt.Errorf("placement binding %s/%s reference is not valid", pb.Name, pb.Namespace)
-}
-
 // SetupWithManager sets up the controller with the Manager.
 func (r *PolicySetReconciler) SetupWithManager(mgr ctrl.Manager) error {
 	return ctrl.NewControllerManagedBy(mgr).