jujud: JobManageState runs cleaner, resumer

jujud: JobManageState runs cleaner, resumer

Nothing to see there... but I also got angry and tweaked Agent so I could
just test the damn tasks themselves and not worry about the side effects.
And then I needed to test RunAgentLoop as well to verify that it does the
right thing with the tasks it gets from a mocked Agent.

https://code.launchpad.net/~fwereade/juju-core/jujud-integrate-cleaner-resumer/+merge/170907

(do not edit description out of merge proposal)

[fwereade, 2013-06-21 20:42:38]

Please take a look.

[jameinel, 2013-06-23 12:53:06]

Some questions about the testing infrastructure, but overall 
LGTM

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent.go#oldcode130
cmd/jujud/agent.go:130: RunOnce(st *state.State, entity AgentState) error
I do really like the rename of RunOnce to StartTasks.
As RunOnce can be interpreted as "only ever run this once", which confuses it a
bit with why you are calling it in a loop.

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:275: conf.MachineNonce = state.BootstrapNonce
I thought all machines that aren't machine/0 were meant to have their own
Nonce's. Is this just to have *something*? (As in, it could just as easily be a
random string, as long as you set it and preserve it?)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:430: s.retryDelay, retryDelay = retryDelay,
time.Millisecond
I appreciate not having delays in the test suite.
I wonder about using a more obvious naming convention for package-global
variables that will be mutated in separate files.

Certainly when I read this, it looks like a function-local 'retryDelay'
variable. I realize there is no ':', and I can do the 'grep' to figure out where
'retryDelay' is defined (somewhere in the cmd/jujud package).

Nothing to do with your proposed changes, just something about "where is this
variable coming from" that I find hard to read with our current idioms. (In
python you at least have to say 'global retryDelay' so you know to go look for
it somewhere else. And even then it would still be in the same file.)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:448: }
Not specifically related to your patch, but "type Agent interface" has no doc
strings for me to understand what it is actually trying to do. And AgentState is
defined in 'upgrade.go' while 'Agent' is defined in agent.go.

Anyway, the point I wanted to make was to think about Mock and what we are
trying to do. The concern is always that a Mock gets out of sync with the real
thing, and your tests just always keep passing because Mock doesn't migrate.

I'm guessing go's "interface" helps us enough here, in that if Agent changes its
interface mockAgent will be updated to match (at least the function signatures).
Like in this case, you changed from RunOnce to StartTasks, you do have to update
the mock or the compiler will complain.


In general, I love to see decoupling of the various bits of Juju and make it
easier to test just one layer. Though I generally prefer tested Doubles (they
act like the thing they are doubling) rather than ultralightweight Mocks (they
record what they were called with).

It also seems odd that you are able to mock out running 'heavyweight tasks', but
you still have to have a State object that connects to a real mongodb running.
It feels like if you are going to mock anything, you would want to remove the
State connection first.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:504: stderrors.New("abritrary"),
arbitrary

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:508: worker.ErrTerminateAgent,
If you are testing 'retry until something happens', shouldn't you have more
errors after the one you expect to terminate the loop, and then you can check
that it stopped at the right step?

For example, what happens if you just remove 'worker.ErrTerminateAgent', from
what I can see it would trigger a panic() in Wait because of a
slice-out-of-bounds error. But I think that only triggers in the goroutine, so
you would still end up with the loop stopping. (I guess maybe it dies with a
timeout instead of a done signal because RunAgentLoop doesn't recover from
panics?)

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:518: }
Maybe adding a 'c.Check(errTask.waitCount, Equals, 6)' to be sure how many times
it waited, and what finally terminated the looping.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:521: func (s *RunAgentLoopSuite)
TestRetryUntilUpgradedError(c *C) {
Similar structure here.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:578: tomb.Kill(stderrors.New("external"))
I'm not sure this test does what you want it to.

I dug into the tomb details, and it says "only the first non-nil error is
recorded as reason", and you are using tomb.Kill(non-nil) to initiate the stop
code.
So even though your waitTask.Stop() code *also* signals an error, I'm pretty
sure this test is functionally identical to the TestRunUntilStopped.

I would consider dropping it unless you can actually observe a difference.

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:253: // This is horrible. But it makes this
functionality somewhat testable...
I can't say I quite understand why this is horrible. Care to elaborate?

[fwereade, 2013-06-24 16:11:28]

Thanks for the review; dropping this due to collision.

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:275: conf.MachineNonce = state.BootstrapNonce
On 2013/06/23 12:53:07, jameinel wrote:
> I thought all machines that aren't machine/0 were meant to have their own
> Nonce's. Is this just to have *something*? (As in, it could just as easily be
a
> random string, as long as you set it and preserve it?)

