Merge lp:~axwalk/juju-core/upgrade-syslog-port into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

upgrades: change syslog-port on upgrade

We upgrade syslog-port to 6514, to work
around a problem with older versions of
rsyslog to do with privileged ports.

There are some incidental changes here:
 - fixed AllMachines handling in upgrades
 - provide *state.State to state server
   upgrade context

https://codereview.appspot.com/69890043/

upgrades: change syslog-port on upgrade

We upgrade syslog-port to 6514, to work
around a problem with older versions of
rsyslog to do with privileged ports.

There are some incidental changes here:
 - fixed AllMachines handling in upgrades
 - provide *state.State to state server
   upgrade context

https://codereview.appspot.com/69890043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+208724_code.launchpad.net,

Message:
Please take a look.

Description:
upgrades: change syslog-port on upgrade

We upgrade syslog-port to 6514, to work
around a problem with older versions of
rsyslog to do with privileged ports.

There are some incidental changes here:
  - fixed AllMachines handling in upgrades
  - provide *state.State to state server
    upgrade context

https://code.launchpad.net/~axwalk/juju-core/upgrade-syslog-port/+merge/208724

(do not edit description out of merge proposal)

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

Affected files (+88, -10 lines):
   A [revision details]
   M cmd/jujud/machine.go
   A upgrades/rsyslogport.go
   M upgrades/steps118.go
   M upgrades/steps118_test.go
   M upgrades/upgrade.go
   M upgrades/upgrade_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: <email address hidden>
+New revision: <email address hidden>

Index: upgrades/rsyslogport.go
=== added file 'upgrades/rsyslogport.go'
--- upgrades/rsyslogport.go 1970-01-01 00:00:00 +0000
+++ upgrades/rsyslogport.go 2014-02-28 04:19:59 +0000
@@ -0,0 +1,20 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades
+
+import (
+ "launchpad.net/juju-core/environs/config"
+)
+
+func updateRsyslogPort(context Context) error {
+ st := context.State()
+ old, err := st.EnvironConfig()
+ if err != nil {
+ return err
+ }
+ cfg, err := old.Apply(map[string]interface{}{
+ "syslog-port": config.DefaultSyslogPort,
+ })
+ return st.SetEnvironConfig(cfg, old)
+}

Index: upgrades/steps118.go
=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go 2014-02-27 08:28:24 +0000
+++ upgrades/steps118.go 2014-02-28 04:19:59 +0000
@@ -17,6 +17,11 @@
     run: ensureSystemSSHKey,
    },
    &upgradeStep{
+ description: "update rsyslog port",
+ targets: []Target{StateServer},
+ run: updateRsyslogPort,
+ },
+ &upgradeStep{
     description: "install rsyslog-gnutls",
     targets: []Target{AllMachines},
     run: installRsyslogGnutls,

Index: upgrades/steps118_test.go
=== modified file 'upgrades/steps118_test.go'
--- upgrades/steps118_test.go 2014-02-27 08:28:24 +0000
+++ upgrades/steps118_test.go 2014-02-28 04:19:59 +0000
@@ -19,6 +19,7 @@
  var expectedSteps = []string{
   "make $DATADIR/locks owned by ubuntu:ubuntu",
   "generate system ssh key",
+ "update rsyslog port",
   "install rsyslog-gnutls",
  }

Index: upgrades/upgrade.go
=== modified file 'upgrades/upgrade.go'
--- upgrades/upgrade.go 2014-02-26 04:21:25 +0000
+++ upgrades/upgrade.go 2014-02-28 04:19:59 +0000
@@ -9,6 +9,7 @@
   "github.com/loggo/loggo"

   "launchpad.net/juju-core/agent"
+ "launchpad.net/juju-core/state"
   "launchpad.net/juju-core/state/api"
   "launchpad.net/juju-core/version"
  )
@@ -78,7 +79,12 @@
   // APIState returns an API connection to state.
   APIState() *api.State

