You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@trafficcontrol.apache.org by GitBox <gi...@apache.org> on 2020/06/12 20:30:52 UTC

[GitHub] [trafficcontrol] rob05c opened a new pull request #4790: Add Topology ATS Config Generation

rob05c opened a new pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790


   Adds Topologies to ATS Config Generation.
   
   Includes tests.
   No docs, docs already exist for Topologies, and details of config gen aren't documented.
   Includes changelog.
   
   - [x] This PR is not related to any other Issue
   
   ## Which Traffic Control components are affected by this PR?
   - Traffic Ops ORT
   
   ## What is the best way to verify this PR?
   Run tests.
   Create Topologies in TO/TP with a Server, and a Delivery Service. Run ORT on that Server, and verify the config generated is correct. Verify servers not in a topology aren't included. Verify DSes without Topologies still use DeliveryServiceServers appropriately. Verify ATS loads config and serves requests to the DS appropriately.
   
   ## If this is a bug fix, what versions of Traffic Control are affected?
   Not a bug fix.
   
   ## The following criteria are ALL met by this PR
   - [x] This PR includes tests OR I have explained why tests are unnecessary
   - [x] This PR includes documentation OR I have explained why documentation is unnecessary
   - [x] This PR includes an update to CHANGELOG.md OR such an update is not necessary
   - [x] This PR includes any and all required license headers
   - [x] This PR ensures that database migration sequence is correct OR this PR does not include a database migration
   - [x] This PR **DOES NOT FIX A SERIOUS SECURITY VULNERABILITY** (see [the Apache Software Foundation's security guidelines](https://www.apache.org/security/) for details)
   
   ## Additional Information
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-668818152


   >toreq.GetServerUpdateStatus() calls the API 1.4 version of GET /servers/{host_name}/update_status. Since #4901 only adds Flexible Topologies support for API 3.0 version of GET /servers/{host_name}/update_status, atstccfg won't get accurate parent_pending and parent_reval_pendinginfo if it uses API 1.4 for this route.
   
   Fixed - ORT/atstccfg will now use the latest API if it exists, and fall back to the prior if it doesn't.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451713333



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?

Review comment:
       Right, this is what I was saying earlier: the last tier should go to the Origin, unless it's MSO. If I'm remembering right.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451693534



##########
File path: lib/go-atscfg/hostingdotconfig.go
##########
@@ -74,3 +92,22 @@ func MakeHostingDotConfig(
 	text += strings.Join(lines, "")
 	return text
 }
+
+func hostingMakeDSTopologies(dses []tc.DeliveryServiceNullable, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if ds.Topology == nil || ds.XMLID == nil {
+			continue
+		}
+		if to, ok := topNames[*ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(*ds.XMLID)] = to
+		} else if *ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + *ds.XMLID + "': has topology '" + *ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")

Review comment:
       Rather than sending context down, I think it would be cleaner to propagate the error up and add the context in the caller. Most of config gen doesn't use errors now, because it needed to emulate the Perl in the TO API.
   
   Once that's done, many functions like this will also distinguish between fatal and non-fatal errors.
   
   I changed it to "hosting.config" for now, and added a TODO to propagate once config gen is returning errors properly.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rawlinp commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rawlinp commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r445175362



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -162,6 +165,24 @@ type OriginURI struct {
 	Port   string
 }
 
+/*

Review comment:
       I imagine this comment was helpful for developing this PR but probably isn't needed anymore.

##########
File path: lib/go-atscfg/hostingdotconfig.go
##########
@@ -74,3 +92,22 @@ func MakeHostingDotConfig(
 	text += strings.Join(lines, "")
 	return text
 }
+
+func hostingMakeDSTopologies(dses []tc.DeliveryServiceNullable, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {

Review comment:
       This function appears to basically duplicate the `makeDSTopologies` function in remapdotconfig.go except for the `== nil` checks and using `tc.DeliveryServiceNullable`. Can they be consolidated and shared?

##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -134,3 +136,64 @@ const ConfigSuffix = ".config"
 func GetConfigFile(prefix string, xmlId string) string {
 	return prefix + xmlId + ConfigSuffix
 }
+
+// topologyIncludesServer returns whether the given topology includes the given server
+func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
+	_, inTopology := serverTopologyTier(server, topology)
+	return inTopology
+}
+
+// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
+func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {

Review comment:
       With first/inner/last header rewrite now it might make sense for this function to specify which of those 3 buckets the server is in and use this when generating those configs.

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {

Review comment:
       `server` and `topology` are unused

##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -134,3 +136,64 @@ const ConfigSuffix = ".config"
 func GetConfigFile(prefix string, xmlId string) string {
 	return prefix + xmlId + ConfigSuffix
 }
+
+// topologyIncludesServer returns whether the given topology includes the given server
+func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
+	_, inTopology := serverTopologyTier(server, topology)
+	return inTopology
+}
+
+// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
+func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+	serverNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			serverNode = node
+			break
+		}
+	}
+	if serverNode.Cachegroup == "" {
+		return false, false
+	}
+
+	return len(serverNode.Parents) == 0, true

Review comment:
       So for MSO purposes I think we may need to include `ORG_LOC` cachegroups in topologies. So a more valid check for this is probably that there are no parents _or_ the parents are all `ORG_LOC`.

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?

Review comment:
       This seems right to me... 🤔 although, if we're the last tier and the DS doesn't have an origin shield or MSO, I think we normally don't include a parent.config line for the DS. So this kind of changes that, right? Every topology-based DS will get a parent.config line on last-tier caches now?
   
   As long as ATS parenting behavior is unchanged, I don't really see a problem with that.

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"

Review comment:
       Should we include the topology name in this comment as well? Seems like that would be pretty helpful

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -166,14 +188,20 @@ func GetServerConfigRemapDotConfigForEdge(
 	cacheURLConfigParams map[string]string,
 	profilesCacheKeyConfigParams map[int]map[string]string,
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
-	server *ServerInfo,
+	serverInfo *ServerInfo,
 	dses []RemapConfigDSData,
 	atsMajorVersion int,
 	header string,
+	server tc.Server,
+	topologies map[tc.DeliveryServiceName]tc.Topology,
 ) string {
 	textLines := []string{}
 
 	for _, ds := range dses {
+		topology, hasTopology := topologies[tc.DeliveryServiceName(ds.Name)]
+		if hasTopology && !topologyIncludesServer(topology, server) {

Review comment:
       I think this needs to check the required capabilities before generating a remap line for a given DS, right?

##########
File path: lib/go-atscfg/hostingdotconfig.go
##########
@@ -74,3 +92,22 @@ func MakeHostingDotConfig(
 	text += strings.Join(lines, "")
 	return text
 }
+
+func hostingMakeDSTopologies(dses []tc.DeliveryServiceNullable, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if ds.Topology == nil || ds.XMLID == nil {
+			continue
+		}
+		if to, ok := topNames[*ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(*ds.XMLID)] = to
+		} else if *ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + *ds.XMLID + "': has topology '" + *ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")

Review comment:
       The error message should say `hosting.config`, but if this function is consolidated with `makeDSTopologies` maybe it should be left to the caller to prepend the `Making *.config` context.

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,

Review comment:
       This parameter is unused

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {

Review comment:
       `server` and `topology` are unused

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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] {

Review comment:
       I think this needs to be `<=`

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive

Review comment:
       This should be correct, because for topology-based DSes, the DS type should no longer determine whether or not it skips the mid tier.

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -74,28 +75,49 @@ func MakeRemapDotConfig(
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
 	serverInfo *ServerInfo, // ServerInfo for this server
 	remapDSData []RemapConfigDSData,
+	topologies []tc.Topology,
 ) string {
-	hdr := GenericHeaderComment(string(serverName), toToolName, toURL)
-	text := ""
+	dsTopologies := makeDSTopologies(remapDSData, topologies)
+	hdr := GenericHeaderComment(server.HostName, toToolName, toURL)
 	if tc.CacheTypeFromString(serverInfo.Type) == tc.CacheTypeMid {
-		text = GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr)
-	} else {
-		text = GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr)
+		return GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr, server, dsTopologies)
 	}
-	return text
+	return GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr, server, dsTopologies)
+}
+
+func makeDSTopologies(dses []RemapConfigDSData, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if to, ok := topNames[ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(ds.Name)] = to
+		} else if ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + ds.Name + "': has topology '" + ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")
+		}
+	}
+	return dsTops
 }
 
 func GetServerConfigRemapDotConfigForMid(
 	atsMajorVersion int,
 	profilesCacheKeyConfigParams map[int]map[string]string,
-	server *ServerInfo,
+	serverInfo *ServerInfo,
 	dses []RemapConfigDSData,
 	header string,
+	server tc.Server,
+	topologies map[tc.DeliveryServiceName]tc.Topology,
 ) string {
 	midRemaps := map[string]string{}
 	for _, ds := range dses {
-		if ds.Type.IsLive() && !ds.Type.IsNational() {
-			continue // Live local delivery services skip mids
+		topology, hasTopology := topologies[tc.DeliveryServiceName(ds.Name)]
+		if hasTopology && !topologyIncludesServer(topology, server) {

Review comment:
       I think this needs to check the required capabilities before generating a remap line for a given DS, right?

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -196,7 +224,11 @@ func GetServerConfigRemapDotConfigForEdge(
 			if ds.ProfileID != nil {
 				profilecacheKeyConfigParams = profilesCacheKeyConfigParams[*ds.ProfileID]
 			}
-			remapText = BuildRemapLine(cacheURLConfigParams, atsMajorVersion, server, serverPackageParamData, remapText, ds, line.From, line.To, profilecacheKeyConfigParams)
+			remapText = BuildRemapLine(cacheURLConfigParams, atsMajorVersion, serverInfo, serverPackageParamData, remapText, ds, line.From, line.To, profilecacheKeyConfigParams)
+			if hasTopology {
+				remapText += " # topology" // debug

Review comment:
       This comment should include the topology name as well

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities

Review comment:
       This TODO seems to be done, but the TODO below doesn't seem to be done because remap.config generation doesn't seem to check capabilities yet. Did I miss where you're doing that now?

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {

Review comment:
       `server` and `topology` are unused

##########
File path: traffic_ops_ort/atstccfg/cfgfile/headerrewritedotconfig.go
##########
@@ -70,7 +70,7 @@ func GetConfigFileCDNHeaderRewrite(toData *config.TOData, fileName string) (stri
 		if server.CDNName != *tcDS.CDNName {
 			continue
 		}
-		if _, ok := dsServerIDs[server.ID]; !ok {
+		if _, ok := dsServerIDs[server.ID]; !ok && tcDS.Topology == nil {

Review comment:
       Do we need to take capabilities into account when determining whether or not a given header-rewrite should be generated for a cache? I suppose as long as the remap is omitted in that case, the header-rewrite configs are just cruft.

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -74,28 +75,49 @@ func MakeRemapDotConfig(
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
 	serverInfo *ServerInfo, // ServerInfo for this server
 	remapDSData []RemapConfigDSData,
+	topologies []tc.Topology,
 ) string {
-	hdr := GenericHeaderComment(string(serverName), toToolName, toURL)
-	text := ""
+	dsTopologies := makeDSTopologies(remapDSData, topologies)
+	hdr := GenericHeaderComment(server.HostName, toToolName, toURL)
 	if tc.CacheTypeFromString(serverInfo.Type) == tc.CacheTypeMid {
-		text = GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr)
-	} else {
-		text = GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr)
+		return GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr, server, dsTopologies)
 	}
-	return text
+	return GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr, server, dsTopologies)
+}
+
+func makeDSTopologies(dses []RemapConfigDSData, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if to, ok := topNames[ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(ds.Name)] = to
+		} else if ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + ds.Name + "': has topology '" + ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")
+		}
+	}
+	return dsTops
 }
 
 func GetServerConfigRemapDotConfigForMid(
 	atsMajorVersion int,
 	profilesCacheKeyConfigParams map[int]map[string]string,
-	server *ServerInfo,
+	serverInfo *ServerInfo,

Review comment:
       `serverInfo` is now unused

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin

Review comment:
       this comment is confusing




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451702918



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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] {

Review comment:
       I'm not understanding why. We dereference that particular node, the parent, `topology.Nodes[svNode.Parents[0]]` further down. We never dereference `topology.Nodes[svNode.Parents[0]+1]`, and I'm not seeing why we would.
   
   Can you elaborate?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451713333



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?

Review comment:
       Right, this is what I was saying earlier: the last tier should go to the Origin, unless it's MSO (in which case the last tier _is_ the Origin). If I'm remembering right.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465869723



##########
File path: traffic_ops_ort/atstccfg/config/config.go
##########
@@ -297,4 +297,8 @@ type TOData struct {
 
 	// SSLKeys must be all the ssl keys for the server's cdn.
 	SSLKeys []tc.CDNSSLKeys
+
+	// Topologies must be all the topologies for the server's cdn.
+	// May incude topologies of other cdns.
+	Topologies []tc.Topology

Review comment:
       Good point, that makes more sense in the long run.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r463884113



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +330,268 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	log.Errorf("DEBUG topo GetTopologyParentConfigLine calling getTopologyPlacement cg '" + server.Cachegroup + "'\n")
+	serverPlacement := getTopologyPlacement(tc.CacheGroupName(server.Cachegroup), topology, cacheGroups)
+	if !serverPlacement.InTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, servers, parentConfigParams, topology, serverPlacement.IsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(ds, serverParams, serverPlacement.IsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(ds, serverPlacement.IsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(ds, serverParams, serverPlacement.IsLastTier)
+	txt += getTopologyParentIsProxyStr(serverPlacement.IsLastTier)
+	txt += " # topology '" + ds.Topology + "'"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier {
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(ds ParentConfigDSTopLevel, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	return "true"
+}
+
+func getTopologyQueryString(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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, errors.New("DS " + string(ds.Name) + " 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] {
+		log.Errorln("DS " + string(ds.Name) + " 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
+	}
+
+	if parentCG == "" {
+		return nil, nil, errors.New("Server '" + server.HostName + "' DS " + string(ds.Name) + " topology '" + ds.Topology + "' cachegroup '" + server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	}
+
+	parentStrs := []string{}
+	secondaryParentStrs := []string{}
+	for _, sv := range servers {
+		if tc.CacheType(sv.Type) != tc.CacheTypeEdge && tc.CacheType(sv.Type) != tc.CacheTypeMid && sv.Type != tc.OriginTypeName {
+			continue // only consider edges, mids, and origins in the CacheGroup.
+		}
+		if !HasRequiredCapabilities(serverCapabilities[sv.ID], ds.RequiredCapabilities) {
+			continue
+		}
+		if sv.Cachegroup == parentCG {
+			parentStr := serverParentStr(sv, parentConfigParams)
+			if parentStr != "" { // will be empty if server is not_a_parent (possibly other reasons)
+				parentStrs = append(parentStrs, parentStr)
+			}
+		}
+		if sv.Cachegroup == secondaryParentCG {
+			secondaryParentStrs = append(secondaryParentStrs, serverParentStr(sv, parentConfigParams))
+		}

Review comment:
       If the immediate parent cachegroup has no servers in it `GetTopologyParents()`, returns empty arrays, even if there are servers in further ancestor topology nodes. Should it keep going until it finds a topology node with a non-empty cachegroup?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465868702



##########
File path: traffic_ops_ort/atstccfg/config/config.go
##########
@@ -297,4 +297,8 @@ type TOData struct {
 
 	// SSLKeys must be all the ssl keys for the server's cdn.
 	SSLKeys []tc.CDNSSLKeys
+
+	// Topologies must be all the topologies for the server's cdn.
+	// May incude topologies of other cdns.
+	Topologies []tc.Topology

Review comment:
       I'd like to keep TOData as the raw data from Traffic Ops, in order to make the config gen library `lib/go-atscfg` more flexible.
   
   As soon as the config routes are removed from TO, the plan is to change `lib/go-atscfg` to take standard API objects.
   
   If we do pre-processing like putting things in maps in ORT/atstccfg, and pass pre-processed data to `lib/go-atscfg`, then `atscfg` is harder to use for anyone wanting to just make a TO request for the necessary data and pass it to a config gen func.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465233816



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +330,268 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	log.Errorf("DEBUG topo GetTopologyParentConfigLine calling getTopologyPlacement cg '" + server.Cachegroup + "'\n")
+	serverPlacement := getTopologyPlacement(tc.CacheGroupName(server.Cachegroup), topology, cacheGroups)
+	if !serverPlacement.InTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, servers, parentConfigParams, topology, serverPlacement.IsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(ds, serverParams, serverPlacement.IsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(ds, serverPlacement.IsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(ds, serverParams, serverPlacement.IsLastTier)
+	txt += getTopologyParentIsProxyStr(serverPlacement.IsLastTier)
+	txt += " # topology '" + ds.Topology + "'"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier {
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(ds ParentConfigDSTopLevel, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	return "true"
+}
+
+func getTopologyQueryString(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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, errors.New("DS " + string(ds.Name) + " 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] {
+		log.Errorln("DS " + string(ds.Name) + " 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
+	}
+
+	if parentCG == "" {
+		return nil, nil, errors.New("Server '" + server.HostName + "' DS " + string(ds.Name) + " topology '" + ds.Topology + "' cachegroup '" + server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	}
+
+	parentStrs := []string{}
+	secondaryParentStrs := []string{}
+	for _, sv := range servers {
+		if tc.CacheType(sv.Type) != tc.CacheTypeEdge && tc.CacheType(sv.Type) != tc.CacheTypeMid && sv.Type != tc.OriginTypeName {
+			continue // only consider edges, mids, and origins in the CacheGroup.
+		}
+		if !HasRequiredCapabilities(serverCapabilities[sv.ID], ds.RequiredCapabilities) {
+			continue
+		}
+		if sv.Cachegroup == parentCG {
+			parentStr := serverParentStr(sv, parentConfigParams)
+			if parentStr != "" { // will be empty if server is not_a_parent (possibly other reasons)
+				parentStrs = append(parentStrs, parentStr)
+			}
+		}
+		if sv.Cachegroup == secondaryParentCG {
+			secondaryParentStrs = append(secondaryParentStrs, serverParentStr(sv, parentConfigParams))
+		}

Review comment:
       (That validation would occur in TO and is outside the scope of this PR)




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451712619



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities

Review comment:
       Yes, first is done, second isn't. I removed the first. I'll do the second shortly.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451711379



##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -196,7 +224,11 @@ func GetServerConfigRemapDotConfigForEdge(
 			if ds.ProfileID != nil {
 				profilecacheKeyConfigParams = profilesCacheKeyConfigParams[*ds.ProfileID]
 			}
-			remapText = BuildRemapLine(cacheURLConfigParams, atsMajorVersion, server, serverPackageParamData, remapText, ds, line.From, line.To, profilecacheKeyConfigParams)
+			remapText = BuildRemapLine(cacheURLConfigParams, atsMajorVersion, serverInfo, serverPackageParamData, remapText, ds, line.From, line.To, profilecacheKeyConfigParams)
+			if hasTopology {
+				remapText += " # topology" // debug

Review comment:
       Done.

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -74,28 +75,49 @@ func MakeRemapDotConfig(
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
 	serverInfo *ServerInfo, // ServerInfo for this server
 	remapDSData []RemapConfigDSData,
+	topologies []tc.Topology,
 ) string {
-	hdr := GenericHeaderComment(string(serverName), toToolName, toURL)
-	text := ""
+	dsTopologies := makeDSTopologies(remapDSData, topologies)
+	hdr := GenericHeaderComment(server.HostName, toToolName, toURL)
 	if tc.CacheTypeFromString(serverInfo.Type) == tc.CacheTypeMid {
-		text = GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr)
-	} else {
-		text = GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr)
+		return GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr, server, dsTopologies)
 	}
-	return text
+	return GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr, server, dsTopologies)
+}
+
+func makeDSTopologies(dses []RemapConfigDSData, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if to, ok := topNames[ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(ds.Name)] = to
+		} else if ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + ds.Name + "': has topology '" + ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")
+		}
+	}
+	return dsTops
 }
 
 func GetServerConfigRemapDotConfigForMid(
 	atsMajorVersion int,
 	profilesCacheKeyConfigParams map[int]map[string]string,
-	server *ServerInfo,
+	serverInfo *ServerInfo,

Review comment:
       Removed

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin

Review comment:
       Removed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451711971



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {

Review comment:
       Removed

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {

Review comment:
       Removed

##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {

Review comment:
       Removed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451698114



##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -134,3 +136,64 @@ const ConfigSuffix = ".config"
 func GetConfigFile(prefix string, xmlId string) string {
 	return prefix + xmlId + ConfigSuffix
 }
+
+// topologyIncludesServer returns whether the given topology includes the given server
+func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
+	_, inTopology := serverTopologyTier(server, topology)
+	return inTopology
+}
+
+// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
+func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+	serverNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			serverNode = node
+			break
+		}
+	}
+	if serverNode.Cachegroup == "" {
+		return false, false
+	}
+
+	return len(serverNode.Parents) == 0, true

Review comment:
       So, it's been a few weeks, I'll have to verify, but I'm pretty sure the rest of the Topology Config expects that.
   
   I believe I wrote it expecting MSO Origins to be the "last tier," and everything generates correctly. In terms of the remapping config, the last tier is no different than an intermediary tier, and multiple MSO origins are set as parents exactly the same way as multiple Mid parents.
   
   That said, that may not work with the First/Middle/Last Header Rewrites. I'm planning to start implementing those soon, so hopefully I'll verify then.
   
   I seem to recall it being very clean and elegant how MSO Origins were treated like any other server in the generation, I think it would be nice to preserve that if possible.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451712887



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive

Review comment:
       I'll take your word for it. Removed the comment.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465310507



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +330,268 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	log.Errorf("DEBUG topo GetTopologyParentConfigLine calling getTopologyPlacement cg '" + server.Cachegroup + "'\n")
+	serverPlacement := getTopologyPlacement(tc.CacheGroupName(server.Cachegroup), topology, cacheGroups)
+	if !serverPlacement.InTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, servers, parentConfigParams, topology, serverPlacement.IsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(ds, serverParams, serverPlacement.IsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(ds, serverPlacement.IsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(ds, serverParams, serverPlacement.IsLastTier)
+	txt += getTopologyParentIsProxyStr(serverPlacement.IsLastTier)
+	txt += " # topology '" + ds.Topology + "'"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier {
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(ds ParentConfigDSTopLevel, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	return "true"
+}
+
+func getTopologyQueryString(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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, errors.New("DS " + string(ds.Name) + " 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] {
+		log.Errorln("DS " + string(ds.Name) + " 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
+	}
+
+	if parentCG == "" {
+		return nil, nil, errors.New("Server '" + server.HostName + "' DS " + string(ds.Name) + " topology '" + ds.Topology + "' cachegroup '" + server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	}
+
+	parentStrs := []string{}
+	secondaryParentStrs := []string{}
+	for _, sv := range servers {
+		if tc.CacheType(sv.Type) != tc.CacheTypeEdge && tc.CacheType(sv.Type) != tc.CacheTypeMid && sv.Type != tc.OriginTypeName {
+			continue // only consider edges, mids, and origins in the CacheGroup.
+		}
+		if !HasRequiredCapabilities(serverCapabilities[sv.ID], ds.RequiredCapabilities) {
+			continue
+		}
+		if sv.Cachegroup == parentCG {
+			parentStr := serverParentStr(sv, parentConfigParams)
+			if parentStr != "" { // will be empty if server is not_a_parent (possibly other reasons)
+				parentStrs = append(parentStrs, parentStr)
+			}
+		}
+		if sv.Cachegroup == secondaryParentCG {
+			secondaryParentStrs = append(secondaryParentStrs, serverParentStr(sv, parentConfigParams))
+		}

Review comment:
       I've been trying to log errors on every possible bad data the config gen can receive.
   I'd agree with doing the validation in TO, but also logging an error in the config gen. I'll make it do that




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451688568



##########
File path: lib/go-atscfg/hostingdotconfig.go
##########
@@ -74,3 +92,22 @@ func MakeHostingDotConfig(
 	text += strings.Join(lines, "")
 	return text
 }
+
+func hostingMakeDSTopologies(dses []tc.DeliveryServiceNullable, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {

Review comment:
       Not without a function to translate `RemapConfigDSData` to `DeliveryServiceNullable`, which would be even more code.
   
   The non-API types like `RemapConfigDSData` exist in order to reduce duplication and unnecessary queries in TO. I'd like to remove them and change all the `atscfg` funcs to take standard API objects, but I was waiting until the config files were removed from the TO API.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451712455



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,

Review comment:
       Removed




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465856430



##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -139,24 +140,109 @@ func GetConfigFile(prefix string, xmlId string) string {
 
 // topologyIncludesServer returns whether the given topology includes the given server
 func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
-	_, inTopology := serverTopologyTier(server, topology)
-	return inTopology
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			return true
+		}
+	}
+	return false
 }
 
-// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
-func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+// TopologyCacheTier is the position of a cache in the topology.
+// Note this is the cache tier itself, notwithstanding MSO. So for an MSO service,
+// Caches immediately before the origin are the TopologyCacheTierLast, even for MSO.
+type TopologyCacheTier string
+
+const (
+	TopologyCacheTierFirst   = TopologyCacheTier("first")
+	TopologyCacheTierInner   = TopologyCacheTier("inner")
+	TopologyCacheTierLast    = TopologyCacheTier("last")
+	TopologyCacheTierInvalid = TopologyCacheTier("")
+)
+
+// TopologyPlacement contains data about the placement of a server in a topology.
+type TopologyPlacement struct {
+	// InTopology is whether the server is in the topology at all.
+	InTopology bool
+	// IsLastTier is whether the server is the last tier in the topology.
+	// Note this is different for MSO vs non-MSO. For MSO, the last tier is the Origin. For non-MSO, the last tier is the last cache tier.
+	IsLastTier bool
+	// CacheTier is the position of the cache in the topology.
+	// Note this is whether the cache is the last cache, even if it has parents in the topology who are origins (MSO).
+	// Thus, it's possible for a server to be CacheTierLast and not IsLastTier.
+	CacheTier TopologyCacheTier
+}
+
+// getTopologyPlacement returns information about the cachegroup's placement in the topology.
+// - Whether the cachegroup is the last tier in the topology.
+// - Whether the cachegroup is in the topology at all.
+// - Whether it's the first, inner, or last cache tier before the Origin.
+func getTopologyPlacement(cacheGroup tc.CacheGroupName, topology tc.Topology, cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable) TopologyPlacement {
 	serverNode := tc.TopologyNode{}
-	for _, node := range topology.Nodes {
-		if node.Cachegroup == server.Cachegroup {
+	serverNodeIndex := -1
+	for nodeI, node := range topology.Nodes {
+		if node.Cachegroup == string(cacheGroup) {
 			serverNode = node
+			serverNodeIndex = nodeI
 			break
 		}
 	}
 	if serverNode.Cachegroup == "" {
-		return false, false
+		return TopologyPlacement{InTopology: false}
 	}
 
-	return len(serverNode.Parents) == 0, true
+	topologyHasChildren := false
+	for _, node := range topology.Nodes {
+		for _, parent := range node.Parents {
+			if parent == serverNodeIndex {
+				topologyHasChildren = true
+				break
+			}
+		}
+	}

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451710180



##########
File path: traffic_ops_ort/atstccfg/cfgfile/headerrewritedotconfig.go
##########
@@ -70,7 +70,7 @@ func GetConfigFileCDNHeaderRewrite(toData *config.TOData, fileName string) (stri
 		if server.CDNName != *tcDS.CDNName {
 			continue
 		}
-		if _, ok := dsServerIDs[server.ID]; !ok {
+		if _, ok := dsServerIDs[server.ID]; !ok && tcDS.Topology == nil {

Review comment:
       Probably, but yeah, not critical.
   
   Header rewrites are their own file, so the place to do that is the meta gen.
   
   The `assignedEdges` here are only used to calculate Max Origin Connections. The change here is just saying "if the DS is in a Topology, include the Edge in the Assigned count for MaxOriginConns."




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451689166



##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -134,3 +136,64 @@ const ConfigSuffix = ".config"
 func GetConfigFile(prefix string, xmlId string) string {
 	return prefix + xmlId + ConfigSuffix
 }
+
+// topologyIncludesServer returns whether the given topology includes the given server
+func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
+	_, inTopology := serverTopologyTier(server, topology)
+	return inTopology
+}
+
+// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
+func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {

Review comment:
       Maybe. I'll make it do whatever makes the most sense when I add those Header Rewrites to the generation. Not sure what that'll be until I start changing it.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-648959256


   Now that #4724 is merged, there's a `CHANGELOG.md` merge conflict


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465819697



##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -139,24 +140,109 @@ func GetConfigFile(prefix string, xmlId string) string {
 
 // topologyIncludesServer returns whether the given topology includes the given server
 func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
-	_, inTopology := serverTopologyTier(server, topology)
-	return inTopology
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			return true
+		}
+	}
+	return false
 }
 
-// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
-func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+// TopologyCacheTier is the position of a cache in the topology.
+// Note this is the cache tier itself, notwithstanding MSO. So for an MSO service,
+// Caches immediately before the origin are the TopologyCacheTierLast, even for MSO.
+type TopologyCacheTier string
+
+const (
+	TopologyCacheTierFirst   = TopologyCacheTier("first")
+	TopologyCacheTierInner   = TopologyCacheTier("inner")
+	TopologyCacheTierLast    = TopologyCacheTier("last")
+	TopologyCacheTierInvalid = TopologyCacheTier("")
+)
+
+// TopologyPlacement contains data about the placement of a server in a topology.
+type TopologyPlacement struct {
+	// InTopology is whether the server is in the topology at all.
+	InTopology bool
+	// IsLastTier is whether the server is the last tier in the topology.
+	// Note this is different for MSO vs non-MSO. For MSO, the last tier is the Origin. For non-MSO, the last tier is the last cache tier.
+	IsLastTier bool
+	// CacheTier is the position of the cache in the topology.
+	// Note this is whether the cache is the last cache, even if it has parents in the topology who are origins (MSO).
+	// Thus, it's possible for a server to be CacheTierLast and not IsLastTier.
+	CacheTier TopologyCacheTier
+}
+
+// getTopologyPlacement returns information about the cachegroup's placement in the topology.
+// - Whether the cachegroup is the last tier in the topology.
+// - Whether the cachegroup is in the topology at all.
+// - Whether it's the first, inner, or last cache tier before the Origin.
+func getTopologyPlacement(cacheGroup tc.CacheGroupName, topology tc.Topology, cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable) TopologyPlacement {
 	serverNode := tc.TopologyNode{}
-	for _, node := range topology.Nodes {
-		if node.Cachegroup == server.Cachegroup {
+	serverNodeIndex := -1
+	for nodeI, node := range topology.Nodes {
+		if node.Cachegroup == string(cacheGroup) {
 			serverNode = node
+			serverNodeIndex = nodeI
 			break
 		}
 	}
 	if serverNode.Cachegroup == "" {
-		return false, false
+		return TopologyPlacement{InTopology: false}
 	}
 
-	return len(serverNode.Parents) == 0, true
+	topologyHasChildren := false
+	for _, node := range topology.Nodes {
+		for _, parent := range node.Parents {
+			if parent == serverNodeIndex {
+				topologyHasChildren = true
+				break
+			}
+		}
+	}

Review comment:
       Yeah, it's not a bug, just less efficient. I'll fix.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465618519



##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -139,24 +140,109 @@ func GetConfigFile(prefix string, xmlId string) string {
 
 // topologyIncludesServer returns whether the given topology includes the given server
 func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
-	_, inTopology := serverTopologyTier(server, topology)
-	return inTopology
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			return true
+		}
+	}
+	return false
 }
 
-// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
-func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+// TopologyCacheTier is the position of a cache in the topology.
+// Note this is the cache tier itself, notwithstanding MSO. So for an MSO service,
+// Caches immediately before the origin are the TopologyCacheTierLast, even for MSO.
+type TopologyCacheTier string
+
+const (
+	TopologyCacheTierFirst   = TopologyCacheTier("first")
+	TopologyCacheTierInner   = TopologyCacheTier("inner")
+	TopologyCacheTierLast    = TopologyCacheTier("last")
+	TopologyCacheTierInvalid = TopologyCacheTier("")
+)
+
+// TopologyPlacement contains data about the placement of a server in a topology.
+type TopologyPlacement struct {
+	// InTopology is whether the server is in the topology at all.
+	InTopology bool
+	// IsLastTier is whether the server is the last tier in the topology.
+	// Note this is different for MSO vs non-MSO. For MSO, the last tier is the Origin. For non-MSO, the last tier is the last cache tier.
+	IsLastTier bool
+	// CacheTier is the position of the cache in the topology.
+	// Note this is whether the cache is the last cache, even if it has parents in the topology who are origins (MSO).
+	// Thus, it's possible for a server to be CacheTierLast and not IsLastTier.
+	CacheTier TopologyCacheTier
+}
+
+// getTopologyPlacement returns information about the cachegroup's placement in the topology.
+// - Whether the cachegroup is the last tier in the topology.
+// - Whether the cachegroup is in the topology at all.
+// - Whether it's the first, inner, or last cache tier before the Origin.
+func getTopologyPlacement(cacheGroup tc.CacheGroupName, topology tc.Topology, cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable) TopologyPlacement {
 	serverNode := tc.TopologyNode{}
-	for _, node := range topology.Nodes {
-		if node.Cachegroup == server.Cachegroup {
+	serverNodeIndex := -1
+	for nodeI, node := range topology.Nodes {
+		if node.Cachegroup == string(cacheGroup) {
 			serverNode = node
+			serverNodeIndex = nodeI
 			break
 		}
 	}
 	if serverNode.Cachegroup == "" {
-		return false, false
+		return TopologyPlacement{InTopology: false}
 	}
 
-	return len(serverNode.Parents) == 0, true
+	topologyHasChildren := false
+	for _, node := range topology.Nodes {
+		for _, parent := range node.Parents {
+			if parent == serverNodeIndex {
+				topologyHasChildren = true
+				break
+			}
+		}
+	}

Review comment:
       Should break out of the outer loop if `topologyHasChildren` is true

##########
File path: lib/go-atscfg/atscfg.go
##########
@@ -139,24 +140,109 @@ func GetConfigFile(prefix string, xmlId string) string {
 
 // topologyIncludesServer returns whether the given topology includes the given server
 func topologyIncludesServer(topology tc.Topology, server tc.Server) bool {
-	_, inTopology := serverTopologyTier(server, topology)
-	return inTopology
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			return true
+		}
+	}
+	return false
 }
 
-// serverTopologyTier returns whether the server is the last tier in the topology, and whether the server is in the topology at all.
-func serverTopologyTier(server tc.Server, topology tc.Topology) (bool, bool) {
+// TopologyCacheTier is the position of a cache in the topology.
+// Note this is the cache tier itself, notwithstanding MSO. So for an MSO service,
+// Caches immediately before the origin are the TopologyCacheTierLast, even for MSO.
+type TopologyCacheTier string
+
+const (
+	TopologyCacheTierFirst   = TopologyCacheTier("first")
+	TopologyCacheTierInner   = TopologyCacheTier("inner")
+	TopologyCacheTierLast    = TopologyCacheTier("last")
+	TopologyCacheTierInvalid = TopologyCacheTier("")
+)
+
+// TopologyPlacement contains data about the placement of a server in a topology.
+type TopologyPlacement struct {
+	// InTopology is whether the server is in the topology at all.
+	InTopology bool
+	// IsLastTier is whether the server is the last tier in the topology.
+	// Note this is different for MSO vs non-MSO. For MSO, the last tier is the Origin. For non-MSO, the last tier is the last cache tier.
+	IsLastTier bool
+	// CacheTier is the position of the cache in the topology.
+	// Note this is whether the cache is the last cache, even if it has parents in the topology who are origins (MSO).
+	// Thus, it's possible for a server to be CacheTierLast and not IsLastTier.
+	CacheTier TopologyCacheTier
+}
+
+// getTopologyPlacement returns information about the cachegroup's placement in the topology.
+// - Whether the cachegroup is the last tier in the topology.
+// - Whether the cachegroup is in the topology at all.
+// - Whether it's the first, inner, or last cache tier before the Origin.
+func getTopologyPlacement(cacheGroup tc.CacheGroupName, topology tc.Topology, cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable) TopologyPlacement {
 	serverNode := tc.TopologyNode{}
-	for _, node := range topology.Nodes {
-		if node.Cachegroup == server.Cachegroup {
+	serverNodeIndex := -1
+	for nodeI, node := range topology.Nodes {
+		if node.Cachegroup == string(cacheGroup) {
 			serverNode = node
+			serverNodeIndex = nodeI
 			break
 		}
 	}
 	if serverNode.Cachegroup == "" {
-		return false, false
+		return TopologyPlacement{InTopology: false}
 	}
 
-	return len(serverNode.Parents) == 0, true
+	topologyHasChildren := false

Review comment:
       A name like `topologyNodeHasChildren` or `nodeHasChildren` might make more sense here

##########
File path: lib/go-atscfg/topologyheaderrewritedotconfig.go
##########
@@ -0,0 +1,140 @@
+package atscfg
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"math"
+	"strconv"
+
+	"github.com/apache/trafficcontrol/lib/go-log"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+)
+
+const HeaderRewriteFirstPrefix = HeaderRewritePrefix + "first_"
+const HeaderRewriteInnerPrefix = HeaderRewritePrefix + "inner_"
+const HeaderRewriteLastPrefix = HeaderRewritePrefix + "last_"
+
+func FirstHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteFirstPrefix + dsName + ConfigSuffix
+}
+
+func InnerHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteInnerPrefix + dsName + ConfigSuffix
+}
+
+func LastHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteLastPrefix + dsName + ConfigSuffix
+}
+
+func MakeTopologyHeaderRewriteDotConfig(
+	server tc.Server,
+	toToolName string, // tm.toolname global parameter (TODO: cache itself?)
+	toURL string, // tm.url global parameter (TODO: cache itself?)
+	ds tc.DeliveryServiceNullable,
+	topologies []tc.Topology,
+	cacheGroupArr []tc.CacheGroupNullable,
+	servers []tc.Server,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	dsRequiredCapabilities map[ServerCapability]struct{},
+) string {
+	text := GenericHeaderComment(server.HostName, toToolName, toURL)
+
+	cacheGroups := MakeCGMap(cacheGroupArr)
+
+	if ds.Topology == nil || *ds.Topology == "" {
+		log.Errorln("Config generation: Topology Header Rewrite called for DS '" + *ds.XMLID + "' with no Topology! This should never be called, a DS with no topology should never have a First, Inner, or Last Header Rewrite config in the list of config files! Returning blank config!")
+		return text
+	}
+
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == *ds.Topology {
+			topology = to
+			break
+		}
+	}

Review comment:
       Another place where a *topologies by DS name* map would be useful (though it doesn't make sense to build it here).

##########
File path: traffic_ops_ort/atstccfg/config/config.go
##########
@@ -297,4 +297,8 @@ type TOData struct {
 
 	// SSLKeys must be all the ssl keys for the server's cdn.
 	SSLKeys []tc.CDNSSLKeys
+
+	// Topologies must be all the topologies for the server's cdn.
+	// May incude topologies of other cdns.
+	Topologies []tc.Topology

Review comment:
       Is there any advantage to having topologies not assigned to any DS? If not, including a *topologies by DS name* map in `TOData` instead of a topology array might make more sense here, since there are several places where we need to get a topology, given a certain DS.

##########
File path: lib/go-atscfg/meta.go
##########
@@ -146,24 +151,64 @@ func AddMetaObjConfigDir(
 			log.Errorln("meta config generation got Delivery Service with nil XMLID - not considering!")
 			continue
 		}
+
 		err := error(nil)
 		// Note we log errors, but don't return them.
 		// If an individual DS has an error, we don't want to break the rest of the CDN.
-		if (ds.EdgeHeaderRewrite != nil || ds.MaxOriginConnections != nil) &&
-			strings.HasPrefix(server.Type, tc.EdgeTypePrefix) {
-			fileName := "hdr_rw_" + *ds.XMLID + ".config"
-			scope := tc.ATSConfigMetaDataConfigFileScopeCDNs
-			if configFilesM, err = ensureConfigFile(configFilesM, fileName, configDir, scope); err != nil {
-				log.Errorln("meta config generation: " + err.Error())
+		if ds.Topology != nil && *ds.Topology != "" {
+			topology := tc.Topology{}
+			for _, to := range topologies {
+				if to.Name == *ds.Topology {
+					topology = to
+					break
+				}
 			}

Review comment:
       We have a `tc.DeliveryServiceNullable` array and a `tc.Topology` array in this scope, consider calling `atscfg.hostingMakeDSTopologies()` to get a  *topologies by DS name* map so that running time is *number of DSes + number of Topologies* instead of *number of DSes \* number of Topologies* like it currently is.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rawlinp commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rawlinp commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-648403463


   With the merging of https://github.com/apache/trafficcontrol/pull/4807, would you like to account for `firstHeaderRewrite`, `innerHeaderRewrite`, and `lastHeaderRewrite` in this PR as well? I figure it might be easier to work those into this PR than tack them on later, but really it's up to you. It looks like it basically ignores `edgeHeaderRewrite` and `midHeaderRewrite` if the DS is assigned to a Topology which is good, but now we can consider those new fields for the Topology case.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451702918



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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] {

Review comment:
       I'm not understanding why. We dereference that particular node, the parent, `topology.Nodes[svNode.Parents[0]]` further down. We never dereference `topology.Nodes[svNode.Parents[0]+1]`, and I'm not seeing why we would.
   
   Can you elaborate?




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-668855041


   Ah, the code was right, just the latest Capability changes broke the tests, needed more data. 
   Fixed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465879335



##########
File path: lib/go-atscfg/topologyheaderrewritedotconfig.go
##########
@@ -0,0 +1,140 @@
+package atscfg
+
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import (
+	"math"
+	"strconv"
+
+	"github.com/apache/trafficcontrol/lib/go-log"
+	"github.com/apache/trafficcontrol/lib/go-tc"
+)
+
+const HeaderRewriteFirstPrefix = HeaderRewritePrefix + "first_"
+const HeaderRewriteInnerPrefix = HeaderRewritePrefix + "inner_"
+const HeaderRewriteLastPrefix = HeaderRewritePrefix + "last_"
+
+func FirstHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteFirstPrefix + dsName + ConfigSuffix
+}
+
+func InnerHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteInnerPrefix + dsName + ConfigSuffix
+}
+
+func LastHeaderRewriteConfigFileName(dsName string) string {
+	return HeaderRewriteLastPrefix + dsName + ConfigSuffix
+}
+
+func MakeTopologyHeaderRewriteDotConfig(
+	server tc.Server,
+	toToolName string, // tm.toolname global parameter (TODO: cache itself?)
+	toURL string, // tm.url global parameter (TODO: cache itself?)
+	ds tc.DeliveryServiceNullable,
+	topologies []tc.Topology,
+	cacheGroupArr []tc.CacheGroupNullable,
+	servers []tc.Server,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	dsRequiredCapabilities map[ServerCapability]struct{},
+) string {
+	text := GenericHeaderComment(server.HostName, toToolName, toURL)
+
+	cacheGroups := MakeCGMap(cacheGroupArr)
+
+	if ds.Topology == nil || *ds.Topology == "" {
+		log.Errorln("Config generation: Topology Header Rewrite called for DS '" + *ds.XMLID + "' with no Topology! This should never be called, a DS with no topology should never have a First, Inner, or Last Header Rewrite config in the list of config files! Returning blank config!")
+		return text
+	}
+
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == *ds.Topology {
+			topology = to
+			break
+		}
+	}

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465879289



##########
File path: lib/go-atscfg/meta.go
##########
@@ -146,24 +151,64 @@ func AddMetaObjConfigDir(
 			log.Errorln("meta config generation got Delivery Service with nil XMLID - not considering!")
 			continue
 		}
+
 		err := error(nil)
 		// Note we log errors, but don't return them.
 		// If an individual DS has an error, we don't want to break the rest of the CDN.
-		if (ds.EdgeHeaderRewrite != nil || ds.MaxOriginConnections != nil) &&
-			strings.HasPrefix(server.Type, tc.EdgeTypePrefix) {
-			fileName := "hdr_rw_" + *ds.XMLID + ".config"
-			scope := tc.ATSConfigMetaDataConfigFileScopeCDNs
-			if configFilesM, err = ensureConfigFile(configFilesM, fileName, configDir, scope); err != nil {
-				log.Errorln("meta config generation: " + err.Error())
+		if ds.Topology != nil && *ds.Topology != "" {
+			topology := tc.Topology{}
+			for _, to := range topologies {
+				if to.Name == *ds.Topology {
+					topology = to
+					break
+				}
 			}

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465822582



##########
File path: lib/go-atscfg/meta.go
##########
@@ -146,24 +151,64 @@ func AddMetaObjConfigDir(
 			log.Errorln("meta config generation got Delivery Service with nil XMLID - not considering!")
 			continue
 		}
+
 		err := error(nil)
 		// Note we log errors, but don't return them.
 		// If an individual DS has an error, we don't want to break the rest of the CDN.
-		if (ds.EdgeHeaderRewrite != nil || ds.MaxOriginConnections != nil) &&
-			strings.HasPrefix(server.Type, tc.EdgeTypePrefix) {
-			fileName := "hdr_rw_" + *ds.XMLID + ".config"
-			scope := tc.ATSConfigMetaDataConfigFileScopeCDNs
-			if configFilesM, err = ensureConfigFile(configFilesM, fileName, configDir, scope); err != nil {
-				log.Errorln("meta config generation: " + err.Error())
+		if ds.Topology != nil && *ds.Topology != "" {
+			topology := tc.Topology{}
+			for _, to := range topologies {
+				if to.Name == *ds.Topology {
+					topology = to
+					break
+				}
 			}

Review comment:
       I went back and forth whether to use a map for these. The performance doesn't matter, unless someone had on the order of a million topologies, the network cost vastly outweighs this. But I don't really object, I'll change them




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451711819



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"

Review comment:
       Done.
   I originally added those for debugging, but I agree, they should be helpful to Ops.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-668855041


   >atscfg.TestMakeParentDotConfigTopologies fails:
   
   Ah, the code was right, just the latest Capability changes broke the tests, needed more data. 
   Fixed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465656652



##########
File path: traffic_ops_ort/atstccfg/config/config.go
##########
@@ -297,4 +297,8 @@ type TOData struct {
 
 	// SSLKeys must be all the ssl keys for the server's cdn.
 	SSLKeys []tc.CDNSSLKeys
+
+	// Topologies must be all the topologies for the server's cdn.
+	// May incude topologies of other cdns.
+	Topologies []tc.Topology

Review comment:
       Is there any advantage to having topologies not assigned to any DS? If not, including a *topologies by DS name* map in `TOData` instead of a topology array might make more sense, since there are several places where we need to get a topology, given a certain DS.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451694186



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -162,6 +165,24 @@ type OriginURI struct {
 	Port   string
 }
 
+/*

Review comment:
       Yep, accidentally left in. Removed.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r464691507



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +330,268 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	log.Errorf("DEBUG topo GetTopologyParentConfigLine calling getTopologyPlacement cg '" + server.Cachegroup + "'\n")
+	serverPlacement := getTopologyPlacement(tc.CacheGroupName(server.Cachegroup), topology, cacheGroups)
+	if !serverPlacement.InTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, servers, parentConfigParams, topology, serverPlacement.IsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(ds, serverParams, serverPlacement.IsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(ds, serverPlacement.IsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(ds, serverParams, serverPlacement.IsLastTier)
+	txt += getTopologyParentIsProxyStr(serverPlacement.IsLastTier)
+	txt += " # topology '" + ds.Topology + "'"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier {
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(ds ParentConfigDSTopLevel, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	return "true"
+}
+
+func getTopologyQueryString(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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, errors.New("DS " + string(ds.Name) + " 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] {
+		log.Errorln("DS " + string(ds.Name) + " 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
+	}
+
+	if parentCG == "" {
+		return nil, nil, errors.New("Server '" + server.HostName + "' DS " + string(ds.Name) + " topology '" + ds.Topology + "' cachegroup '" + server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	}
+
+	parentStrs := []string{}
+	secondaryParentStrs := []string{}
+	for _, sv := range servers {
+		if tc.CacheType(sv.Type) != tc.CacheTypeEdge && tc.CacheType(sv.Type) != tc.CacheTypeMid && sv.Type != tc.OriginTypeName {
+			continue // only consider edges, mids, and origins in the CacheGroup.
+		}
+		if !HasRequiredCapabilities(serverCapabilities[sv.ID], ds.RequiredCapabilities) {
+			continue
+		}
+		if sv.Cachegroup == parentCG {
+			parentStr := serverParentStr(sv, parentConfigParams)
+			if parentStr != "" { // will be empty if server is not_a_parent (possibly other reasons)
+				parentStrs = append(parentStrs, parentStr)
+			}
+		}
+		if sv.Cachegroup == secondaryParentCG {
+			secondaryParentStrs = append(secondaryParentStrs, serverParentStr(sv, parentConfigParams))
+		}

Review comment:
       On second thought, maybe we should perform validation to make sure a topology node's cachegroup contains at least 1 server.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c edited a comment on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c edited a comment on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-668855041


   >atscfg.TestMakeParentDotConfigTopologies fails:
   Ah, the code was right, just the latest Capability changes broke the tests, needed more data. 
   Fixed.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r452606961



##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -166,14 +188,20 @@ func GetServerConfigRemapDotConfigForEdge(
 	cacheURLConfigParams map[string]string,
 	profilesCacheKeyConfigParams map[int]map[string]string,
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
-	server *ServerInfo,
+	serverInfo *ServerInfo,
 	dses []RemapConfigDSData,
 	atsMajorVersion int,
 	header string,
+	server tc.Server,
+	topologies map[tc.DeliveryServiceName]tc.Topology,
 ) string {
 	textLines := []string{}
 
 	for _, ds := range dses {
+		topology, hasTopology := topologies[tc.DeliveryServiceName(ds.Name)]
+		if hasTopology && !topologyIncludesServer(topology, server) {

Review comment:
       Done

##########
File path: lib/go-atscfg/remapdotconfig.go
##########
@@ -74,28 +75,49 @@ func MakeRemapDotConfig(
 	serverPackageParamData map[string]string, // map[paramName]paramVal for this server, config file 'package'
 	serverInfo *ServerInfo, // ServerInfo for this server
 	remapDSData []RemapConfigDSData,
+	topologies []tc.Topology,
 ) string {
-	hdr := GenericHeaderComment(string(serverName), toToolName, toURL)
-	text := ""
+	dsTopologies := makeDSTopologies(remapDSData, topologies)
+	hdr := GenericHeaderComment(server.HostName, toToolName, toURL)
 	if tc.CacheTypeFromString(serverInfo.Type) == tc.CacheTypeMid {
-		text = GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr)
-	} else {
-		text = GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr)
+		return GetServerConfigRemapDotConfigForMid(atsMajorVersion, dsProfilesCacheKeyConfigParams, serverInfo, remapDSData, hdr, server, dsTopologies)
 	}
-	return text
+	return GetServerConfigRemapDotConfigForEdge(cacheURLConfigParams, dsProfilesCacheKeyConfigParams, serverPackageParamData, serverInfo, remapDSData, atsMajorVersion, hdr, server, dsTopologies)
+}
+
+func makeDSTopologies(dses []RemapConfigDSData, topologies []tc.Topology) map[tc.DeliveryServiceName]tc.Topology {
+	dsTops := map[tc.DeliveryServiceName]tc.Topology{}
+	topNames := map[string]tc.Topology{}
+	for _, to := range topologies {
+		topNames[to.Name] = to
+	}
+	for _, ds := range dses {
+		if to, ok := topNames[ds.Topology]; ok {
+			dsTops[tc.DeliveryServiceName(ds.Name)] = to
+		} else if ds.Topology != "" {
+			log.Errorln("Making remap.config for Delivery Service '" + ds.Name + "': has topology '" + ds.Topology + "', but that topology doesn't exist! Treating as if DS has no Topology!")
+		}
+	}
+	return dsTops
 }
 
 func GetServerConfigRemapDotConfigForMid(
 	atsMajorVersion int,
 	profilesCacheKeyConfigParams map[int]map[string]string,
-	server *ServerInfo,
+	serverInfo *ServerInfo,
 	dses []RemapConfigDSData,
 	header string,
+	server tc.Server,
+	topologies map[tc.DeliveryServiceName]tc.Topology,
 ) string {
 	midRemaps := map[string]string{}
 	for _, ds := range dses {
-		if ds.Type.IsLive() && !ds.Type.IsNational() {
-			continue // Live local delivery services skip mids
+		topology, hasTopology := topologies[tc.DeliveryServiceName(ds.Name)]
+		if hasTopology && !topologyIncludesServer(topology, server) {

Review comment:
       Done




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r451798876



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +345,270 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	serverIsLastTier, serverInTopology := serverTopologyTier(server, topology)
+	if !serverInTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO omit server and parents without necessary Capabilities
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, serverParams, servers, parentConfigParams, topology, serverIsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(server, ds, topology, serverParams, serverIsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(server, ds, topology, serverIsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(server, ds, topology, serverParams, serverIsLastTier)
+	txt += getTopologyParentIsProxyStr(serverIsLastTier)
+	txt += " # topology"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier { // && ds.MultiSiteOrigin
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		// TODO make sure this is correct for DSTypeHTTPNoCache || DSTypeHTTPLive || DSTypeDNSLive
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	// TODO make sure this is correct. If we're the last tier, we should go direct to the origin, right?
+	return "true"
+}
+
+func getTopologyQueryString(server tc.Server, ds ParentConfigDSTopLevel, topology tc.Topology, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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] {

Review comment:
       Ah, yes, the panic. I'll fix.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-662725805


   Yep, Max Origin Conns is in there, on the LastHeaderRewrite: https://github.com/apache/trafficcontrol/pull/4790/files#diff-4943b4d6fbf18539076e8631e37f740cR98


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
mitchell852 commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-662722944


   @rob05c - does this PR add the value of ds.max_origin_conns to header_rewrite_last_dsname.config


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] zrhoffman commented on pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
zrhoffman commented on pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#issuecomment-668840690


   `atscfg.TestMakeParentDotConfigTopologies` fails:
   
   ```go
   --- FAIL: TestMakeParentDotConfigTopologies (0.00s)
       parentdotconfig_test.go:488: expected parent 'dest_domain=ds0.example.net', actual: '# DO NOT EDIT - Generated for myserver by myToolName (https://myto.example.net) on Tue Aug 4 21:44:15 UTC 2020
           dest_domain=ds0.example.net port=80 parent=""  round_robin=consistent_hash go_direct=false qstring=myQStringHandlingParam
           dest_domain=. parent="" round_robin=consistent_hash go_direct=false qstring=myQstringParam
           '
   --- FAIL: TestMakeParentDotConfigTopologiesCapabilities (0.00s)
       parentdotconfig_test.go:721: expected parent 'dest_domain=ds0.example.net' without required capabilities: '# DO NOT EDIT - Generated for myserver by myToolName (https://myto.example.net) on Tue Aug 4 21:44:15 UTC 2020
           dest_domain=. parent="" round_robin=consistent_hash go_direct=false qstring=myQstringParam
           '
       parentdotconfig_test.go:724: expected parent 'dest_domain=ds1.example.net' with necessary required capabilities, actual: '# DO NOT EDIT - Generated for myserver by myToolName (https://myto.example.net) on Tue Aug 4 21:44:15 UTC 2020
           dest_domain=. parent="" round_robin=consistent_hash go_direct=false qstring=myQstringParam
           '
   FAIL
   exit status 1
   FAIL    github.com/apache/trafficcontrol/lib/go-atscfg  0.007s
   ```


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] mitchell852 merged pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
mitchell852 merged pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



[GitHub] [trafficcontrol] rob05c commented on a change in pull request #4790: Add Topology ATS Config Generation

Posted by GitBox <gi...@apache.org>.
rob05c commented on a change in pull request #4790:
URL: https://github.com/apache/trafficcontrol/pull/4790#discussion_r465312296



##########
File path: lib/go-atscfg/parentdotconfig.go
##########
@@ -331,13 +330,268 @@ func MakeParentDotConfig(
 			defaultDestText += ` qstring=` + qStr
 		}
 		defaultDestText += "\n"
-
-		sort.Sort(sort.StringSlice(textArr))
-		text = hdr + strings.Join(textArr, "") + defaultDestText
 	}
+
+	sort.Sort(sort.StringSlice(textArr))
+	text := hdr + strings.Join(textArr, "") + defaultDestText
 	return text
 }
 
+func GetTopologyParentConfigLine(
+	server tc.Server,
+	servers []tc.Server,
+	ds ParentConfigDSTopLevel,
+	serverParams map[string]string,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.config
+	topologies []tc.Topology,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+	cacheGroups map[tc.CacheGroupName]tc.CacheGroupNullable,
+) (string, error) {
+	txt := ""
+
+	if !HasRequiredCapabilities(serverCapabilities[server.ID], ds.RequiredCapabilities) {
+		return "", nil
+	}
+
+	orgURI, err := GetOriginURI(ds.OriginFQDN)
+	if err != nil {
+		return "", errors.New("Malformed ds '" + string(ds.Name) + "' origin  URI: '" + ds.OriginFQDN + "': skipping!" + err.Error())
+	}
+
+	// This could be put in a map beforehand to only iterate once, if performance mattered
+	topology := tc.Topology{}
+	for _, to := range topologies {
+		if to.Name == ds.Topology {
+			topology = to
+			break
+		}
+	}
+	if topology.Name == "" {
+		return "", errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' not found in Topologies!")
+	}
+
+	txt += "dest_domain=" + orgURI.Hostname() + " port=" + orgURI.Port()
+
+	log.Errorf("DEBUG topo GetTopologyParentConfigLine calling getTopologyPlacement cg '" + server.Cachegroup + "'\n")
+	serverPlacement := getTopologyPlacement(tc.CacheGroupName(server.Cachegroup), topology, cacheGroups)
+	if !serverPlacement.InTopology {
+		return "", nil // server isn't in topology, no error
+	}
+	// TODO add Topology/Capabilities to remap.config
+
+	parents, secondaryParents, err := GetTopologyParents(server, ds, servers, parentConfigParams, topology, serverPlacement.IsLastTier, serverCapabilities)
+	if err != nil {
+		return "", errors.New("getting topology parents for '" + string(ds.Name) + "': skipping! " + err.Error())
+	}
+	txt += ` parent="` + strings.Join(parents, `;`) + `"`
+	if len(secondaryParents) > 0 {
+		txt += ` secondary_parent="` + strings.Join(secondaryParents, `;`) + `"`
+	}
+	txt += ` round_robin=` + getTopologyRoundRobin(ds, serverParams, serverPlacement.IsLastTier)
+	txt += ` go_direct=` + getTopologyGoDirect(ds, serverPlacement.IsLastTier)
+	txt += ` qstring=` + getTopologyQueryString(ds, serverParams, serverPlacement.IsLastTier)
+	txt += getTopologyParentIsProxyStr(serverPlacement.IsLastTier)
+	txt += " # topology '" + ds.Topology + "'"
+	txt += "\n"
+	return txt, nil
+}
+
+func getTopologyParentIsProxyStr(serverIsLastTier bool) string {
+	if serverIsLastTier {
+		return ` parent_is_proxy=false`
+	}
+	return ""
+}
+
+func getTopologyRoundRobin(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	roundRobinConsistentHash := "consistent_hash"
+	if !serverIsLastTier {
+		return roundRobinConsistentHash
+	}
+	if parentSelectAlg := serverParams[ParentConfigParamAlgorithm]; ds.OriginShield != "" && strings.TrimSpace(parentSelectAlg) != "" {
+		return parentSelectAlg
+	}
+	if ds.MultiSiteOrigin {
+		return ds.MSOAlgorithm
+	}
+	return roundRobinConsistentHash
+}
+
+func getTopologyGoDirect(ds ParentConfigDSTopLevel, serverIsLastTier bool) string {
+	if !serverIsLastTier {
+		return "false"
+	}
+	if ds.OriginShield != "" {
+		return "true"
+	}
+	if ds.MultiSiteOrigin {
+		return "false"
+	}
+	return "true"
+}
+
+func getTopologyQueryString(ds ParentConfigDSTopLevel, serverParams map[string]string, serverIsLastTier bool) string {
+	if serverIsLastTier {
+		if ds.MultiSiteOrigin && ds.QStringHandling == "" && ds.MSOAlgorithm == tc.AlgorithmConsistentHash && ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+			return "consider"
+		}
+		return "ignore"
+	}
+
+	if param := serverParams[ParentConfigParamQStringHandling]; param != "" {
+		return param
+	}
+	if ds.QStringHandling != "" {
+		return ds.QStringHandling
+	}
+	if ds.QStringIgnore == tc.QStringIgnoreUseInCacheKeyAndPassUp {
+		return "consider"
+	}
+	return "ignore"
+}
+
+// serverParentageParams gets the Parameters used for parent= line, or defaults if they don't exist
+// Returns the Parameters used for parent= lines, for the given server.
+func serverParentageParams(sv tc.Server, params []ParameterWithProfilesMap) ProfileCache {
+	// TODO deduplicate with atstccfg/parentdotconfig.go
+	profileCache := DefaultProfileCache()
+	profileCache.Port = sv.TCPPort
+	for _, param := range params {
+		if _, ok := param.ProfileNames[sv.Profile]; !ok {
+			continue
+		}
+		switch param.Name {
+		case ParentConfigCacheParamWeight:
+			profileCache.Weight = param.Value
+		case ParentConfigCacheParamPort:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: port param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Port = int(i)
+			}
+		case ParentConfigCacheParamUseIP:
+			profileCache.UseIP = param.Value == "1"
+		case ParentConfigCacheParamRank:
+			i, err := strconv.ParseInt(param.Value, 10, 64)
+			if err != nil {
+				log.Errorln("parent.config generation: rank param is not an integer, skipping! : " + err.Error())
+			} else {
+				profileCache.Rank = int(i)
+			}
+		case ParentConfigCacheParamNotAParent:
+			profileCache.NotAParent = param.Value != "false"
+		}
+	}
+	return profileCache
+}
+
+func serverParentStr(sv tc.Server, params []ParameterWithProfilesMap) string {
+	svParams := serverParentageParams(sv, params)
+	if svParams.NotAParent {
+		return ""
+	}
+	host := ""
+	if svParams.UseIP {
+		host = sv.IPAddress
+	} else {
+		host = sv.HostName + "." + sv.DomainName
+	}
+	return host + ":" + strconv.Itoa(svParams.Port) + "|" + svParams.Weight
+}
+
+func GetTopologyParents(
+	server tc.Server,
+	ds ParentConfigDSTopLevel,
+	servers []tc.Server,
+	parentConfigParams []ParameterWithProfilesMap, // all params with configFile parent.confign
+	topology tc.Topology,
+	serverIsLastTier bool,
+	serverCapabilities map[int]map[ServerCapability]struct{},
+) ([]string, []string, error) {
+	// 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, err := GetOriginURI(ds.OriginFQDN) // TODO pass, instead of calling again
+		if err != nil {
+			return nil, nil, err
+		}
+		return []string{orgURI.Host}, nil, nil
+	}
+
+	svNode := tc.TopologyNode{}
+	for _, node := range topology.Nodes {
+		if node.Cachegroup == server.Cachegroup {
+			svNode = node
+			break
+		}
+	}
+	if svNode.Cachegroup == "" {
+		return nil, nil, errors.New("This server '" + server.HostName + "' not in DS " + string(ds.Name) + " topology, skipping")
+	}
+
+	if len(svNode.Parents) == 0 {
+		return nil, nil, errors.New("DS " + string(ds.Name) + " topology '" + ds.Topology + "' is last tier, but NonLastTier called! Should never happen")
+	}
+	if numParents := len(svNode.Parents); numParents > 2 {
+		log.Errorln("DS " + string(ds.Name) + " 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, errors.New("DS " + string(ds.Name) + " 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] {
+		log.Errorln("DS " + string(ds.Name) + " 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
+	}
+
+	if parentCG == "" {
+		return nil, nil, errors.New("Server '" + server.HostName + "' DS " + string(ds.Name) + " topology '" + ds.Topology + "' cachegroup '" + server.Cachegroup + "' topology node parent " + strconv.Itoa(svNode.Parents[0]) + " is not in the topology!")
+	}
+
+	parentStrs := []string{}
+	secondaryParentStrs := []string{}
+	for _, sv := range servers {
+		if tc.CacheType(sv.Type) != tc.CacheTypeEdge && tc.CacheType(sv.Type) != tc.CacheTypeMid && sv.Type != tc.OriginTypeName {
+			continue // only consider edges, mids, and origins in the CacheGroup.
+		}
+		if !HasRequiredCapabilities(serverCapabilities[sv.ID], ds.RequiredCapabilities) {
+			continue
+		}
+		if sv.Cachegroup == parentCG {
+			parentStr := serverParentStr(sv, parentConfigParams)
+			if parentStr != "" { // will be empty if server is not_a_parent (possibly other reasons)
+				parentStrs = append(parentStrs, parentStr)
+			}
+		}
+		if sv.Cachegroup == secondaryParentCG {
+			secondaryParentStrs = append(secondaryParentStrs, serverParentStr(sv, parentConfigParams))
+		}

Review comment:
       Ok, I made it log the error




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org