Merge lp:~axwalk/juju-core/state-addmachineparams-expose into lp:~go-bot/juju-core/trunk

[Andrew Wilkins]

state: modify InjectMachine to take params struct

InjectMachine is modified to take AddMachineParams
struct, and that struct's fields are now all
exported. Additionally, AddMachineWithConstraints
(which already took the params struct), now checks
that the formerly non-exported fields are blank
upon entry.

These changes are to accommodate manual provisioning,
where we want to inject a machine into state, but
set the nonce to something other than the bootstrap
nonce. Additionally, we are likely to add provider
type into the machine document soon.

https://codereview.appspot.com/13169043/

state: modify InjectMachine to take params struct

InjectMachine is modified to take AddMachineParams
struct, and that struct's fields are now all
exported. Additionally, AddMachineWithConstraints
(which already took the params struct), now checks
that the formerly non-exported fields are blank
upon entry.

These changes are to accommodate manual provisioning,
where we want to inject a machine into state, but
set the nonce to something other than the bootstrap
nonce. Additionally, we are likely to add provider
type into the machine document soon.

https://codereview.appspot.com/13169043/

[Andrew Wilkins (axwalk)]

Reviewers: mp+181488_code.launchpad.net,

Message:
Please take a look.

Description:
state: modify InjectMachine to take params struct

InjectMachine is modified to take AddMachineParams
struct, and that struct's fields are now all
exported. Additionally, AddMachineWithConstraints
(which already took the params struct), now checks
that the formerly non-exported fields are blank
upon entry.

These changes are to accommodate manual provisioning,
where we want to inject a machine into state, but
set the nonce to something other than the bootstrap
nonce. Additionally, we are likely to add provider
type into the machine document soon.

https://code.launchpad.net/~axwalk/juju-core/state-addmachineparams-expose/+merge/181488

(do not edit description out of merge proposal)

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

Affected files:
   A [revision details]
   M cmd/jujud/machine_test.go
   M environs/bootstrap.go
   M state/state.go
   M state/state_test.go

[William Reade (fwereade)]

LGTM; I'll leave the naming question to your judgment.

https://codereview.appspot.com/13169043/diff/1/state/state.go
File state/state.go (left):

