Code review comment for lp:~jameinel/juju-core/desiredversion-not-newer-1304340

Reviewers: mp+214743_code.launchpad.net,

Message:
Please take a look.

Description:
state/apiserver/upgrader: Managers upgrade first

This addresses bug #1304340. It gives us a step along the path of having
Manager nodes (API Servers) upgrade themselves before we have all the
other machine agents upgrade (before we have the Unit agents upgrade).
We already had the last two steps. This isn't 100% reliable in HA
circumstances, because we aren't waiting for *all* nodes to be upgraded.

We could potentially change the check so it was just if DesiredVersion
!= CurrentVersion (rather than >= CurrentVersion). It also doesn't probe
the database unless there is an upgrade pending, so DesiredVersion
should still be cheap. This doesn't change FindTools, but it doesn't
seem like it needs to. (All places that call FindTools first call
DesiredVersion, because we broke stuff in the past when we didn't have
API Credentials in time.)

Also, this falls back to returning version.Current rather than returning
the version of the machine/unit/etc had recorded in State. I'm not sure
if that is worth implementing, but when we do the DB lookup to find out
if this entity.IsManager() we could grab its current version.

https://code.launchpad.net/~jameinel/juju-core/desiredversion-not-newer-1304340/+merge/214743

(do not edit description out of merge proposal)

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

Affected files (+86, -1 lines):
   A [revision details]
   M state/apiserver/upgrader/upgrader.go
   M state/apiserver/upgrader/upgrader_test.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-20140408095416-cm5185gdi4wm6d6n
+New revision: <email address hidden>

Index: state/apiserver/upgrader/upgrader.go
=== modified file 'state/apiserver/upgrader/upgrader.go'
--- state/apiserver/upgrader/upgrader.go 2014-02-18 00:15:30 +0000
+++ state/apiserver/upgrader/upgrader.go 2014-04-08 12:50:26 +0000
@@ -6,6 +6,8 @@
  import (
   "errors"

+ "github.com/juju/loggo"
+
   "launchpad.net/juju-core/environs/config"
   "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api/params"
@@ -14,6 +16,8 @@
   "launchpad.net/juju-core/version"
  )

+var logger = loggo.GetLogger("juju.state.apiserver.upgrader")
+
  type Upgrader interface {
   WatchAPIVersion(args params.Entities) (params.NotifyWatchResults, error)
   DesiredVersion(args params.Entities) (params.VersionResults, error)
@@ -91,6 +95,22 @@
   return agentVersion, cfg, nil
  }

+type hasIsManager interface {
+ IsManager() bool
+}
+
+func (u *UpgraderAPI) entityIsManager(tag string) bool {
+ entity, err := u.st.FindEntity(tag)
+ if err != nil {
+ return false
+ }
+ if m, ok := entity.(hasIsManager); !ok {
+ return false
+ } else {
+ return m.IsManager()
+ }
+}
+
  // DesiredVersion reports the Agent Version that we want that agent to be
running
  func (u *UpgraderAPI) DesiredVersion(args params.Entities)
