Fix upgrade 1.14 -> 1.15

Fix upgrade 1.14 -> 1.15

This adds a Tools() API call to provisioner API,
similarly to the upgrader API. Common code factored
out into apiserver/common/tools.go.

SetAgentTools() renamed to SetAgentVersion() in
machine and unit state types. Also renamed a few
types in params to be shorter or better reflect
what are they about. Added DEPRECATE(v1.18) tags
to bits of the API that need cleanup.

Live tested on EC2 - after copying the tools from
/tools/releases/ to /tools/ the upgrade proceeds
and finishes successfully.

https://code.launchpad.net/~dimitern/juju-core/150-fix-1.14-upgrade-upgrader/+merge/188802

(do not edit description out of merge proposal)

[dimitern, 2013-10-02 10:24:58]

Please take a look.

[fwereade, 2013-10-02 11:22:04]

comments mostly as discussed live, I think

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:377: s.machine.SetAgentTools(curTools)
Eww. Good enough, I guess, the provisioner can just keep on failing out until
the upgrader sets some.

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

https://codereview.appspot.com/14231044/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:72: // Tools returns the agent tools for the
given entity.
tools that should run on the given entity?

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:703: // having to pass it in
again
Technically not true, we could get them from the machine doc and its hardware
doc. But as stated before we can live with this for now -- just clarify the
comment a little please.

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:705: URL:     "",
can't we just drop that field?

https://codereview.appspot.com/14231044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/14231044/diff/1/state/machine.go#newcode210
state/machine.go:210: // 1.15. It needs to be removed in the futute.
Can't we just fix Machine.SetAgentTools to accept a version.Binary, do the same
for Unit, and just completely ignore this other rubbish?

https://codereview.appspot.com/14231044/diff/1/state/tools_test.go
File state/tools_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/tools_test.go#newcode54
state/tools_test.go:54: // size and checksum in SetAgentTools().
Definitely not. Anyone setting that rubbish in state will feel my wrath :).

https://codereview.appspot.com/14231044/diff/1/worker/provisioner/lxc-broker_...
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/14231044/diff/1/worker/provisioner/lxc-broker_...
worker/provisioner/lxc-broker_test.go:191: err =
m.SetAgentTools(&coretools.Tools{Version: version.Current})
This is similarly worth a "this is crack but just good enough" comment

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:101: currentTools, err := u.st.Tools(u.tag)
Whaaa?

currentTools := &coretools.Tools{Version: version.Current}

that's *all* we need.

[jameinel, 2013-10-02 12:21:31]

I think the change in worker/upgrader/upgrader.go will actually break the worker
because it can't get Tools without first calling SetTools (where we look up the
series and arch for that agent).

https://codereview.appspot.com/14231044/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (left):

https://codereview.appspot.com/14231044/diff/1/cmd/juju/destroyenvironment.go...
cmd/juju/destroyenvironment.go:49: }
I don't see how this change is related. Can you elaborate?

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:377: s.machine.SetAgentTools(curTools)
On 2013/10/02 11:22:04, fwereade wrote:
> Eww. Good enough, I guess, the provisioner can just keep on failing out until
> the upgrader sets some.

I *think* the ordering is ok. Because SetTools is called as the first thing in
Upgrader, and Provisioner should be waiting until we have the Environment
available. There might still be a race condition, I guess.

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:101: currentTools, err := u.st.Tools(u.tag)
On 2013/10/02 11:22:04, fwereade wrote:
> Whaaa?
> 
> currentTools := &coretools.Tools{Version: version.Current}
> 
> that's *all* we need.

Right, I think calling Tools first here is actually broken. We need the call to
SetTools in 2 lines in order for us to properly find Tools.

And the Tools here have a URL we won't use, and require us to have enabled
Environment credentials which we don't have until someone runs "juju status".

So +1 on William's suggestion.

[dimitern, 2013-10-02 13:33:32]

Please take a look.

https://codereview.appspot.com/14231044/diff/1/cmd/juju/destroyenvironment.go
File cmd/juju/destroyenvironment.go (left):

