Traffic Ops Golang parent.config #3075

rob05c · 2018-12-03T07:02:52Z

What does this PR do?

Traffic Ops Golang parent.config

~~Still WIP. I've tested against a single edge and mid; still need to test against a large number of configurations of edges and mids.~~

Removing WIP - I think this is good to be merged. I've manually diffed versus the old Perl parent.config, against every edge and mid in our production CDN.

The only differences vs Perl, are where duplicate origins exist with different configurations (a data bug), and Perl arbitrarily selects one with no warning or error. Go now logs an error when that happens. Unfortunately, Perl is ordering them by an internal Perl hash of the object, which isn't feasible to replicate in Go. But again, it's a data bug to have that scenario at all.

Fixes #3071 in 48ba440
Does not fix #2725 - that's a much more invasive fix, I think should be a separate PR.

Includes API tests.

Which TC components are affected by this PR?

What is the best way to verify this PR?

Check all that apply

This PR includes tests
This PR includes documentation updates
This PR includes an update to CHANGELOG.md
This PR includes all required license headers
This PR includes a database migration (ensure that migration sequence is correct)
This PR fixes a serious security flaw. Read more: www.apache.org/security

asfgit · 2018-12-03T07:27:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2861/
Test PASSed.

asfgit · 2018-12-03T15:42:47Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2862/
Test FAILed.

asfgit · 2018-12-03T21:37:41Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2868/
Test FAILed.

asfgit · 2018-12-03T23:08:16Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2874/
Test PASSed.

asfgit · 2018-12-05T21:13:30Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2891/
Test FAILed.

asfgit · 2018-12-05T21:16:11Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2892/
Test FAILed.

ezelkow1

looks good other than those 3 err check spots which Im not sure if they need to be moved higher or not

traffic_ops/client/atsconfig.go

asfgit · 2018-12-10T22:23:52Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2926/
Test FAILed.

asfgit · 2018-12-10T22:28:58Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/2927/
Test FAILed.

asfgit · 2019-01-25T18:50:27Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3104/
Test FAILed.

asfgit · 2019-01-25T20:15:09Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3106/
Test FAILed.

rob05c · 2019-02-19T16:24:00Z

retest this please

asfgit · 2019-02-19T16:30:05Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3241/
Test FAILed.

As requested in PR Review.

Also renames local variables, per PR Review.

Per PR Review.

rob05c · 2019-04-26T15:29:57Z

@ocket8888 Odd, this PR shouldn't affect those files. I just rebased to the latest master, try now?

asfgit · 2019-04-26T15:45:28Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3591/
Test PASSed.

ocket8888 · 2019-04-26T17:01:42Z

Builds now. Problem was new files introduced to the repo that were untracked in your branch - caused compilation errors because of docker's recursive copy and go's implicit build system.

ocket8888

I just have a couple of nitpicks and some questions, but after testing against both the default CDN-in-a-Box environment and a dump of production data this seems to work exactly as advertised.

Also, fwiw I think it's perfectly fine to log warnings even where Perl didn't - it doesn't change the logic and everywhere you put the //TODO it seemed like a good idea to me. Not gonna block on that, but if you wanted to add them in it'd be an improvement imo.

traffic_ops/traffic_ops_golang/ats/parentdotconfig.go

ocket8888 · 2019-04-29T13:41:04Z

traffic_ops/traffic_ops_golang/ats/parentdotconfig.go

+					}
+					textLine += ` max_simple_retries=` + ds.MSOMaxSimpleRetries + ` max_unavailable_server_retries=` + ds.MSOMaxUnavailableServerRetries
+				}
+				textLine += "\n"


Would it make more sense to instead later join the array with "\n" instead of doing this every time and joining with an empty string?

Maybe. At this point, I'd really prefer not to change it. These variables are reused so much, as an artifact of Perl, I'd be afraid of accidentally changing the logic and breaking something. I added a TODO so it'll be looked at with the rest of #3515 if that's ok?

It's not a bug, so I'm fine with it as long as it's captured somehow.

traffic_ops/traffic_ops_golang/ats/parentdotconfig.go

Per PR Review

asfgit · 2019-04-29T15:25:48Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3597/
Test PASSed.

rawlinp · 2019-04-29T16:20:39Z

Also, fwiw I think it's perfectly fine to log warnings even where Perl didn't - it doesn't change the logic

