Merge lp:~rogpeppe/juju-core/037-startinstance-returns-tools into lp:~juju/juju-core/trunk

[Roger Peppe]

environs: make StartInstance return the chosen tools.

This means that the provisioner can set the proposed tools
for a new machine without trying to second-guess the
provider choice.

This requires lp:~rogpeppe/goamz/predictable-urls
for the tests to pass.

https://codereview.appspot.com/6475058/

[Roger Peppe (rogpeppe)]

Reviewers: mp+121248_code.launchpad.net,

Message:
Please take a look.

Description:
environs: make StartInstance return the chosen tools.

This means that the provisioner can set the proposed tools
for a new machine without trying to second-guess the
provider choice.

We also make the provisioner and the bootstrap init set the proposed
tools for
the new machine.

This requires lp:~rogpeppe/goamz/predictable-urls
for the tests to pass.

https://code.launchpad.net/~rogpeppe/juju-core/037-startinstance-returns-tools/+merge/121248

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/juju/status_test.go
   M cmd/jujud/bootstrap.go
   M environs/dummy/environs.go
   M environs/ec2/ec2.go
   M environs/ec2/live_test.go
   M environs/ec2/local_test.go
   M environs/interface.go
   M environs/jujutest/livetests.go
   M environs/jujutest/tests.go
   M worker/firewaller/firewaller_test.go
   M worker/provisioner/provisioner.go

[lp:~rogpeppe/juju-core/037-startinstance-returns-tools]

430. By Roger Peppe on 2012-08-24

revert SetProposedTools behaviour

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6475058/

[lp:~rogpeppe/juju-core/037-startinstance-returns-tools]

431. By Roger Peppe on 2012-08-24

worker/provisioner: fix StartInstance call

[Roger Peppe (rogpeppe)]

Please take a look.

https://codereview.appspot.com/6475058/

[Gustavo Niemeyer (niemeyer)]

https://codereview.appspot.com/6475058/diff/6001/environs/ec2/ec2.go
File environs/ec2/ec2.go (right):

https://codereview.appspot.com/6475058/diff/6001/environs/ec2/ec2.go#newcode339
environs/ec2/ec2.go:339: tools, err = environs.FindTools(e,
version.Current)
Right now we're passing a nil tools parameter to the provider, and then
calling back onto generic logic to decide which tools to use in that
case. This seems like a bad choice. It means the decision of what to do
is distributed, and as a side effect we're having to have it both as an
incoming parameter and as an out-going one so we can manage the fact
everybody is responsible for the decision, but no one is.

Instead, what about never providing nil tools, and instead asserting
that StartInstance *needs* good tools? The algorithm to decide what to
run is already generic, and already lives outside the environment.

https://codereview.appspot.com/6475058/

Unmerged revisions

431. By Roger Peppe on 2012-08-24

worker/provisioner: fix StartInstance call

430. By Roger Peppe on 2012-08-24

revert SetProposedTools behaviour

429. By Roger Peppe on 2012-08-24

gofmt

428. By Roger Peppe on 2012-08-24

environs: make StartInstance return tools

427. By Aram Hăvărneanu on 2012-08-24

mstate: add config node

R=niemeyer
CC=
https://codereview.appspot.com/6460105

426. By Aram Hăvărneanu on 2012-08-24

mstate: use juju-core/testing for MongoDB

Use juju-core/testing for starting MongoDB instead of starting it
by hand.

R=niemeyer
CC=
https://codereview.appspot.com/6459100

425. By Aram Hăvărneanu on 2012-08-24

testing: add MongoDB support

R=niemeyer
CC=
https://codereview.appspot.com/6443148

424. By Frank Mueller on 2012-08-24

mstate: changed lifecycle implementation in unit

Unit now has Life(), Kill() and Die() and uses the utility
function ensureLife(). The test for removing a unit has
been changed.

R=aram, niemeyer
CC=
https://codereview.appspot.com/6476044

423. By Dave Cheney on 2012-08-24

state: add initial config map to Initialise

state.Initialise now takes a config map that is inserted into
/environment on creation.

R=niemeyer
CC=
https://codereview.appspot.com/6458161

422. By Roger Peppe on 2012-08-22

environs: tweaks to ListTools interface

This makes it easier for upgrade logic to operate on the set of all
available tools.

R=niemeyer
CC=
https://codereview.appspot.com/6485044

