Refactor haveOverlappingMetroZones to return a list of cluster lists

As a part of deprecating spec.Region from DRCluster, the way to process
overlapping metro DRPolicies was amended to return different maps
from the corresponsing region based or peerClass based policies.

This is now refactored to return only a list of list of clusters as
required to detect overlapping metro zones.

Signed-off-by: Shyamsundar Ranganathan <[email protected]>

Author: ShyamsundarR

internal/controller/drplacementcontrol.go

@@ -1890,64 +1890,85 @@ func (d *DRPCInstance) newVRGSpecSync() *rmn.VRGSyncSpec {
 	}
 }
 
-func dRPolicySupportsMetro(drpolicy *rmn.DRPolicy, drclusters []rmn.DRCluster) (bool, map[string][]string) {
-	syncPeerClasses := drpolicy.Status.Sync.PeerClasses
-	aSyncPeerClasses := drpolicy.Status.Async.PeerClasses
-
-	if len(syncPeerClasses) == 0 && len(aSyncPeerClasses) == 0 {
+// dRPolicySupportsMetro returns a boolean indicating that the policy supports a Metro(Sync) DR capability, and a
+// list of list of strings, where each list of strings contains the name of the clusters that are in a Sync
+// relationship
+func dRPolicySupportsMetro(drpolicy *rmn.DRPolicy, drclusters []rmn.DRCluster, drClusterIDsToNames map[string]string) (
+	supportsMetro bool,
+	metroClustersInPolicy [][]string,
+) {
+	if len(drpolicy.Status.Sync.PeerClasses) == 0 && len(drpolicy.Status.Async.PeerClasses) == 0 {
 		return checkMetroSupportUsingRegion(drpolicy, drclusters)
 	}
 
-	if len(syncPeerClasses) != 0 {
-		return checkMetroSupportUsingPeerClass(drpolicy)
+	if len(drpolicy.Status.Sync.PeerClasses) != 0 {
+		return checkMetroSupportUsingPeerClass(drpolicy, drClusterIDsToNames)
 	}
 
 	return false, nil
 }
 
-func checkMetroSupportUsingPeerClass(drPolicy *rmn.DRPolicy) (bool, map[string][]string) {
-	peerClasses := drPolicy.Status.Sync.PeerClasses
-	if len(peerClasses) == 0 {
+func checkMetroSupportUsingPeerClass(drPolicy *rmn.DRPolicy, drClusterIDsToNames map[string]string) (bool, [][]string) {
+	if len(drPolicy.Status.Sync.PeerClasses) == 0 {
 		return false, nil
 	}
 
-	metroMap := make(map[string][]string)
-	for _, peerClass := range peerClasses {
-		metroMap[peerClass.StorageClassName] = peerClass.ClusterIDs
+	if drClusterIDsToNames == nil {
+		return true, nil
 	}
 
-	return true, metroMap
+	metroClustersInPolicy := [][]string{}
+
+	for idx := range drPolicy.Status.Sync.PeerClasses {
+		if len(drPolicy.Status.Sync.PeerClasses[idx].StorageID) != 1 {
+			continue
+		}
+
+		if len(drPolicy.Status.Sync.PeerClasses[idx].ClusterIDs) == 0 {
+			return false, nil
+		}
+
+		peerClusterNames := []string{}
+		for _, cID := range drPolicy.Status.Sync.PeerClasses[idx].ClusterIDs {
+			peerClusterNames = append(peerClusterNames, drClusterIDsToNames[cID])
+		}
+
+		if len(peerClusterNames) != len(drPolicy.Status.Sync.PeerClasses[idx].ClusterIDs) {
+			return false, nil
+		}
+
+		metroClustersInPolicy = append(metroClustersInPolicy, peerClusterNames)
+	}
+
+	return true, metroClustersInPolicy
 }
 
 func checkMetroSupportUsingRegion(drpolicy *rmn.DRPolicy, drclusters []rmn.DRCluster) (
 	supportsMetro bool,
-	metroMap map[string][]string,
+	metroClustersInPolicy [][]string,
 ) {
 	allRegionsMap := make(map[rmn.Region][]string)
-	metroMap = make(map[string][]string)
+	metroClustersInPolicy = [][]string{}
 
 	for _, managedCluster := range rmnutil.DRPolicyClusterNames(drpolicy) {
 		for _, v := range drclusters {
 			if v.Name == managedCluster {
-				if v.Spec.Region == "" {
-					return supportsMetro, metroMap
-				}
-
 				allRegionsMap[v.Spec.Region] = append(
 					allRegionsMap[v.Spec.Region],
 					managedCluster)
 			}
 		}
 	}
 
-	for k, v := range allRegionsMap {
+	for _, v := range allRegionsMap {
 		if len(v) > 1 {
 			supportsMetro = true
-			metroMap[string(k)] = v
+
+			metroClustersInPolicy = append(metroClustersInPolicy, v)
 		}
 	}
 
-	return supportsMetro, metroMap
+	return supportsMetro, metroClustersInPolicy
 }
 
 func (d *DRPCInstance) ensureNamespaceManifestWork(homeCluster string) error {

internal/controller/drplacementcontrol_controller.go

@@ -406,7 +406,7 @@ func (r *DRPlacementControlReconciler) createDRPCInstance(
 
 	d.drType = DRTypeAsync
 
-	isMetro, _ := dRPolicySupportsMetro(drPolicy, drClusters)
+	isMetro, _ := dRPolicySupportsMetro(drPolicy, drClusters, nil)
 	if isMetro {
 		d.volSyncDisabled = true
 		d.drType = DRTypeSync
@@ -1466,7 +1466,7 @@ func (r *DRPlacementControlReconciler) setDRPCMetrics(ctx context.Context,
 	}
 
 	// do not set sync metrics if metro-dr
-	isMetro, _ := dRPolicySupportsMetro(drPolicy, drClusters)
+	isMetro, _ := dRPolicySupportsMetro(drPolicy, drClusters, nil)
 	if isMetro {
 		return nil
 	}

internal/controller/drplacementcontrol_watcher.go

@@ -478,7 +478,7 @@ func DRPCsFailingOverToCluster(k8sclient client.Client, log logr.Logger, drclust
 			}
 
 			// Skip if policy is of type metro, fake the from and to cluster
-			if metro, _ := dRPolicySupportsMetro(drpolicy, drClusters); metro {
+			if metro, _ := dRPolicySupportsMetro(drpolicy, drClusters, nil); metro {
 				log.Info("Sync DRPolicy detected, skipping!")
 
 				break

internal/controller/drpolicy_controller.go

@@ -104,10 +104,9 @@ func (r *DRPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 			u.validatedSetFalse("NamespaceCreateFailed", err))
 	}
 
-	drclusters := &ramen.DRClusterList{}
-
-	if err := r.Client.List(ctx, drclusters); err != nil {
-		return ctrl.Result{}, fmt.Errorf("drclusters list: %w", u.validatedSetFalse("drClusterListFailed", err))
+	drclusters, drClusterIDsToNames, err := r.getDRClusterDetails(ctx)
+	if err != nil {
+		return ctrl.Result{}, fmt.Errorf("drclusters details: %w", u.validatedSetFalse("drClusterDetailsFailed", err))
 	}
 
 	secretsUtil := &util.SecretsUtil{Client: r.Client, APIReader: r.APIReader, Ctx: ctx, Log: log}
@@ -119,7 +118,7 @@ func (r *DRPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Request) (c
 
 	log.Info("create/update")
 
-	reason, err := validateDRPolicy(ctx, drpolicy, drclusters, r.APIReader)
+	reason, err := validateDRPolicy(ctx, drpolicy, drclusters, r.APIReader, drClusterIDsToNames)
 	if err != nil {
 		statusErr := u.validatedSetFalse(reason, err)
 		if !errors.Is(statusErr, err) || reason != ReasonDRClusterNotFound {
@@ -166,7 +165,7 @@ func (r *DRPolicyReconciler) reconcile(
 }
 
 func (r *DRPolicyReconciler) initiateDRPolicyMetrics(drpolicy *ramen.DRPolicy, drclusters *ramen.DRClusterList) error {
-	isMetro, _ := dRPolicySupportsMetro(drpolicy, drclusters.Items)
+	isMetro, _ := dRPolicySupportsMetro(drpolicy, drclusters.Items, nil)
 
 	// Do not set metric for metro-dr
 	if !isMetro {
@@ -178,10 +177,36 @@ func (r *DRPolicyReconciler) initiateDRPolicyMetrics(drpolicy *ramen.DRPolicy, d
 	return nil
 }
 
+func (r *DRPolicyReconciler) getDRClusterDetails(ctx context.Context) (*ramen.DRClusterList, map[string]string, error) {
+	drClusters := &ramen.DRClusterList{}
+	if err := r.Client.List(ctx, drClusters); err != nil {
+		return nil, nil, fmt.Errorf("drclusters list: %w", err)
+	}
+
+	drClusterIDsToNames := map[string]string{}
+
+	for idx := range drClusters.Items {
+		mc, err := util.NewManagedClusterInstance(ctx, r.Client, drClusters.Items[idx].GetName())
+		if err != nil {
+			return nil, nil, fmt.Errorf("drclusters ManagedCluster (%s): %w", drClusters.Items[idx].GetName(), err)
+		}
+
+		clID, err := mc.ClusterID()
+		if err != nil {
+			return nil, nil, fmt.Errorf("drclusters cluster ID (%s): %w", drClusters.Items[idx].GetName(), err)
+		}
+
+		drClusterIDsToNames[clID] = drClusters.Items[idx].GetName()
+	}
+
+	return drClusters, drClusterIDsToNames, nil
+}
+
 func validateDRPolicy(ctx context.Context,
 	drpolicy *ramen.DRPolicy,
 	drclusters *ramen.DRClusterList,
 	apiReader client.Reader,
+	drClusterIDsToNames map[string]string,
 ) (string, error) {
 	// DRPolicy does not support both Sync and Async configurations in one single DRPolicy
 	if len(drpolicy.Status.Sync.PeerClasses) > 0 && len(drpolicy.Status.Async.PeerClasses) > 0 {
@@ -200,7 +225,7 @@ func validateDRPolicy(ctx context.Context,
 		return reason, err
 	}
 
-	err = validatePolicyConflicts(ctx, apiReader, drpolicy, drclusters)
+	err = validatePolicyConflicts(ctx, apiReader, drpolicy, drclusters, drClusterIDsToNames)
 	if err != nil {
 		return ReasonValidationFailed, err
 	}
@@ -258,13 +283,14 @@ func validatePolicyConflicts(ctx context.Context,
 	apiReader client.Reader,
 	drpolicy *ramen.DRPolicy,
 	drclusters *ramen.DRClusterList,
+	drClusterIDsToNames map[string]string,
 ) error {
 	drpolicies, err := util.GetAllDRPolicies(ctx, apiReader)
 	if err != nil {
 		return fmt.Errorf("validate managed cluster in drpolicy %v failed: %w", drpolicy.Name, err)
 	}
 
-	err = hasConflictingDRPolicy(drpolicy, drclusters, drpolicies)
+	err = hasConflictingDRPolicy(drpolicy, drclusters, drpolicies, drClusterIDsToNames)
 	if err != nil {
 		return fmt.Errorf("validate managed cluster in drpolicy failed: %w", err)
 	}
@@ -274,7 +300,12 @@ func validatePolicyConflicts(ctx context.Context,
 
 // If two drpolicies have common managed cluster(s) and at least one of them is
 // a metro supported drpolicy, then fail.
-func hasConflictingDRPolicy(match *ramen.DRPolicy, drclusters *ramen.DRClusterList, list ramen.DRPolicyList) error {
+func hasConflictingDRPolicy(
+	match *ramen.DRPolicy,
+	drclusters *ramen.DRClusterList,
+	list ramen.DRPolicyList,
+	drClusterIDsToNames map[string]string,
+) error {
 	// Valid cases
 	// [e1,w1] [e1,c1]
 	// [e1,w1] [e1,w1]
@@ -294,80 +325,47 @@ func hasConflictingDRPolicy(match *ramen.DRPolicy, drclusters *ramen.DRClusterLi
 			continue
 		}
 
-		// None of the common managed clusters should belong to Metro Regions in either of the drpolicies.
-		if haveOverlappingMetroZones(match, drp, drclusters) {
+		// None of the common managed clusters should belong to Metro clusters in either of the drpolicies.
+		if haveOverlappingMetroZones(match, drp, drclusters, drClusterIDsToNames) {
 			return fmt.Errorf("drpolicy: %v has overlapping metro region with another drpolicy %v", match.Name, drp.Name)
 		}
 	}
 
 	return nil
 }
 
-func getClusterIDSFromPolicy(drpolicy *ramen.DRPolicy) []string {
-	cids := []string{}
-
-	if len(drpolicy.Status.Sync.PeerClasses) > 0 {
-		for _, pc := range drpolicy.Status.Sync.PeerClasses {
-			cids = append(cids, pc.ClusterIDs...)
-		}
-
-		return cids
-	}
-
-	if len(drpolicy.Status.Async.PeerClasses) > 0 {
-		for _, pc := range drpolicy.Status.Async.PeerClasses {
-			cids = append(cids, pc.ClusterIDs...)
-		}
-
-		return cids
-	}
-
-	return cids
-}
-
-func hasOverlapWithMetro(metroMap map[string][]string, commonClusterIDs, commonClusters []string) bool {
-	for _, v := range metroMap {
-		if sets.NewString(v...).HasAny(commonClusterIDs...) {
-			return true
-		}
-
-		if sets.NewString(v...).HasAny(commonClusters...) {
-			return true
-		}
-	}
-
-	return false
-}
-
-func haveOverlappingMetroZones(d1 *ramen.DRPolicy, d2 *ramen.DRPolicy, drclusters *ramen.DRClusterList) bool {
+func haveOverlappingMetroZones(
+	d1, d2 *ramen.DRPolicy,
+	drclusters *ramen.DRClusterList,
+	drClusterIDsToNames map[string]string,
+) bool {
 	d1ClusterNames := sets.NewString(util.DRPolicyClusterNames(d1)...)
-	d1ClusterIDs := sets.NewString(getClusterIDSFromPolicy(d1)...)
-	d1SupportsMetro, d1MetroMap := dRPolicySupportsMetro(d1, drclusters.Items)
-
+	d1SupportsMetro, d1MetroClusters := dRPolicySupportsMetro(d1, drclusters.Items, drClusterIDsToNames)
 	d2ClusterNames := sets.NewString(util.DRPolicyClusterNames(d2)...)
-	d2ClusterIDs := sets.NewString(getClusterIDSFromPolicy(d2)...)
-	d2SupportsMetro, d2MetroMap := dRPolicySupportsMetro(d2, drclusters.Items)
-
+	d2SupportsMetro, d2MetroClusters := dRPolicySupportsMetro(d2, drclusters.Items, drClusterIDsToNames)
 	commonClusters := d1ClusterNames.Intersection(d2ClusterNames)
-	commonClusterIDs := d1ClusterIDs.Intersection(d2ClusterIDs)
 
 	// No common managed clusters, so we are good
-	if commonClusterIDs.Len() == 0 && commonClusters.Len() == 0 {
+	if commonClusters.Len() == 0 {
 		return false
 	}
 
 	// Lets check if the metro clusters in DRPolicy d2 belong to common managed clusters list
 	if d2SupportsMetro {
-		return hasOverlapWithMetro(d2MetroMap,
-			commonClusterIDs.List(),
-			commonClusters.List())
+		for _, v := range d2MetroClusters {
+			if sets.NewString(v...).HasAny(commonClusters.List()...) {
+				return true
+			}
+		}
 	}
 
 	// Lets check if the metro clusters in DRPolicy d1 belong to common managed clusters list
 	if d1SupportsMetro {
-		return hasOverlapWithMetro(d1MetroMap,
-			commonClusterIDs.List(),
-			commonClusters.List())
+		for _, v := range d1MetroClusters {
+			if sets.NewString(v...).HasAny(commonClusters.List()...) {
+				return true
+			}
+		}
 	}
 
 	return false
@@ -407,7 +405,7 @@ func (u *drpolicyUpdater) deleteDRPolicy(drclusters *ramen.DRClusterList,
 	}
 
 	// proceed to delete metrics if non-metro-dr
-	isMetro, _ := dRPolicySupportsMetro(u.object, drclusters.Items)
+	isMetro, _ := dRPolicySupportsMetro(u.object, drclusters.Items, nil)
 	if !isMetro {
 		// delete metrics if matching labels are found
 		metricLabels := DRPolicySyncIntervalMetricLabels(u.object)