Traffic Ops Golang Incremental Rewrite App #729

rob05c · 2017-07-13T14:57:51Z

This adds an app, which serves Traffic Ops endpoints as they're written (currently, just monitoring.json), and reverse-proxies everything else to the old Perl Traffic Ops.

Includes RPM and Service files, to deploy it alongside the old TO.

This can be trivially deployed alongside the old TO with no config (Puppet) changes. It reads the old TO config, and Postinstall sets the new port it needs.

Things left before this can be merged:

dewrich · 2017-07-13T15:31:42Z

I'm not sure why you would couple the Golang Microservices with the Traffic Op Perl Monolith. If anything you should begin a new "rpm" that only contains the Microservices (which brings up a question about should the MS's be separated into different rpms, or even RPMs at all?)

I think they need to be separated.

rob05c · 2017-07-13T15:36:57Z

The reason is to make it as operationally simple as possible. That's the only reason. We can make more RPMs, microservices, etc after we've moved away from Perl. I have no objection to microservices, I'm just afraid if we try to do both at once, we'll never get it done.

dewrich · 2017-07-13T15:45:32Z

I agree with simple, but I also don't want to "couple" the two. I don't find it egregious, to implement them as a "sidecar" rpm. We also might want to scale them differently because they are more efficient than Perl. If at one point we develop enough functionality we want to retire TO Perl, then we will have to unravel the coupling at that point in time.

If the two are separate then it'll be easy to just "uninstall" the TO Monolith.

rob05c · 2017-07-13T15:47:17Z

It should be easy to retire. Once all endpoints are in Golang, simply delete the Perl from the RPM and Service files.

dewrich · 2017-07-13T16:22:45Z

Also, should traffic_ops/traffic_ops_golang/auth.go be in traffic_ops/experimental/traffic_ops_golang? We haven't gotten any buy in from the community to put that code into traffic_ops proper yet. Which might also cause build issues.

rob05c · 2017-07-13T16:31:19Z

Right, it will cause build changes, don't merge this until we do get consensus from the community on the mailing list (I sent an email to users and forwarded to dev, "Traffic Ops Golang Migration Proposal".

alficles · 2017-07-13T17:06:44Z

From a user's perspective, this creates some extra (as-yet-poorly-documented) obligations for configuration. An RPM upgrade should take care of itself. There should be enough information for the upgrade to perform all the config changes necessary to maintain nearly exactly the same set of functionality users have today.

You'll need to allocate a new, high, port, but that's a fairly reasonable requirement, easily met. Notify the user via a message during upgrade and I think it would be ok.

I'd love to see this merged; I think it's the right direction. I think we should attend a bit more carefully to the user experience, though. We don't want people afraid to upgrade TO because we break their installs.

rob05c · 2017-07-13T17:09:41Z

upgrade to perform all the config changes necessary to maintain
allocate a new, high, port
Notify the user via a message during upgrade

@alficles Agree, will do. It'll take me a bit though, I'm not an RPM Wizard.

dneuman64 · 2017-07-17T18:54:38Z

It would be nice to see some documentation included with this PR before it is merged.
Also, I don't see any tests, it would be nice if there were some tests included as well.

dewrich · 2017-07-17T20:11:57Z

So, instead of using your own config, why not just use the existing /opt/traffic_ops/app/conf/production/database.conf. Then you wouldn't have to prefix each database property with db_. Also, postinstall sets up that file after the installer Q&A, so then you wouldn't have to touch poinstall

{
"port": "",
"db_user": "bill",
"db_pass": "thelizard",
"db_server": "db.trafficops.example.net",
"db_name": "trafficops",
"db_ssl": true,
"to_secret": "walrus",
"to_url": "https://trafficops.example.net:60443",
"no_auth": false,
"cert_path": "/opt/traffic_ops/cert",
"key_path": "/opt/traffic_ops/key"
}
[root@localhost init.d]# cat /opt/traffic_ops/app/conf/production/database.conf
{
"password" : "twelve",
"user" : "traffic_ops",
"type" : "Pg",
"hostname" : "localhost",
"description" : "Pg database on localhost:5432",
"port" : "5432",
"dbname" : "traffic_ops"
}

If you need properties that aren't in that config then you could use and groom your own

dewrich · 2017-07-17T20:34:57Z

