Re: Traffic Ops Golang Migration Proposal

[Dewayne Richardson]

When:   Read · Wed, Jul 19. If Cc: read

[image: Timyo expectation line]
I had a knee jerk reaction to separate the RPMs because of the potential
for a "new" approach, but I've now been convinced otherwise based upon the
"Strangler Pattern" where the Perl gets rewritten into Golang overtime and
the TO clients know none the difference.

+1

-Dew

On Wed, Jul 19, 2017 at 12:52 PM, Jeff Elsloo  wrote:

> I see. If that's the case, it's a hard requirement to run the golang
> portion from whenever this is introduced onward. As long as we have
> discipline around removing migrated routes, that should work okay and
> would solve the "two watches" issue Mark mentioned when we discussed
> this in person: A man with two watches (old API route, golang route)
> does not know what time it is.
>
> I don't think that it makes a lot of sense to have a separate RPM
> since the dependency goes the other direction, and users are required
> to run the golang component no matter what. We might as well just
> build that into the existing RPM build process for traffic_ops.
>
> Do we really need to ask the user for the port to move mojo to?
> Obviously we can ask them to provide a port, but we could also just
> pick a random, unused high port, and have mojo listen only on the
> loopback interface. Maybe that's too "magical"?
>
> Does the golang app run as trafops:trafops and drop privileges after
> opening :443?
> --
> Thanks,
> Jeff
>
>
> On Wed, Jul 19, 2017 at 11:58 AM, Robert Butts 
> wrote:
> >> This means that the traffic_ops side would have to check to see whether
> >> traffic_ops_golang
> >
> >> Because the traffic_ops_golang package will depend on traffic_ops, not
> the
> >> other way around
> >
> > I was suggesting the other way around - traffic_ops will depend on
> > traffic_ops_golang. Which means upgrading traffic_ops automatically
> installs
> > traffic_ops_golang, and we don't need to do the check. It'd mean you
> > couldn't remove `traffic_ops_golang`, but the plan is to remove old
> > endpoints from old TO anyway. Which is another reason making
> > traffic_ops_golang a dependency of traffic_ops makes sense: it really is,
> > traffic_ops really does require it for the migrated endpoints.
> >
> > I agree with moving away from manual post-installation scripts, but I
> don't
> > think we can avoid it here, because we need the user to set a new port.
> >
> >
> > On Wed, Jul 19, 2017 at 11:40 AM, Jeff Elsloo  wrote:
> >>
> >> I'm +1 on most of what you suggest, except for doing the takeover in
> >> postinstall in traffic_ops.
> >>
> >> While we can do whatever we want with postinstall, I think it's
> >> awkward to have a tool within the traffic_ops package configuring
> >> something under the traffic_ops_golang package, when the latter
> >> package might not be installed. This means that the traffic_ops side
> >> would have to check to see whether traffic_ops_golang is installed
> >> outside of the normal RPM dependencies, adding more platform specific
> >> code to postinstall. I don't see a generic way to implement the
> >> "check" for the golang package within postinstall that will work. We
> >> would have to check for a path that is not likely to exist for most
> >> users, or check for a package. Both approaches require platform
> >> specific code and assumptions.
> >>
> >> Because the traffic_ops_golang package will depend on traffic_ops, not
> >> the other way around, it makes more sense to place the configuration
> >> piece in the golang package. When the golang package is installed, we
> >> can "take over" the port in the listen directive of cdn.conf, because
> >> we know for a fact that it is on disk because of the RPM dependency on
> >> traffic_ops. We also know that cdn.conf will be left alone if/when
> >> traffic_ops is upgraded due to being marked as a config file. If the
> >> user has installed either component outside of the normal RPM process,
> >> they will have to figure out how to run the golang package separately,
> >> as one would expect.
> >>
> >> We can do the configuration during the postinstall step of the
> >> traffic_ops_golang RPM. It's advantageous to manage that piece within
> >> the RPM, because if, for example, one wanted to remove the golang
> >> portion, we could have a postuninstall step that reverts changes made
> >> to cdn.conf (put the port we took over back into cdn.conf). We could
> >> seamlessly add and remove the traffic_ops_golang component without
> >> disturbing anything in traffic_ops, and without having to run some
> >> script manually. The platform specific things that would need to be
> >> done in postinstall should be done in the RPM, because then we know
> >> for sure which platform we're on, and assumptions about packages and
> >> paths will be accurate.
> >>
> >> Ideally we should be moving away from any manual run of any script
> >> after an 

