jujud: integrate cleaner, resumer

jujud: integrate cleaner, resumer

the stateReporter chan may be useful elsewhere, but I haven't trawled for
further simplifications. Main point is integrating these tasks and testing wat
we can.

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

(do not edit description out of merge proposal)

[fwereade, 2013-07-10 18:11:56]

Please take a look.

[dimitern, 2013-07-10 18:28:46]

LGTM, only a couple of clarification requests.

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

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State, 1000)
why 1000? why not unbuffered?

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
this a but confusing - perhaps a comment before this whole block explaining
what's being done will help

[jameinel, 2013-07-11 05:50:16]

LGTM

I have some comments, but I can live with the code as it is written. So if
having a helper is too much distraction, just land it.

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

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State, 1000)
On 2013/07/10 18:28:46, dimitern wrote:
> why 1000? why not unbuffered?

I don't think we want unbuffered, because then when the other loop starts up it
will block until we do our select (I think).
The trick is that once you call 'sendOpenedStates' *all* things that start up
are going to be trying to send their state to this channel.


The general idea is: "I'm going to start something up, and I want a backdoor
into the State object it is using, so send it onto this channel so I can pull it
out in this code."

However, once you set the channel, more things are going to be sent on it than
you really need. We could just create a local goroutine that consumes them all
and puts it somewhere we can use it.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
On 2013/07/10 18:28:46, dimitern wrote:
> this a but confusing - perhaps a comment before this whole block explaining
> what's being done will help

A slightly better pattern is probably:

oldChan := sendOpenedStates(agentStates)
defer sendOpenedStates(oldChan)

For things like this, I usually prefer to have the sendOpenedStates type
function actually return a func() that does the cleanup. So the code becomes:

func sendOpenedStates(ch <-chan *state.State) {
  old, agentStates := agentStates, ch
  return func() { agentStates := old) }
}

As then the code in the test case is:

cleanup := sendOpenedStates(agentStates)
defer cleanup()

And that is a common pattern regardless of what internal poking a given test
helper has to do.

Nested function calls and defer are hard to parse for mere mortals, which is why
I don't spell it as:

defer sendOpenedStates(agentStates)()
    ^------------------------------^
The indicator of what you are deferring is very far from 'defer' vs

cleanup := ...
defer cleanup()

It is also why I'm not 100% sold on the:

go func() {
...
...
...
...
...
..
...
.
.
.
.
..
}()

Syntax. At least it is "go func() {" which makes it clear you are thinking about
calling func at the end of this. But when you leave off those last two
parenthesis it gets pretty confusing. :)

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:329: case <-time.After(500 * time.Millisecond):
Please use the testing.LongWait constants. It will help signal *why* you are
picking a given timeout, and lets us tweak them across the codebase if we find
they are too long/short.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:386: timeout := time.After(500 * time.Millisecond)
Same here on the timeout.

This loop has been used elsewhere, but I'm not quite sure why we have to call
s.State.StartSync() every 50ms. Is it because there are multiple steps being
done, and only 1 is done per sync?

Because if we *didn't* need to trigger the sync every 50ms, this would be
exactly the NotifyWatcherC code, and it would be nice if we could reuse that
object.

Maybe it needs to be another helper, 