- // AgentConfig returns the agent config for the...

[Ian Booth (wallyworld)]

Looks great but there are missing tests for the rsyslogport upgrade
function as well as for machine agent upgrade in
cmd/jujud/upgrade_test.go

https://codereview.appspot.com/69890043/diff/1/upgrades/rsyslogport.go
File upgrades/rsyslogport.go (right):

https://codereview.appspot.com/69890043/diff/1/upgrades/rsyslogport.go#newcode18
upgrades/rsyslogport.go:18: })
we're missing an error check

https://codereview.appspot.com/69890043/

[Andrew Wilkins (axwalk)]

Please take a look.

https://codereview.appspot.com/69890043/diff/1/upgrades/rsyslogport.go
File upgrades/rsyslogport.go (right):

https://codereview.appspot.com/69890043/diff/1/upgrades/rsyslogport.go#newcode18
upgrades/rsyslogport.go:18: })
On 2014/02/28 04:43:51, wallyworld wrote:
> we're missing an error check

Done.

https://codereview.appspot.com/69890043/

[Ian Booth (wallyworld)]

LGTM

https://codereview.appspot.com/69890043/

[Ian Booth (wallyworld)]

review: Approve

=== modified file 'cmd/jujud/machine.go'
--- cmd/jujud/machine.go	2014-02-27 14:16:26 +0000
+++ cmd/jujud/machine.go	2014-02-28 05:45:22 +0000
@@ -68,6 +68,8 @@
 	MachineId       string
 	runner          worker.Runner
 	upgradeComplete chan struct{}
+	stateOpened     chan struct{}
+	st              *state.State
 }
 
 // Info returns usage information for the command.
@@ -93,6 +95,7 @@
 	}
 	a.runner = newRunner(isFatal, moreImportant)
 	a.upgradeComplete = make(chan struct{})
+	a.stateOpened = make(chan struct{})
 	return nil
 }
 
@@ -310,6 +313,8 @@
 	if err != nil {
 		return nil, err
 	}
+	a.st = st
+	close(a.stateOpened)
 	reportOpenedState(st)
 	m := entity.(*state.Machine)
 
@@ -413,7 +418,18 @@
 			return nil
 		default:
 		}
-		err := a.runUpgrades(apiState, jobs)
+		// If the machine agent is a state server, wait until state is opened.
+		var st *state.State
+		for _, job := range jobs {
+			if job == params.JobManageEnviron {
+				select {
+				case <-a.stateOpened:
+				}
+				st = a.st
+				break
+			}
+		}
+		err := a.runUpgrades(st, apiState, jobs)
 		if err != nil {
 			return err
 		}
@@ -425,7 +441,7 @@
 }
 
 // runUpgrades runs the upgrade operations for each job type and updates the updatedToVersion on success.
