worker/upgrader: new package

worker/upgrader: new package

Substantially simplified logic from when it
lived in cmd/jujud, but it still has the
same basic contract (it dies with a NeedsUpgrade
error when upgraded).

https://code.launchpad.net/~rogpeppe/juju-core/347-upgrader-api-worker/+merge/177190

(do not edit description out of merge proposal)

[rog, 2013-07-26 17:14:30]

Please take a look.

[fwereade, 2013-07-26 17:24:56]

Looks great, but I have a few quibbles.

I was kinda expecting a NotifyHandler, but I think the download-retrying is
worth it.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:93: err = u.st.SetTools(u.tag, currentTools)
Do we definitely want to set zeroed tools in state here?

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:100: }
defer stop?

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:108: }
You can drop the preceding two blocks, I think...

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:128: }
...so long as you move the above block below the select.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
File worker/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
worker/upgrader/upgrader_test.go:95: return &tools.Tools{URL: url, Version:
vers}
reuse the bits in environs/testing/tools.go?

[dimitern, 2013-07-26 18:01:14]

LGTM, modulo the suggestions below

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/export_test.go
File worker/upgrader/export_test.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/export_test...
worker/upgrader/export_test.go:1: package upgrader
copyright comments

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:40: var logger = loggo.GetLogger("juju.upgrader")
juju.worker.upgrader

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:42: type Upgrader struct {
doc comment?

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:100: }
On 2013/07/26 17:24:56, fwereade wrote:
> defer stop?

+1

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:111: if wantTools.Version.Number !=
currentTools.Version.Number {
this whole if block seems a good candidate for a helper function, to reduce the
nesting level here.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:151: return fmt.Errorf("bad http response: %v",
resp.Status)
s/http/HTTP/

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
File worker/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
worker/upgrader/upgrader_test.go:108: c.Assert(u.Stop(), gc.IsNil)
statetesting.AssertStop(c, u) ?

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
worker/upgrader/upgrader_test.go:127: c.Assert(u.Stop(), gc.IsNil)
ditto

[rog, 2013-07-29 09:03:37]

Please take a look.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:40: var logger = loggo.GetLogger("juju.upgrader")
On 2013/07/26 18:01:15, dimitern wrote:
> juju.worker.upgrader

Done.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:42: type Upgrader struct {
On 2013/07/26 18:01:15, dimitern wrote:
> doc comment?

Done.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:93: err = u.st.SetTools(u.tag, currentTools)
On 2013/07/26 17:24:56, fwereade wrote:
> Do we definitely want to set zeroed tools in state here?

I can't see how they can ever be entirely zero, given
the test above.

They might have an empty URL if we can't read the tools
directly, but I think that's probably better than
bombing out in that case.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:100: }
On 2013/07/26 18:01:15, dimitern wrote:
> On 2013/07/26 17:24:56, fwereade wrote:
> > defer stop?
> 
> +1

Done.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:111: if wantTools.Version.Number !=
currentTools.Version.Number {
On 2013/07/26 18:01:15, dimitern wrote:
> this whole if block seems a good candidate for a helper function, to reduce
the
> nesting level here.

I'm not sure. I think the indentation level isn't too bad;
I quite like seeing the logic inline and the fact that
it returns from within the middle and also sets the retry
variable means that you'd still need to have substantially
similar logic even with a separate function.

I'll leave it as is if you don't mind (feel free to make
concrete suggestions if you feel strongly about it)

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:128: }
On 2013/07/26 17:24:56, fwereade wrote:
> ...so long as you move the above block below the select.

that would break the intended logic, which is
that if there's an upgrade needed, we upgrade
immediately even if we've been stopped already.

we could change the Dying channel into a local
variable that we only set once we've got changes
the first time. i'll see how that turns out.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader.go...
worker/upgrader/upgrader.go:151: return fmt.Errorf("bad http response: %v",
resp.Status)
On 2013/07/26 18:01:15, dimitern wrote:
> s/http/HTTP/

Done.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
File worker/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
worker/upgrader/upgrader_test.go:95: return &tools.Tools{URL: url, Version:
vers}
On 2013/07/26 17:24:56, fwereade wrote:
> reuse the bits in environs/testing/tools.go?

They're not quite sufficient.
I've left a TODO to avoid widening
the scope of this CL unnecessarily.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
worker/upgrader/upgrader_test.go:108: c.Assert(u.Stop(), gc.IsNil)
On 2013/07/26 18:01:15, dimitern wrote:
> statetesting.AssertStop(c, u) ?

Meh. More characters and less obvious; I'd
prefer to keep it as it is. What's the point
of AssertStop again?

[rog, 2013-07-29 09:07:13]

Please take a look.

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/export_test.go
File worker/upgrader/export_test.go (right):

https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/export_test...
worker/upgrader/export_test.go:1: package upgrader
On 2013/07/26 18:01:15, dimitern wrote:
> copyright comments

Done.

[dimitern, 2013-07-29 09:10:08]

>
https://codereview.appspot.com/11932043/diff/3001/worker/upgrader/upgrader_te...
> worker/upgrader/upgrader_test.go:108: c.Assert(u.Stop(), gc.IsNil)
> On 2013/07/26 18:01:15, dimitern wrote:
> > statetesting.AssertStop(c, u) ?
> 
> Meh. More characters and less obvious; I'd
> prefer to keep it as it is. What's the point
> of AssertStop again?

The point is common language of reusable constructs to write tests, kinda like
using gocheck :) Take a look at all watcher/worker tests about how statetesting
is used.

[fwereade, 2013-07-29 09:21:33]

LGTM -- but, re AssertStop, I do think it is actually kinda useful to have a
common vocabulary for common assertions, even if it's a bit more verbose than
handcrafting each individual assert ;).

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader.g...
worker/upgrader/upgrader.go:168: }
Little quibble, but it might be nicer to return (agentTools, err) and construct
the UpgradeReadyError all at once.

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader_t...
File worker/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader_t...
worker/upgrader/upgrader_test.go:175: defer u.Stop()
One of the points of statetesting.AssertStop is that you can defer it and not
drop these sorts of errors on the floor.

[rog, 2013-07-29 09:41:44]

Please take a look.

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader.g...
worker/upgrader/upgrader.go:168: }
On 2013/07/29 09:21:33, fwereade wrote:
> Little quibble, but it might be nicer to return (agentTools, err) and
construct
> the UpgradeReadyError all at once.

done, slightly differently.

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader_t...
File worker/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/11932043/diff/23001/worker/upgrader/upgrader_t...
worker/upgrader/upgrader_test.go:175: defer u.Stop()
as it happens, AssertStop is not appropriate
here, as the worker is known to die with an error.

i've changed the other occurrences to
AssertStop though, despite my preference
otherwise.