upgrader: add Upgrader.DesiredVersion

upgrader: add Upgrader.DesiredVersion

The initial implementation of the API version of the Upgrader had a
single API endpoint Tools() that it used to find both the desired agent
version to run, and the URL to download tools matching that version.
This was simple, but caused bug #1218256. Specifically, it means that
the API Server needs to have provider (eg ec2) credentials in order to
answer the Tools request. (So it can search the storage and see what the
URL for downloading the tools is.)

As such after bootstrap (before the first connect), the Upgrader would
keep bouncing as it got an error trying to determine what agent version
it should run as.

This adds one more api: Upgrader.DesiredVersion(). This only returns the
agent version string. Which can be served directly from the DB after
bootstrap, without having to look at the provider at all.

And even beyond the first bootstrap, the current implementation of
WatchAPIVersion is very naive and will trigger a probe on any change to
EnvironConfig. At present that means it also triggers a listing of all
Storage buckets to find the tools that will match for that agent. With
the new implementation, it only provokes a DesiredVersion() call which
is relatively small and doesn't talk to a 3rd party data source.

This isn't strictly backwards compatible. If we upgrade a machine-1
agent before upgrading the machine-0 agent, machine-1 will keep bouncing
until machine-0 has finished upgrading and has the new API available.
That was already true of upgrading from 1.12 to 1.13.x, so we haven't
made the upgrade-to-1.14 story any worse. (Agents will still notice they
have to upgrade, and start trying to upgrade, they will just keep
bouncing after the upgrade until machine-0 finishes its upgrade.)

We *could* do something where if DesiredVersion can't be called, we fall
back to Tools and if that fails we fall back to direct DB access. But I
don't think it is worth spending the time on that code path right now.

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

(do not edit description out of merge proposal)

[jameinel, 2013-08-29 10:22:42]

Please take a look.

[jameinel, 2013-08-29 10:28:59]

I should add: I was not able to think of a way to properly test that we don't
call Tools unless DesiredVersion returns a changed version. I can think of a way
to do it with mocking out the actual api.State connection, but otherwise it
seems hard to do so.

I *did* however test this live. Bootstrapping an environment with 1.13.2, seeing
that the agent was bouncing. Then bootstrapping with the proposed patch and
seeing that the agent came up, made the single DesiredVersion request, and then
became quiet.

I did see it make further DesiredVersion requests as I started poking other
things, which means we *did* avoid futher Tools() requests when EnvironConfig
changes. (Such as on first connect when you do finally set all the secret
attributes.)

[rog, 2013-08-29 10:49:30]

This is generally really nice, but I have a few thoughts below.

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

