Merge lp://staging/~dave-cheney/juju-core/069-worker-stater-I into lp://staging/~juju/juju-core/trunk

A few comments; the large-scale concern is that I'm not sure we benefit
by making this a worker; I don't think that really fits the context in
which it will be used.

https://codereview.appspot.com/7134051/diff/8001/worker/stater/export_test.go
File worker/stater/export_test.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/export_test.go#newcode16
worker/stater/export_test.go:16: func (c *Config) MongoDataDirX() string
{ return c.mongoDataDir() }
This all feels a bit awkward.

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go
File worker/stater/stater.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go#newcode22
worker/stater/stater.go:22: // unpacked too
s/too/to/

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go#newcode42
worker/stater/stater.go:42: // a mongodb state server on any machine
that it is started on.
I think it should probably install an upstart conf for mongo too. Sane?

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater_test.go
File worker/stater/stater_test.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater_test.go#newcode25
worker/stater/stater_test.go:25:
http.DefaultTransport.(*http.Transport).RegisterProtocol("file",
http.NewFileTransport(http.Dir("testdata")))
Nice.

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater_test.go#newcode108
worker/stater/stater_test.go:108: c.Assert(err, IsNil)
It feels to me as though you could probably just checkJournal and
checkContents here, and lose (1) all the other tests in this file and
(2) all those export_test bits.
I can see the sanity behind splitting the Stater's methods up as they
are, but I'm not sure that we gain a great deal by testing them
independently.

https://codereview.appspot.com/7134051/

On 2013/01/17 10:50:20, dfc wrote:
> Thanks for your feedback William. Addressing your first comment

> "A few comments; the large-scale concern is that I'm not sure we
benefit by
> making this a worker; I don't think that really fits the context in
which it
> will be used."

> This CL is the design that Gustavo and I agreed on in Dec. The larger
plan for
> this worker is it will become a worker, that watches the state and
manages the
> replication configuration for a mongo process.

OK, but what we have at the moment is basically a function that looks
like it's maybe intended to conform to the task interface, but actually
doesn't. I'm not necessarily opposed [0] to the intended direction, but
I'm not sure it's justified yet.

[0] I *am* a bit suspicious, but need to think more. Might try to chat
to niemeyer about this today if I remember.

https://codereview.appspot.com/7134051/

A couple more comments...

https://codereview.appspot.com/7134051/diff/8001/worker/stater/export_test.go
File worker/stater/export_test.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/export_test.go#newcode16
worker/stater/export_test.go:16: func (c *Config) MongoDataDirX() string
{ return c.mongoDataDir() }
On 2013/01/17 10:50:20, dfc wrote:
> On 2013/01/17 10:36:36, fwereade wrote:
> > This all feels a bit awkward.

> I is, I would prefer to test in package stater if there are no
objections.

I'd really prefer it if the public interface was tested in such a way as
to expose the various problems handled by the individual tests...

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go
File worker/stater/stater.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go#newcode42
worker/stater/stater.go:42: // a mongodb state server on any machine
that it is started on.
On 2013/01/17 10:50:20, dfc wrote:
> On 2013/01/17 10:36:36, fwereade wrote:
> > I think it should probably install an upstart conf for mongo too.
Sane?

> Yup, coming in a following CL.

Explicit TODO comment would be nice then (IMO).

https://codereview.appspot.com/7134051/diff/8001/worker/stater/tools.go
File worker/stater/tools.go (right):

