deployer: Refactor, remove watchers

deployer: Refactor, remove watchers

This is the first of several CLs which will
enable the Deployer to use the API instead of
state package directly.
As a preliminary step, and to reduce the number
of API objects (watchers) that need implementing
later, we remove the UnitsWatcher argument from
NewDeployer and pass machine id instead. Then the
machine is retrieved from state by that id and
machine.WatchUnits() is used to handle both cases:
deploying principal units and deploying subordinates.
Unit agent no longer needs to create its own Deployer
task for subordinates, as this is now also handled
by the machine agent's Deployer task.

In follow-ups these changes will happen:
* Replace log package with loggo (as suggested);
* Remove Deployer shutdown code from the Uniter;
* Implement client and server API interface for Deployer;
* Replace state calls with API calls in Deployer.

https://code.launchpad.net/~dimitern/juju-core/059-deployer-remove-unitswatcher/+merge/171587

(do not edit description out of merge proposal)

[dimitern, 2013-06-26 15:30:50]

Please take a look.

[fwereade, 2013-06-26 16:47:28]

Looks great apart from the error return from newDeployer.

LGTM with that fixed, plus verification live that you can actually deploy and
recall a subordinate or two :).

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

https://codereview.appspot.com/10617043/diff/1/cmd/jujud/machine.go#newcode120
cmd/jujud/machine.go:120: if err != nil {
Can't do this, a broken deployer will totally screw up the agent. Errors have to
come out of Wait or Stop, I think.

https://codereview.appspot.com/10617043/diff/1/cmd/jujud/unit_test.go
File cmd/jujud/unit_test.go (left):

https://codereview.appspot.com/10617043/diff/1/cmd/jujud/unit_test.go#oldcode90
cmd/jujud/unit_test.go:90: ctx, reset := patchDeployContext(c, conf.StateInfo,
conf.DataDir)
Yay :)

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go#ne...
worker/deployer/deployer.go:48: func NewDeployer(st *state.State, ctx Context,
machineId string) (*Deployer, error) {
Ah, I see. Just create the machine and start the watcher inside the loop method
instead of out here; any errors will be handled elsewhere.

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go#ne...
worker/deployer/deployer.go:98: if machineId, err := unit.AssignedMachineId();
err != nil {
Please explicitly check for a NotAssignedError (or whatever it is) rather than
any error.

And rejoice in the fact that you can rewrite this logic for the API and just
trust that, if you can see the unit, it's assigned to you.

[dimitern, 2013-06-27 09:33:45]

Please take a look.

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

https://codereview.appspot.com/10617043/diff/1/cmd/jujud/machine.go#newcode120
cmd/jujud/machine.go:120: if err != nil {
On 2013/06/26 16:47:28, fwereade wrote:
> Can't do this, a broken deployer will totally screw up the agent. Errors have
to
> come out of Wait or Stop, I think.

Done.

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go#ne...
worker/deployer/deployer.go:48: func NewDeployer(st *state.State, ctx Context,
machineId string) (*Deployer, error) {
On 2013/06/26 16:47:28, fwereade wrote:
> Ah, I see. Just create the machine and start the watcher inside the loop
method
> instead of out here; any errors will be handled elsewhere.

Done.

https://codereview.appspot.com/10617043/diff/1/worker/deployer/deployer.go#ne...
worker/deployer/deployer.go:98: if machineId, err := unit.AssignedMachineId();
err != nil {
On 2013/06/26 16:47:28, fwereade wrote:
> Please explicitly check for a NotAssignedError (or whatever it is) rather than
> any error.
> 
> And rejoice in the fact that you can rewrite this logic for the API and just
> trust that, if you can see the unit, it's assigned to you.

Done.

[jtv.canonical, 2013-06-27 10:12:33]

LGTM, provided the points I raised are addressed.  I see that you took care to
fix even places where the old code would still have worked but had the wrong
identifiers, so kudos for being thorough!

Stepping away from the code for a moment, the description of the change was
helpful but only as a summary of what the code changes were.  It'd be much more
interesting to know what the bigger picture is: what's happening in abstract
terms, and what is the purpose of the change?  Is it perhaps part of a bigger
job that spans multiple branches?  Or is this a cleanup?  Did something become
unnecessary, or is something being solved in a different way and are you
cleaning up the old way?  These may seem really stupid questions when you're
just finished doing the actual work, but getting in the habit of explaining
these things can make things clearer for everyone in the long run.

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:94: log.Warningf("worker/deployer: ignoring unit %q
(not assigned)", unitName)

Don't know if it's relevant but I was just told to use loggo, not log.

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:96: } else if err != nil {

You've got a triple if/else nested in an else here.  Can you come up with a
phrasing that's easier for an engineer (think engineer under pressure to fix a
bug, and not sure if this is the right place) to follow?  A little bit of extra
convenience can save a surprising amount of time in the long run.

The convention in Go is to "escape early" when you hit an error, so that the
structure of the rest of the code isn't affected by that code path.  Maybe
there's a way to un-else one or two of these error clauses?

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:99: responsible = machineId == d.machineId

To my untrained eye (and many eyes on this code will be similarly untrained)
this looks like a nontrivial decision being made here.  It'd be good to have a
very brief comment explaining the higher meaning of this equation.

Some disambiguating parentheses might also be nice, for those of us who are used
to working in multiple languages with different precedence tables.

[dimitern, 2013-06-27 10:57:04]

Please take a look.

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go
File worker/deployer/deployer.go (right):

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:94: log.Warningf("worker/deployer: ignoring unit %q
(not assigned)", unitName)
On 2013/06/27 10:12:34, jtv.canonical wrote:
> 
> Don't know if it's relevant but I was just told to use loggo, not log.

We should start using it, I agree, but I'll do it in a follow-up not to clutter
this CL.

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:96: } else if err != nil {
On 2013/06/27 10:12:34, jtv.canonical wrote:
> 
> You've got a triple if/else nested in an else here.  Can you come up with a
> phrasing that's easier for an engineer (think engineer under pressure to fix a
> bug, and not sure if this is the right place) to follow?  A little bit of
extra
> convenience can save a surprising amount of time in the long run.
> 
> The convention in Go is to "escape early" when you hit an error, so that the
> structure of the rest of the code isn't affected by that code path.  Maybe
> there's a way to un-else one or two of these error clauses?

This will be simplified once we introduce the API to the deployer, so that
calling st.Unit(name) will return ErrPerm when you're not responsible for its
deployment, so we'll get rid of most of these cases.

https://codereview.appspot.com/10617043/diff/7001/worker/deployer/deployer.go...
worker/deployer/deployer.go:99: responsible = machineId == d.machineId
On 2013/06/27 10:12:34, jtv.canonical wrote:
> 
> To my untrained eye (and many eyes on this code will be similarly untrained)
> this looks like a nontrivial decision being made here.  It'd be good to have a
> very brief comment explaining the higher meaning of this equation.
> 
> Some disambiguating parentheses might also be nice, for those of us who are
used
> to working in multiple languages with different precedence tables.

Added some comments to describe what happens.