=== modified file 'cmd/juju/status_test.go'
--- cmd/juju/status_test.go	2012-08-16 10:24:36 +0000
+++ cmd/juju/status_test.go	2012-08-24 17:44:18 +0000
@@ -88,7 +88,7 @@
 		func(st *state.State, conn *juju.Conn, c *C) {
 			m, err := st.Machine(0)
 			c.Assert(err, IsNil)
-			inst, err := conn.Environ.StartInstance(m.Id(), nil, nil)
+			inst, _, err := conn.Environ.StartInstance(m.Id(), nil, nil)
 			c.Assert(err, IsNil)
 			err = m.SetInstanceId(inst.Id())
 			c.Assert(err, IsNil)
@@ -207,7 +207,7 @@
 				m, err := st.AddMachine()
 				c.Assert(err, IsNil)
 				c.Assert(m.Id(), Equals, i)
-				inst, err := conn.Environ.StartInstance(m.Id(), nil, nil)
+				inst, _, err := conn.Environ.StartInstance(m.Id(), nil, nil)
 				c.Assert(err, IsNil)
 				err = m.SetInstanceId(inst.Id())
 				c.Assert(err, IsNil)

=== modified file 'environs/dummy/environs.go'
--- environs/dummy/environs.go	2012-08-24 01:45:40 +0000
+++ environs/dummy/environs.go	2012-08-24 17:44:18 +0000
@@ -29,6 +29,7 @@
 	"launchpad.net/juju-core/schema"
 	"launchpad.net/juju-core/state"
 	"launchpad.net/juju-core/testing"
+	"launchpad.net/juju-core/version"
 	"net"
 	"net/http"
 	"strings"
@@ -417,15 +418,21 @@
 	return nil
 }
 
-func (e *environ) StartInstance(machineId int, info *state.Info, tools *state.Tools) (environs.Instance, error) {
+func (e *environ) StartInstance(machineId int, info *state.Info, tools *state.Tools) (environs.Instance, *state.Tools, error) {
 	log.Printf("dummy startinstance, machine %d", machineId)
 	if err := e.checkBroken("StartInstance"); err != nil {
-		return nil, err
+		return nil, nil, err
 	}
 	e.state.mu.Lock()
 	defer e.state.mu.Unlock()
 	if tools != nil && (strings.HasPrefix(tools.Series, "unknown") || strings.HasPrefix(tools.Arch, "unknown")) {
-		return nil, fmt.Errorf("cannot find image for %s-%s", tools.Series, tools.Arch)
+		return nil, nil, fmt.Errorf("cannot find image for %s-%s", tools.Series, tools.Arch)
+	}
+	if tools == nil {
+		tools = &state.Tools{
+			Binary: version.Current,
+			URL:    "fake url for dummy tools",
+		}
 	}
 	i := &instance{
 		state:     e.state,
@@ -441,7 +448,7 @@
 		Instance:  i,
 		Info:      info,
 	}
-	return i, nil
+	return i, tools, nil
 }
 
 func (e *environ) StopInstances(is []environs.Instance) error {

=== modified file 'environs/ec2/ec2.go'
--- environs/ec2/ec2.go	2012-08-22 03:06:05 +0000
+++ environs/ec2/ec2.go	2012-08-24 17:44:18 +0000
@@ -246,7 +246,7 @@
 	if err != nil {
 		return fmt.Errorf("unable to determine inital configuration: %v", err)
 	}
-	inst, err := e.startInstance(0, nil, tools, true, config)
+	inst, _, err := e.startInstance(0, nil, tools, true, config)
 	if err != nil {
 		return fmt.Errorf("cannot start bootstrap instance: %v", err)
 	}
@@ -307,7 +307,7 @@
 	return state.AssignUnused
 }
 
-func (e *environ) StartInstance(machineId int, info *state.Info, tools *state.Tools) (environs.Instance, error) {
+func (e *environ) StartInstance(machineId int, info *state.Info, tools *state.Tools) (environs.Instance, *state.Tools, error) {
 	return e.startInstance(machineId, info, tools, false, nil)
 }
 
@@ -333,12 +333,12 @@
 // startInstance is the internal version of StartInstance, used by Bootstrap
 // as well as via StartInstance itself. If master is true, a bootstrap
 // instance will be started.
-func (e *environ) startInstance(machineId int, info *state.Info, tools *state.Tools, master bool, config map[string]interface{}) (environs.Instance, error) {
+func (e *environ) startInstance(machineId int, info *state.Info, tools *state.Tools, master bool, config map[string]interface{}) (environs.Instance, *state.Tools, error) {
 	if tools == nil {
 		var err error
 		tools, err = environs.FindTools(e, version.Current)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 	}
 	log.Printf("environs/ec2: starting machine %d in %q running tools version %q from %q", machineId, e.name, tools.Binary, tools.URL)
@@ -348,16 +348,16 @@
 		region: e.ecfg().region(),
 	})
 	if err != nil {
-		return nil, fmt.Errorf("cannot find image satisfying constraints: %v", err)
+		return nil, nil, fmt.Errorf("cannot find image satisfying constraints: %v", err)
 	}
 	// TODO quick sanity check that we can access the tools URL?
 	userData, err := e.userData(machineId, info, tools, master, config)
 	if err != nil {
-		return nil, fmt.Errorf("cannot make user data: %v", err)
+		return nil, nil, fmt.Errorf("cannot make user data: %v", err)
 	}
 	groups, err := e.setUpGroups(machineId)
 	if err != nil {
-		return nil, fmt.Errorf("cannot set up groups: %v", err)
+		return nil, nil, fmt.Errorf("cannot set up groups: %v", err)
 	}
 	var instances *ec2.RunInstancesResp
 
@@ -375,14 +375,14 @@
 		}
 	}
 	if err != nil {
-		return nil, fmt.Errorf("cannot run instances: %v", err)
+		return nil, nil, fmt.Errorf("cannot run instances: %v", err)
 	}
 	if len(instances.Instances) != 1 {
-		return nil, fmt.Errorf("expected 1 started instance, got %d", len(instances.Instances))
+		return nil, nil, fmt.Errorf("expected 1 started instance, got %d", len(instances.Instances))
 	}
 	inst := &instance{e, &instances.Instances[0]}
 	log.Printf("environs/ec2: started instance %q", inst.Id())