https://codereview.appspot.com/14231044/diff/1/cmd/juju/destroyenvironment.go...
cmd/juju/destroyenvironment.go:49: }
On 2013/10/02 12:21:31, jameinel wrote:
> I don't see how this change is related. Can you elaborate?

It's something I found - you can't destroy a 1.14 env with 1.15 tools because of
this, but rog already fixed that, so I'll remove it.

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
File state/api/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/api/provisioner/provisio...
state/api/provisioner/provisioner_test.go:377: s.machine.SetAgentTools(curTools)
On 2013/10/02 12:21:31, jameinel wrote:
> On 2013/10/02 11:22:04, fwereade wrote:
> > Eww. Good enough, I guess, the provisioner can just keep on failing out
until
> > the upgrader sets some.
> 
> I *think* the ordering is ok. Because SetTools is called as the first thing in
> Upgrader, and Provisioner should be waiting until we have the Environment
> available. There might still be a race condition, I guess.

Yeah.

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

https://codereview.appspot.com/14231044/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:72: // Tools returns the agent tools for the
given entity.
On 2013/10/02 11:22:04, fwereade wrote:
> tools that should run on the given entity?

Done.

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
File state/apiserver/provisioner/provisioner_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:703: // having to pass it in
again
On 2013/10/02 11:22:04, fwereade wrote:
> Technically not true, we could get them from the machine doc and its hardware
> doc. But as stated before we can live with this for now -- just clarify the
> comment a little please.

I'm open to suggestions on a better comment. I don't get even the current one
entirely - it's just copied from the upgrader tests.

https://codereview.appspot.com/14231044/diff/1/state/apiserver/provisioner/pr...
state/apiserver/provisioner/provisioner_test.go:705: URL:     "",
On 2013/10/02 11:22:04, fwereade wrote:
> can't we just drop that field?

Done.

https://codereview.appspot.com/14231044/diff/1/state/machine.go
File state/machine.go (right):

https://codereview.appspot.com/14231044/diff/1/state/machine.go#newcode210
state/machine.go:210: // 1.15. It needs to be removed in the futute.
On 2013/10/02 11:22:04, fwereade wrote:
> Can't we just fix Machine.SetAgentTools to accept a version.Binary, do the
same
> for Unit, and just completely ignore this other rubbish?

I'll rename it to SetAgentVersion, as discussed.

https://codereview.appspot.com/14231044/diff/1/state/tools_test.go
File state/tools_test.go (right):

https://codereview.appspot.com/14231044/diff/1/state/tools_test.go#newcode54
state/tools_test.go:54: // size and checksum in SetAgentTools().
On 2013/10/02 11:22:04, fwereade wrote:
> Definitely not. Anyone setting that rubbish in state will feel my wrath :).

OK :)

https://codereview.appspot.com/14231044/diff/1/worker/provisioner/lxc-broker_...
File worker/provisioner/lxc-broker_test.go (right):

https://codereview.appspot.com/14231044/diff/1/worker/provisioner/lxc-broker_...
worker/provisioner/lxc-broker_test.go:191: err =
m.SetAgentTools(&coretools.Tools{Version: version.Current})
On 2013/10/02 11:22:04, fwereade wrote:
> This is similarly worth a "this is crack but just good enough" comment

Done.

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:101: currentTools, err := u.st.Tools(u.tag)
On 2013/10/02 11:22:04, fwereade wrote:
> Whaaa?
> 
> currentTools := &coretools.Tools{Version: version.Current}
> 
> that's *all* we need.

Done.

https://codereview.appspot.com/14231044/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:101: currentTools, err := u.st.Tools(u.tag)
On 2013/10/02 12:21:31, jameinel wrote:
> On 2013/10/02 11:22:04, fwereade wrote:
> > Whaaa?
> > 
> > currentTools := &coretools.Tools{Version: version.Current}
> > 
> > that's *all* we need.
> 
> Right, I think calling Tools first here is actually broken. We need the call
to
> SetTools in 2 lines in order for us to properly find Tools.
> 
> And the Tools here have a URL we won't use, and require us to have enabled
> Environment credentials which we don't have until someone runs "juju status".
> 
> So +1 on William's suggestion.

Done.