Ha, yeah, I moved that method without reading it. Not sure why that's there at
all.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:430: s.retryDelay, retryDelay = retryDelay,
time.Millisecond
On 2013/06/23 12:53:07, jameinel wrote:
> I appreciate not having delays in the test suite.
> I wonder about using a more obvious naming convention for package-global
> variables that will be mutated in separate files.
> 
> Certainly when I read this, it looks like a function-local 'retryDelay'
> variable. I realize there is no ':', and I can do the 'grep' to figure out
where
> 'retryDelay' is defined (somewhere in the cmd/jujud package).
> 
> Nothing to do with your proposed changes, just something about "where is this
> variable coming from" that I find hard to read with our current idioms. (In
> python you at least have to say 'global retryDelay' so you know to go look for
> it somewhere else. And even then it would still be in the same file.)

I think we've got a bit of a general problem with code being pretty randomly
scattered across files in a package... that's not for want of bikeshedding,
though. It's worse here because we're testing in-package and the usual
restrictions don't apply.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:448: }
On 2013/06/23 12:53:07, jameinel wrote:
> Not specifically related to your patch, but "type Agent interface" has no doc
> strings for me to understand what it is actually trying to do. And AgentState
is
> defined in 'upgrade.go' while 'Agent' is defined in agent.go.

I hate spelunking through this package for exactly these reasons :).

> Anyway, the point I wanted to make was to think about Mock and what we are
> trying to do. The concern is always that a Mock gets out of sync with the real
> thing, and your tests just always keep passing because Mock doesn't migrate.
> 
> I'm guessing go's "interface" helps us enough here, in that if Agent changes
its
> interface mockAgent will be updated to match (at least the function
signatures).
> Like in this case, you changed from RunOnce to StartTasks, you do have to
update
> the mock or the compiler will complain.

Yeah; doesn't help with semantic changes, but it's handy all the same.

> In general, I love to see decoupling of the various bits of Juju and make it
> easier to test just one layer. Though I generally prefer tested Doubles (they
> act like the thing they are doubling) rather than ultralightweight Mocks (they
> record what they were called with).

No theoretical argument, but I'm not sure it's necessary to be any heavier here.
It's just a vehicle for navigating the connection dance until we get to the
point where we can inject our tasks.

> It also seems odd that you are able to mock out running 'heavyweight tasks',
but
> you still have to have a State object that connects to a real mongodb running.
> It feels like if you are going to mock anything, you would want to remove the
> State connection first.

I mocked what I needed to test what I changed, with a bias for avoidance of
potential rats' nests ;).

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:508: worker.ErrTerminateAgent,
On 2013/06/23 12:53:07, jameinel wrote:
> If you are testing 'retry until something happens', shouldn't you have more
> errors after the one you expect to terminate the loop, and then you can check
> that it stopped at the right step?
> 
> For example, what happens if you just remove 'worker.ErrTerminateAgent', from
> what I can see it would trigger a panic() in Wait because of a
> slice-out-of-bounds error. But I think that only triggers in the goroutine, so
> you would still end up with the loop stopping. (I guess maybe it dies with a
> timeout instead of a done signal because RunAgentLoop doesn't recover from
> panics?)

Good points all. Cheers.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:518: }
On 2013/06/23 12:53:07, jameinel wrote:
> Maybe adding a 'c.Check(errTask.waitCount, Equals, 6)' to be sure how many
times
> it waited, and what finally terminated the looping.

Bit more inclined to put a C into errTask, but we'll see.

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/agent_test.go#newcod...
cmd/jujud/agent_test.go:578: tomb.Kill(stderrors.New("external"))
On 2013/06/23 12:53:07, jameinel wrote:
> I'm not sure this test does what you want it to.
> 
> I dug into the tomb details, and it says "only the first non-nil error is
> recorded as reason", and you are using tomb.Kill(non-nil) to initiate the stop
> code.
> So even though your waitTask.Stop() code *also* signals an error, I'm pretty
> sure this test is functionally identical to the TestRunUntilStopped.
> 
> I would consider dropping it unless you can actually observe a difference.

I'm actively testing that the task error while stopping does not leak out of
RunAgentLoop -- it's a different test to the one above, in which the task exits
cleanly. I could probably just Kill(nil) in both cases, but turning a nil into a
nil isn't very impressive.

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

https://codereview.appspot.com/10439046/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:253: // This is horrible. But it makes this
functionality somewhat testable...
On 2013/06/23 12:53:07, jameinel wrote:
> I can't say I quite understand why this is horrible. Care to elaborate?

I don't have any way to get an agent instance into this useful state without
mucking directly with internals, and it makes me nervous that the test might not
catch some relevant change at some point in the future.