Skip to content

Refactor haveOverlappingMetroZones to return a list of cluster lists #2006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 19, 2025
Merged

Refactor haveOverlappingMetroZones to return a list of cluster lists #2006

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 44 additions & 23 deletions internal/controller/drplacementcontrol.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be true, nil or false, nil?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be true, nil since we want to process further only when ClusterIDToNames are present and these are present only when validatingConflicts. we can just return true,nil in all other cases where we just want to check if Metro is true or not.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The previous check is sufficient to determine that Sync is supported, so when callers do not pass in the ID map, the answer is true and a nil return clusters list. So I am stating this is correct as is.

}

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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't line 1948-1950 be deleted?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, merge issue, good catch.

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 {
Expand Down
4 changes: 2 additions & 2 deletions internal/controller/drplacementcontrol_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion internal/controller/drplacementcontrol_watcher.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
124 changes: 61 additions & 63 deletions internal/controller/drpolicy_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand All @@ -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
}
Expand Down Expand Up @@ -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)
}
Expand All @@ -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]
Expand All @@ -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
Expand Down Expand Up @@ -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)
Expand Down
Loading