Breakout packages

[tgross]

@justenwalker per our discussion in #83 this expands on and includes #118.

Unit tests pass and I'm working on the build integration tests but I'd like to see what you think of this approach. Note that "NewService" ends up being a no-so-optimal approach just because we don't want to change the JSON syntax and injecting the discovery service became super painful. I'd certainly be open to suggestions for improvement here though.

cc @misterbisson just as an FYI

[tgross]

I'm wondering whether maybe we change utils to be core, move signals.go and poll.go into it, and then take everything in main.go and move it into the top-level main.go for the whole binary? There's not much left in core/main.go as it is.

[tgross]

Huzzah, all tests pass!

[justenwalker]

Note that "NewService" ends up being a no-so-optimal approach just because we don't want to change the JSON syntax and injecting the discovery service became super painful. I'd certainly be open to suggestions for improvement here though.

I can see how this would be painful. I still think we are complicating the idea of external configuration (ie from a file) with our application state (internal configuration) - I believe the fact that the structures are very similar is incidental.

Perhaps this isn't the time to undertake such a refactor, but does the idea of separating these concepts from each other seem like a good goal? Maybe we can create another issue for that.

Here are some of my thoughts:

instead of

// Config is the top-level Containerbuddy Configuration
type Config struct {
    Consul       string                    `json:"consul,omitempty"`
    Etcd         json.RawMessage           `json:"etcd,omitempty"`
    LogConfig    *LogConfig                `json:"logging,omitempty"`
    OnStart      json.RawMessage           `json:"onStart,omitempty"`
    PreStart     json.RawMessage           `json:"preStart,omitempty"`
    PreStop      json.RawMessage           `json:"preStop,omitempty"`
    PostStop     json.RawMessage           `json:"postStop,omitempty"`
    StopTimeout  int                       `json:"stopTimeout"`
    Services     []*services.ServiceConfig `json:"services"`
    Backends     []*backends.BackendConfig `json:"backends"`
    PreStartCmd  *exec.Cmd
    PreStopCmd   *exec.Cmd
    PostStopCmd  *exec.Cmd
    Command      *exec.Cmd
    QuitChannels []chan bool
    ConfigFlag   string
}

something like

type ApplicationState struct {
    PreStartCmd  *exec.Cmd
    PreStopCmd   *exec.Cmd
    PostStopCmd  *exec.Cmd
    Command      *exec.Cmd
    QuitChannels []chan bool
    Backends []backends.Backend
    Service  []services.Service
    ConfigFlag   string
}

Inside loadConfig some logic happens to parse the map[string]interface{} into several config structs
which will be passed as arguments to the NewX functions in packages which are responsible for converting them to the appropriate interfaces (github.com/mitchellh/mapstructure might help). Those config structs live in the package itself.

For instance, after loading up the var backendConfig backends.BackendConfig we get backend := backends.NewBackend(backendConfig) which is put into the ApplicationState struct

More concrete steps:

• loadConfig(config map[string]interface{}) will be a pure function which takes a raw config and produces a ApplicationState
• We store it as the currentState
• Then it is Run()
• Later, the application might want to reloadConfig() which:
  • config := loadConfigMap()
  • newState := loadConfig(config)
  • currentState.Stop()
  • currentState := newState
  • currentState.Run()

In retrospect I realize I oversimplified, this does not account for the fact that we don't re-run the shimmed application and preStart commands but it is part of the ApplicationState.

[tgross]

I can see how this would be painful. I still think we are complicating the idea of external configuration (ie from a file) with our application state (internal configuration) - I believe the fact that the structures are very similar is incidental.

I think you're absolutely on the right track here, and I think this sounds like a good approach. It's not that different than what I've got here already but is semantically a lot cleaner. I'm open to the idea of trying to get that idea in, but I'm also trying to ship metrics sooner rather than later. So I'll take a little bit of time today to see how much of a stretch it would be to roll that into this PR. If it's not too much of a hassle I think we can get it in, otherwise would you be open to keeping #83 open for continuing the refactor in stages?

[tgross]

@justenwalker I've had another chance to look at this and it's probably another day's work. I'm a bit pressed for time because I've got some otherwise unrelated work pushing to try to get the metrics stuff in.

So here's my proposal:

• We keep this PR open and you and I keep going with the discussion above with the goal of making this the way we ultimately want it to be going forward. (And/or including you making a PR to this branch yourself.)
• In the meantime we review and try to get merged-in the refactoring I did in Refactor of Pollable for modularization effort #118 which was entirely included within the metrics PR [no merge] Initial implementation of telemetry #119. I don't think anything in that PR is irreconcilable with what's in here outside of a version bump, which I think we're headed for anyways here.

[justenwalker]

In the meantime we review and try to get merged-in the refactoring I did in #118 which was entirely included within the metrics PR #119.

Sounds good @tgross - I'll try to crystalize some of my thoughts on another branch which will reveal some of the finer details I glossed over when posting before.

