Refactor haveOverlappingMetroZones to return a list of cluster lists #2006
Conversation
ShyamsundarR
requested review from
rakeshgm and
raghavendra-talur
and removed request for
BenamarMk and
rakeshgm
|
We still need fixes from #2001 This also requires some unit tests added, and also to be run through a e2e run with non-region specifics. |
| return nil, nil, fmt.Errorf("drclusters list: %w", err) | ||
| } | ||
|
|
||
| drClusterIDToName := map[string]string{} |
There was a problem hiding this comment.
would it make sense to rename this map to drClusterIDsToNames or drClusterIDNameMap. This drClusterIDToName looked like it holds a single value.
| d2ClusterIDs := sets.NewString(getClusterIDSFromPolicy(d2)...) | ||
| d2SupportsMetro, d2MetroMap := dRPolicySupportsMetro(d2, drclusters.Items) | ||
|
|
||
| d2SupportsMetro, d2MetroRegions := dRPolicySupportsMetro(d2, drclusters.Items, drClusterIDToName) |
There was a problem hiding this comment.
what about renaming d2MetroRegions to d2MetroMap since we are not returning region anymore. The same goes to d1MetroRegions
| return true, nil | ||
| } | ||
|
|
||
| metroMap := [][]string{} |
There was a problem hiding this comment.
(nit) name maybe misleading thinking is a map but it is a list of lists.
There was a problem hiding this comment.
True, some cleanup of names is still required. Will update with better names (for this and IDsToNames comment as well)
| clList = append(clList, drClusterIDToName[cID]) | ||
| } | ||
|
|
||
| metroMap = append(metroMap, clList) |
There was a problem hiding this comment.
We might add an empty list if drPolicy.Status.Sync.PeerClasses[idx].ClusterIDs is empty.
It is caught in haveOverlappingMetroZones but it is better to return false, nil if metroMap has no valid metroRegions.
| ) { | ||
| allRegionsMap := make(map[rmn.Region][]string) | ||
| metroMap = make(map[string][]string) | ||
| metroMap = [][]string{} | ||
|
|
||
| for _, managedCluster := range rmnutil.DRPolicyClusterNames(drpolicy) { | ||
| for _, v := range drclusters { |
There was a problem hiding this comment.
Shouldn't line 1948-1950 be deleted?
There was a problem hiding this comment.
Yes, merge issue, good catch.
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]>
ShyamsundarR
force-pushed
the
overlap-refactor
branch
from
7064057 to
b66848a
Compare
|
All comments (till now) are addressed @BenamarMk and @rakeshgm |
| for _, peerClass := range peerClasses { | ||
| metroMap[peerClass.StorageClassName] = peerClass.ClusterIDs | ||
| if drClusterIDsToNames == nil { | ||
| return true, nil |
There was a problem hiding this comment.
Should this be true, nil or false, nil?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.