state/api/apiserver/upgrader: Implement Upgrader

state/api/apiserver/upgrader: Implement Upgrader

This implements the basic API for Upgrader for machine agents.
It exposes the ability to watch for API Version changes, and
then query for what the correct version of Tools.

At the moment, WatchAPIVersion starts an EnvironConfigWatcher
which matches the existing upgrader code. However, it means
the underlying watcher differs from the "NotifyWatcher" that
the other code paths would depend on. In talking with William
there isn't a strong case for doing it this way, and it
would be better to implement an NotifyWatcher on the Environment
document, rather than implementing a Next that returns actual
content.

This also doesn't try to implement Auth checking for Unit agents,
which will also need to use this API. But I'd like to get this
reviewed and landed as a general outline of what the API will look 
like.

Originally, I wanted just 1 API which would be a watcher that
on changes would return the Tools objects directly. I also thought
we could create a multi-watcher rather than many watchers.
But this was straightforward and easy, and we can live with it
for now.

https://code.launchpad.net/~jameinel/juju-core/upgrader-api/+merge/172239

(do not edit description out of merge proposal)

[jameinel, 2013-06-30 12:31:24]

Please take a look.

[fwereade, 2013-06-30 21:35:15]

Solid in essence: a few trivials, one missing method (fine as a followup), but
am not quite comfortable about the EntityWatcher/EnvironConfigWatcher fuzziness.

Can get away with it until there's a client, I guess.

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
state/api/upgrader/upgrader_test.go:8: //"launchpad.net/juju-core/errors"
d

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
state/api/upgrader/upgrader_test.go:14:
//"launchpad.net/juju-core/state/api/params"
d

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:21: // New creates a new client-side
UpgraderAPI facade.
NewUpgraderAPI

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:34: const machineTagPrefix = "machine-"
unused?

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:48: result.Results[i].EntityWatcherId =
u.resources.Register(envWatcher)
This is a bit funky, because it's not an EntityWatcher today, and I worry
something'll get confused. Once I've landed the first two branches of my current
pipeline, the equivalent EntityWatcher method will be trivial:

return newEntityWatcher(st, st.settings, environGlobalKey)

...and even today it's less complex than Unit.WatchConfigSettings (or whatever
exactly it's called).

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:56: func (u *UpgraderAPI) Tools(args
params.Agents) (params.AgentToolsResults, error) {
I'm still fine with this, although I think that ian's instance-characteristics
branch renders it *nearly* redundant. Doubt there's time to hook that up cleanly
now though (not to mention the lack of hardware info on the bootstrap node).

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:72: // TODO: What error do we give here?
In theory, it can't happen -- SetEnvironConfig, for all its flaws, should
guarantee that agent-version is always set. ErrCorruptState?

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:91: tool, err :=
environs.FindExactTools(env, requested)
This is fine for now, but please flag it as monstrously inefficient and desirous
of some sort of caching (ie probably actually in state and updated by some other
mechanism).

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:106: }
There's one more method needed in the API: SetAgentTools. (so we can observe
what units are running what versions in status, not otherwise used AFAIR)

[jameinel, 2013-07-01 07:18:02]

Please take a look.

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
state/api/upgrader/upgrader_test.go:8: //"launchpad.net/juju-core/errors"
On 2013/06/30 21:35:15, fwereade wrote:
> d

Done.

https://codereview.appspot.com/10809043/diff/1/state/api/upgrader/upgrader_te...
state/api/upgrader/upgrader_test.go:14:
//"launchpad.net/juju-core/state/api/params"
On 2013/06/30 21:35:15, fwereade wrote:
> d

Done.

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:21: // New creates a new client-side
UpgraderAPI facade.
On 2013/06/30 21:35:15, fwereade wrote:
> NewUpgraderAPI

I don't quite understand this comment. The function is NewUpgraderAPI, but it
returns a new UpgraderAPI...

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:34: const machineTagPrefix = "machine-"
On 2013/06/30 21:35:15, fwereade wrote:
> unused?

Yeah, originally I thought I would need to grab the actual Machine or Unit, but
I can actually check Auth from just the tag itself AuthOwner(tag).