-func (a *MachineAgent) runUpgrades(st *api.State, jobs []params.MachineJob) error {
+func (a *MachineAgent) runUpgrades(st *state.State, apiState *api.State, jobs []params.MachineJob) error {
 	agentConfig := a.Conf.config
 	from := version.Current
 	from.Number = agentConfig.UpgradedToVersion()
@@ -433,7 +449,7 @@
 		logger.Infof("Upgrade to %v already completed.", version.Current)
 		return nil
 	}
-	context := upgrades.NewContext(agentConfig, st)
+	context := upgrades.NewContext(agentConfig, apiState, st)
 	for _, job := range jobs {
 		var target upgrades.Target
 		switch job {

=== modified file 'cmd/jujud/upgrade_test.go'
--- cmd/jujud/upgrade_test.go	2014-02-26 05:45:36 +0000
+++ cmd/jujud/upgrade_test.go	2014-02-28 05:45:22 +0000
@@ -5,20 +5,24 @@
 
 import (
 	"os"
+	"os/exec"
 	"path/filepath"
 
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/environs/config"
 	"launchpad.net/juju-core/state"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
+	"launchpad.net/juju-core/utils"
 	"launchpad.net/juju-core/version"
 )
 
 type UpgradeSuite struct {
 	commonMachineSuite
 
+	aptCmds          []*exec.Cmd
 	machine          *state.Machine
 	upgradeToVersion version.Binary
 }
@@ -30,6 +34,15 @@
 func (s *UpgradeSuite) SetUpTest(c *gc.C) {
 	s.commonMachineSuite.SetUpTest(c)
 
+	// Capture all apt commands.
+	s.aptCmds = nil
+	aptCmds := s.HookCommandOutput(&utils.AptCommandOutput, nil, nil)
+	go func() {
+		for cmd := range aptCmds {
+			s.aptCmds = append(s.aptCmds, cmd)
+		}
+	}()
+
 	// As Juju versions increase, update the version to which we are upgrading.
 	s.upgradeToVersion = version.Current
 	s.upgradeToVersion.Number.Minor++
@@ -84,17 +97,36 @@
 	return filepath.Join(s.DataDir(), "system-identity")
 }
 
+func (s *UpgradeSuite) assertCommonUpgrades(c *gc.C) {
+	// rsyslog-gnutls should have been installed.
+	c.Assert(s.aptCmds, gc.HasLen, 1)
+	args := s.aptCmds[0].Args
+	c.Assert(len(args), jc.GreaterThan, 1)
+	c.Assert(args[0], gc.Equals, "apt-get")
+	c.Assert(args[len(args)-1], gc.Equals, "rsyslog-gnutls")
+}
+
 func (s *UpgradeSuite) assertStateServerUpgrades(c *gc.C) {
+	s.assertCommonUpgrades(c)
 	// System SSH key
 	c.Assert(s.keyFile(), jc.IsNonEmptyFile)
+	// Syslog port should have been updated
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)
 }
 
 func (s *UpgradeSuite) assertHostUpgrades(c *gc.C) {
+	s.assertCommonUpgrades(c)
 	// Lock directory
 	lockdir := filepath.Join(s.DataDir(), "locks")
 	c.Assert(lockdir, jc.IsDirectory)
 	// SSH key file should not be generated for hosts.
 	_, err := os.Stat(s.keyFile())
 	c.Assert(err, jc.Satisfies, os.IsNotExist)
+	// Syslog port should not have been updated
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg.SyslogPort(), gc.Not(gc.Equals), config.DefaultSyslogPort)
 	// Add other checks as needed...
 }

=== modified file 'testing/testbase/cmd.go'
--- testing/testbase/cmd.go	2013-11-15 06:36:39 +0000
+++ testing/testbase/cmd.go	2014-02-28 05:45:22 +0000
@@ -17,6 +17,7 @@
 	cmdChan := make(chan *exec.Cmd, 1)
 	origCommandOutput := *outputFunc
 	cleanup := func() {
+		close(cmdChan)
 		*outputFunc = origCommandOutput
 	}
 	*outputFunc = func(cmd *exec.Cmd) ([]byte, error) {

=== modified file 'upgrades/export_test.go'
--- upgrades/export_test.go	2014-02-27 03:06:54 +0000
+++ upgrades/export_test.go	2014-02-28 05:45:22 +0000
@@ -11,4 +11,5 @@
 	StepsFor118                          = stepsFor118
 	EnsureLockDirExistsAndUbuntuWritable = ensureLockDirExistsAndUbuntuWritable
 	EnsureSystemSSHKey                   = ensureSystemSSHKey
+	UpdateRsyslogPort                    = updateRsyslogPort
 )

=== added file 'upgrades/rsyslogport.go'
--- upgrades/rsyslogport.go	1970-01-01 00:00:00 +0000
+++ upgrades/rsyslogport.go	2014-02-28 05:45:22 +0000
@@ -0,0 +1,23 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades
+
+import (
+	"launchpad.net/juju-core/environs/config"
+)
+
+func updateRsyslogPort(context Context) error {
+	st := context.State()
+	old, err := st.EnvironConfig()
+	if err != nil {
+		return err
+	}
+	cfg, err := old.Apply(map[string]interface{}{
+		"syslog-port": config.DefaultSyslogPort,
+	})
+	if err != nil {
+		return err
+	}
+	return st.SetEnvironConfig(cfg, old)
+}

=== added file 'upgrades/rsyslogport_test.go'
--- upgrades/rsyslogport_test.go	1970-01-01 00:00:00 +0000
+++ upgrades/rsyslogport_test.go	2014-02-28 05:45:22 +0000
@@ -0,0 +1,51 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package upgrades_test
+
+import (
+	gc "launchpad.net/gocheck"
+
+	"launchpad.net/juju-core/environs/config"
+	jujutesting "launchpad.net/juju-core/juju/testing"
+	"launchpad.net/juju-core/state"
+	"launchpad.net/juju-core/upgrades"
+)
+
+type rsyslogPortSuite struct {
+	jujutesting.JujuConnSuite
+	ctx upgrades.Context
+}
+
+var _ = gc.Suite(&rsyslogPortSuite{})
+
+func (s *rsyslogPortSuite) SetUpTest(c *gc.C) {
+	s.JujuConnSuite.SetUpTest(c)
+	apiState, _ := s.OpenAPIAsNewMachine(c, state.JobManageEnviron)
+	s.ctx = &mockContext{
+		agentConfig: &mockAgentConfig{dataDir: s.DataDir()},
+		apiState:    apiState,
+		state:       s.State,
+	}
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg.SyslogPort(), gc.Not(gc.Equals), config.DefaultSyslogPort)
+}
+
+func (s *rsyslogPortSuite) TestSyslogPortChanged(c *gc.C) {
+	err := upgrades.UpdateRsyslogPort(s.ctx)
+	c.Assert(err, gc.IsNil)
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)
+}
+
+func (s *rsyslogPortSuite) TestIdempotent(c *gc.C) {
+	err := upgrades.UpdateRsyslogPort(s.ctx)
+	c.Assert(err, gc.IsNil)
+	err = upgrades.UpdateRsyslogPort(s.ctx)
+	c.Assert(err, gc.IsNil)
+	cfg, err := s.State.EnvironConfig()
+	c.Assert(err, gc.IsNil)
+	c.Assert(cfg.SyslogPort(), gc.Equals, config.DefaultSyslogPort)
+}