Re: Traffic Ops Golang Migration Proposal

[Jeff Elsloo]

I see. If that's the case, it's a hard requirement to run the golang
portion from whenever this is introduced onward. As long as we have
discipline around removing migrated routes, that should work okay and
would solve the "two watches" issue Mark mentioned when we discussed
this in person: A man with two watches (old API route, golang route)
does not know what time it is.

I don't think that it makes a lot of sense to have a separate RPM
since the dependency goes the other direction, and users are required
to run the golang component no matter what. We might as well just
build that into the existing RPM build process for traffic_ops.

Do we really need to ask the user for the port to move mojo to?
Obviously we can ask them to provide a port, but we could also just
pick a random, unused high port, and have mojo listen only on the
loopback interface. Maybe that's too "magical"?

Does the golang app run as trafops:trafops and drop privileges after
opening :443?
--
Thanks,
Jeff


On Wed, Jul 19, 2017 at 11:58 AM, Robert Butts  wrote:
>> This means that the traffic_ops side would have to check to see whether
>> traffic_ops_golang
>
>> Because the traffic_ops_golang package will depend on traffic_ops, not the
>> other way around
>
> I was suggesting the other way around - traffic_ops will depend on
> traffic_ops_golang. Which means upgrading traffic_ops automatically installs
> traffic_ops_golang, and we don't need to do the check. It'd mean you
> couldn't remove `traffic_ops_golang`, but the plan is to remove old
> endpoints from old TO anyway. Which is another reason making
> traffic_ops_golang a dependency of traffic_ops makes sense: it really is,
> traffic_ops really does require it for the migrated endpoints.
>
> I agree with moving away from manual post-installation scripts, but I don't
> think we can avoid it here, because we need the user to set a new port.
>
>
> On Wed, Jul 19, 2017 at 11:40 AM, Jeff Elsloo  wrote:
>>
>> I'm +1 on most of what you suggest, except for doing the takeover in
>> postinstall in traffic_ops.
>>
>> While we can do whatever we want with postinstall, I think it's
>> awkward to have a tool within the traffic_ops package configuring
>> something under the traffic_ops_golang package, when the latter
>> package might not be installed. This means that the traffic_ops side
>> would have to check to see whether traffic_ops_golang is installed
>> outside of the normal RPM dependencies, adding more platform specific
>> code to postinstall. I don't see a generic way to implement the
>> "check" for the golang package within postinstall that will work. We
>> would have to check for a path that is not likely to exist for most
>> users, or check for a package. Both approaches require platform
>> specific code and assumptions.
>>
>> Because the traffic_ops_golang package will depend on traffic_ops, not
>> the other way around, it makes more sense to place the configuration
>> piece in the golang package. When the golang package is installed, we
>> can "take over" the port in the listen directive of cdn.conf, because
>> we know for a fact that it is on disk because of the RPM dependency on
>> traffic_ops. We also know that cdn.conf will be left alone if/when
>> traffic_ops is upgraded due to being marked as a config file. If the
>> user has installed either component outside of the normal RPM process,
>> they will have to figure out how to run the golang package separately,
>> as one would expect.
>>
>> We can do the configuration during the postinstall step of the
>> traffic_ops_golang RPM. It's advantageous to manage that piece within
>> the RPM, because if, for example, one wanted to remove the golang
>> portion, we could have a postuninstall step that reverts changes made
>> to cdn.conf (put the port we took over back into cdn.conf). We could
>> seamlessly add and remove the traffic_ops_golang component without
>> disturbing anything in traffic_ops, and without having to run some
>> script manually. The platform specific things that would need to be
>> done in postinstall should be done in the RPM, because then we know
>> for sure which platform we're on, and assumptions about packages and
>> paths will be accurate.
>>
>> Ideally we should be moving away from any manual run of any script
>> after an installation or upgrade, including postinstall, if that work
>> can be done within the RPM.
>> --
>> Thanks,
>> Jeff
>>
>>
>> On Wed, Jul 19, 2017 at 9:08 AM, Robert Butts 
>> wrote:
>> > It sounds like the only -1 is having a unified Service and RPM.
>> >
>> > How about the following compromise: a separate RPM and Service for the
>> > New
>> > TO, and the New TO RPM is a dependency of Old TO, and likewise the
>> > Service
>> > is a dependency of `traffic_ops`. This way, upgrades will still require
>> > building the New TO RPM and adding it to Yum, but `yum upgrade` will
>> > automatically install it without additional ops 