[jameinel, 2013-10-02 13:53:21]

generally LGTM, though we discussed on IRC what was concerning me

https://codereview.appspot.com/14231044/diff/10001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/params/internal....
state/api/params/internal.go:390: // *tools.Tools here.
I'm not particularly comfortable changing the struct name at this point, though
I can understand it is "ok" (won't actually break anything).

[fwereade, 2013-10-02 14:21:03]

More thoughts, open to some argument on most of them I think.

https://codereview.appspot.com/14231044/diff/10001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/14231044/diff/10001/environs/testing/tools.go#...
environs/testing/tools.go:59: c.Assert(obtained.URL, gc.Equals, expected.URL)
hmm, where's this used? Shouldn't we know exactly what we expect?

I guess it's in line with the existing implementation, but it sucks pretty hard.
Bug it?

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
state/api/upgrader/upgrader.go:32: // DEPRECATE(v1.18) Rename this to
SetVersion.
can't this just be renamed right now?

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go
File state/tools_test.go (left):

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go#oldcode60
state/tools_test.go:60: t3, err = obj.AgentTools()
we should fix this in a followup

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:192: return p.st.Tools(p.agentConfig.Tag())
is it feasible to just drop that HasTools nonsense entirely and grab the tools
from the API at provisioning time instead of setup time?

https://codereview.appspot.com/14231044/diff/10001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/upgrader/upgrader.g...
worker/upgrader/upgrader.go:102: err := u.st.SetTools(u.tag, currentTools)
yeah, this would make a good SetVersion right now

[dimitern, 2013-10-02 14:33:56]

Please take a look.

https://codereview.appspot.com/14231044/diff/10001/environs/testing/tools.go
File environs/testing/tools.go (right):

https://codereview.appspot.com/14231044/diff/10001/environs/testing/tools.go#...
environs/testing/tools.go:59: c.Assert(obtained.URL, gc.Equals, expected.URL)
On 2013/10/02 14:21:04, fwereade wrote:
> hmm, where's this used? Shouldn't we know exactly what we expect?
> 
> I guess it's in line with the existing implementation, but it sucks pretty
hard.
> Bug it?

Done.

https://codereview.appspot.com/14231044/diff/10001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/params/internal....
state/api/params/internal.go:390: // *tools.Tools here.
On 2013/10/02 13:53:22, jameinel wrote:
> I'm not particularly comfortable changing the struct name at this point,
though
> I can understand it is "ok" (won't actually break anything).

As discussed with fwereade, will change the type of Tools to be struct Version
{Version version.Binary}, as it is compatible with tools.Tools.

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
state/api/upgrader/upgrader.go:32: // DEPRECATE(v1.18) Rename this to
SetVersion.
On 2013/10/02 14:21:04, fwereade wrote:
> can't this just be renamed right now?

No, because it will be backwards-incompatible.

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go
File state/tools_test.go (left):

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go#oldcode60
state/tools_test.go:60: t3, err = obj.AgentTools()
On 2013/10/02 14:21:04, fwereade wrote:
> we should fix this in a followup

Fix what?

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:192: return p.st.Tools(p.agentConfig.Tag())
On 2013/10/02 14:21:04, fwereade wrote:
> is it feasible to just drop that HasTools nonsense entirely and grab the tools
> from the API at provisioning time instead of setup time?

I don't know, but it seems way out of scope.

https://codereview.appspot.com/14231044/diff/10001/worker/upgrader/upgrader.go
File worker/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/upgrader/upgrader.g...
worker/upgrader/upgrader.go:102: err := u.st.SetTools(u.tag, currentTools)
On 2013/10/02 14:21:04, fwereade wrote:
> yeah, this would make a good SetVersion right now

It would, if it wasn't incompatible.

[fwereade, 2013-10-02 16:06:49]

LGTM, but please drop the DEPRECATE business in favour of a bug to clean up the
API. There's nothing 1.18 specific AFAICT.

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
state/api/upgrader/upgrader.go:32: // DEPRECATE(v1.18) Rename this to
SetVersion.
On 2013/10/02 14:33:57, dimitern wrote:
> On 2013/10/02 14:21:04, fwereade wrote:
> > can't this just be renamed right now?
> 
> No, because it will be backwards-incompatible.

This is completely internal to the code running a particular version, though.

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go
File state/tools_test.go (left):

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go#oldcode60
state/tools_test.go:60: t3, err = obj.AgentTools()
On 2013/10/02 14:33:57, dimitern wrote:
> On 2013/10/02 14:21:04, fwereade wrote:
> > we should fix this in a followup
> 
> Fix what?

Agents returning tools.

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:192: return p.st.Tools(p.agentConfig.Tag())
On 2013/10/02 14:33:57, dimitern wrote:
> On 2013/10/02 14:21:04, fwereade wrote:
> > is it feasible to just drop that HasTools nonsense entirely and grab the
tools
> > from the API at provisioning time instead of setup time?
> 
> I don't know, but it seems way out of scope.

Fair enough. I think we should have a followup that trashes HasTools, the lxc
broker.Tools, and ReadTools, because they're all basically useless.

https://codereview.appspot.com/14231044/diff/19001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/14231044/diff/19001/state/api/params/internal....
state/api/params/internal.go:388: // DEPRECATE(v1.18) Remove this and use
version.Binary directly.
Why? Please don't commit to changing the API unless we have to. "wrong" we have
to change, but "a little bit sucky" is not worth the pain and churn IMO.

[dimitern, 2013-10-02 16:29:55]

Please take a look.

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
File state/api/upgrader/upgrader.go (right):

https://codereview.appspot.com/14231044/diff/10001/state/api/upgrader/upgrade...
state/api/upgrader/upgrader.go:32: // DEPRECATE(v1.18) Rename this to
SetVersion.
On 2013/10/02 16:06:50, fwereade wrote:
> On 2013/10/02 14:33:57, dimitern wrote:
> > On 2013/10/02 14:21:04, fwereade wrote:
> > > can't this just be renamed right now?
> > 
> > No, because it will be backwards-incompatible.
> 
> This is completely internal to the code running a particular version, though.

Renamed to SetVersion.

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go
File state/tools_test.go (left):

https://codereview.appspot.com/14231044/diff/10001/state/tools_test.go#oldcode60
state/tools_test.go:60: t3, err = obj.AgentTools()
On 2013/10/02 16:06:50, fwereade wrote:
> On 2013/10/02 14:33:57, dimitern wrote:
> > On 2013/10/02 14:21:04, fwereade wrote:
> > > we should fix this in a followup
> > 
> > Fix what?
> 
> Agents returning tools.

I'm not comfortable with this, unless we discuss it first.

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
File worker/provisioner/provisioner.go (right):

https://codereview.appspot.com/14231044/diff/10001/worker/provisioner/provisi...
worker/provisioner/provisioner.go:192: return p.st.Tools(p.agentConfig.Tag())
On 2013/10/02 16:06:50, fwereade wrote:
> On 2013/10/02 14:33:57, dimitern wrote:
> > On 2013/10/02 14:21:04, fwereade wrote:
> > > is it feasible to just drop that HasTools nonsense entirely and grab the
> tools
> > > from the API at provisioning time instead of setup time?
> > 
> > I don't know, but it seems way out of scope.
> 
> Fair enough. I think we should have a followup that trashes HasTools, the lxc
> broker.Tools, and ReadTools, because they're all basically useless.

Ok, will do.

https://codereview.appspot.com/14231044/diff/19001/state/api/params/internal.go
File state/api/params/internal.go (right):

https://codereview.appspot.com/14231044/diff/19001/state/api/params/internal....
state/api/params/internal.go:388: // DEPRECATE(v1.18) Remove this and use
version.Binary directly.
On 2013/10/02 16:06:50, fwereade wrote:
> Why? Please don't commit to changing the API unless we have to. "wrong" we
have
> to change, but "a little bit sucky" is not worth the pain and churn IMO.

Removed the DEPRECATE(v1.18) tags *everywhere*. I'm giving up.

[dimitern, 2013-10-02 16:30:59]

I also filed a bug about API cleanup:
https://bugs.launchpad.net/juju-core/+bug/1234287

So I'm landing this now.