We need to be mindful of the amount of logging we add and decide whether or not it really needs to be added at all, and if it truly is needed, it should be logged at the right level. If we add unnecessary warnings to code that gets called thousands of times during a Queue Updates operation, then we now have thousands of warnings that could otherwise be drowning out actual problems that need addressed.

Basically, if a warning is just "normal operation" of the system, then it should not be logged as a warning. A warning generally means that that the system is operating sub-optimally, and that something needs addressed before the warnings start leading to errors. If someone is digging into the logs, they should be able to quickly identify issues without having to sift through a bunch of meaningless information. A good rule of thumb is to imagine you're the ops engineer that's been tasked with triaging an issue in Production and has to dig into the logs without any knowledge of the code internals. You'll want to do yourself a favor at that point.

Just my 2 cents :)

rob05c · 2019-04-29T16:25:07Z

if a warning is just "normal operation" of the system, then it should not be logged as a warning. A warning generally means that that the system is operating sub-optimally, and that something needs addressed before the warnings start leading to errors

I agree. As far as I'm aware, all the warnings I added are things that should be addressed. But I could be mistaken, this code in particular is pretty easy to mistake normal operation for an issue, I'm open to changing anything that was misunderstood and acceptable as normal operation.

asfgit · 2019-04-29T16:26:12Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3598/
Test FAILed.

asfgit · 2019-04-29T16:29:55Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3599/
Test PASSed.

asfgit · 2019-04-29T16:33:22Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3600/
Test FAILed.

rawlinp · 2019-04-29T16:34:24Z

I think the current warnings in this PR are fine. I don't really know which warnings @ocket8888 was referring to, but from what I can remember about my initial review a long time ago, most of my comments about removing logging were for logs that seemed like informational debug logs that would only be useful for developing/testing this implementation. ¯\_(ツ)_/¯

ocket8888 · 2019-04-29T17:05:31Z

I was referring to logs that aren't there, there are two or three places where data is missing or malformed that just causes the current function to bail or skip the current item and he has e.g. // TODO log a warning? Perl doesn't

rob05c · 2019-04-29T20:39:31Z

Yeah, those particular comments, I'm not sure whether it's normal operation, or an issue

rob05c · 2019-04-29T20:39:50Z

retest this please

asfgit · 2019-04-29T20:45:10Z

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/trafficcontrol-PR/3604/
Test PASSed.

I believe all my comments have been addressed so I'm dismissing my review for now

rob05c added new feature A new feature, capability or behavior Traffic Ops related to Traffic Ops WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) labels Dec 3, 2018

rob05c force-pushed the to-parent-dot-config branch from 77dac18 to afd6a37 Compare December 3, 2018 21:35

rob05c force-pushed the to-parent-dot-config branch from afd6a37 to c902d45 Compare December 3, 2018 22:59

rob05c added the cache-config Cache config generation label Dec 5, 2018

rob05c mentioned this pull request Dec 5, 2018

Fixed bad default parent.config line generation #3091

Merged

17 tasks

rob05c removed the WIP "Work-in-Progress" - do not merge! (use 'draft' pull requests from now on) label Dec 5, 2018

rob05c changed the title ~~WIP Traffic Ops Golang parent.config~~ Traffic Ops Golang parent.config Dec 5, 2018

ezelkow1 reviewed Dec 10, 2018

View reviewed changes

traffic_ops/client/atsconfig.go Outdated Show resolved Hide resolved

traffic_ops/client/atsconfig.go Show resolved Hide resolved

traffic_ops/client/atsconfig.go Show resolved Hide resolved

rob05c force-pushed the to-parent-dot-config branch from 0def924 to bef472f Compare December 10, 2018 22:23

ezelkow1 approved these changes Dec 10, 2018

View reviewed changes

rob05c mentioned this pull request Dec 11, 2018

ATS config file content ordering #3106

Closed

rob05c mentioned this pull request Jan 17, 2019

Add Traffic Ops Golang regex_revalidate.config #3235

Merged

17 tasks

rob05c force-pushed the to-parent-dot-config branch from bef472f to d6a672d Compare January 25, 2019 18:44

rob05c force-pushed the to-parent-dot-config branch 2 times, most recently from 49b3b4c to 0516d71 Compare January 25, 2019 19:25