https://codereview.appspot.com/7134051/diff/8001/worker/stater/tools.go#newcode10
worker/stater/tools.go:10: func sha1sum(file string) ([]byte, error) {
We use SHA256 for charms (and hex-encode the result). Why do we do it
differently here?

FWIW, IMO there are enough hash-fiddling places in the code that it
would be worth doing a quick audit to determine whether a trivial
function or two could be added...

https://codereview.appspot.com/7134051/diff/8001/worker/stater/tools.go#newcode25
worker/stater/tools.go:25: // createPrallocFile creates name
preallocated to size.
s/Pralloc/Prealloc/

https://codereview.appspot.com/7134051/

https://codereview.appspot.com/7134051/diff/17001/worker/stater/stater.go
File worker/stater/stater.go (right):

https://codereview.appspot.com/7134051/diff/17001/worker/stater/stater.go#newcode46
worker/stater/stater.go:46: tomb.Tomb
This should not be an anonymous field, I think. We don't want to expose
all the details of tomb as a public API.

https://codereview.appspot.com/7134051/diff/17001/worker/stater/stater.go#newcode70
worker/stater/stater.go:70: return sr.initMongoDataDir()
Nice to see all that logic clearly structured.

https://codereview.appspot.com/7134051/diff/17001/worker/stater/stater.go#newcode87
worker/stater/stater.go:87: // the mongo database. This function can
destroy data, callers
"can destroy data" is a bit too general.

https://codereview.appspot.com/7134051/diff/17001/worker/stater/stater.go#newcode190
worker/stater/stater.go:190: func (c *Config) mongoDir() string {
Rather than wrapping every field, NewStater could initialize the fields
properly in a copy of the original config, if they're found to be empty.
That's just a suggestion, though.

https://codereview.appspot.com/7134051/diff/17001/worker/stater/tools.go
File worker/stater/tools.go (right):

https://codereview.appspot.com/7134051/diff/17001/worker/stater/tools.go#newcode14
worker/stater/tools.go:14: // TODO(dfc) this is inefficient
Unnecessarily so. Seek + Write does the job.

https://codereview.appspot.com/7134051/

great direction. some comments and queries below.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go
File worker/stater/stater.go (right):

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode27
worker/stater/stater.go:27: defaultMongoDataDir = "/var/lib/juju/db"
rather than having all these default constants and special methods to
return them, why not just define:

var DefaultConfig = Config{
     MongoDir: "/opt",
     MongoDataDir: "/var/lib/juju/db",
     MongoURL: mustParseURL("http://juju-dist.s3.amazonaws.com/tools"),
}

?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode31
worker/stater/stater.go:31: defaultPreallocSize = 1 << 20
s/defaultPreallocSize/preallocSize/

(it's not overridable, so "default" is a misnomer)

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode43
worker/stater/stater.go:43: // TODO(dfc) add support for watching the
state
i don't think we'll need to watch the state here.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode58
worker/stater/stater.go:58: func (sr *Stater) run() {
given the comment above, i'm not sure that structuring this as a worker
makes sense. the only thing that's continually running is mongod itself,
and that's managed by upstart.

i think perhaps we could just have two functions:

func Start(config *Config) error
func Stop(config *Config) error

each would be idempotent, so when the machiner is not running
JobServeState (or whatever that constant will be), it can call
stater.Stop to ensure that the state server is not running;
likewise it can call stater.Start to start it running (and maybe
download the mongo tar archive, install upstart script, etc).

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode63
worker/stater/stater.go:63: func (sr *Stater) Wait() error {
i think this can go, see above.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode69
worker/stater/stater.go:69: // the lifetime of the agent.
this isn't an exported function, so this comment seems inappropriate
here. the only callers are in this package.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode110
worker/stater/stater.go:110: log.Debugf("creating journal file: %v, size
%d", file, defaultPreallocSize)
i think we could probably do without the debug statements in this
function. creating files isn't that interesting. perhaps just one for
the function, if you think it's worth it.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode121
worker/stater/stater.go:121: log.Printf("fetching mongo from: %v",
sr.config.mongoUrl())
s/://

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode127
worker/stater/stater.go:127: return nil, fmt.Errorf("fetch %q return
status: %d %v", sr.config.mongoUrl(), resp.StatusCode, resp.Status)
"cannot fetch %q: %s", sr.config.mongoUrl, resp.Status)

(i believe the status string includes the status code).

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#newcode133
worker/stater/sta...

Setting this to WIP; nobody's focusing on it at the moment.