-	return inst, nil
+	return inst, tools, nil
 }
 
 func (e *environ) StopInstances(insts []environs.Instance) error {

=== modified file 'environs/ec2/live_test.go'
--- environs/ec2/live_test.go	2012-07-30 16:37:39 +0000
+++ environs/ec2/live_test.go	2012-08-24 17:44:18 +0000
@@ -98,7 +98,7 @@
 }
 
 func (t *LiveTests) TestInstanceDNSName(c *C) {
-	inst, err := t.Env.StartInstance(30, jujutest.InvalidStateInfo, nil)
+	inst, _, err := t.Env.StartInstance(30, jujutest.InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst})
 	dns, err := inst.WaitDNSName()
@@ -148,7 +148,7 @@
 		})
 	c.Assert(err, IsNil)
 
-	inst0, err := t.Env.StartInstance(98, jujutest.InvalidStateInfo, nil)
+	inst0, _, err := t.Env.StartInstance(98, jujutest.InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst0})
 
@@ -156,7 +156,7 @@
 	// before starting it, to check that it's reused correctly.
 	oldMachineGroup := createGroup(c, ec2conn, groups[2].Name, "old machine group")
 
-	inst1, err := t.Env.StartInstance(99, jujutest.InvalidStateInfo, nil)
+	inst1, _, err := t.Env.StartInstance(99, jujutest.InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	defer t.Env.StopInstances([]environs.Instance{inst1})
 
@@ -258,12 +258,12 @@
 	// It would be nice if this test was in jujutest, but
 	// there's no way for jujutest to fabricate a valid-looking
 	// instance id.
-	inst0, err := t.Env.StartInstance(40, jujutest.InvalidStateInfo, nil)
+	inst0, _, err := t.Env.StartInstance(40, jujutest.InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 
 	inst1 := ec2.FabricateInstance(inst0, "i-aaaaaaaa")
 
-	inst2, err := t.Env.StartInstance(41, jujutest.InvalidStateInfo, nil)
+	inst2, _, err := t.Env.StartInstance(41, jujutest.InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 
 	err = t.Env.StopInstances([]environs.Instance{inst0, inst1, inst2})

=== modified file 'environs/ec2/local_test.go'
--- environs/ec2/local_test.go	2012-08-22 02:53:44 +0000
+++ environs/ec2/local_test.go	2012-08-24 17:44:18 +0000
@@ -239,7 +239,7 @@
 	// check that a new instance will be started without
 	// zookeeper, with a machine agent, and without a
 	// provisioning agent.
-	inst1, err := t.env.StartInstance(1, info, nil)
+	inst1, _, err := t.env.StartInstance(1, info, nil)
 	c.Assert(err, IsNil)
 	inst = t.srv.ec2srv.Instance(inst1.Id())
 	c.Assert(inst, NotNil)

=== modified file 'environs/interface.go'
--- environs/interface.go	2012-08-09 09:58:22 +0000
+++ environs/interface.go	2012-08-24 17:44:18 +0000
@@ -147,9 +147,10 @@
 	// can lead to undefined results. The juju tools that will run
 	// on the new machine are given by tools - if nil,
 	// the Environ will find a set of tools compatible with the
-	// current version.
+	// current version. StartInstance returns the new
+	// instance and the set of tools it will run.
 	// TODO add arguments to specify type of new machine.
-	StartInstance(machineId int, info *state.Info, tools *state.Tools) (Instance, error)
+	StartInstance(machineId int, info *state.Info, tools *state.Tools) (Instance, *state.Tools, error)
 
 	// StopInstances shuts down the given instances.
 	StopInstances([]Instance) error

=== modified file 'environs/jujutest/livetests.go'
--- environs/jujutest/livetests.go	2012-08-24 01:45:40 +0000
+++ environs/jujutest/livetests.go	2012-08-24 17:44:18 +0000
@@ -17,9 +17,10 @@
 	c.Assert(err, IsNil)
 	c.Check(insts, HasLen, 0)
 
-	inst, err := t.Env.StartInstance(0, InvalidStateInfo, nil)
+	inst, tools, err := t.Env.StartInstance(0, InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst, NotNil)
+	c.Assert(tools.Binary, Equals, version.Current)
 	id0 := inst.Id()
 
 	insts, err = t.Env.Instances([]string{id0, id0})
@@ -62,7 +63,7 @@
 }
 
 func (t *LiveTests) TestPorts(c *C) {
-	inst1, err := t.Env.StartInstance(1, InvalidStateInfo, nil)
+	inst1, _, err := t.Env.StartInstance(1, InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst1, NotNil)
 	defer t.Env.StopInstances([]environs.Instance{inst1})
@@ -71,7 +72,7 @@
 	c.Assert(err, IsNil)
 	c.Assert(ports, HasLen, 0)
 
-	inst2, err := t.Env.StartInstance(2, InvalidStateInfo, nil)
+	inst2, _, err := t.Env.StartInstance(2, InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst2, NotNil)
 	ports, err = inst2.Ports(2)
@@ -185,7 +186,7 @@
 	m, err = st.AddMachine()
 	c.Assert(err, IsNil)
 
-	t.assertStartInstance(c, m)
+	t.assertStartedInstance(c, m)
 
 	// now remove it
 	c.Assert(st.RemoveMachine(m.Id()), IsNil)
@@ -240,7 +241,7 @@
 	Delay: 1 * time.Second,
 }
 
-func (t *LiveTests) assertStartInstance(c *C, m *state.Machine) {
+func (t *LiveTests) assertStartedInstance(c *C, m *state.Machine) {
 	// Wait for machine to get an instance id.
 	for a := waitAgent.Start(); a.Next(); {
 		instId, err := m.InstanceId()
@@ -351,11 +352,36 @@
 		URL:    url,
 	}
 
-	inst, err := t.Env.StartInstance(4, InvalidStateInfo, tools)
+	inst, gotTools, err := t.Env.StartInstance(4, InvalidStateInfo, tools)
 	if inst != nil {
 		err := t.Env.StopInstances([]environs.Instance{inst})
 		c.Check(err, IsNil)
 	}
 	c.Assert(inst, IsNil)
+	c.Assert(gotTools, IsNil)
 	c.Assert(err, ErrorMatches, "cannot find image.*")
 }
+
+// Check that StartInstance returns the same tools we passed it.
+func (t *LiveTests) TestStartInstanceReturnsSameTools(c *C) {
+	name := environs.ToolsStoragePath(version.Current)
+	storage := t.Env.Storage()
+	checkPutFile(c, storage, name, []byte("fake tools on invalid series"))
+	defer storage.Remove(name)
+
+	url, err := storage.URL(name)
+	c.Assert(err, IsNil)
+	tools := &state.Tools{
+		Binary: version.Current,
+		URL:    url,
+	}
+
+	inst, gotTools, err := t.Env.StartInstance(4, InvalidStateInfo, tools)
+	if inst != nil {
+		err := t.Env.StopInstances([]environs.Instance{inst})
+		c.Check(err, IsNil)
+	}
+	c.Assert(err, IsNil)
+	c.Assert(gotTools, Equals, tools)
+	c.Assert(inst, NotNil)
+}

=== modified file 'environs/jujutest/tests.go'
--- environs/jujutest/tests.go	2012-07-30 16:37:39 +0000
+++ environs/jujutest/tests.go	2012-08-24 17:44:18 +0000
@@ -6,6 +6,7 @@
 	"io/ioutil"
 	. "launchpad.net/gocheck"
 	"launchpad.net/juju-core/environs"
+	"launchpad.net/juju-core/version"
 	"net/http"
 	"time"
 )
@@ -17,14 +18,16 @@
 	c.Assert(err, IsNil)
 	c.Assert(insts, HasLen, 0)
 
-	inst0, err := e.StartInstance(0, InvalidStateInfo, nil)
+	inst0, tools0, err := e.StartInstance(0, InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst0, NotNil)
+	c.Assert(tools0.Binary, Equals, version.Current)
 	id0 := inst0.Id()
 
-	inst1, err := e.StartInstance(1, InvalidStateInfo, nil)
+	inst1, tools1, err := e.StartInstance(1, InvalidStateInfo, nil)
 	c.Assert(err, IsNil)
 	c.Assert(inst1, NotNil)
+	c.Assert(tools1, DeepEquals, tools0)
 	id1 := inst1.Id()
 
 	insts, err = e.Instances([]string{id0, id1})

=== modified file 'worker/firewaller/firewaller_test.go'
--- worker/firewaller/firewaller_test.go	2012-08-17 09:31:37 +0000
+++ worker/firewaller/firewaller_test.go	2012-08-24 17:44:18 +0000
@@ -66,7 +66,7 @@
 
 	m, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, _, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)
@@ -96,7 +96,7 @@
 
 	m, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, _, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)