https://codereview.appspot.com/13169043/diff/1/state/state.go#oldcode273
state/state.go:273: type AddMachineParams struct {
Quibble quibble, should we call it something like MachineSpec? It's not
really Add-specific.

https://codereview.appspot.com/13169043/

[Andrew Wilkins (axwalk)]

On 2013/08/22 16:33:17, fwereade wrote:
> LGTM; I'll leave the naming question to your judgment.

Thanks for reviewing.

> https://codereview.appspot.com/13169043/diff/1/state/state.go
> File state/state.go (left):

https://codereview.appspot.com/13169043/diff/1/state/state.go#oldcode273
> state/state.go:273: type AddMachineParams struct {
> Quibble quibble, should we call it something like MachineSpec? It's
not really
> Add-specific.

This crossed my mind too, but I'm going to leave it. I want to keep it
xParams, because that's what it is: a parameter structure. I wouldn't
want to inadvertently suggest that the structure could be used for other
purposes.

If this becomes contentious, we could consolidate all AddMachine* and
InjectMachine functions to one function: AddMachine(params
AddMachineParams, mode AddMachineMode), where mode tells it what type of
addition it is. Alternatively, that can be conveyed in the presence/lack
of instance ID, though that's a bit more subtle and error prone.

https://codereview.appspot.com/13169043/

[Go Bot (go-bot)]

Attempt to merge into lp:juju-core failed due to conflicts:

text conflict in cmd/jujud/machine_test.go

=== modified file 'cmd/jujud/machine_test.go'
--- cmd/jujud/machine_test.go	2013-08-22 23:29:46 +0000
+++ cmd/jujud/machine_test.go	2013-08-23 07:58:33 +0000
@@ -13,11 +13,9 @@
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/charm"
 	"launchpad.net/juju-core/cmd"
-	"launchpad.net/juju-core/constraints"
 	"launchpad.net/juju-core/container/lxc"
 	envtesting "launchpad.net/juju-core/environs/testing"
 	"launchpad.net/juju-core/errors"
-	"launchpad.net/juju-core/instance"
 	"launchpad.net/juju-core/names"
 	"launchpad.net/juju-core/provider/dummy"
 	"launchpad.net/juju-core/state"
@@ -68,7 +66,12 @@
 // machine agent's directory.  It returns the new machine, the
 // agent's configuration and the tools currently running.
 func (s *MachineSuite) primeAgent(c *gc.C, jobs ...state.MachineJob) (m *state.Machine, config agent.Config, tools *tools.Tools) {
-	m, err := s.State.InjectMachine("series", constraints.Value{}, "ardbeg-0", instance.HardwareCharacteristics{}, jobs...)
+	m, err := s.State.InjectMachine(&state.AddMachineParams{
+		Series:     "series",
+		InstanceId: "ardbeg-0",
+		Nonce:      state.BootstrapNonce,
+		Jobs:       jobs,
+	})
 	c.Assert(err, gc.IsNil)
 	err = m.SetMongoPassword(initialMachinePassword)
 	c.Assert(err, gc.IsNil)

=== modified file 'environs/bootstrap/bootstrap.go'
--- environs/bootstrap/bootstrap.go	2013-08-22 23:39:47 +0000
+++ environs/bootstrap/bootstrap.go	2013-08-23 07:58:33 +0000
@@ -108,7 +108,14 @@
 	}
 
 	logger.Debugf("create bootstrap machine in state")
-	m, err := st.InjectMachine(version.Current.Series, cons, instId, characteristics, jobs...)
+	m, err := st.InjectMachine(&state.AddMachineParams{
+		Series:                  version.Current.Series,
+		Nonce:                   state.BootstrapNonce,
+		Constraints:             cons,
+		InstanceId:              instId,
+		HardwareCharacteristics: characteristics,
+		Jobs: jobs,
+	})
 	if err != nil {
 		return err
 	}

=== modified file 'state/state.go'
--- state/state.go	2013-08-22 22:24:34 +0000
+++ state/state.go	2013-08-23 07:58:33 +0000
@@ -170,6 +170,12 @@
 // supplied series. The machine's constraints and other configuration will be taken from
 // the supplied params struct.
 func (st *State) AddMachineWithConstraints(params *AddMachineParams) (m *Machine, err error) {
+	if params.InstanceId != "" {
+		return nil, fmt.Errorf("cannot specify an instance id when adding a new machine")
+	}
+	if params.Nonce != "" {
+		return nil, fmt.Errorf("cannot specify a nonce when adding a new machine")
+	}
 
 	// TODO(wallyworld) - if a container is required, and when the actual machine characteristics
 	// are made available, we need to check the machine constraints to ensure the container can be
@@ -180,20 +186,15 @@
 }
 
 // InjectMachine adds a new machine, corresponding to an existing provider
-// instance, configured to run the supplied jobs on the supplied series, using
-// the specified constraints.
-func (st *State) InjectMachine(series string, cons constraints.Value, instanceId instance.Id, hc instance.HardwareCharacteristics, jobs ...MachineJob) (m *Machine, err error) {
-	if instanceId == "" {
+// instance, configured according to the supplied params struct.
+func (st *State) InjectMachine(params *AddMachineParams) (m *Machine, err error) {
+	if params.InstanceId == "" {
 		return nil, fmt.Errorf("cannot inject a machine without an instance id")
 	}
-	return st.addMachine(&AddMachineParams{
-		Series:          series,
-		Constraints:     cons,
-		instanceId:      instanceId,
-		characteristics: hc,
-		nonce:           BootstrapNonce,
-		Jobs:            jobs,
-	})
+	if params.Nonce == "" {
+		return nil, fmt.Errorf("cannot inject a machine without a nonce")
+	}
+	return st.addMachine(params)
 }
 
 // containerRefParams specify how a machineContainers document is to be created.
@@ -273,14 +274,14 @@
 
 // AddMachineParams encapsulates the parameters used to create a new machine.
 type AddMachineParams struct {
-	Series          string
-	Constraints     constraints.Value
-	ParentId        string
-	ContainerType   instance.ContainerType
-	instanceId      instance.Id
-	characteristics instance.HardwareCharacteristics
-	nonce           string
-	Jobs            []MachineJob
+	Series                  string
+	Constraints             constraints.Value
+	ParentId                string
+	ContainerType           instance.ContainerType
+	InstanceId              instance.Id
+	HardwareCharacteristics instance.HardwareCharacteristics
+	Nonce                   string
+	Jobs                    []MachineJob
 }
 
 // addMachineContainerOps returns txn operations and associated Mongo records used to create a new machine,
@@ -291,14 +292,14 @@
 // 2. AssignToNewMachine, which is used to create a new machine on which to deploy a unit.
 func (st *State) addMachineContainerOps(params *AddMachineParams, cons constraints.Value) ([]txn.Op, *instanceData, *containerRefParams, error) {
 	var instData *instanceData
-	if params.instanceId != "" {
+	if params.InstanceId != "" {
 		instData = &instanceData{
-			InstanceId: params.instanceId,
-			Arch:       params.characteristics.Arch,
-			Mem:        params.characteristics.Mem,
-			RootDisk:   params.characteristics.RootDisk,
-			CpuCores:   params.characteristics.CpuCores,
-			CpuPower:   params.characteristics.CpuPower,
+			InstanceId: params.InstanceId,
+			Arch:       params.HardwareCharacteristics.Arch,
+			Mem:        params.HardwareCharacteristics.Mem,
+			RootDisk:   params.HardwareCharacteristics.RootDisk,
+			CpuCores:   params.HardwareCharacteristics.CpuCores,
+			CpuPower:   params.HardwareCharacteristics.CpuPower,
 		}
 	}
 	var ops []txn.Op
@@ -356,8 +357,8 @@
 		Clean:         true,
 	}
 	if mdoc.ContainerType == "" {
-		mdoc.InstanceId = params.instanceId
-		mdoc.Nonce = params.nonce
+		mdoc.InstanceId = params.InstanceId
+		mdoc.Nonce = params.Nonce
 	}
 	mdoc, machineOps, err := st.addMachineOps(mdoc, instData, cons, containerParams)
 	if err != nil {

=== modified file 'state/state_test.go'
--- state/state_test.go	2013-08-22 21:59:32 +0000
+++ state/state_test.go	2013-08-23 07:58:33 +0000
@@ -279,7 +279,15 @@
 		Constraints:   cons,
 		Jobs:          oneJob,
 	}
+	params.InstanceId = "id"
 	m, err := s.State.AddMachineWithConstraints(&params)
+	c.Assert(err, gc.ErrorMatches, "cannot specify an instance id when adding a new machine")
+	params.InstanceId = ""
+	params.Nonce = "nonce"
+	m, err = s.State.AddMachineWithConstraints(&params)
+	c.Assert(err, gc.ErrorMatches, "cannot specify a nonce when adding a new machine")
+	params.Nonce = ""
+	m, err = s.State.AddMachineWithConstraints(&params)
 	c.Assert(err, gc.IsNil)
 	c.Assert(m.Id(), gc.Equals, "0/lxc/0")
 	c.Assert(m.Series(), gc.Equals, "series")
@@ -307,12 +315,23 @@
 }
 
 func (s *StateSuite) TestInjectMachineErrors(c *gc.C) {
-	hc := instance.HardwareCharacteristics{}
-	_, err := s.State.InjectMachine("", emptyCons, instance.Id("i-minvalid"), hc, state.JobHostUnits)
+	injectMachine := func(series, instanceId, nonce string, jobs ...state.MachineJob) error {
+		params := &state.AddMachineParams{
+			InstanceId: instance.Id(instanceId),
+			Series:     series,
+			Nonce:      nonce,
+			Jobs:       jobs,
+		}
+		_, err := s.State.InjectMachine(params)
+		return err
+	}
+	err := injectMachine("", "i-minvalid", state.BootstrapNonce, state.JobHostUnits)
 	c.Assert(err, gc.ErrorMatches, "cannot add a new machine: no series specified")
-	_, err = s.State.InjectMachine("series", emptyCons, instance.Id(""), hc, state.JobHostUnits)
+	err = injectMachine("series", "", state.BootstrapNonce, state.JobHostUnits)
 	c.Assert(err, gc.ErrorMatches, "cannot inject a machine without an instance id")
-	_, err = s.State.InjectMachine("series", emptyCons, instance.Id("i-mlazy"), hc)
+	err = injectMachine("series", "i-minvalid", "", state.JobHostUnits)
+	c.Assert(err, gc.ErrorMatches, "cannot inject a machine without a nonce")
+	err = injectMachine("series", state.BootstrapNonce, "i-mlazy")
 	c.Assert(err, gc.ErrorMatches, "cannot add a new machine: no jobs specified")
 }
 
@@ -321,23 +340,30 @@
 	arch := "amd64"
 	mem := uint64(1024)
 	disk := uint64(1024)
-	hc := instance.HardwareCharacteristics{
-		Arch:     &arch,
-		Mem:      &mem,
-		RootDisk: &disk,
+	params := &state.AddMachineParams{
+		Series:      "series",
+		Constraints: cons,
+		InstanceId:  instance.Id("i-mindustrious"),
+		Nonce:       state.BootstrapNonce,
+		HardwareCharacteristics: instance.HardwareCharacteristics{
+			Arch:     &arch,
+			Mem:      &mem,
+			RootDisk: &disk,
+		},
+		Jobs: []state.MachineJob{state.JobHostUnits, state.JobManageEnviron},
 	}
-	m, err := s.State.InjectMachine("series", cons, instance.Id("i-mindustrious"), hc, state.JobHostUnits, state.JobManageEnviron)
+	m, err := s.State.InjectMachine(params)
 	c.Assert(err, gc.IsNil)
-	c.Assert(m.Jobs(), gc.DeepEquals, []state.MachineJob{state.JobHostUnits, state.JobManageEnviron})
+	c.Assert(m.Jobs(), gc.DeepEquals, params.Jobs)
 	instanceId, err := m.InstanceId()
 	c.Assert(err, gc.IsNil)
-	c.Assert(instanceId, gc.Equals, instance.Id("i-mindustrious"))
+	c.Assert(instanceId, gc.Equals, params.InstanceId)
 	mcons, err := m.Constraints()
 	c.Assert(err, gc.IsNil)
 	c.Assert(cons, gc.DeepEquals, mcons)
 	characteristics, err := m.HardwareCharacteristics()
 	c.Assert(err, gc.IsNil)
-	c.Assert(*characteristics, gc.DeepEquals, hc)
+	c.Assert(*characteristics, gc.DeepEquals, params.HardwareCharacteristics)
 
 	// Make sure the bootstrap nonce value is set.
 	c.Assert(m.CheckProvisioned(state.BootstrapNonce), gc.Equals, true)
@@ -345,12 +371,17 @@
 
 func (s *StateSuite) TestAddContainerToInjectedMachine(c *gc.C) {
 	oneJob := []state.MachineJob{state.JobHostUnits}
-	hc := instance.HardwareCharacteristics{}
-	m0, err := s.State.InjectMachine("series", emptyCons, instance.Id("i-mindustrious"), hc, state.JobHostUnits, state.JobManageEnviron)
+	params := state.AddMachineParams{
+		Series:     "series",
+		InstanceId: instance.Id("i-mindustrious"),
+		Nonce:      state.BootstrapNonce,
+		Jobs:       []state.MachineJob{state.JobHostUnits, state.JobManageEnviron},
+	}
+	m0, err := s.State.InjectMachine(&params)
 	c.Assert(err, gc.IsNil)
 
 	// Add first container.
-	params := state.AddMachineParams{
+	params = state.AddMachineParams{
 		ParentId:      "0",
 		ContainerType: instance.LXC,
 		Series:        "series",