=== modified file 'upgrades/steps118.go'
--- upgrades/steps118.go	2014-02-27 08:28:24 +0000
+++ upgrades/steps118.go	2014-02-28 05:45:22 +0000
@@ -17,6 +17,11 @@
 			run:         ensureSystemSSHKey,
 		},
 		&upgradeStep{
+			description: "update rsyslog port",
+			targets:     []Target{StateServer},
+			run:         updateRsyslogPort,
+		},
+		&upgradeStep{
 			description: "install rsyslog-gnutls",
 			targets:     []Target{AllMachines},
 			run:         installRsyslogGnutls,

=== modified file 'upgrades/steps118_test.go'
--- upgrades/steps118_test.go	2014-02-27 08:28:24 +0000
+++ upgrades/steps118_test.go	2014-02-28 05:45:22 +0000
@@ -19,6 +19,7 @@
 var expectedSteps = []string{
 	"make $DATADIR/locks owned by ubuntu:ubuntu",
 	"generate system ssh key",
+	"update rsyslog port",
 	"install rsyslog-gnutls",
 }
 

=== modified file 'upgrades/upgrade.go'
--- upgrades/upgrade.go	2014-02-26 04:21:25 +0000
+++ upgrades/upgrade.go	2014-02-28 05:45:22 +0000
@@ -9,6 +9,7 @@
 	"github.com/loggo/loggo"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	"launchpad.net/juju-core/version"
 )
@@ -78,7 +79,12 @@
 	// APIState returns an API connection to state.
 	APIState() *api.State
 