Removed.

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:48: result.Results[i].EntityWatcherId =
u.resources.Register(envWatcher)
I agree that newEntityWatcher is reasonably easy to spell. The biggest problem
is that you have 3 private variables/functions above. So I have to figure out
where to put this thing so that it can do what I want.

Would it just be a function in state/watchers.go ?

Right now those hang off things like state.Machine, but I guess you could put it
as state.State.WatchEnviron ?

On 2013/06/30 21:35:15, fwereade wrote:
> This is a bit funky, because it's not an EntityWatcher today, and I worry
> something'll get confused. Once I've landed the first two branches of my
current
> pipeline, the equivalent EntityWatcher method will be trivial:
> 
> return newEntityWatcher(st, st.settings, environGlobalKey)
> 
> ...and even today it's less complex than Unit.WatchConfigSettings (or whatever
> exactly it's called).

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:72: // TODO: What error do we give here?
On 2013/06/30 21:35:15, fwereade wrote:
> In theory, it can't happen -- SetEnvironConfig, for all its flaws, should
> guarantee that agent-version is always set. ErrCorruptState?

It is the sort of case that if we didn't have bool, I would just go with it.
Since we have the return value, I feel we should check it.

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:91: tool, err :=
environs.FindExactTools(env, requested)
On 2013/06/30 21:35:15, fwereade wrote:
> This is fine for now, but please flag it as monstrously inefficient and
desirous
> of some sort of caching (ie probably actually in state and updated by some
other
> mechanism).

I added a comment.

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:106: }
On 2013/06/30 21:35:15, fwereade wrote:
> There's one more method needed in the API: SetAgentTools. (so we can observe
> what units are running what versions in status, not otherwise used AFAIR)

This sort of thing is where I want us to get actual clients written so that we
can see what is missing. I can certainly add this, but once we have things
hooked up end-to-end it will be much clearer what is missing (or just slightly
incorrect for the use cases).

I implemented SetTools, which it would be great if you could look over as well.

[gz, 2013-07-02 11:25:52]

LGTM with some comments on minor doc and code tidiness things.

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:21: // New creates a new client-side
UpgraderAPI facade.
On 2013/07/01 07:18:02, jameinel wrote:
> On 2013/06/30 21:35:15, fwereade wrote:
> > NewUpgraderAPI
> 
> I don't quite understand this comment. The function is NewUpgraderAPI, but it
> returns a new UpgraderAPI...

William means the comment must start "NewUpgraderAPI creates..." rather than
"New creates..." for the go magic doc comment stuff to work.

