fix: Only include joined clusters when selecting by cluster names

apis/v1alpha1/clusterresourceplacement_types.go

@@ -42,7 +42,7 @@ type ClusterResourcePlacementSpec struct {
 	ResourceSelectors []ClusterResourceSelector `json:"resourceSelectors"`
 
 	// Policy represents the placement policy to select clusters to place all the selected resources.
-	// Default is place to the entire fleet if this field is omitted.
+	// If unspecified, all the joined clusters are selected.
 	// +optional
 	Policy *PlacementPolicy `json:"policy,omitempty"`
 }
@@ -76,10 +76,13 @@ type ClusterResourceSelector struct {
 	LabelSelector *metav1.LabelSelector `json:"labelSelector,omitempty"`
 }
 
-// PlacementPolicy represents the rule for select clusters.
+// PlacementPolicy contains the rules to select member clusters to place the selected resources to.
+// Note that only clusters that are both joined and satisfying the rules will be selected.
+// You should only specify at most one of the two fields: ClusterNames and Affinity.
+// If none is specified, all the joined clusters are selected.
 type PlacementPolicy struct {
-	// ClusterNames is a request to schedule the selected resource to a list of member clusters.
-	// If exists, we only place the resources within the clusters in this list.
+	// ClusterNames contains a list of names of MemberCluster to place the selected resources to.
+	// If the list is not empty, Affinity is ignored.
 	// kubebuilder:validation:MaxItems=100
 	// +optional
 	ClusterNames []string `json:"clusterNames,omitempty"`
@@ -122,11 +125,12 @@ type ClusterResourcePlacementStatus struct {
 	// +listMapKey=type
 	Conditions []metav1.Condition `json:"conditions"`
 
-	// SelectedResources is a list of the resources the resource selector selects.
+	// SelectedResources contains a list of resources selected by the resource selector.
 	// +optional
 	SelectedResources []ResourceIdentifier `json:"selectedResources,omitempty"`
 
-	// TargetClusters is a list of cluster names that this resource should run on.
+	// TargetClusters contains a list of member cluster names selected by PlacementPolicy.
+	// Note that the clusters must be both joined and meeting PlacementPolicy.
 	// +optional
 	TargetClusters []string `json:"targetClusters,omitempty"`
 

pkg/controllers/clusterresourceplacement/cluster_selector.go

@@ -43,20 +43,11 @@ func (r *Reconciler) selectClusters(placement *fleetv1alpha1.ClusterResourcePlac
 	if len(placement.Spec.Policy.ClusterNames) != 0 {
 		klog.V(4).InfoS("use the cluster names provided as the list of cluster we select",
 			"placement", placement.Name, "clusters", placement.Spec.Policy.ClusterNames)
-		// TODO: filter by cluster health
-		var selectedClusters []string
-		for _, clusterName := range placement.Spec.Policy.ClusterNames {
-			_, err = r.InformerManager.Lister(utils.MemberClusterGVR).Get(clusterName)
-			if err != nil {
-				klog.ErrorS(err, "cannot get the cluster", "clusterName", clusterName)
-				if !apierrors.IsNotFound(err) {
-					return nil, err
-				}
-			} else {
-				selectedClusters = append(selectedClusters, clusterName)
-			}
+		clusterNames, err = r.getClusters(placement.Spec.Policy.ClusterNames)
+		if err != nil {
+			return nil, err
 		}
-		return selectedClusters, nil
+		return
 	}
 
 	// no Affinity or ClusterAffinity set
@@ -101,18 +92,54 @@ func (r *Reconciler) listClusters(labelSelector labels.Selector) ([]string, erro
 
 	clusterNames := make([]string, 0)
 	for _, obj := range objs {
-		uObj := obj.DeepCopyObject().(*unstructured.Unstructured)
-		var clusterObj fleetv1alpha1.MemberCluster
-		err = runtime.DefaultUnstructuredConverter.FromUnstructured(uObj.Object, &clusterObj)
+		clusterObj, err := convertObjToMemberCluster(obj)
 		if err != nil {
-			return nil, errors.Wrap(err, "cannot decode the member cluster object")
+			return nil, err
 		}
 		// only schedule the resource to an eligible cluster
-		// TODO: check the health condition of the cluster when its aggregated
-		joinCond := clusterObj.GetCondition(fleetv1alpha1.ConditionTypeMemberClusterJoin)
-		if joinCond != nil && joinCond.Status == metav1.ConditionTrue && joinCond.ObservedGeneration == clusterObj.Generation {
+		if isClusterEligible(clusterObj) {
 			clusterNames = append(clusterNames, clusterObj.GetName())
 		}
 	}
 	return clusterNames, nil
 }
+
+// getClusters retrieves the given clusters from the informer cache, and selects the ones found and eligible.
+func (r *Reconciler) getClusters(clusterNames []string) ([]string, error) {
+	selectedClusters := make([]string, 0)
+	for _, clusterName := range clusterNames {
+		obj, err := r.InformerManager.Lister(utils.MemberClusterGVR).Get(clusterName)
+		if err != nil {
+			klog.ErrorS(err, "cannot get the cluster", "clusterName", clusterName)
+			if !apierrors.IsNotFound(err) {
+				return nil, err
+			}
+			continue
+		}
+		clusterObj, err := convertObjToMemberCluster(obj)
+		if err != nil {
+			return nil, err
+		}
+		// only schedule the resource to an eligible cluster
+		if isClusterEligible(clusterObj) {
+			selectedClusters = append(selectedClusters, clusterObj.GetName())
+		}
+	}
+	return selectedClusters, nil
+}
+
+func convertObjToMemberCluster(obj runtime.Object) (*fleetv1alpha1.MemberCluster, error) {
+	uObj := obj.DeepCopyObject().(*unstructured.Unstructured)
+	var clusterObj fleetv1alpha1.MemberCluster
+	if err := runtime.DefaultUnstructuredConverter.FromUnstructured(uObj.Object, &clusterObj); err != nil {
+		return nil, errors.Wrap(err, "cannot decode the member cluster object")
+	}
+	return &clusterObj, nil
+}
+
+// isClusterEligible checks whether a member cluster is eligible to be selected in CRP.
+func isClusterEligible(mc *fleetv1alpha1.MemberCluster) bool {
+	// TODO: check the health condition of the cluster when its aggregated
+	joinCond := mc.GetCondition(fleetv1alpha1.ConditionTypeMemberClusterJoin)
+	return joinCond != nil && joinCond.Status == metav1.ConditionTrue && joinCond.ObservedGeneration == mc.Generation
+}

test/integration/cluster_placement_test.go

@@ -745,7 +745,8 @@ var _ = Describe("Test Cluster Resource Placement Controller", func() {
 						},
 					},
 					Policy: &fleetv1alpha1.PlacementPolicy{
-						ClusterNames: []string{clusterA.Name},
+						// Although both clusters are listed, only clusterA is selected as clusterB hasn't joined yet.
+						ClusterNames: []string{clusterA.Name, clusterB.Name},
 					},
 				},
 			}
@@ -876,7 +877,7 @@ var _ = Describe("Test Cluster Resource Placement Controller", func() {
 			By("Verified that the work is removed from cluster A")
 		})
 
-		It("Test  member cluster join/leave trigger placement", func() {
+		It("Test member cluster join/leave trigger placement", func() {
 			crp = &fleetv1alpha1.ClusterResourcePlacement{
 				ObjectMeta: metav1.ObjectMeta{
 					Name: "test-list-resource",