I rolled an RPM to see how it could work, and I'm not seeing the process running. Which brought up my next question, where do the logs for troubleshooting go?

rob05c · 2017-07-17T23:56:31Z

If you didn't set up the config, it will fail to start.

Right now, failure to start will be logged to the SystemD service log. I'll make log locations part of the config, consistent with our existing apps.

rob05c · 2017-07-17T23:57:25Z

Putting a reminder here: Also need to make setting up the config part of Postinstall.

rob05c · 2017-07-17T23:59:05Z

so then you wouldn't have to touch poinstall

If we want to automatically set up the config, we're going to have to touch postinstall, if nothing else, to determine the new port to serve old-TO on.

rob05c · 2017-07-18T00:16:51Z

@dneuman64 I agree, I'll add docs and tests.

rob05c · 2017-07-19T21:05:28Z

We're also going to need a Perl serialized hash parser in Go, for cdn.conf. It must be modular, so replacing it with JSON is easy once Perl TO is dead. Tight coupling with Perl hash configs is not acceptable.

rob05c · 2017-07-20T05:11:34Z

From @elsloo : Need to let the Golang app run as a user and serve 443. Looks like the way to do that is to add setcap 'cap_net_bind_service=+ep' /opt/traffic_ops/traffic_ops_golang to the RPM, and a dependency on libcap2. See https://stackoverflow.com/a/414258/292623

elsloo · 2017-07-20T17:01:29Z

@rob05c awesome, looking forward to seeing how that works, as we might want to adopt that approach for any/all of our golang components that need to open ports like this.

dewrich · 2017-07-24T15:18:28Z

traffic_ops/traffic_ops_golang/config.go

