t3c parent.config fix for MSO non-topo mid cache group no parent cache groups

CHANGELOG.md

@@ -100,6 +100,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - [#7411](https://github.com/apache/trafficcontrol/pull/7411) *Traffic Control Cache Config (t3c)* Fixed issue with wrong parent ordering with MSO non-topology delivery services.
 - [#7425](https://github.com/apache/trafficcontrol/pull/7425) *Traffic Control Cache Config (t3c)* Fixed issue with layered profile iteration being done in the wrong order.
 - [#6385](https://github.com/apache/trafficcontrol/issues/6385) *Traffic Ops* Reserved consistentHashQueryParameters cause internal server error
+- [#7471](https://github.com/apache/trafficcontrol/pull/7471) *Traffic Control Cache Config (t3c)* Fixed issue with MSO non topo origins from multiple cache groups.
 
 ### Removed
 - [#7271](https://github.com/apache/trafficcontrol/pull/7271) Remove components in `infrastructre/docker/`, not in use as cdn-in-a-box performs the same functionality.

lib/go-atscfg/parentdotconfig.go

@@ -165,14 +165,20 @@ func MakeParentDotConfig(
 	}, nil
 }
 
-// primarySecondary contains the names of the primary and secondary parent cache groups.
-type primarySecondary struct {
+// parentCacheGroups holds parent cache groups names for OTF topologies.
+// Primaries and Secondaries are from origin parent cache groups with
+// assigned primary and secondary cache groups.
+// For last, any origin parent cache groups that aren't primary or secondary.
+// The Null parent cache groups aren't considered unless the current server
+// cache group has no primary/secondary parents selected.
+type parentCacheGroups struct {
 	Primary   string
 	Secondary string
+	Nulls     []string
 }
 
 // createTopology creates an on the fly topology for this server and non topology delivery service.
-func createTopology(server *Server, ds DeliveryService, nameTopologies map[TopologyName]tc.Topology, ocgmap map[OriginHost]primarySecondary) (string, tc.Topology, []string) {
+func createTopology(server *Server, ds DeliveryService, nameTopologies map[TopologyName]tc.Topology, ocgmap map[OriginHost]parentCacheGroups) (string, tc.Topology, []string) {
 
 	topoName := ""
 	topo := tc.Topology{}
@@ -214,7 +220,7 @@ func createTopology(server *Server, ds DeliveryService, nameTopologies map[Topol
 		topo.Nodes = append(topo.Nodes, node)
 	} else {
 		// If mid cache group, insert fake edge cache group.
-		// This is incorrect if there are multiple MID tiers.
+		// This is technically incorrect if there are multiple MID tiers.
 		pind := 1
 		if strings.HasPrefix(server.Type, tc.MidTypePrefix) {
 			parents := []int{pind}
@@ -226,16 +232,30 @@ func createTopology(server *Server, ds DeliveryService, nameTopologies map[Topol
 			topo.Nodes = append(topo.Nodes, edgeNode)
 		}
 
+		hasprimsec := false
 		parents := []int{}
 		if cgprimsec.Primary != "" {
+			hasprimsec = true
 			parents = append(parents, pind)
 			pind++
 		}
 		if cgprimsec.Secondary != "" {
+			hasprimsec = true
 			parents = append(parents, pind)
 			pind++
 		}
 
+		// Only consider arbitrarily assigned parents if the current
+		// cache group does not have primary or secondary parents assigned.
+		if !hasprimsec && 0 < len(cgprimsec.Nulls) {
+			for _, _ = range cgprimsec.Nulls {
+				parents = append(parents, pind)
+				pind++
+			}
+			// Adding a '-1' in the parent indices indicates that this has non topology MSO parents, all should just go into primary
+			parents = append(parents, -1)
+		}
+
 		// create the cache group this server belongs to
 		node := tc.TopologyNode{
 			Cachegroup: *server.Cachegroup,
@@ -249,6 +269,11 @@ func createTopology(server *Server, ds DeliveryService, nameTopologies map[Topol
 		if cgprimsec.Secondary != "" {
 			topo.Nodes = append(topo.Nodes, tc.TopologyNode{Cachegroup: cgprimsec.Secondary})
 		}
+		if !hasprimsec {
+			for _, null := range cgprimsec.Nulls {
+				topo.Nodes = append(topo.Nodes, tc.TopologyNode{Cachegroup: null})
+			}
+		}
 	}
 	return topoName, topo, warns
 }
@@ -324,7 +349,7 @@ func makeParentDotConfigData(
 	// serverParams are the parent.config params for this particular server
 	serverParams := getServerParentConfigParams(server, parentConfigParams)
 
-	parentCacheGroups := map[string]struct{}{}
+	parentCGs := map[string]struct{}{}
 	if cacheIsTopLevel {
 		for _, cg := range cacheGroups {
 			if cg.Type == nil {
@@ -337,7 +362,7 @@ func makeParentDotConfigData(
 			if *cg.Type != tc.CacheGroupOriginTypeName {
 				continue
 			}
-			parentCacheGroups[*cg.Name] = struct{}{}
+			parentCGs[*cg.Name] = struct{}{}
 		}
 	} else {
 		for _, cg := range cacheGroups {
@@ -350,10 +375,10 @@ func makeParentDotConfigData(
 
 			if *cg.Name == *server.Cachegroup {
 				if cg.ParentName != nil && *cg.ParentName != "" {
-					parentCacheGroups[*cg.ParentName] = struct{}{}
+					parentCGs[*cg.ParentName] = struct{}{}
 				}
 				if cg.SecondaryParentName != nil && *cg.SecondaryParentName != "" {
-					parentCacheGroups[*cg.SecondaryParentName] = struct{}{}
+					parentCGs[*cg.SecondaryParentName] = struct{}{}
 				}
 				break
 			}
@@ -393,7 +418,7 @@ func makeParentDotConfigData(
 			}
 			continue
 		}
-		if _, ok := parentCacheGroups[*sv.Cachegroup]; !ok {
+		if _, ok := parentCGs[*sv.Cachegroup]; !ok {
 			continue
 		}
 		if sv.Type != tc.OriginTypeName &&
@@ -425,6 +450,7 @@ func makeParentDotConfigData(
 	cgServerIDs[*server.ID] = struct{}{}
 
 	cgDSServers := filterDSS(dss, nil, cgServerIDs)
+
 	parentServerDSes := map[int]map[int]struct{}{} // map[serverID][dsID]
 	for _, dss := range cgDSServers {
 		if parentServerDSes[dss.Server] == nil {
@@ -446,7 +472,7 @@ func makeParentDotConfigData(
 	warnings = append(warnings, dsOriginWarns...)
 
 	// Note map cache group lists are ordered, prim first, sec second
-	ocgmap := map[OriginHost]primarySecondary{}
+	ocgmap := map[OriginHost]parentCacheGroups{}
 
 	for _, ds := range dses {
 
@@ -483,7 +509,7 @@ func makeParentDotConfigData(
 			if len(ocgmap) == 0 {
 				ocgmap = makeOCGMap(parentInfos)
 				if len(ocgmap) == 0 {
-					ocgmap[""] = primarySecondary{}
+					ocgmap[""] = parentCacheGroups{}
 				}
 			}
 
@@ -630,21 +656,29 @@ func (p parentInfo) ToAbstract() *ParentAbstractionServiceParent {
 type parentInfos map[OriginHost]parentInfo
 
 // Returns a map of parent cache groups names per origin host.
-func makeOCGMap(opis map[OriginHost][]parentInfo) map[OriginHost]primarySecondary {
-	ocgnames := map[OriginHost]primarySecondary{}
+func makeOCGMap(opis map[OriginHost][]parentInfo) map[OriginHost]parentCacheGroups {
+	ocgnames := map[OriginHost]parentCacheGroups{}
+
+	type primSec struct {
+		isPrim bool
+		isSec  bool
+	}
 
 	for host, pis := range opis {
-		cgnames := make(map[string]bool)
+		cgnames := map[string]primSec{}
 		for _, pi := range pis {
-			cgnames[string(pi.Cachegroup)] = pi.PrimaryParent
+			cgnames[string(pi.Cachegroup)] =
+				primSec{isPrim: pi.PrimaryParent, isSec: pi.SecondaryParent}
 		}
 
-		ps := primarySecondary{}
-		for cg, isPrim := range cgnames {
-			if isPrim {
+		ps := parentCacheGroups{}
+		for cg, primsec := range cgnames {
+			if primsec.isPrim {
 				ps.Primary = cg
-			} else {
+			} else if primsec.isSec {
 				ps.Secondary = cg
+			} else {
+				ps.Nulls = append(ps.Nulls, cg)
 			}
 		}
 		ocgnames[host] = ps
@@ -1311,9 +1345,9 @@ func getTopologyParents(
 	dsMergeGroups []string, // sorted parent merge groups for this ds
 ) ([]*ParentAbstractionServiceParent, []*ParentAbstractionServiceParent, []string, error) {
 	warnings := []string{}
+
 	// If it's the last tier, then the parent is the origin.
 	// Note this doesn't include MSO, whose final tier cachegroup points to the origin cachegroup.
-
 	if serverIsLastTier {
 		orgURI, orgWarns, err := getOriginURI(*ds.OrgServerFQDN) // TODO pass, instead of calling again
 		warnings = append(warnings, orgWarns...)
@@ -1346,27 +1380,54 @@ func getTopologyParents(
 		return nil, nil, warnings, errors.New("This server '" + *server.HostName + "' not in DS " + *ds.XMLID + " topology, skipping")
 	}
 
-	if len(svNode.Parents) == 0 {
+	numParents := len(svNode.Parents)
+
+	// No parents (or special case mso non topo)
+	if numParents == 0 || -1 == svNode.Parents[0] {
 		return nil, nil, warnings, errors.New("DS " + *ds.XMLID + " topology '" + *ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
 	}
-	if numParents := len(svNode.Parents); numParents > 2 {
-		warnings = append(warnings, "DS "+*ds.XMLID+" topology '"+*ds.Topology+"' has "+strconv.Itoa(numParents)+" parent nodes, but Apache Traffic Server only supports Primary and Secondary (2) lists of parents. CacheGroup nodes after the first 2 will be ignored!")
-	}
-	if len(topology.Nodes) <= svNode.Parents[0] {
-		return nil, nil, warnings, errors.New("DS " + *ds.XMLID + " topology '" + *ds.Topology + "' node parent " + strconv.Itoa(svNode.Parents[0]) + " greater than number of topology nodes " + strconv.Itoa(len(topology.Nodes)) + ". Cannot create parents!")
-	}
-	if len(svNode.Parents) > 1 && len(topology.Nodes) <= svNode.Parents[1] {
-		warnings = append(warnings, "DS "+*ds.XMLID+" topology '"+*ds.Topology+"' node secondary parent "+strconv.Itoa(svNode.Parents[1])+" greater than number of topology nodes "+strconv.Itoa(len(topology.Nodes))+". Secondary parent will be ignored!")
-	}
 
-	parentCG := topology.Nodes[svNode.Parents[0]].Cachegroup
-	secondaryParentCG := ""
-	if len(svNode.Parents) > 1 && len(topology.Nodes) > svNode.Parents[1] {
-		secondaryParentCG = topology.Nodes[svNode.Parents[1]].Cachegroup
-	}
+	primaryCGs := map[string]struct{}{}
+	secondaryCG := ""
 
-	if parentCG == "" {
-		return nil, nil, warnings, errors.New("Server '" + *server.HostName + "' DS " + *ds.XMLID + " topology '" + *ds.Topology + "' cachegroup '" + *server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	// Check for special case of MSO non topology with no parent
+	// cache groups assigned to the server cache group.
+	if -1 == svNode.Parents[numParents-1] {
+		for _, pind := range svNode.Parents {
+			if -1 == pind {
+				break
+			}
+			parentCG := topology.Nodes[pind].Cachegroup
+			if parentCG != "" {
+				primaryCGs[parentCG] = struct{}{}
+			} else {
+				warnings = append(warnings, "Server '"+*server.HostName+"' DS "+*ds.XMLID+" topology '"+*ds.Topology+"' cachegroup '"+*server.Cachegroup+"' topology node parent "+strconv.Itoa(pind)+" is not in the topology!")
+			}
+		}
+	} else { // normal topology/mso configuration
+		if 2 < numParents && 0 <= svNode.Parents[0] {
+			warnings = append(warnings, "DS "+*ds.XMLID+" topology '"+*ds.Topology+"' has "+strconv.Itoa(numParents)+" parent nodes, but Apache Traffic Server only supports Primary and Secondary (2) lists of parents. CacheGroup nodes after the first 2 will be ignored!")
+		}
+
+		numNodes := len(topology.Nodes)
+
+		if numNodes <= svNode.Parents[0] {
+			return nil, nil, warnings, errors.New("DS " + *ds.XMLID + " topology '" + *ds.Topology + "' node parent " + strconv.Itoa(svNode.Parents[0]) + " greater than number of topology nodes " + strconv.Itoa(numNodes) + ". Cannot create parents!")
+		}
+		if 1 < numParents && numNodes <= svNode.Parents[1] {
+			warnings = append(warnings, "DS "+*ds.XMLID+" topology '"+*ds.Topology+"' node secondary parent "+strconv.Itoa(svNode.Parents[1])+" greater than number of topology nodes "+strconv.Itoa(numNodes)+". Secondary parent will be ignored!")
+		}
+
+		parentCG := topology.Nodes[svNode.Parents[0]].Cachegroup
+		if 1 < numParents && svNode.Parents[1] < numNodes {
+			secondaryCG = topology.Nodes[svNode.Parents[1]].Cachegroup
+		}
+
+		if parentCG == "" {
+			return nil, nil, warnings, errors.New("Server '" + *server.HostName + "' DS " + *ds.XMLID + " topology '" + *ds.Topology + "' cachegroup '" + *server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+		}
+
+		primaryCGs[parentCG] = struct{}{}
 	}
 
 	parentStrs := []*ParentAbstractionServiceParent{}
@@ -1403,16 +1464,15 @@ func getTopologyParents(
 		if sv.Type != tc.OriginTypeName && !hasRequiredCapabilities(serverCapabilities[*sv.ID], dsRequiredCapabilities[*ds.ID]) {
 			continue
 		}
-		if *sv.Cachegroup == parentCG {
+		if _, ok := primaryCGs[*sv.Cachegroup]; ok {
 			parentStr, err := serverParentStr(&sv.Server, sv.Params)
 			if err != nil {
 				return nil, nil, warnings, errors.New("getting server parent string: " + err.Error())
 			}
 			if parentStr != nil { // will be nil if server is not_a_parent (possibly other reasons)
 				parentStrs = append(parentStrs, parentStr)
 			}
-		}
-		if *sv.Cachegroup == secondaryParentCG {
+		} else if *sv.Cachegroup == secondaryCG {
 			parentStr, err := serverParentStr(&sv.Server, sv.Params)
 			if err != nil {
 				return nil, nil, warnings, errors.New("getting server parent string: " + err.Error())
@@ -1422,7 +1482,12 @@ func getTopologyParents(
 	}
 
 	if 0 < len(dsMergeGroups) && 0 < len(secondaryParentStrs) {
-		if util.ContainsStr(dsMergeGroups, parentCG) && util.ContainsStr(dsMergeGroups, secondaryParentCG) {
+		parentCG := ""
+		for key, _ := range primaryCGs {
+			parentCG = key
+			break
+		}
+		if util.ContainsStr(dsMergeGroups, parentCG) && util.ContainsStr(dsMergeGroups, secondaryCG) {
 			parentStrs = append(parentStrs, secondaryParentStrs...)
 			secondaryParentStrs = nil
 		}