-	// AgentConfig returns the agent config for the machine that is being upgraded.
+	// State returns a connection to state. This will be non-nil
+	// only in the context of a state server.
+	State() *state.State
+
+	// AgentConfig returns the agent config for the machine that is being
+	// upgraded.
 	AgentConfig() agent.Config
 }
 
@@ -87,12 +93,18 @@
 	// Work in progress........
 	// Exactly what a context needs is to be determined as the
 	// implementation evolves.
-	st          *api.State
+	api         *api.State
+	st          *state.State
 	agentConfig agent.Config
 }
 
 // APIState is defined on the Context interface.
 func (c *upgradeContext) APIState() *api.State {
+	return c.api
+}
+
+// State is defined on the Context interface.
+func (c *upgradeContext) State() *state.State {
 	return c.st
 }
 
@@ -102,8 +114,9 @@
 }
 
 // NewContext returns a new upgrade context.
-func NewContext(agentConfig agent.Config, st *api.State) Context {
+func NewContext(agentConfig agent.Config, api *api.State, st *state.State) Context {
 	return &upgradeContext{
+		api:         api,
 		st:          st,
 		agentConfig: agentConfig,
 	}
@@ -146,7 +159,7 @@
 // validTarget returns true if target is in step.Targets().
 func validTarget(target Target, step Step) bool {
 	for _, opTarget := range step.Targets() {
-		if target == AllMachines || target == opTarget {
+		if opTarget == AllMachines || target == opTarget {
 			return true
 		}
 	}

=== modified file 'upgrades/upgrade_test.go'
--- upgrades/upgrade_test.go	2014-02-20 01:09:59 +0000
+++ upgrades/upgrade_test.go	2014-02-28 05:45:22 +0000
@@ -11,6 +11,7 @@
 	gc "launchpad.net/gocheck"
 
 	"launchpad.net/juju-core/agent"
+	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/state/api"
 	coretesting "launchpad.net/juju-core/testing"
 	jc "launchpad.net/juju-core/testing/checkers"
@@ -78,12 +79,17 @@
 	messages    []string
 	agentConfig *mockAgentConfig
 	apiState    *api.State
+	state       *state.State
 }
 
 func (c *mockContext) APIState() *api.State {
 	return c.apiState
 }
 
+func (c *mockContext) State() *state.State {
+	return c.state
+}
+
 func (c *mockContext) AgentConfig() agent.Config {
 	return c.agentConfig
 }
@@ -160,6 +166,14 @@
 				&mockUpgradeStep{"step 2 - 1.18.0", targets(upgrades.StateServer)},
 			},
 		},
+		&mockUpgradeOperation{
+			targetVersion: version.MustParse("1.20.0"),
+			steps: []upgrades.Step{
+				&mockUpgradeStep{"step 1 - 1.20.0", targets(upgrades.AllMachines)},
+				&mockUpgradeStep{"step 2 - 1.20.0", targets(upgrades.HostMachine)},
+				&mockUpgradeStep{"step 3 - 1.20.0", targets(upgrades.StateServer)},
+			},
+		},
 	}
 	return steps
 }
@@ -200,9 +214,17 @@
 	},
 	{
 		about:         "allMachines matches everything",
-		fromVersion:   "1.17.1",
-		target:        upgrades.AllMachines,
-		expectedSteps: []string{"step 1 - 1.18.0", "step 2 - 1.18.0"},
+		fromVersion:   "1.18.1",
+		toVersion:     "1.20.0",
+		target:        upgrades.HostMachine,
+		expectedSteps: []string{"step 1 - 1.20.0", "step 2 - 1.20.0"},
+	},
+	{
+		about:         "allMachines matches everything",
+		fromVersion:   "1.18.1",
+		toVersion:     "1.20.0",
+		target:        upgrades.StateServer,
+		expectedSteps: []string{"step 1 - 1.20.0", "step 3 - 1.20.0"},
 	},
 	{
 		about:         "error aborts, subsequent steps not run",