+}
+
+// ParseConfig validates required fields, and parses non-JSON types
+func ParseConfig(cfg Config) (Config, error) {


Feedback I got from the Traffic Logs Generator was each config error should be listed at once. It's painful for the developer to fix the config problems one by one when they could just look at an error list

dewrich · 2017-07-24T15:41:20Z

traffic_ops/traffic_ops_golang/perlconfig.go

+		return "", fmt.Errorf("missing traffic_ops_golang_port key")
+	}
+	portStr, ok := portStrI.(string)
+	if !ok {


Shouldn't the port be defined as an "int16"?

It's a string just because it was easier to work with in the code, e.g. the Go HTTP server takes a string. I can make it an int or uint if you want. I'd rather not uint16 though, it's unusual, and even if performance mattered it's not any faster on a 64-bit processor.

Was wondering why the decision was made to use a string for a port. Not what I usually see in config files

The reason Go uses strings for ports is to support things like "http" instead of "80". In general, you shouldn't use strconv.Atoi to convert port strings. Take a look at https://golang.org/pkg/net/#LookupPort

dewrich · 2017-07-27T16:32:09Z

Can we call perl_access.log traffic_ops_access.log?

dewrich · 2017-07-27T16:36:31Z

Looks like the latest changes broke the Mojolicious Cookie

alficles · 2017-07-27T16:54:50Z

@dewrich traffic_ops_access.log is a bit ambiguous, since the whole construct, go+perl, is Traffic Ops. It should be something that clearly indicates it a log for the perl component.

dewrich · 2017-07-27T17:17:29Z

If we plan on making this a "seamless" deployment how does the to_url get configured from traffic_ops_golang.config? That property doesn't exist in the cdn.conf

rob05c · 2017-07-27T17:19:19Z

It reads the port from cdn.conf and sets it to 127.0.0.1:port

dewrich · 2017-07-27T20:58:30Z

docs/source/admin/traffic_ops/configuration.rst


-
+By default, the postinstall script configures the Go application to behave and transparently serve as the old Perl Traffic Ops did in previous versions. This includes reading the old ``cdn.conf`` and ``database.conf`` config files, and logging to the old ``access.log`` location. However, if you wish to customize the Go Traffic Ops application, you can do so by running it with the ``-oldcfg=false`` argument. By default, it will then look for a config file in ``/opt/traffic_ops/conf/traffic_ops_golang.json``. The new config file location may also be customized via the ``-cfg`` flag. A sample config file is installed by the RPM at ``/opt/traffic_ops/conf/traffic_ops_golang.json``. If you wish to run the new Go Traffic Ops application as a service with a new config file, the ``-oldcfg=false`` and  ``-cfg`` flags may be added to the ``start`` function in the service file, located by default at ``etc/init.d/traffic_ops``.


Should this /opt/traffic_ops/conf/traffic_ops_golang.json be /opt/traffic_ops/conf/traffic_ops_golang.config?

conf/config seemed redundant, whereas .json immediately tells anyone looking at it what the format is.

where do I find this /opt/traffic_ops/conf/traffic_ops_golang.json I only see /opt/traffic_ops/conf/traffic_ops_golang.config in the project, which is why I commented on it being improperly documented as .json

Ah, you're right. I meant to rename the file and forgot. I'll change the documentation.

dewrich · 2017-07-27T21:00:01Z

traffic_monitor_golang/common/log/log.go

@@ -95,6 +97,11 @@ func Eventf(t time.Time, format string, v ...interface{}) {
 	Event.Printf("%.3f %s", float64(t.Unix())+(float64(t.Nanosecond())/1e9), fmt.Sprintf(format, v...))
 }

+// EventfRaw writes to the event log with no prefix.
+func EventfRaw(format string, v ...interface{}) {


Should this function be called AccessfRawinstead of EventfRaw? Or any of the other functions that write to the access.log?

No, it's using the Event logger for access.log. Which seemed to make sense, accesses are events for this app.

@dangogh

c/o @dangogh @dewrich

@dewrich

As requested by @dewrich

rob05c · 2017-08-08T16:25:18Z

Must be merged after #786 which fixes an ORT bug this exposes.

dewrich · 2017-08-08T21:03:56Z

Just ran a JSON comparison tool against the Golang v. Perl responses and found that the JSON keys are different. Unfortunately, this can't change until we rev the API.

Golang
+ hashId: "xxxx"
+ hostName: "xxxx"
+ interfaceName: "xxxx"

Perl
- hashid: "xxxx"
- hostname: "xxxx"
- interfacename: "xxxx"

dewrich · 2017-08-08T21:07:30Z

Per our discussion:

Mojo HTTP Header is sending this back when the Golang proxy does not.

Cache-Control: no-cache, no-store, max-age=0, must-revalidate

dewrich · 2017-08-08T21:08:16Z

This Mojo HTTP Header is also coming back and I don't see it in the Golang HTTP header

Connection: keep-alive

dewrich · 2017-08-08T21:11:35Z

Test cases failed:

--- FAIL: TestGetCDNConf (0.00s)
	perlconfig_test.go:84: expected {HTTPPort:443 DBUser: DBPass: DBServer: DBDB: DBSSL:false TOSecret:walrus TOURLStr:https://127.0.0.1:60443 TOURL:https://127.0.0.1:60443 Insecure:false CertPath:/etc/pki/tls/certs/localhost.crt KeyPath:/etc/pki/tls/private/localhost.key MaxDBConnections:0 LogLocationError: LogLocationWarning: LogLocationInfo: LogLocationDebug: LogLocationEvent:} actual {HTTPPort:443 DBUser: DBPass: DBServer: DBDB: DBSSL:false TOSecret:walrus TOURLStr:https://127.0.0.1:60443 TOURL:https://127.0.0.1:60443 Insecure:false CertPath:/etc/pki/tls/certs/localhost.crt KeyPath:/etc/pki/tls/private/localhost.key MaxDBConnections:50 LogLocationError: LogLocationWarning: LogLocationInfo: LogLocationDebug: LogLocationEvent:}```

asfgit · 2017-08-08T22:46:41Z

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

asfgit · 2017-08-08T22:52:00Z

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

dewrich · 2017-08-09T19:12:27Z

"test this please"

asfgit · 2017-08-09T19:21:04Z

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

rob05c force-pushed the to-gomonitoring branch from 340de8d to 1c98d0d Compare July 21, 2017 23:04

dewrich reviewed Jul 24, 2017

View reviewed changes

rob05c force-pushed the to-gomonitoring branch 2 times, most recently from d15d638 to 1c39287 Compare July 25, 2017 01:38

dewrich reviewed Jul 27, 2017

View reviewed changes

rob05c added 8 commits August 6, 2017 10:10

Fix merge conflicts

be63ea5

Add licenses go-sqlmock refs

95ec647

Fix TO Golang setting port in cdn.conf

55ae572

c/o @dangogh @dewrich

Add TO Golang DBMaxConns config

23c85a7

Fix TO Golang docs config file name

f500ecd

Fix TO Golang postinstall to be executable

c247666

Fix TO Golang test for MaxDBConnections default

43e85d8

Fix TO Golang build spec needing TO client

5677c75

rob05c force-pushed the to-gomonitoring branch from 8d7b139 to 5677c75 Compare August 6, 2017 16:11

rob05c mentioned this pull request Aug 6, 2017

Traffic Ops Golang cacheconfig #780

Closed

rob05c added 2 commits August 6, 2017 10:30

Move TO Golang DBMaxConns from db.conf to cdn.conf

d857164

As requested by @dewrich

Fix TO Golang Postinstall to keep existing port

60c7e72

rob05c added 2 commits August 8, 2017 16:38

Fix TO Golang test for default MaxDBConns

8712b15

Fix TO Golang monitoring.json capitalisation

a9a7ea7

asfgit closed this in 77c0dae Aug 10, 2017

dewrich added this to Backlog in TO API Golang Rewrite Sep 21, 2017

dewrich removed this from In Backlog in TO API Golang Rewrite Sep 21, 2017

rob05c deleted the to-gomonitoring branch November 13, 2017 18:18

mitchell852 assigned dewrich Dec 14, 2017

dewrich added the change log label Dec 14, 2017

dewrich added this to the 2.2.0 milestone Dec 14, 2017



		By default, the postinstall script configures the Go application to behave and transparently serve as the old Perl Traffic Ops did in previous versions. This includes reading the old ``cdn.conf`` and ``database.conf`` config files, and logging to the old ``access.log`` location. However, if you wish to customize the Go Traffic Ops application, you can do so by running it with the ``-oldcfg=false`` argument. By default, it will then look for a config file in ``/opt/traffic_ops/conf/traffic_ops_golang.json``. The new config file location may also be customized via the ``-cfg`` flag. A sample config file is installed by the RPM at ``/opt/traffic_ops/conf/traffic_ops_golang.json``. If you wish to run the new Go Traffic Ops application as a service with a new config file, the ``-oldcfg=false`` and ``-cfg`` flags may be added to the ``start`` function in the service file, located by default at ``etc/init.d/traffic_ops``.

Traffic Ops Golang Incremental Rewrite App #729

Traffic Ops Golang Incremental Rewrite App #729

Conversation

rob05c commented Jul 13, 2017 • edited

dewrich commented Jul 13, 2017 • edited

rob05c commented Jul 13, 2017

dewrich commented Jul 13, 2017

rob05c commented Jul 13, 2017

dewrich commented Jul 13, 2017 • edited

rob05c commented Jul 13, 2017

alficles commented Jul 13, 2017

rob05c commented Jul 13, 2017

dneuman64 commented Jul 17, 2017

dewrich commented Jul 17, 2017

dewrich commented Jul 17, 2017

rob05c commented Jul 17, 2017

rob05c commented Jul 17, 2017

rob05c commented Jul 17, 2017 • edited

rob05c commented Jul 18, 2017

rob05c commented Jul 19, 2017

rob05c commented Jul 20, 2017 • edited

elsloo commented Jul 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dewrich Jul 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dewrich commented Jul 27, 2017

dewrich commented Jul 27, 2017

alficles commented Jul 27, 2017

dewrich commented Jul 27, 2017

rob05c commented Jul 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dewrich Aug 1, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rob05c commented Aug 8, 2017

dewrich commented Aug 8, 2017 • edited

dewrich commented Aug 8, 2017

dewrich commented Aug 8, 2017

dewrich commented Aug 8, 2017

asfgit commented Aug 8, 2017

asfgit commented Aug 8, 2017

dewrich commented Aug 9, 2017

asfgit commented Aug 9, 2017

rob05c commented Jul 13, 2017 •

edited

dewrich commented Jul 13, 2017 •

edited

dewrich commented Jul 13, 2017 •

edited

rob05c commented Jul 17, 2017 •

edited

rob05c commented Jul 20, 2017 •

edited

dewrich Jul 24, 2017 •

edited

dewrich Aug 1, 2017 •

edited

dewrich commented Aug 8, 2017 •

edited