Add Topology ATS Config Generation by rob05c · Pull Request #4790 · apache/trafficcontrol

rob05c · 2020-06-12T20:30:35Z

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.

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

This PR includes tests OR I have explained why tests are unnecessary
This PR includes documentation OR I have explained why documentation is unnecessary
This PR includes an update to CHANGELOG.md OR such an update is not necessary
This PR includes any and all required license headers
This PR ensures that database migration sequence is correct OR this PR does not include a database migration
This PR DOES NOT FIX A SERIOUS SECURITY VULNERABILITY (see the Apache Software Foundation's security guidelines for details)

Additional Information

rawlinp · 2020-06-23T20:35:45Z

With the merging of #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.

zrhoffman · 2020-06-24T17:29:56Z

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

rawlinp · 2020-06-24T20:42:32Z

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.

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.

rawlinp · 2020-06-24T20:58:38Z

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?

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.

rawlinp · 2020-06-24T21:01:12Z

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.

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.

rawlinp · 2020-06-24T21:13:43Z

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

Yep, accidentally left in. Removed.

rawlinp · 2020-06-24T21:44:39Z

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.

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.

rawlinp · 2020-06-24T23:19:56Z

serverInfo is now unused

rawlinp · 2020-06-24T23:30:51Z

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

rawlinp · 2020-06-24T23:31:29Z

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

rawlinp · 2020-06-24T23:32:22Z

This comment should include the topology name as well

rawlinp · 2020-06-24T23:52:37Z

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.

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."

mitchell852 · 2020-07-22T22:10:16Z

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

rob05c · 2020-07-22T22:18:32Z

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

zrhoffman

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.

zrhoffman · 2020-07-31T23:20:49Z

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?

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

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

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

Ok, I made it log the error

Adds ORT location param inference. If location Parameters do not exist the files are created and added anyway with the directory determined from the local ATS install, for: - cache.config - hosting.config - ip_allow.config - parent.config - plugin.config - records.config - remap.config - storage.config - volume.config - Delivery Services with - Edge Header Rewrite - Mid Header Rewrite, - Cache URL - URL Sig - URI Signing Note this is not an exhaustive of required files, and many files must be dynamic and cannot be inferred. But this does remove the manual configuration for this list. More files may be added in the future.

Refactors parent.config generation to have a single DS for-loop, instead of different loops for top-level vs non-top-level caches. This is to prepare for Topologies, which will need their own behavior, irrespective of "top level" Edge vs Mid, for every DS.

Also adds a TODO to propogate, once config gen returns errors.

Necessary because the latest API considers Topologies, previous API doesn't.

rob05c · 2020-08-04T20:50:01Z

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.

zrhoffman · 2020-08-04T21:45:37Z

atscfg.TestMakeParentDotConfigTopologies fails:

--- 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

rob05c · 2020-08-04T22:25:48Z

atscfg.TestMakeParentDotConfigTopologies fails:

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

zrhoffman · 2020-08-05T09:57:01Z

 	}

-	return len(serverNode.Parents) == 0, true
+	topologyHasChildren := false


A name like topologyNodeHasChildren or nodeHasChildren might make more sense here

zrhoffman · 2020-08-05T10:09:24Z

+	for _, node := range topology.Nodes {
+		for _, parent := range node.Parents {
+			if parent == serverNodeIndex {
+				topologyHasChildren = true
+				break
+			}
+		}
+	}


Should break out of the outer loop if topologyHasChildren is true

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

zrhoffman · 2020-08-05T10:15:46Z

+			for _, to := range topologies {
+				if to.Name == *ds.Topology {
+					topology = to
+					break
+				}
 			}


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.

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

zrhoffman · 2020-08-05T10:20:20Z

+	for _, to := range topologies {
+		if to.Name == *ds.Topology {
+			topology = to
+			break
+		}
+	}


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

zrhoffman · 2020-08-05T11:22:16Z

+
+	// Topologies must be all the topologies for the server's cdn.
+	// May incude topologies of other cdns.
+	Topologies []tc.Topology


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.

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.

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

Optimization.

Per code review.

More performant for large numbers of topologies. Per PR review.

zrhoffman

Code is formatted correctly
Unit tests pass
atstccfg respects Topologies parentage but only for topology-based DSes, ATS serves requests appropriately
Non-topology-based DSes continue to work with DeliveryServiceServers as expected
FirstHeaderRewrite/InnerHeaderRewrite/LastHeaderRewrite rules are places on the appropriate caches

LGTM!

rob05c added new feature A new feature, capability or behavior Traffic Ops ORT *DEPRECATED* related to the traffic_ops_ort.pl script labels Jun 12, 2020

rawlinp suggested changes Jun 25, 2020

View reviewed changes

rob05c force-pushed the add-topology-config-gen branch 3 times, most recently from 3f73867 to 0742063 Compare July 9, 2020 21:24

zrhoffman suggested changes Jul 31, 2020

View reviewed changes

rob05c added 8 commits August 4, 2020 14:21

Add ORT atstccfg Topology support

e89df6a

Fix code broken from changed TO Client functions

417cb62

Fix log typo

490e2f6

Also adds a TODO to propogate, once config gen returns errors.

Removed commented code

cce09de

Remove unused code, add topology name to comment

1417cdf

Add ORT Topology Header Rewrites

4b65101

rob05c force-pushed the add-topology-config-gen branch from de1c7fc to 4b65101 Compare August 4, 2020 20:22

rob05c added 2 commits August 4, 2020 14:29

Add ORT atstccfg error log for parentless topology

77b0d9a

Add ORT atstccfg new req for update_status

4fe8f25

Necessary because the latest API considers Topologies, previous API doesn't.

Fix tests for atscfg topology/capability change

3e70308

zrhoffman suggested changes Aug 5, 2020

View reviewed changes

rob05c added 3 commits August 5, 2020 10:34

Change ORT/atstccfg to break outer loop

56c21c5

Optimization.

Change ORT atscfg variable name

18718a3

Per code review.

Change ORT atscfg to use topology map

88b8405

More performant for large numbers of topologies. Per PR review.

zrhoffman approved these changes Aug 6, 2020

View reviewed changes

mitchell852 merged commit dababd8 into apache:master Aug 6, 2020

zrhoffman added this to the Flexible Topologies milestone Sep 18, 2020

zrhoffman removed this from the Flexible Topologies milestone Oct 16, 2020

Conversation

rob05c commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which Traffic Control components are affected by this PR?

What is the best way to verify this PR?

If this is a bug fix, what versions of Traffic Control are affected?

The following criteria are ALL met by this PR

Additional Information

Uh oh!

rawlinp commented Jun 23, 2020

Uh oh!

zrhoffman commented Jun 24, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mitchell852 commented Jul 22, 2020

Uh oh!

rob05c commented Jul 22, 2020

Uh oh!

zrhoffman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rob05c commented Aug 4, 2020

Uh oh!

zrhoffman commented Aug 4, 2020

Uh oh!

rob05c commented Aug 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

rob05c commented Jun 12, 2020 •

edited

Loading

rob05c commented Aug 4, 2020 •

edited

Loading

zrhoffman Aug 5, 2020 •

edited

Loading

zrhoffman left a comment •

edited

Loading