https://codereview.appspot.com/10809043/diff/6001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/params/params.go#...
state/api/params/params.go:254: // This is a flatted state.Tools description
Does this comment start to start "AgentToolsResult is a..."?

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader.go:8: //import
"launchpad.net/juju-core/state/api/params"
Commented out import?

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:102: c.Assert(err, IsNil)
Should this kind of thing be Check rather than Assert so the following cleanup
also happens, or are we screwed on clean teardown anyway when the test fails?

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:118: func (s *upgraderSuite)
TestVerifiesAuth(c *C) {
A completely empty test as a stub seems less useful than a comment saying there
needs to be a TestVerifiesAuth written.

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:118: func (s *upgraderSuite)
TestVerifiesAuth(c *C) {
A completely empty test as a stub seems less useful than a comment saying there
needs to be a TestVerifiesAuth written.

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:110: // Find the Tools necessary for the
given agents
This is the comment for the function above, not this one.

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader_test.go:83: defer func() {
You should be able to use state/testing/watcher.go for this now.

[fwereade, 2013-07-02 13:46:38]

OK, dammit, I didn't recall (didn't know?) how we stored agent tools in state in
the first place. Given that we *always* SetAgentTools first thing in the
upgrader, and that that *always* apparently included series/arch, I am starting
to think that actually including those in either Watch or in Tools is completely
redundant.

Given that,can you think of any reason to keep them around for the future?

(I think I was fooled by status output skipping those...)

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:118: func (s *upgraderSuite)
TestVerifiesAuth(c *C) {
..?

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:91: //       information could just be
cached in state, or
fwiw I'm +1 on making available tools objects in state,just like charms. Not
actionable today though...

[jameinel, 2013-07-03 04:50:09]

On 2013/07/02 13:46:38, fwereade wrote:
> OK, dammit, I didn't recall (didn't know?) how we stored agent tools in state
in
> the first place. Given that we *always* SetAgentTools first thing in the
> upgrader, and that that *always* apparently included series/arch, I am
starting
> to think that actually including those in either Watch or in Tools is
completely
> redundant.
> 
> Given that,can you think of any reason to keep them around for the future?
> 
> (I think I was fooled by status output skipping those...)

There are certainly cases where it isn't set in the test suite, but you're right
that the first thing the Upgrader task does is report its current version.

I would be happy not sending it to the API, as I had other use cases for an
"Agent" params request, and I was just leaving those fields blank. I'll try to
update it for that.

[jameinel, 2013-07-03 05:14:12]

Please take a look.

https://codereview.appspot.com/10809043/diff/6001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/params/params.go#...
state/api/params/params.go:254: // This is a flatted state.Tools description
On 2013/07/02 11:25:52, gz wrote:
> Does this comment start to start "AgentToolsResult is a..."?

Done.

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader.go
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader.go:8: //import
"launchpad.net/juju-core/state/api/params"
On 2013/07/02 11:25:52, gz wrote:
> Commented out import?

Done.

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:102: c.Assert(err, IsNil)
On 2013/07/02 11:25:52, gz wrote:
> Should this kind of thing be Check rather than Assert so the following cleanup
> also happens, or are we screwed on clean teardown anyway when the test fails?

Reasonable thing to consider. TearDown probably should always use Check so that
tear-down completes.

https://codereview.appspot.com/10809043/diff/6001/state/api/upgrader/upgrader...
state/api/upgrader/upgrader_test.go:118: func (s *upgraderSuite)
TestVerifiesAuth(c *C) {
On 2013/07/02 13:46:38, fwereade wrote:
> ..?

The client-side of the API wasn't fleshed out very well. I thought at first this
was where the round-trip testing should be done, but it wasn't how the other
bits of code were done.

The Auth testing is done inside the server side code.

Also, I originally thought Upgrader was the charm upgrade code, not the
jujud-binary upgrader code.

Removed for now.

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader.go:110: // Find the Tools necessary for the
given agents
On 2013/07/02 11:25:52, gz wrote:
> This is the comment for the function above, not this one.

Nice catch.

Done

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/6001/state/apiserver/upgrader/up...
state/apiserver/upgrader/upgrader_test.go:83: defer func() {
On 2013/07/02 11:25:52, gz wrote:
> You should be able to use state/testing/watcher.go for this now.

Right. When I had written this, that code wasn't in trunk yet.

Also, is that capable of testing a non-Entity watcher? Right now this is an
EnvironConfigWatcher, though we want it to become an EntityWatcher (to be called
NotifyWatcher.)

[fwereade, 2013-07-03 09:36:57]

LGTM

https://codereview.appspot.com/10809043/diff/17001/state/api/params/params.go
File state/api/params/params.go (right):

https://codereview.appspot.com/10809043/diff/17001/state/api/params/params.go...
state/api/params/params.go:75: // Details about a single agent
Agent identifies a single agent.

https://codereview.appspot.com/10809043/diff/17001/state/api/upgrader/upgrade...
File state/api/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/17001/state/api/upgrader/upgrade...
state/api/upgrader/upgrader_test.go:54: s.rawCharm, err =
s.State.Charm(charmURL(0))
not immediately clear how that charm gets into state in the first place, but if
it does, then fair enough:)

https://codereview.appspot.com/10809043/diff/17001/state/apiserver/upgrader/u...
File state/apiserver/upgrader/upgrader_test.go (right):

https://codereview.appspot.com/10809043/diff/17001/state/apiserver/upgrader/u...
state/apiserver/upgrader/upgrader_test.go:83: // TODO: When this becomes a true
EntityWatcher, use
I really appreciate blank lines before comments