t3c parentdotconfig: ensure ocgmap is ordered by prim, sec (#7411)

* t3c parentdotconfig: ensure ocgmap is ordered by prim, sec

* parentdotconfig_test: clean up unneeded test case setup

* add comment to primarySecondary struct

---------

Co-authored-by: Brian Olsen <bolsen149@comcast.com>

CHANGELOG.md

@@ -94,6 +94,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/).
 - [#7352](https://github.com/apache/trafficcontrol/pull/7352) *Traffic Control Cache Config (t3c)* Fixed issue with application locking which would allow multiple instances of `t3c apply` to run concurrently.
 - [#6775](https://github.com/apache/trafficcontrol/issues/6775) *Traffic Ops* Invalid "orgServerFqdn" in Delivery Service creation/update causes Internal Server Error
 - [#6695](https://github.com/apache/trafficcontrol/issues/6695) *Traffic Control Cache Config (t3c)* Directory creation was erroneously reporting an error when actually succeeding.
+- [#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.
 
 ## [7.0.0] - 2022-07-19
 ### Added

lib/go-atscfg/parentdotconfig.go

@@ -165,8 +165,14 @@ func MakeParentDotConfig(
 	}, nil
 }
 
-// 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][]string) (string, tc.Topology, []string) {
+// primarySecondary contains the names of the primary and secondary parent cache groups.
+type primarySecondary struct {
+	Primary   string
+	Secondary 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) {
 
 	topoName := ""
 	topo := tc.Topology{}
@@ -181,9 +187,9 @@ func CreateTopology(server *Server, ds DeliveryService, nameTopologies map[Topol
 	}
 
 	// use the topology name for the fqdn
-	cgnames, ok := ocgmap[OriginHost(orgURI.Hostname())]
+	cgprimsec, ok := ocgmap[OriginHost(orgURI.Hostname())]
 	if !ok {
-		cgnames, ok = ocgmap[OriginHost(deliveryServicesAllParentsKey)]
+		cgprimsec, ok = ocgmap[OriginHost(deliveryServicesAllParentsKey)]
 		if !ok {
 			warns = append(warns, "DS '"+*ds.XMLID+"' has no parent cache groups! Skipping!")
 			return topoName, topo, warns
@@ -221,19 +227,27 @@ func CreateTopology(server *Server, ds DeliveryService, nameTopologies map[Topol
 		}
 
 		parents := []int{}
-		for ind := 0; ind < len(cgnames); ind++ {
+		if cgprimsec.Primary != "" {
+			parents = append(parents, pind)
+			pind++
+		}
+		if cgprimsec.Secondary != "" {
 			parents = append(parents, pind)
 			pind++
 		}
 
+		// create the cache group this server belongs to
 		node := tc.TopologyNode{
 			Cachegroup: *server.Cachegroup,
 			Parents:    parents,
 		}
 		topo.Nodes = append(topo.Nodes, node)
 
-		for _, cg := range cgnames {
-			topo.Nodes = append(topo.Nodes, tc.TopologyNode{Cachegroup: cg})
+		if cgprimsec.Primary != "" {
+			topo.Nodes = append(topo.Nodes, tc.TopologyNode{Cachegroup: cgprimsec.Primary})
+		}
+		if cgprimsec.Secondary != "" {
+			topo.Nodes = append(topo.Nodes, tc.TopologyNode{Cachegroup: cgprimsec.Secondary})
 		}
 	}
 	return topoName, topo, warns
@@ -425,13 +439,14 @@ func makeParentDotConfigData(
 		return nil, warnings, errors.New("getting origin servers and profile caches: " + err.Error())
 	}
 
-	parentInfos, piWarns := makeParentInfo(serverParentCGData, serverCDNDomain, originServers, serverCapabilities)
+	parentInfos, piWarns := makeParentInfos(serverParentCGData, serverCDNDomain, originServers, serverCapabilities)
 	warnings = append(warnings, piWarns...)
 
 	dsOrigins, dsOriginWarns := makeDSOrigins(dss, dses, servers)
 	warnings = append(warnings, dsOriginWarns...)
 
-	ocgmap := map[OriginHost][]string{}
+	// Note map cache group lists are ordered, prim first, sec second
+	ocgmap := map[OriginHost]primarySecondary{}
 
 	for _, ds := range dses {
 
@@ -468,11 +483,16 @@ func makeParentDotConfigData(
 			if len(ocgmap) == 0 {
 				ocgmap = makeOCGMap(parentInfos)
 				if len(ocgmap) == 0 {
-					ocgmap[""] = []string{}
+					ocgmap[""] = primarySecondary{}
 				}
 			}
 
-			topoName, topo, warns := CreateTopology(server, ds, nameTopologies, ocgmap)
+			topoName, topo, warns := createTopology(
+				server,
+				ds,
+				nameTopologies,
+				ocgmap,
+			)
 
 			warnings = append(warnings, warns...)
 			if topoName == "" {
@@ -610,18 +630,24 @@ 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][]string {
-	ocgnames := map[OriginHost][]string{}
+func makeOCGMap(opis map[OriginHost][]parentInfo) map[OriginHost]primarySecondary {
+	ocgnames := map[OriginHost]primarySecondary{}
 
 	for host, pis := range opis {
-		cgnames := make(map[string]struct{})
+		cgnames := make(map[string]bool)
 		for _, pi := range pis {
-			cgnames[string(pi.Cachegroup)] = struct{}{}
+			cgnames[string(pi.Cachegroup)] = pi.PrimaryParent
 		}
 
-		for cg, _ := range cgnames {
-			ocgnames[host] = append(ocgnames[host], cg)
+		ps := primarySecondary{}
+		for cg, isPrim := range cgnames {
+			if isPrim {
+				ps.Primary = cg
+			} else {
+				ps.Secondary = cg
+			}
 		}
+		ocgnames[host] = ps
 	}
 
 	return ocgnames
@@ -1546,7 +1572,11 @@ func getMSOParentStrs(
 	// if atsMajorVersion >= 6 && msoAlgorithm == "consistent_hash" && len(secondaryParentStr) > 0 {
 	// parents = `parent="` + strings.Join(parentInfoTxt, "") + `"`
 	// secondaryParents = ` secondary_parent="` + secondaryParentStr + `"`
-	secondaryMode, secondaryModeWarnings := getSecondaryModeStr(tryAllPrimariesBeforeSecondary, atsMajorVersion, dsName)
+	secondaryMode, secondaryModeWarnings := getSecondaryModeStr(
+		tryAllPrimariesBeforeSecondary,
+		atsMajorVersion,
+		dsName,
+	)
 	warnings = append(warnings, secondaryModeWarnings...)
 	// 	secondaryParents += secondaryModeStr
 	// } else {
@@ -1555,8 +1585,8 @@ func getMSOParentStrs(
 	return parentInfoTxt, secondaryParentInfo, secondaryMode, warnings
 }
 
-// makeParentInfo returns the parent info and any warnings
-func makeParentInfo(
+// makeParentInfos returns the parent info and any warnings
+func makeParentInfos(
 	serverParentCGData serverParentCacheGroupData,
 	serverDomain string, // getCDNDomainByProfileID(tx, server.ProfileID)
 	originServers map[OriginHost][]serverWithParams,

lib/go-atscfg/parentdotconfig_test.go

@@ -3957,7 +3957,7 @@ func TestMakeParentDotConfigFirstLastNoTopo(t *testing.T) {
 	}
 
 	serverParams := []tc.Parameter{
-		{
+		tc.Parameter{
 			Name:       "trafficserver",
 			ConfigFile: "package",
 			Value:      "9",
@@ -4094,11 +4094,6 @@ func TestMakeParentDotConfigFirstLastNoTopo(t *testing.T) {
 		Name:       "my-cdn-name",
 	}
 
-	dsstrs := []string{
-		`dest_domain=ds0.example.net `,
-		`dest_domain=ds1.example.net `,
-	}
-
 	{ // test edge config
 		cfg, err := MakeParentDotConfig(dses, edge, servers, topologies, serverParams, parentConfigParams, serverCapabilities, dsRequiredCapabilities, cgs, dss, cdn, hdr)
 		if err != nil {
@@ -4120,6 +4115,11 @@ func TestMakeParentDotConfigFirstLastNoTopo(t *testing.T) {
 			` unavailable_server_retry_responses="501,502"`,
 		}
 
+		dsstrs := []string{
+			`dest_domain=ds0.example.net `,
+			`dest_domain=ds1.example.net `,
+		}
+
 		for _, dsstr := range dsstrs {
 			cnt := strings.Count(txt, dsstr)
 			if 1 != cnt {
@@ -4169,6 +4169,24 @@ func TestMakeParentDotConfigFirstLastNoTopo(t *testing.T) {
 				}
 			}
 		}
+
+		// Check parent ordering (based on cache group prim/sec parents)
+		if !strings.Contains(txt, `parent="myorg0`) {
+			t.Errorf("Incorrect parent ordering, got %v", txt)
+		}
+	}
+
+	{
+		cfg, err := MakeParentDotConfig(dses, mid1, servers, topologies, serverParams, parentConfigParams, serverCapabilities, dsRequiredCapabilities, cgs, dss, cdn, hdr)
+		if err != nil {
+			t.Fatal(err)
+		}
+		txt := cfg.Text
+
+		// Check parent ordering (based on cache group prim/sec parents)
+		if !strings.Contains(txt, `parent="myorg1`) {
+			t.Errorf("Incorrect parent ordering, got %v", txt)
+		}
 	}
 }