func (wc *NotifyWatcherC) AssertNoticeChanges(observe func() bool) {
  wc.State.Sync()
  timeout := time.After(testing.LongWait)
  for finished := false; !finished ; {
     select {
       case <- timeout:
         c.Fatalf("failed to notice the desired changes after %s", LongWait)
       case <-time.After(ShortWait):
         c.State.StartSync()
       case <-wc.w.Changes():
         finished = observe()
  }
}

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:396: return
'return' in a test suite is often confusing. Because it means if someone adds
assertions after this loop, they might not notice that they aren't run.

I think using the 'done bool' pattern would work well here.

for finished := false; !finished ; {
...
  if err != nil {
    // We got an error, so we are done looping
    finished = true
    c.Assert(err, jc.Satifies, errors.IsNotFoundError)
  }
}

Or if we add AssertNoticeChanges this becomes:

wc.AssertNoticeChanges(func() bool {
  err := unit.Refresh()
  if err == nil {
    return false
  }
  c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
  return true
}

(One can argue whether the return value should be "continue searching" vs "done
searching". I tend to prefer the positive value [opposite to what I have
written])

[fwereade, 2013-07-11 12:11:35]

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

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:318: agentStates := make(chan *state.State, 1000)
On 2013/07/11 05:50:16, jameinel wrote:
> On 2013/07/10 18:28:46, dimitern wrote:
> > why 1000? why not unbuffered?
> 
> I don't think we want unbuffered, because then when the other loop starts up
it
> will block until we do our select (I think).
> The trick is that once you call 'sendOpenedStates' *all* things that start up
> are going to be trying to send their state to this channel.
> 
> 
> The general idea is: "I'm going to start something up, and I want a backdoor
> into the State object it is using, so send it onto this channel so I can pull
it
> out in this code."
> 
> However, once you set the channel, more things are going to be sent on it than
> you really need. We could just create a local goroutine that consumes them all
> and puts it somewhere we can use it.

In practice, only one state should be sent, because we're not expecting errors
and restarts. But if we get them, that buffer should be big enough to ensure
nothing blocks. That's all :). I don't *think* it's worth the extra
infrastructure to check that no more come in; if things are broken I think we'll
detect it in other ways.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:319: defer
sendOpenedStates(sendOpenedStates(agentStates))
On 2013/07/11 05:50:16, jameinel wrote:
> On 2013/07/10 18:28:46, dimitern wrote:
> > this a but confusing - perhaps a comment before this whole block explaining
> > what's being done will help
> 
> A slightly better pattern is probably:
> 
> oldChan := sendOpenedStates(agentStates)
> defer sendOpenedStates(oldChan)
> 
> For things like this, I usually prefer to have the sendOpenedStates type
> function actually return a func() that does the cleanup. So the code becomes:
> 
> func sendOpenedStates(ch <-chan *state.State) {
>   old, agentStates := agentStates, ch
>   return func() { agentStates := old) }
> }
> 
> As then the code in the test case is:
> 
> cleanup := sendOpenedStates(agentStates)
> defer cleanup()
> 
> And that is a common pattern regardless of what internal poking a given test
> helper has to do.
> 
> Nested function calls and defer are hard to parse for mere mortals, which is
why
> I don't spell it as:
> 
> defer sendOpenedStates(agentStates)()
>     ^------------------------------^
> The indicator of what you are deferring is very far from 'defer' vs
> 
> cleanup := ...
> defer cleanup()
> 
> It is also why I'm not 100% sold on the:
> 
> go func() {
> ...
> ...
> ...
> ...
> ...
> ..
> ...
> .
> .
> .
> .
> ..
> }()
> 
> Syntax. At least it is "go func() {" which makes it clear you are thinking
about
> calling func at the end of this. But when you leave off those last two
> parenthesis it gets pretty confusing. :)

Done as undo func.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:329: case <-time.After(500 * time.Millisecond):
On 2013/07/11 05:50:16, jameinel wrote:
> Please use the testing.LongWait constants. It will help signal *why* you are
> picking a given timeout, and lets us tweak them across the codebase if we find
> they are too long/short.

Done.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:386: timeout := time.After(500 * time.Millisecond)
On 2013/07/11 05:50:16, jameinel wrote:
> Same here on the timeout.
> 
> This loop has been used elsewhere, but I'm not quite sure why we have to call
> s.State.StartSync() every 50ms. Is it because there are multiple steps being
> done, and only 1 is done per sync?

It's because the Sync only guarantees that the cleanup-triggering event is
delivered to the cleanups watcher, and the actual cleanup will finish a bit
after that, triggering the unit removal. There's nothing we can wait on to
ensure we start the sync after the cleanup completes, so instead we fire of
StartSyncs at intervals in the hope that one of them will come after the change
and hence trigger the unit watcher.

> Because if we *didn't* need to trigger the sync every 50ms, this would be
> exactly the NotifyWatcherC code, and it would be nice if we could reuse that
> object.
> 
> Maybe it needs to be another helper, 
> 
> func (wc *NotifyWatcherC) AssertNoticeChanges(observe func() bool) {
>   wc.State.Sync()
>   timeout := time.After(testing.LongWait)
>   for finished := false; !finished ; {
>      select {
>        case <- timeout:
>          c.Fatalf("failed to notice the desired changes after %s", LongWait)
>        case <-time.After(ShortWait):
>          c.State.StartSync()
>        case <-wc.w.Changes():
>          finished = observe()
>   }
> }

Don't think it's quite worth it tbh. If we start to see many more of them I'll
change my tune.

https://codereview.appspot.com/10825044/diff/1/cmd/jujud/machine_test.go#newc...
cmd/jujud/machine_test.go:396: return
On 2013/07/11 05:50:16, jameinel wrote:
> 'return' in a test suite is often confusing. Because it means if someone adds
> assertions after this loop, they might not notice that they aren't run.
> 
> I think using the 'done bool' pattern would work well here.
> 
> for finished := false; !finished ; {
> ...
>   if err != nil {
>     // We got an error, so we are done looping
>     finished = true
>     c.Assert(err, jc.Satifies, errors.IsNotFoundError)
>   }
> }
> 
> Or if we add AssertNoticeChanges this becomes:
> 
> wc.AssertNoticeChanges(func() bool {
>   err := unit.Refresh()
>   if err == nil {
>     return false
>   }
>   c.Assert(err, jc.Satisfies, errors.IsNotFoundError)
>   return true
> }
> 
> (One can argue whether the return value should be "continue searching" vs
"done
> searching". I tend to prefer the positive value [opposite to what I have
> written])

Done.