rob05c force-pushed the to-parent-dot-config branch from 0516d71 to 5371a52 Compare February 27, 2019 23:14

rob05c added 5 commits April 26, 2019 09:28

Renamed TO variable

0917762

As requested in PR Review.

Rename TO parentdotconfig symbols

03eb438

As requested in PR Review.

Fix TO parent.config missing defaults

f3d1afb

Move TO RemoveStrDuplicates to lib/go-util

bb35f7e

Also renames local variables, per PR Review.

Rename TO parent.config variable

ce7619b

Per PR Review.

rob05c force-pushed the to-parent-dot-config branch from 3130567 to ce7619b Compare April 26, 2019 15:29

ocket8888 requested changes Apr 29, 2019

View reviewed changes

rob05c added 4 commits April 29, 2019 08:26

Change TO comment to TODO

a2e6379

Fix TO error log typo

59a9e6a

Add TO TODO for improving parent.config logic

4aa2da0

Change TO parent.config var to longer name

6c3b914

Per PR Review

ocket8888 approved these changes Apr 29, 2019

View reviewed changes

rawlinp approved these changes May 31, 2019

View reviewed changes

rawlinp merged commit 695cd3f into apache:master May 31, 2019

ocket8888 mentioned this pull request Aug 6, 2019

Rewrite /servers/{{server}}/configfiles/ats/{{configFile}} to Go #3783

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Traffic Ops Golang parent.config #3075

Traffic Ops Golang parent.config #3075

rob05c commented Dec 3, 2018 •

edited

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 5, 2018

asfgit commented Dec 5, 2018

ezelkow1 left a comment

asfgit commented Dec 10, 2018

asfgit commented Dec 10, 2018

asfgit commented Jan 25, 2019

asfgit commented Jan 25, 2019

rob05c commented Feb 19, 2019

asfgit commented Feb 19, 2019

rob05c commented Apr 26, 2019

asfgit commented Apr 26, 2019

ocket8888 commented Apr 26, 2019

ocket8888 left a comment

ocket8888 Apr 29, 2019

rob05c Apr 29, 2019

ocket8888 Apr 29, 2019

asfgit commented Apr 29, 2019

rawlinp commented Apr 29, 2019

rob05c commented Apr 29, 2019

asfgit commented Apr 29, 2019

asfgit commented Apr 29, 2019

asfgit commented Apr 29, 2019

rawlinp commented Apr 29, 2019 •

edited

ocket8888 commented Apr 29, 2019

rob05c commented Apr 29, 2019

rob05c commented Apr 29, 2019

asfgit commented Apr 29, 2019

Traffic Ops Golang parent.config #3075

Traffic Ops Golang parent.config #3075

Conversation

rob05c commented Dec 3, 2018 • edited

What does this PR do?

Which TC components are affected by this PR?

What is the best way to verify this PR?

Check all that apply

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 3, 2018

asfgit commented Dec 5, 2018

asfgit commented Dec 5, 2018

ezelkow1 left a comment

Choose a reason for hiding this comment

asfgit commented Dec 10, 2018

asfgit commented Dec 10, 2018

asfgit commented Jan 25, 2019

asfgit commented Jan 25, 2019

rob05c commented Feb 19, 2019

asfgit commented Feb 19, 2019

rob05c commented Apr 26, 2019

asfgit commented Apr 26, 2019

ocket8888 commented Apr 26, 2019

ocket8888 left a comment

Choose a reason for hiding this comment

ocket8888 Apr 29, 2019

Choose a reason for hiding this comment

rob05c Apr 29, 2019

Choose a reason for hiding this comment

ocket8888 Apr 29, 2019

Choose a reason for hiding this comment

asfgit commented Apr 29, 2019

rawlinp commented Apr 29, 2019

rob05c commented Apr 29, 2019

asfgit commented Apr 29, 2019

asfgit commented Apr 29, 2019

asfgit commented Apr 29, 2019

rawlinp commented Apr 29, 2019 • edited

ocket8888 commented Apr 29, 2019

rob05c commented Apr 29, 2019

rob05c commented Apr 29, 2019

asfgit commented Apr 29, 2019

rob05c commented Dec 3, 2018 •

edited

rawlinp commented Apr 29, 2019 •

edited