From b8b258a03f05a1af75b63930bd4f89048af7ebd9 Mon Sep 17 00:00:00 2001 From: Liqian Luo <13264318+circy9@users.noreply.github.com> Date: Thu, 1 Sep 2022 09:25:41 -0700 Subject: [PATCH 1/3] Only select joined clusters by cluster names --- .../cluster_selector.go | 69 +++++++++++++------ test/integration/cluster_placement_test.go | 5 +- 2 files changed, 52 insertions(+), 22 deletions(-) diff --git a/pkg/controllers/clusterresourceplacement/cluster_selector.go b/pkg/controllers/clusterresourceplacement/cluster_selector.go index be82fd6f6..edd714844 100644 --- a/pkg/controllers/clusterresourceplacement/cluster_selector.go +++ b/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,56 @@ 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 + } + // IsNotFound. + klog.ErrorS(err, "cluster not found", "clusterName", clusterName) + 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 +} diff --git a/test/integration/cluster_placement_test.go b/test/integration/cluster_placement_test.go index 65eed274f..15b9992d5 100644 --- a/test/integration/cluster_placement_test.go +++ b/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", From 68f9f3fac5d58c52fa7c8e9cc8316090655219ee Mon Sep 17 00:00:00 2001 From: Liqian Luo <13264318+circy9@users.noreply.github.com> Date: Thu, 1 Sep 2022 13:50:21 -0700 Subject: [PATCH 2/3] address comments --- apis/v1alpha1/clusterresourceplacement_types.go | 8 +++++--- .../clusterresourceplacement/cluster_selector.go | 2 -- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/apis/v1alpha1/clusterresourceplacement_types.go b/apis/v1alpha1/clusterresourceplacement_types.go index cad0f8ba4..e9e6f38f2 100644 --- a/apis/v1alpha1/clusterresourceplacement_types.go +++ b/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"` } @@ -77,9 +77,11 @@ type ClusterResourceSelector struct { } // PlacementPolicy represents the rule for select clusters. +// 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"` diff --git a/pkg/controllers/clusterresourceplacement/cluster_selector.go b/pkg/controllers/clusterresourceplacement/cluster_selector.go index edd714844..853d1896e 100644 --- a/pkg/controllers/clusterresourceplacement/cluster_selector.go +++ b/pkg/controllers/clusterresourceplacement/cluster_selector.go @@ -114,8 +114,6 @@ func (r *Reconciler) getClusters(clusterNames []string) ([]string, error) { if !apierrors.IsNotFound(err) { return nil, err } - // IsNotFound. - klog.ErrorS(err, "cluster not found", "clusterName", clusterName) continue } clusterObj, err := convertObjToMemberCluster(obj) From 0a9e0b975b87fac36c60ee90ab1b48dfb16fde7e Mon Sep 17 00:00:00 2001 From: Liqian Luo <13264318+circy9@users.noreply.github.com> Date: Thu, 1 Sep 2022 14:02:29 -0700 Subject: [PATCH 3/3] address comments --- apis/v1alpha1/clusterresourceplacement_types.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/apis/v1alpha1/clusterresourceplacement_types.go b/apis/v1alpha1/clusterresourceplacement_types.go index e9e6f38f2..1ea7acd74 100644 --- a/apis/v1alpha1/clusterresourceplacement_types.go +++ b/apis/v1alpha1/clusterresourceplacement_types.go @@ -76,7 +76,8 @@ 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 { @@ -124,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"`