Re: Traffic Ops Golang Migration Proposal

[Jeff Elsloo]

I'm +1 on most of what you suggest, except for doing the takeover in
postinstall in traffic_ops.

While we can do whatever we want with postinstall, I think it's
awkward to have a tool within the traffic_ops package configuring
something under the traffic_ops_golang package, when the latter
package might not be installed. This means that the traffic_ops side
would have to check to see whether traffic_ops_golang is installed
outside of the normal RPM dependencies, adding more platform specific
code to postinstall. I don't see a generic way to implement the
"check" for the golang package within postinstall that will work. We
would have to check for a path that is not likely to exist for most
users, or check for a package. Both approaches require platform
specific code and assumptions.

Because the traffic_ops_golang package will depend on traffic_ops, not
the other way around, it makes more sense to place the configuration
piece in the golang package. When the golang package is installed, we
can "take over" the port in the listen directive of cdn.conf, because
we know for a fact that it is on disk because of the RPM dependency on
traffic_ops. We also know that cdn.conf will be left alone if/when
traffic_ops is upgraded due to being marked as a config file. If the
user has installed either component outside of the normal RPM process,
they will have to figure out how to run the golang package separately,
as one would expect.

We can do the configuration during the postinstall step of the
traffic_ops_golang RPM. It's advantageous to manage that piece within
the RPM, because if, for example, one wanted to remove the golang
portion, we could have a postuninstall step that reverts changes made
to cdn.conf (put the port we took over back into cdn.conf). We could
seamlessly add and remove the traffic_ops_golang component without
disturbing anything in traffic_ops, and without having to run some
script manually. The platform specific things that would need to be
done in postinstall should be done in the RPM, because then we know
for sure which platform we're on, and assumptions about packages and
paths will be accurate.

Ideally we should be moving away from any manual run of any script
after an installation or upgrade, including postinstall, if that work
can be done within the RPM.
--
Thanks,
Jeff


On Wed, Jul 19, 2017 at 9:08 AM, Robert Butts  wrote:
> It sounds like the only -1 is having a unified Service and RPM.
>
> How about the following compromise: a separate RPM and Service for the New
> TO, and the New TO RPM is a dependency of Old TO, and likewise the Service
> is a dependency of `traffic_ops`. This way, upgrades will still require
> building the New TO RPM and adding it to Yum, but `yum upgrade` will
> automatically install it without additional ops work, and `service
> traffic_ops start` will also start the New TO. Bearing in mind this
> double-service awkwardness will go away when all endpoints are migrated.
>
> Also, @alficles suggested configuring the New TO Config in Postinstall
> (which must be run after upgrading anyway). Because the New is a dependency
> of the Old, we're guaranteed `/opt/traffic_ops_golang` exists in
> Postinstall, and can populate its config.
>
> Also, I realized the New TO needs moved from
> `/traffic_ops/traffic_ops_golang` to `/traffic_ops_golang`, because
> `build_all.sh` requires projects be in the root directory.
>
> Also, I will add tests, docs, and configurable logging before the PR is
> merged. (Just wanted to wait until we had consensus before putting more work
> into it.)
>
> How does that sound?
>
>
> On Thu, Jul 13, 2017 at 1:00 PM, Robert Butts 
> wrote:
>>
>> @dewrich It's not that putting both services in the same Service and RPM
>> is a good long-term solution; it's that it's easier to configure and deploy.
>> A separate RPM and service is another step to install, and another step to
>> start the service. It's not that it's good, it's that it's the lesser of two
>> evils. My fear is, the more complex we make this, the less chance of getting
>> it done at all.
>>
>> @efriedri
>> 1. Same answer: long-term, I absolutely agree proxies should be separate.
>> But this is the simplest way to deploy an incremental migration.
>> 2. Yes, the Perl GUI can transparently request what it needs. That's a
>> goal of this: transparent new endpoints, that existing services don't know
>> or care that they come from a different backend.
>>
>> Also bear in mind, this "proxy" only exists until Perl TO goes away.
>> Having it in the same Service and RPM as old TO, and having the proxy be the
>> same binary as the new endpoints, makes it easier to completely remove the
>> Proxy and Perl, once every endpoint is rewritten. If we make the Service and
>> RPM separate, you have to change configs and uninstall the separate Service
>> and RPM of the Perl TO. If the Proxy is a separate binary, you have to
>> uninstall that, and then change 