@@ -136,14 +136,14 @@
 	m1, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
 
-	inst1, err := s.Conn.Environ.StartInstance(m1.Id(), s.StateInfo(c), nil)
+	inst1, _, err := s.Conn.Environ.StartInstance(m1.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m1.SetInstanceId(inst1.Id())
 	c.Assert(err, IsNil)
 
 	m2, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst2, err := s.Conn.Environ.StartInstance(m2.Id(), s.StateInfo(c), nil)
+	inst2, _, err := s.Conn.Environ.StartInstance(m2.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m2.SetInstanceId(inst2.Id())
 	c.Assert(err, IsNil)
@@ -180,7 +180,7 @@
 func (s *FirewallerSuite) TestFirewallerStartWithState(c *C) {
 	m, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, _, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)
@@ -211,7 +211,7 @@
 func (s *FirewallerSuite) TestFirewallerStartWithPartialState(c *C) {
 	m, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, _, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)
@@ -244,7 +244,7 @@
 
 	m, err := s.State.AddMachine()
 	c.Assert(err, IsNil)
-	inst, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
+	inst, _, err := s.Conn.Environ.StartInstance(m.Id(), s.StateInfo(c), nil)
 	c.Assert(err, IsNil)
 	err = m.SetInstanceId(inst.Id())
 	c.Assert(err, IsNil)

=== modified file 'worker/provisioner/provisioner.go'
--- worker/provisioner/provisioner.go	2012-08-15 15:29:08 +0000
+++ worker/provisioner/provisioner.go	2012-08-24 17:44:18 +0000
@@ -225,7 +225,7 @@
 	// however as the PA only knows one state.Info, and that info is used by MAs and 
 	// UAs to locate the ZK for this environment, it is logical to use the same 
 	// state.Info as the PA. 
-	inst, err := p.environ.StartInstance(m.Id(), p.info, nil)
+	inst, _, err := p.environ.StartInstance(m.Id(), p.info, nil)
 	if err != nil {
 		log.Printf("provisioner cannot start machine %s: %v", m, err)
 		return err