cmd/jujud: add Upgrader type.

cmd/jujud: add Upgrader type.

This conforms to the task interface as expected by
runTasks, which should make it simple to add upgrading
logic to agents.

https://code.launchpad.net/~rogpeppe/juju-core/021-jujud-upgrader/+merge/120089

(do not edit description out of merge proposal)

[rog, 2012-08-17 09:29:11]

Please take a look.

[rog, 2012-08-17 09:47:37]

Please take a look.

[fwereade, 2012-08-17 12:53:22]

looks good, but a few points:

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode15
cmd/jujud/upgrade.go:15: type Upgrader struct {
docs

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode21
cmd/jujud/upgrade.go:21: type UpgradedError struct {
docs

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode29
cmd/jujud/upgrade.go:29: type AgentState interface {
docs

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode77
cmd/jujud/upgrade.go:77: w := u.agentState.WatchProposedAgentTools()
defer watcher.Stop(w, &u.tomb)

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode128
cmd/jujud/upgrade.go:128: log.Printf("%s agent: cannot remove temporary download
file: %v", err)
s/err/u.agentName, err/

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:29: s.LoggingSuite.SetUpTest(c)
do we still need this?

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:51: st.Close()
defer the close when it's opened?

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:63: st, err := state.Open(s.StateInfo(c))
defer st.Close()

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:221: // TODO(rog) add logic to make this return an
error
should this be addressed before we merge?

[niemeyer, 2012-08-17 14:49:23]

Very nice!

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode26
cmd/jujud/upgrade.go:26: return fmt.Sprintf("agent has been upgraded to %v (from
%q)", e.tools.Binary, e.tools.URL)
"must restart: agent has been ..."

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode36
cmd/jujud/upgrade.go:36: // will return an UpgradedError describing the new
tools.
// ... given name. If an upgrade happens, Wait and Stop will return an
// UpgradedError to notify that a version replacement is necessary.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode43
cmd/jujud/upgrade.go:43: defer u.tomb.Done()
FWIW, this would work:

u.tomb.Kill(u.run())

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode67
cmd/jujud/upgrade.go:67: log.Printf("cannot read tools directory: %v", err)
"upgrader cannot read current tools: %v", err

This is the local context. The error itself will have more details of the issue.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode95
cmd/jujud/upgrade.go:95: // If there's a download in progress, stop it if we
need to.
It's significantly easier to both read and comment logic if the comment tells
what the branch is covering, rather than introducing a conditional that forces
the reader to compute it again in his mind to tell what the branch is doing.

Compare this to the version in the diff below:

if download != nil {
    // There's a download in progress.
    if *tools == *downloadTools {
        // We are already downloading the requested tools.
        break
    }
    // Tools changed. We need to stop and restart.
    ...
}

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode112
cmd/jujud/upgrade.go:112: if tools, err :=
environs.ChangeAgentTools(u.agentName, tools.Binary); err == nil {
This needs to change since it can't happen concurrently, as we covered online.

[rog, 2012-08-17 15:15:32]

Please take a look.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode15
cmd/jujud/upgrade.go:15: type Upgrader struct {
On 2012/08/17 12:53:22, fwereade wrote:
> docs

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode21
cmd/jujud/upgrade.go:21: type UpgradedError struct {
On 2012/08/17 12:53:22, fwereade wrote:
> docs

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode26
cmd/jujud/upgrade.go:26: return fmt.Sprintf("agent has been upgraded to %v (from
%q)", e.tools.Binary, e.tools.URL)
On 2012/08/17 14:49:23, niemeyer wrote:
> "must restart: agent has been ..."

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode29
cmd/jujud/upgrade.go:29: type AgentState interface {
On 2012/08/17 12:53:22, fwereade wrote:
> docs

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode36
cmd/jujud/upgrade.go:36: // will return an UpgradedError describing the new
tools.
On 2012/08/17 14:49:23, niemeyer wrote:
> // ... given name. If an upgrade happens, Wait and Stop will return an
> // UpgradedError to notify that a version replacement is necessary.

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode43
cmd/jujud/upgrade.go:43: defer u.tomb.Done()
On 2012/08/17 14:49:23, niemeyer wrote:
> FWIW, this would work:
> 
> u.tomb.Kill(u.run())

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode67
cmd/jujud/upgrade.go:67: log.Printf("cannot read tools directory: %v", err)
On 2012/08/17 14:49:23, niemeyer wrote:
> "upgrader cannot read current tools: %v", err
> 
> This is the local context. The error itself will have more details of the
issue.

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode77
cmd/jujud/upgrade.go:77: w := u.agentState.WatchProposedAgentTools()
On 2012/08/17 12:53:22, fwereade wrote:
> defer watcher.Stop(w, &u.tomb)

good catch. done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode95
cmd/jujud/upgrade.go:95: // If there's a download in progress, stop it if we
need to.
On 2012/08/17 14:49:23, niemeyer wrote:
> It's significantly easier to both read and comment logic if the comment tells
> what the branch is covering, rather than introducing a conditional that forces
> the reader to compute it again in his mind to tell what the branch is doing.
> 
> Compare this to the version in the diff below:
> 
> if download != nil {
>     // There's a download in progress.
>     if *tools == *downloadTools {
>         // We are already downloading the requested tools.
>         break
>     }
>     // Tools changed. We need to stop and restart.
>     ...
> }

i've never been sure which way is best here. thanks for the guidance. done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode112
cmd/jujud/upgrade.go:112: if tools, err :=
environs.ChangeAgentTools(u.agentName, tools.Binary); err == nil {
On 2012/08/17 14:49:23, niemeyer wrote:
> This needs to change since it can't happen concurrently, as we covered online.

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade.go#newcode128
cmd/jujud/upgrade.go:128: log.Printf("%s agent: cannot remove temporary download
file: %v", err)
On 2012/08/17 12:53:22, fwereade wrote:
> s/err/u.agentName, err/

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:29: s.LoggingSuite.SetUpTest(c)
On 2012/08/17 12:53:22, fwereade wrote:
> do we still need this?

Done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:51: st.Close()
On 2012/08/17 12:53:22, fwereade wrote:
> defer the close when it's opened?

no, the whole point of this test is that we close the State underneath the
upgrader.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:63: st, err := state.Open(s.StateInfo(c))
On 2012/08/17 12:53:22, fwereade wrote:
> defer st.Close()

... or just use s.State. done.

https://codereview.appspot.com/6464074/diff/1043/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:221: // TODO(rog) add logic to make this return an
error
On 2012/08/17 12:53:22, fwereade wrote:
> should this be addressed before we merge?

nah, the logic is simple enough to be trivial (and the error path is checked
elsewhere). i'll remove the TODO.

[rog, 2012-08-17 15:38:58]

Please take a look.

[niemeyer, 2012-08-17 15:57:09]

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode15
cmd/jujud/upgrade.go:15: // Upgrader represents a upgrader task.
// An Upgrader observes the version information for an agent in the
// environment state, and handles the downloading and unpacking of
// new versions of the juju tools when necessary.
//
// When a new version is available Wait and Stop return UpgradedError.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode22
cmd/jujud/upgrade.go:22: // UpgradedError the status from an Upgrader when
// UpgradedError is returned by an Upgrader to report that
// an upgrade has been performed and a restart is due.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode32
cmd/jujud/upgrade.go:32: // AgentState represents the state of an agent.
// The AgentState interface is implemented by state types
// that represent running agents.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode43
cmd/jujud/upgrade.go:43: // tools for the agent with the given name.  given
name.  If an upgrade
// NewUpgrader returns a new Upgrader watching the given agent.

The rest is documented in the respective types.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode46
cmd/jujud/upgrade.go:46: func NewUpgrader(agentName string, as AgentState)
*Upgrader {
s/as/agentState/

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode129
cmd/jujud/upgrade.go:129: log.Printf("download %v failed: %v", tools.Binary,
status.Err)
s/download/& of/

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode135
cmd/jujud/upgrade.go:135: log.Printf("%s agent: cannot remove temporary download
file: %v", u.agentName, err)
Why is this prefixed by "%s agent:" and not everything else?

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode138
cmd/jujud/upgrade.go:138: log.Printf("unpack error: %v", err)
"cannot unpack %v tools: %v", tools.Binary, err

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:116: }
storage := s.Conn.Environ.Storage()

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:186: case <-time.After(200 * time.Millisecond):
Can we reduce this to 100?  It's a guess anyway, so we may as well not delay the
test further.

[niemeyer, 2012-08-17 15:58:42]

Oh, LGTM with those fixed.

[rog, 2012-08-17 16:12:49]

*** Submitted:

cmd/jujud: add Upgrader type.

This conforms to the task interface as expected by
runTasks, which should make it simple to add upgrading
logic to agents.

R=fwereade, niemeyer
CC=
https://codereview.appspot.com/6464074

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go
File cmd/jujud/upgrade.go (right):

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode15
cmd/jujud/upgrade.go:15: // Upgrader represents a upgrader task.
On 2012/08/17 15:57:09, niemeyer wrote:
> // An Upgrader observes the version information for an agent in the
> // environment state, and handles the downloading and unpacking of
> // new versions of the juju tools when necessary.
> //
> // When a new version is available Wait and Stop return UpgradedError.

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode22
cmd/jujud/upgrade.go:22: // UpgradedError the status from an Upgrader when
On 2012/08/17 15:57:09, niemeyer wrote:
> // UpgradedError is returned by an Upgrader to report that
> // an upgrade has been performed and a restart is due.

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode32
cmd/jujud/upgrade.go:32: // AgentState represents the state of an agent.
On 2012/08/17 15:57:09, niemeyer wrote:
> // The AgentState interface is implemented by state types
> // that represent running agents.

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode43
cmd/jujud/upgrade.go:43: // tools for the agent with the given name.  given
name.  If an upgrade
On 2012/08/17 15:57:09, niemeyer wrote:
> // NewUpgrader returns a new Upgrader watching the given agent.
> 
> The rest is documented in the respective types.

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode46
cmd/jujud/upgrade.go:46: func NewUpgrader(agentName string, as AgentState)
*Upgrader {
On 2012/08/17 15:57:09, niemeyer wrote:
> s/as/agentState/

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode129
cmd/jujud/upgrade.go:129: log.Printf("download %v failed: %v", tools.Binary,
status.Err)
On 2012/08/17 15:57:09, niemeyer wrote:
> s/download/& of/

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade.go#newcode135
cmd/jujud/upgrade.go:135: log.Printf("%s agent: cannot remove temporary download
file: %v", u.agentName, err)
On 2012/08/17 15:57:09, niemeyer wrote:
> Why is this prefixed by "%s agent:" and not everything else?

no reason. done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go
File cmd/jujud/upgrade_test.go (right):

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:116: }
On 2012/08/17 15:57:09, niemeyer wrote:
> storage := s.Conn.Environ.Storage()

Done.

https://codereview.appspot.com/6464074/diff/2005/cmd/jujud/upgrade_test.go#ne...
cmd/jujud/upgrade_test.go:186: case <-time.After(200 * time.Millisecond):
On 2012/08/17 15:57:09, niemeyer wrote:
> Can we reduce this to 100?  It's a guess anyway, so we may as well not delay
the
> test further.

Done.