cmd/jujud: initial machine agent implementation

cmd/jujud: initial machine agent implementation



https://code.launchpad.net/~rogpeppe/juju-core/machine-agent/+merge/111256

(do not edit description out of merge proposal)

[rog, 2012-06-20 16:55:03]

Please take a look.

[fwereade, 2012-06-20 17:10:06]

Not sure it's quite ready yet; would appreciate your thoughts on the following.

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode67
cmd/jujud/machine.go:67: type Machiner struct {
Docs :). I only know about the MachineAgent/Machiner distinction via IRC.

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode77
cmd/jujud/machine.go:77: watcher := m.machine.WatchUnits()
I still think what we're really after here is WatchPrincipalUnits; we wouldn't
have to worry about the IsPrincipal checks below.

I'm uncertain about the symetry argument wrt Machine.Units; what other clients
do we have for that? Might Machine.PrincipalUnits be more useful in practice?

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode88
cmd/jujud/machine.go:88: m.tomb.Kill(err)
This feels like a candidate for the mustErr treatment.

[rog, 2012-06-20 17:43:17]

Please take a look.

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode67
cmd/jujud/machine.go:67: type Machiner struct {
On 2012/06/20 17:10:06, fwereade wrote:
> Docs :). I only know about the MachineAgent/Machiner distinction via IRC.

Done.

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode77
cmd/jujud/machine.go:77: watcher := m.machine.WatchUnits()
On 2012/06/20 17:10:06, fwereade wrote:
> I still think what we're really after here is WatchPrincipalUnits; we wouldn't
> have to worry about the IsPrincipal checks below.
> 
> I'm uncertain about the symetry argument wrt Machine.Units; what other clients
> do we have for that? Might Machine.PrincipalUnits be more useful in practice?

i think it's not bad that Machine.Units returns all units, not just principal
units. the call to IsPrincipal is essentially free, so we're really not doing
any more work.

i'm happy the way things are.
MachinePrincipalUnitsWatcher is a bit much of a mouthful.

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode88
cmd/jujud/machine.go:88: m.tomb.Kill(err)
On 2012/06/20 17:10:06, fwereade wrote:
> This feels like a candidate for the mustErr treatment.

i'm happy not to use mustErr - panic on Stop returning nil seems enough to me,
which i've done.

[niemeyer, 2012-06-20 21:08:14]

LGTM. Very nice.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode43
cmd/jujud/machine.go:43: // TODO reconnect when the agent fails.
s/agent/machiner/

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode48
cmd/jujud/machine.go:48: go m.loop()
Oops.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode56
cmd/jujud/machine.go:56: var err error
(m *Machiner, err error)

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode84
cmd/jujud/machine.go:84: // and restart them if not.
... and track them so that we don't deploy units below
that are already running.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode101
cmd/jujud/machine.go:101: // TODO what should we do in this case?
I suggest we don't do anything else. We need to run a verification anyway when
the process starts so that we destroy things that were removed while we were
off, so we can as well just delegate failed destructions to the next restart
until that proves not enough.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode109
cmd/jujud/machine.go:109: // TODO what should we do in this case?
This one we can act more proactively, putting it in a local queue for retrying,
in addition to doing the normal verification when the process starts.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine_test.go
File cmd/jujud/machine_test.go (right):

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:66: func (s *MachineSuite) TestMachinerDeployDestroy(c
*C) {
Great testing scheme.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine_test.go#ne...
cmd/jujud/machine_test.go:116: []string{"+d1/0"},
Nice.

[roger.peppe, 2012-06-20 21:14:30]

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode48
cmd/jujud/machine.go:48: go m.loop()
On 2012/06/20 21:08:14, niemeyer wrote:
> Oops.

?

[niemeyer, 2012-06-20 21:23:46]

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/4002/cmd/jujud/machine.go#newcode77
cmd/jujud/machine.go:77: watcher := m.machine.WatchUnits()
On 2012/06/20 17:10:06, fwereade wrote:
> I still think what we're really after here is WatchPrincipalUnits; we wouldn't
> have to worry about the IsPrincipal checks below.
> 
> I'm uncertain about the symetry argument wrt Machine.Units; what other clients
> do we have for that? Might Machine.PrincipalUnits be more useful in practice?

I voted earlier for us to avoid this for now. There's no great reason to single
out principal units in the API at this point. Doing "if principal" inside the
API has almost he same cost as doing it outside, and reduces the API surface and
increases the chances we'll not start writing principal-only features by
mistake.

[rog, 2012-06-21 09:18:16]

*** Submitted:

cmd/jujud: initial machine agent implementation

R=fwereade, niemeyer, roger.peppe
CC=
https://codereview.appspot.com/6294096

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode43
cmd/jujud/machine.go:43: // TODO reconnect when the agent fails.
On 2012/06/20 21:08:14, niemeyer wrote:
> s/agent/machiner/

Done.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode56
cmd/jujud/machine.go:56: var err error
On 2012/06/20 21:08:14, niemeyer wrote:
> (m *Machiner, err error)

not entirely convinced, but done.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode84
cmd/jujud/machine.go:84: // and restart them if not.
On 2012/06/20 21:08:14, niemeyer wrote:
> ... and track them so that we don't deploy units below
> that are already running.

Done.

https://codereview.appspot.com/6294096/diff/5007/cmd/jujud/machine.go#newcode109
cmd/jujud/machine.go:109: // TODO what should we do in this case?
On 2012/06/20 21:08:14, niemeyer wrote:
> This one we can act more proactively, putting it in a local queue for
retrying,
> in addition to doing the normal verification when the process starts.

Done.