https://codereview.appspot.com/13380043/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:47: func (st *State) DesiredVersion(tag string)
(*version.Number, error) {
version.Number is conventionally passed around by value, not pointer.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:93: func (u *UpgraderAPI)
DesiredVersion(args params.Entities) (params.AgentVersionResults, error) {
<rant>
I still think it's amazing that we have this API
that is specifically designed for the upgrader logic,
and we know the agent that's connected, but we
still insist on passing in a load of entities,
only to check that they're the same as the
connected entity (and then we ignore the entity
names anyway, as it happens)

Why can't we just return the agent version for
the connected agent and not worry about any of
this bulk crapola?

The same applies to Tools too of course, but I realise
we can't change that now.
</rant>

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:98: // For now, all agents get the same
proposed version
It would be nice to eliminate the code duplication
here and in Tools.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:233: func (s *upgraderSuite)
TestDesiredVersionRefusesWrongAgent(c *gc.C) {
If we are going to make this a bulk call, you should probably check what happens
if you pass more than one entity in the request.

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

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:128: var wantVersion *version.Number
I don't think there's any need to use a pointer here.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:145: if wantVersion != nil && *wantVersion !=
currentTools.Version.Number {
wantVersion can never be nil here, as we are always guaranteed that
we've passed through the DesiredVersion call when we get here.

If you wanted to be paranoid but still use a value type as
suggested, you could compare against version.Zero;
but I'd be inclined to just delete the first expression.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:149: logger.Errorf("failed to find tools matching
new requested version %v: %v", wantVersion, err)
I think it's probably reasonable to just return here.
That's what we usually do when we encounter API errors,
and this should never happen unless the API connection
has broken or something very unusual has happened
that probably won't be fixed by waiting a little while.

[rog, 2013-08-29 10:49:31]

This is generally really nice, but I have a few thoughts below.

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

https://codereview.appspot.com/13380043/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:47: func (st *State) DesiredVersion(tag string)
(*version.Number, error) {
version.Number is conventionally passed around by value, not pointer.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:93: func (u *UpgraderAPI)
DesiredVersion(args params.Entities) (params.AgentVersionResults, error) {
<rant>
I still think it's amazing that we have this API
that is specifically designed for the upgrader logic,
and we know the agent that's connected, but we
still insist on passing in a load of entities,
only to check that they're the same as the
connected entity (and then we ignore the entity
names anyway, as it happens)

Why can't we just return the agent version for
the connected agent and not worry about any of
this bulk crapola?

The same applies to Tools too of course, but I realise
we can't change that now.
</rant>

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:98: // For now, all agents get the same
proposed version
It would be nice to eliminate the code duplication
here and in Tools.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:233: func (s *upgraderSuite)
TestDesiredVersionRefusesWrongAgent(c *gc.C) {
If we are going to make this a bulk call, you should probably check what happens
if you pass more than one entity in the request.

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

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:128: var wantVersion *version.Number
I don't think there's any need to use a pointer here.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:145: if wantVersion != nil && *wantVersion !=
currentTools.Version.Number {
wantVersion can never be nil here, as we are always guaranteed that
we've passed through the DesiredVersion call when we get here.

If you wanted to be paranoid but still use a value type as
suggested, you could compare against version.Zero;
but I'd be inclined to just delete the first expression.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:149: logger.Errorf("failed to find tools matching
new requested version %v: %v", wantVersion, err)
I think it's probably reasonable to just return here.
That's what we usually do when we encounter API errors,
and this should never happen unless the API connection
has broken or something very unusual has happened
that probably won't be fixed by waiting a little while.

[jameinel, 2013-08-29 11:50:05]

Please take a look.

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

https://codereview.appspot.com/13380043/diff/1/state/api/upgrader/upgrader.go...
state/api/upgrader/upgrader.go:47: func (st *State) DesiredVersion(tag string)
(*version.Number, error) {
On 2013/08/29 10:49:31, rog wrote:
> version.Number is conventionally passed around by value, not pointer.

Done.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:98: // For now, all agents get the same
proposed version
On 2013/08/29 10:49:31, rog wrote:
> It would be nice to eliminate the code duplication
> here and in Tools.

There isn't actually much duplication. They have different return types, and the
only function in common is to grab the agentVersion from environ config.

Tools then needs to grab the environ itself and probe it, so its inner loop is
quite different.

I do think that when we have logic to say "upgrade machine-0 first" we'll want
to pull all of that out into a common bit of code.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader_test.go:233: func (s *upgraderSuite)
TestDesiredVersionRefusesWrongAgent(c *gc.C) {
On 2013/08/29 10:49:31, rog wrote:
> If we are going to make this a bulk call, you should probably check what
happens
> if you pass more than one entity in the request.

Done.

TestDesiredVersionNoticesMixing

This is a slightly different test to when the agent itself is unknown.

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

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:128: var wantVersion *version.Number
On 2013/08/29 10:49:31, rog wrote:
> I don't think there's any need to use a pointer here.

Right, it was just matching the way the rest of the API was done, but if we do
*Tools and not *Number then I'll play along.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:145: if wantVersion != nil && *wantVersion !=
currentTools.Version.Number {
On 2013/08/29 10:49:31, rog wrote:
> wantVersion can never be nil here, as we are always guaranteed that
> we've passed through the DesiredVersion call when we get here.
> 
> If you wanted to be paranoid but still use a value type as
> suggested, you could compare against version.Zero;
> but I'd be inclined to just delete the first expression.

It could be nil because DesiredVersion could have returned nil, nil.
But now it cannot because that api.Upgrader.DesiredVersion returns an value
rather than a pointer.

The biggest reason I did it, is that if I'm going to dereference a pointer
(*wantVersion) I want to make sure it isn't nil generating a panic. Now it
doesn't matter.

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:149: logger.Errorf("failed to find tools matching
new requested version %v: %v", wantVersion, err)
On 2013/08/29 10:49:31, rog wrote:
> I think it's probably reasonable to just return here.
> That's what we usually do when we encounter API errors,
> and this should never happen unless the API connection
> has broken or something very unusual has happened
> that probably won't be fixed by waiting a little while.

Done.

Though given this is Upgrader code, it is possible that we would get a
DesiredVersion that says to upgrade, but inbetween that an the next call the API
server reboots itself and then we make our next call which fails until the API
server is back up again.

But theoretically we just bounce ourselves and then try again anyway. I don't
*think* we'll get into a point where we can't make progress.

[rog, 2013-08-29 12:18:01]

LGTM modulo the below suggestion.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:98: // For now, all agents get the same
proposed version
On 2013/08/29 11:50:05, jameinel wrote:
> On 2013/08/29 10:49:31, rog wrote:
> > It would be nice to eliminate the code duplication
> > here and in Tools.
> 
> There isn't actually much duplication. They have different return types, and
the
> only function in common is to grab the agentVersion from environ config.
> 
> Tools then needs to grab the environ itself and probe it, so its inner loop is
> quite different.
> 
> I do think that when we have logic to say "upgrade machine-0 first" we'll want
> to pull all of that out into a common bit of code.

I was thinking of lines 98 to 106, which are almost identical to lines
124 to 132. 

I'd factor them out into something like this, to
make it obvious that they're doing exactly the same
thing (and it'll be easier to change when the agent
version isn't stored in the config any more for example).

func (u *UpgraderAPI) agentVersion() (version.Number, error) {
    cfg, err := u.st.EnvironConfig()
    if err != nil {
         return version.Zero, err
     }
     agentVersion, ok := cfg.AgentVersion()
     if !ok {
         return version.Zero, errors.New("agent version not set in 
     environment config")
     }
     return agentVersion, nil
}

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

https://codereview.appspot.com/13380043/diff/1/worker/upgrader/upgrader.go#ne...
worker/upgrader/upgrader.go:149: logger.Errorf("failed to find tools matching
new requested version %v: %v", wantVersion, err)
On 2013/08/29 11:50:05, jameinel wrote:
> On 2013/08/29 10:49:31, rog wrote:
> > I think it's probably reasonable to just return here.
> > That's what we usually do when we encounter API errors,
> > and this should never happen unless the API connection
> > has broken or something very unusual has happened
> > that probably won't be fixed by waiting a little while.
> 
> Done.
> 
> Though given this is Upgrader code, it is possible that we would get a
> DesiredVersion that says to upgrade, but inbetween that an the next call the
API
> server reboots itself and then we make our next call which fails until the API
> server is back up again.

If the API server bounces, our API calls will continually fail
until we re-connect to the API server, which won't happen until
we bounce, so returning is correct in that case too, I think.

[jameinel, 2013-08-29 12:31:45]

Please take a look.

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

https://codereview.appspot.com/13380043/diff/1/state/apiserver/upgrader/upgra...
state/apiserver/upgrader/upgrader.go:98: // For now, all agents get the same
proposed version

> I was thinking of lines 98 to 106, which are almost identical to lines
> 124 to 132. 
> 
> I'd factor them out into something like this, to
> make it obvious that they're doing exactly the same
> thing (and it'll be easier to change when the agent
> version isn't stored in the config any more for example).
> 
> func (u *UpgraderAPI) agentVersion() (version.Number, error) {
>     cfg, err := u.st.EnvironConfig()
>     if err != nil {
>          return version.Zero, err
>      }
>      agentVersion, ok := cfg.AgentVersion()
>      if !ok {
>          return version.Zero, errors.New("agent version not set in 
>      environment config")
>      }
>      return agentVersion, nil
> }

So I did try this, as the code is similar. But in the case of DesiredVersion we
don't need the cfg object anymore (and an agentVersion function looks like it
shouldn't return the cfg). But in the case of Tools we *do* need the cfg around
because we use it in the next line to open up the connection to the actual
environment.
So either you need to actually call u.st.EnvironConfig 2 times, or you need to
return it with the agentVersion, but then you've lost the coherency of what a
separate function buys you.

I'll do it, just to get it landed without having another go around, but it
wasn't that great of a fit.

[rog, 2013-08-29 14:48:50]

LGTM

https://codereview.appspot.com/13380043/diff/13001/state/apiserver/upgrader/u...
File state/apiserver/upgrader/upgrader.go (right):

https://codereview.appspot.com/13380043/diff/13001/state/apiserver/upgrader/u...
state/apiserver/upgrader/upgrader.go:93: func (u *UpgraderAPI)
getGlobalAgentVersion() (version.Number, *config.Config, error) {
Thanks for doing this - even though the values needed are slightly different
in both cases, I think it looks nicer with it factored out.