[tgross]

@justenwalker @misterbisson I've updated this PR with the following:

• merged the updated telemetry work into this PR
• renamed ServiceConfig to Service and moved the parsing of that config into the services package so that config gets it via NewServices as suggested by @justenwalker
• renamed BackendConfig to Backend and moved the parsing of that config into the backends package so that config gets it via NewBackends as suggested by @justenwalker
• moved the top-level stuff in core/Main() into the top-level github.com/joyent/containerbuddy package
• put everything under the proper package namespacing so that tools like golang-builder (ref Set canonical import path #127) and go get should be able to build packages from this repo.

What I'd like to do is cut this branch as 1.4.0-rc3 so that we can incorporate it into autopilotpattern/touchbase (ref autopilotpattern/touchbase#18). If we're happy with that, then I'd like to merge this more or less as-is (modulo any review comments). This exercise got a little bit away from us but I think it's better that we have it done now so that future projects aren't harder to implement.

The only thing that isn't yet done here from the above discussion is completely separating out the ApplicationState struct from the Config struct as @justenwalker was suggesting. I'd like to have us do that in another exercise so that we can get all the major open branches merged and avoid further drift from master.

[justenwalker]

I'm pretty sure we're abusing godep here - I have never seen any project contain several godep manifests and vendor folders in sub modules - I don't this will scale well.

Maybe we can just play with the mount locations and move vendor/godeps back to the top-level?

[justenwalker]

When doing a SIGHUP to reload the config - I get this error:

INFO[0073] Reloading configuration.
ERRO[0073] Could not reload config: duplicate metrics collector registration attempted

[justenwalker]

I tried fixing up the makefile to remove the redundant godeps

check it out here: https://github.com/justenwalker/containerbuddy/tree/fix-makefile

Update: that branch is failing tests because I made lint warnings into errors. If we don't want that to break tests, it would be an easy fix to remove that feature"

One thing I discovered while doing this: GOPATH can have multiple directories. The first one is used for go get so that is where I mounted the vendor directory: -v ./vendor:/go/src. Then I mounted containerbuddy under its own root: -v .:/cb/src/gitbhub.com/joyent/containerbuddy

[tgross]

The problem I was trying to solve there is that this breaks using the subpackages independently. If you tried to go get github.com/joyent/containerbuddy/blah and the HEAD of one of the dependencies has broken, the build will fail because the sub-package dependency hasn't been pinned.

Honestly, I'm fine with that and feel like the packages should just be an organizational thing rather than trying to optimize for a corner case. Your approach looks good to me.

I tried fixing up the makefile to remove the redundant godeps
check it out here: https://github.com/justenwalker/containerbuddy/tree/fix-makefile

That branch looks like it was forked from somewhere stale -- it relies on a makefile inside the container. But the essence of what you're doing seems correct... I'll see if I can pull those changes here.

Edit: I see what you're doing -- you're mounting the make file inside the container. I think we can just call bash directly without calling make though. Lemme see if I can make that work.

[tgross]

When doing a SIGHUP to reload the config - I get this error:

Good catch. Will try to fix.

[tgross]

@justenwalker I've merged your changes and tweaked so as to eliminate the extra make indirection. Going to try to fix the SIGHUP problem next.

[justenwalker]

Edit: I see what you're doing -- you're mounting the make file inside the container. I think we can just call bash directly without calling make though. Lemme see if I can make that work.

Yeah - sorry I was being "clever". If we want to avoid using make at all inside the container that's fine - the main benefit of make is it's ability to use file modification dates to avoid doing work - since the containers are destroyed after execution, it doesn't really do anything useful.

[justenwalker]

The problem I was trying to solve there is that this breaks using the subpackages independently. If you tried to go get github.com/joyent/containerbuddy/blah and the HEAD of one of the dependencies has broken, the build will fail because the sub-package dependency hasn't been pinned.

I never thought of that. I did run unto a potential problem using that method though:

When you make a method in package A that takes an argument that a 3rd-party library defines (Lets say logrus.Level) and you try to call that method from package B, they conflict because the dependencies are different. I think this is some evidence for keeping dependencies at the top level.

[tgross]

I passed - to the makefile to prevent lint from breaking the build. It couldn't hurt to come back to try and fix those at some point though.

I think I've got the SIGHUP bug licked, but just trying to make sure our tests exercise it correctly.

[tgross]

I think I've got the SIGHUP bug licked, but just trying to make sure our tests exercise it correctly.

I thought wrong... the problem I'm running into is that our HTTP server doesn't have any way to gracefully reload. There are two libraries I'm examining now to do this.

[tgross]

Ok @justenwalker I think I've got that figured out. There's a minor caveat to that which is that we can't change the telemetry port. But I'd like to revisit that under a separate body of work when we expand the server with things like #27 (comment)

[misterbisson]

🏡 🚶

[tgross]

I've got this tested-out with https://github.com/autopilotpattern/touchbase and we're feeling good about merging this in finally.