worker/stater: new package

worker/stater: new package

This proposal introduces a new jujud worker, a Stater. The roll of the Stater is to handle installing and configuring a state server (read mongodb). In this initial implementation the Stater is capable of replacing the bash scripting in environs/cloudinit/cloudinit, although The actual change to cloudinit is the following proposal.

https://code.launchpad.net/~dave-cheney/juju-core/069-worker-stater-I/+merge/143629

(do not edit description out of merge proposal)

[dave_cheney.net, 2013-01-17 06:57:49]

Please take a look.

[dave_cheney.net, 2013-01-17 07:04:34]

Please take a look.

[dave_cheney.net, 2013-01-17 07:15:08]

On 2013/01/17 07:04:34, dfc wrote:
> Please take a look.

WIP followup proposal that introduces the jujud stater subcommand,
https://codereview.appspot.com/7149043

[fwereade, 2013-01-17 10:36:36]

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...
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#newc...
worker/stater/stater.go:22: // unpacked too
s/too/to/

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go#newc...
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...
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...
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.

[dave_cheney.net, 2013-01-17 10:50:20]

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.

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...
worker/stater/export_test.go:16: func (c *Config) MongoDataDirX() string {
return c.mongoDataDir() }
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.

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#newc...
worker/stater/stater.go:42: // a mongodb state server on any machine that it is
started on.
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.

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...
worker/stater/stater_test.go:25:
http.DefaultTransport.(*http.Transport).RegisterProtocol("file",
http.NewFileTransport(http.Dir("testdata")))
On 2013/01/17 10:36:36, fwereade wrote:
> Nice.

I can't take credit for this, I stole it from Roger's environs/ec2/image_test.go

[fwereade, 2013-01-18 07:21:13]

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.

[fwereade, 2013-01-18 07:26:12]

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...
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#newc...
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#newco...
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#newco...
worker/stater/tools.go:25: // createPrallocFile creates name preallocated to
size.
s/Pralloc/Prealloc/

[dave_cheney.net, 2013-01-18 19:49:40]

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#newc...
worker/stater/stater.go:22: // unpacked too
On 2013/01/17 10:36:36, fwereade wrote:
> s/too/to/

Done.

https://codereview.appspot.com/7134051/diff/8001/worker/stater/stater.go#newc...
worker/stater/stater.go:42: // a mongodb state server on any machine that it is
started on.
On 2013/01/18 07:26:12, fwereade wrote:
> 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).

Done.

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...
worker/stater/stater_test.go:108: c.Assert(err, IsNil)
On 2013/01/17 10:36:36, fwereade wrote:
> 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.

I've moved this back to package stater, which reduces the export_test.go noise

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#newco...
worker/stater/tools.go:10: func sha1sum(file string) ([]byte, error) {
On 2013/01/18 07:26:12, fwereade wrote:
> 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...

I've dropped this function, it was not needed. In the future when the stater is
run as a service, rather than one shot on bootstrap, I felt it would need a way
to determine if mongo was corrupt/uptodate on disk, but YANGI

https://codereview.appspot.com/7134051/diff/8001/worker/stater/tools.go#newco...
worker/stater/tools.go:25: // createPrallocFile creates name preallocated to
size.
On 2013/01/18 07:26:12, fwereade wrote:
> s/Pralloc/Prealloc/

Done.

[dave_cheney.net, 2013-01-18 19:50:36]

Please take a look.

[niemeyer, 2013-01-23 17:22:08]

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#new...
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#new...
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#new...
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#new...
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#newc...
worker/stater/tools.go:14: // TODO(dfc) this is inefficient
Unnecessarily so. Seek + Write does the job.

[dave_cheney.net, 2013-01-25 05:51:17]

Please take a look.

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#new...
worker/stater/stater.go:46: tomb.Tomb
On 2013/01/23 17:22:08, niemeyer wrote:
> This should not be an anonymous field, I think. We don't want to expose all
the
> details of tomb as a public API.

Done.

[rog, 2013-01-25 17:51:04]

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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
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#new...
worker/stater/stater.go:133: // archive, using the base directory of
sr.config.mongoDir().
// unpack reads a gzipped tar archive from r
// and unpacks it into sr.config.MongoDir.
?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#new...
worker/stater/stater.go:148: return err
should we clean up the partially unpacked directory if this happens?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#new...
worker/stater/stater.go:170: return fmt.Errorf("cannot handle tar file with
type: %d", hdr.Typeflag)
"cannot handle tar file %q with type %d", hdr.Name, hdr.Typeflag)

?

https://codereview.appspot.com/7134051/diff/20001/worker/stater/stater.go#new...
worker/stater/stater.go:189: MongoUrl *url.URL
if we're only ever going to use it as a string (http.Get takes a string), why
not store it as a string?

also
s/Url/URL/

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

https://codereview.appspot.com/7134051/diff/20001/worker/stater/tools.go#newc...
worker/stater/tools.go:9: func createPreallocFile(name string, size int) error {
i'm not sure this function justifies its own file.

https://codereview.appspot.com/7134051/diff/20001/worker/stater/tools.go#newc...
worker/stater/tools.go:18: if _, err := f.Seek(int64(size)-1, 0); err != nil {
f.Truncate(int64(size)) ?