Merge lp://staging/~menno.smits/juju-core/upgrade-comments into lp://staging/~go-bot/juju-core/trunk

Reviewers: mp+221354_code.launchpad.net,

Message:
Please take a look.

Description:
Comment improvements relating to upgrades

A comment correction and a comment addition relating to different bits
of upgrade infrastructure (I've been learning how juju upgrades work).

https://code.launchpad.net/~menno.smits/juju-core/upgrade-comments/+merge/221354

(do not edit description out of merge proposal)

Please review this at https://codereview.appspot.com/92700043/

Affected files (+14, -1 lines):
   A [revision details]
   M cmd/jujud/machine.go
   M state/apiserver/upgrader/upgrader.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: tarmac-20140528163454-1tbj23pzx5x4g4g0
+New revision: <email address hidden>

Index: cmd/jujud/machine.go
=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go 2014-05-28 10:24:07 +0000
+++ cmd/jujud/machine.go 2014-05-28 22:24:23 +0000
@@ -693,7 +693,8 @@
     return nil
    default:
    }
- // If the machine agent is a state server, wait until state is opened.
+ // If the machine agent is a state server, open state before
+ // running upgrade steps
    needsState := false
    for _, job := range jobs {
     if job == params.JobManageEnviron {

Index: state/apiserver/upgrader/upgrader.go
=== modified file 'state/apiserver/upgrader/upgrader.go'
--- state/apiserver/upgrader/upgrader.go 2014-04-09 12:20:55 +0000
+++ state/apiserver/upgrader/upgrader.go 2014-05-28 22:24:23 +0000
@@ -126,6 +126,16 @@
   for i, entity := range args.Entities {
    err := common.ErrPerm
    if u.authorizer.AuthOwner(entity.Tag) {
+ // Only return the globally desired agent version if the
+ // asking entity is a machine agent with JobManageEnviron or
+ // if this API server is running the globally desired agent
+ // version. Otherwise report this API server's current
+ // agent version.
+ //
+ // This ensures that state machine agents will upgrade
+ // first - once they have restarted and are running the
+ // new version other agents will start to see the new
+ // agent version.
     if !isNewerVersion || u.entityIsManager(entity.Tag) {
      results[i].Version = &agentVersion
     } else {

LGTM

https://codereview.appspot.com/92700043/

LGTM just had a trivial non-blocking comment.

https://codereview.appspot.com/92700043/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/92700043/diff/1/cmd/jujud/machine.go#newcode696
cmd/jujud/machine.go:696: // If the machine agent is a state server,
open state before
The comment threw me a bit, I feel like it should read something like
flag needState or what have you, since the the loop isn't actually doing
the opening, that is happening on #714 after checking needState *shrug*

https://codereview.appspot.com/92700043/

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.

https://codereview.appspot.com/92700043/diff/1/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/92700043/diff/1/cmd/jujud/machine.go#newcode696
cmd/jujud/machine.go:696: // If the machine agent is a state server,
open state before
On 2014/05/30 16:44:31, wwitzel3 wrote:
> The comment threw me a bit, I feel like it should read something like
flag
> needState or what have you, since the the loop isn't actually doing
the opening,
> that is happening on #714 after checking needState *shrug*

I've (hopefully) clarified the comment. Thanks.

https://codereview.appspot.com/92700043/