Re: Proposal for PR requirements

[Dan Kirkwood]

Thanks,   Jason..I'll look into that.

+1 on Dave's comments -- the process shouldn't be that heavy-weight.
Yes -- there *should* be Jira tickets for anything not trivial,  but
should be at the discretion of the PR reviewer whether to request it
or not..

-dan

On Wed, Jul 19, 2017 at 7:55 AM, Jason Tucker  wrote:
> Re: the github PR trigger thing
>
> It looks like those hooks are available:
> https://blogs.apache.org/infra/entry/github_pull_request_builds_now
>
> Here's an example of it being used by the kafka project:
>
> https://github.com/apache/kafka/pull/3546
>
> Once those status checks are being returned, then the status gates can be
> turned on under branch protection.
>
> __Jason
>
> On Tue, Jul 18, 2017 at 10:33 PM, Dave Neuman  wrote:
>
>> I personally don't think that we should require a Jira for every single
>> PR.  I think blanket statements like "you must create a Jira ticket for
>> each PR", while made with good intentions, are too hard to enforce, don't
>> take every situation into account, and are sometimes a deterrent to new
>> contributors.  If Joe somebody from the internet wants to submit a PR, we
>> should welcome it and not say "we are not accepting this without first
>> creating a Jira account and then creating a Jira ticket".
>>
>> The changelog thing is a real problem.  We had a plan to have an
>> auto-generated changelog, but that was reliant on us moving to full github
>> which we haven't done yet.  I think maybe the changelog conversation might
>> need to be re-visited.
>>
>> Have you taken a look at our contributing guide[1] on github?  It already
>> outlines a lot of the things you are proposing here and I think its the
>> best place to have our PR requirements.
>>
>> I like Jason's idea, though not sure if it is possible.
>>
>> [1]
>> https://github.com/apache/incubator-trafficcontrol/blob/
>> master/CONTRIBUTING.md
>>
>> Thanks,
>> Dave
>>
>> On Tue, Jul 18, 2017 at 8:08 PM, Jason Tucker 
>> wrote:
>>
>> > Do you know if the the apache jenkins instance supports PR-triggered
>> > builds? If so, then repo branches can be protected so that PRs can only
>> be
>> > merged if the build status is successful. Might be a good safeguard to
>> have
>> > in place, if we have the ability to enable it on the jenkins side.
>> >
>> > __Jason
>> >
>> > On Wed, Jul 19, 2017 at 12:19 AM, Gelinas, Derek <
>> > derek_geli...@comcast.com>
>> > wrote:
>> >
>> > > As the project grows in complexity and the speed of updates, we're
>> > finding
>> > > a real need for changelogs and a reduction in the number of commits.
>> At
>> > > this time, creating a changelog over even a short period of time is a
>> > > tedious and time consuming activity, and it is making even incremental
>> > > updates difficult for us here at Comcast.
>> > >
>> > > I would like to propose the following:
>> > >
>> > >
>> > >   *   Each PR must have a linked jira issue.  This is as simple as
>> > > creating the ticket and then adding [TC-XXX] to the beginning of the PR
>> > > title.  The jira ticket title should briefly describe the fix or new
>> > > feature in a manner which lends itself to inclusion in a changelog.
>> > >   *   Large changes should have as much information as possible about
>> the
>> > > nature of the change and how this change affects the system.
>> > >   *   Testing should be done on all changes before the PR is submitted!
>> > > Please do not submit anything that has not been fully tested,
>> regardless
>> > of
>> > > how simple or obvious it may seem.  You are the first line of defense
>> > > against bugs!  If the work is not yet complete, be sure to add [WIP] to
>> > the
>> > > title of the PR.
>> > >   *   Commits should be squashed as much as possible before the PR is
>> > > submitted.  27 commits for minor changes make review and later analysis
>> > > very difficult.  Commit messages should be descriptive so it is clear
>> > what
>> > > was changed.
>> > >   *   New features or changes to functionality should include
>> > > documentation changes whenever possible.
>> > >   *   Integration testing should be included whenever possible - we now
>> > > have automated integration testing, so this is another valuable method
>> to
>> > > isolate bugs before they hit the field.
>> > >   *   When in doubt, be clear about what you are doing!
>> > >
>> > > I would also like to propose that if these requirements are not met
>> > > without specific reasoning, PRs should be rejected until they are
>> > corrected.
>> > >
>> > > Derek
>> > >
>> >
>>