(params.VersionResults, error) {
   results := make([]params.VersionResult, len(args.Entities))
@@ -101,10 +121,17 @@
   if err != nil {
    return params.VersionResults{}, common.ServerError(err)
   }
+ // Is the desired version greater than the current API server version?
+ isNewerVersion := agentVersion.Compare(version.Current.Number) > 0
   for i, entity := range args.Entities {
    err := common.ErrPerm
    if u.authorizer.AuthOwner(entity.Tag) {
- results[i].Version = &agentVersion
+ if !isNewerVersion || u.entityIsManager(entity.Tag) {
+ results[i].Version = &agentVersion
+ } else {
+ logger.Debugf("desired version is %s, but current version is %s and
agent is not a manager node", agentVersion, version.Current.Number)
+ results[i].Version = &version.Current.Number
+ }
     err = nil
    }
    results[i].Error = common.ServerError(err)

Index: state/apiserver/upgrader/upgrader_test.go
=== modified file 'state/apiserver/upgrader/upgrader_test.go'
--- state/apiserver/upgrader/upgrader_test.go 2014-03-13 07:54:56 +0000
+++ state/apiserver/upgrader/upgrader_test.go 2014-04-08 12:50:26 +0000
@@ -25,6 +25,7 @@
   // These are raw State objects. Use them for setup and assertions, but
   // should never be touched by the API calls themselves
   rawMachine *state.Machine
+ apiMachine *state.Machine
   upgrader *upgrader.UpgraderAPI
   resources *common.Resources
   authorizer apiservertesting.FakeAuthorizer
@@ -38,6 +39,11 @@

   // Create a machine to work with
   var err error
+ // The first machine created is the only one allowed to
+ // JobManageEnviron
+ s.apiMachine, err = s.State.AddMachine("quantal", state.JobHostUnits,
+ state.JobManageEnviron)
+ c.Assert(err, gc.IsNil)
   s.rawMachine, err = s.State.AddMachine("quantal", state.JobHostUnits)
   c.Assert(err, gc.IsNil)

@@ -277,3 +283,53 @@
   c.Assert(agentVersion, gc.NotNil)
   c.Check(*agentVersion, gc.DeepEquals, version.Current.Number)
  }
+
+func (s *upgraderSuite) bumpDesiredAgentVersion(c *gc.C) version.Number {
+ // In order to call SetEnvironAgentVersion we have to first SetTools on
+ // all the existing machines
+ s.apiMachine.SetAgentVersion(version.Current)
+ s.rawMachine.SetAgentVersion(version.Current)
+ newer := version.Current
+ newer.Patch++
+ err := s.State.SetEnvironAgentVersion(newer.Number)
+ c.Assert(err, gc.IsNil)
+ cfg, err := s.State.EnvironConfig()
+ c.Assert(err, gc.IsNil)
+ vers, ok := cfg.AgentVersion()
+ c.Assert(ok, jc.IsTrue)
+ c.Check(vers, gc.Equals, newer.Number)
+ return newer.Number
+}
+
+func (s *upgraderSuite) TestDesiredVersionUnrestrictedForAPIAgents(c
*gc.C) {
+ newVersion := s.bumpDesiredAgentVersion(c)
+ // Grab a different Upgrader for the apiMachine
+ authorizer := apiservertesting.FakeAuthorizer{
+ Tag: s.apiMachine.Tag(),
+ LoggedIn: true,
+ MachineAgent: true,
+ }
+ upgraderAPI, err := upgrader.NewUpgraderAPI(s.State, s.resources,
authorizer)
+ c.Assert(err, gc.IsNil)
+ args := params.Entities{Entities: []params.Entity{{Tag:
s.apiMachine.Tag()}}}
+ results, err := upgraderAPI.DesiredVersion(args)
+ c.Assert(err, gc.IsNil)
+ c.Check(results.Results, gc.HasLen, 1)
+ c.Assert(results.Results[0].Error, gc.IsNil)
+ agentVersion := results.Results[0].Version
+ c.Assert(agentVersion, gc.NotNil)
+ c.Check(*agentVersion, gc.DeepEquals, newVersion)
+}
+
+func (s *upgraderSuite) TestDesiredVersionRestrictedForNonAPIAgents(c
*gc.C) {
+ newVersion := s.bumpDesiredAgentVersion(c)
+ c.Assert(newVersion, gc.Not(gc.Equals), version.Current.Number)
+ args := params.Entities{Entities: []params.Entity{{Tag:
s.rawMachine.Tag()}}}
+ results, err := s.upgrader.DesiredVersion(args)
+ c.Assert(err, gc.IsNil)
+ c.Check(results.Results, gc.HasLen, 1)
+ c.Assert(results.Results[0].Error, gc.IsNil)
+ agentVersion := results.Results[0].Version
+ c.Assert(agentVersion, gc.NotNil)
+ c.Check(*agentVersion, gc.DeepEquals, version.Current.Number)
+}