state: refactor addmachine.go

state: refactor addmachine.go

The current code had become a twisty maze. This
change is intended to make things somewhat more
straightforward.

The key insight is that there are only four
pathways through the code that require
different control flow:

- adding a top level machine
- adding a top level machine with associated instance id
- adding a machine within an existing machine
- adding a machine within a newly created machine

Each of these pathways now has its own function.

One pathway was catered for by the old code
but is not by the new: adding a machine within
a machine with associated instance id.
I don't believe that's a case we need to be
bothered about (and it's easy to add if we are).

For the time being, I have kept the external interface
identical, but I'd like to change it to be a little
more like the above set of functions, rather
than having a single set of AddMachineParams
suitable for every case.

https://code.launchpad.net/~rogpeppe/juju-core/463-state-ensure-availability/+merge/196177

(do not edit description out of merge proposal)

[rog, 2013-11-21 18:35:16]

Please take a look.

[rog, 2013-11-21 18:39:50]

Please take a look.

[wallyworld, 2013-11-22 06:47:48]

I like the idea behind the refactoring. My personal view is that there is too
much cut and paste with regard to the commonality between machineTemplate and
AddMachineParams structs. But I guess apart from composing one struct from
another there's not much that can be done.  There's also a lot of similar/almost
identical code used to construct a machineDoc from a machineTemplate. I'd
personally move this to a helper function, again to reduce the amount of
boilerplate. The more boilerplate, the more potential for bugs. YMMV.

LGTM - the code is sound even though I have difference of opinion about aspects
of the implementation.

https://codereview.appspot.com/30390043/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/30390043/diff/40001/state/state.go#newcode92
state/state.go:92: logger.Infof("transaction 'after' hook start")
Can we make all of these Infos Debugs or even Traces. I don't think we generally
want to pollute the output with such lines.

https://codereview.appspot.com/30390043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/30390043/diff/40001/state/unit.go#newcode902
state/unit.go:902: panic("no container type")
This should return an error instead of panicing

[rog, 2013-11-22 07:59:31]

Please take a look.

https://codereview.appspot.com/30390043/diff/40001/state/state.go
File state/state.go (right):

https://codereview.appspot.com/30390043/diff/40001/state/state.go#newcode92
state/state.go:92: logger.Infof("transaction 'after' hook start")
On 2013/11/22 06:47:48, wallyworld wrote:
> Can we make all of these Infos Debugs or even Traces. I don't think we
generally
> want to pollute the output with such lines.

This code should never be used in production.
I've added a comment to that effect.

https://codereview.appspot.com/30390043/diff/40001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/30390043/diff/40001/state/unit.go#newcode902
state/unit.go:902: panic("no container type")
On 2013/11/22 06:47:48, wallyworld wrote:
> This should return an error instead of panicing

I'm not sure - the callers ensure that if there's
a parent id, there's a container type (otherwise
they should be calling assignToMachine instead).

Done anyway.

[axw, 2013-11-22 09:04:29]

On 2013/11/22 07:59:31, rog wrote:
> Please take a look.

LGTM

> https://codereview.appspot.com/30390043/diff/40001/state/state.go
> File state/state.go (right):
> 
> https://codereview.appspot.com/30390043/diff/40001/state/state.go#newcode92
> state/state.go:92: logger.Infof("transaction 'after' hook start")
> On 2013/11/22 06:47:48, wallyworld wrote:
> > Can we make all of these Infos Debugs or even Traces. I don't think we
> generally
> > want to pollute the output with such lines.
> 
> This code should never be used in production.
> I've added a comment to that effect.
> 
> https://codereview.appspot.com/30390043/diff/40001/state/unit.go
> File state/unit.go (right):
> 
> https://codereview.appspot.com/30390043/diff/40001/state/unit.go#newcode902
> state/unit.go:902: panic("no container type")
> On 2013/11/22 06:47:48, wallyworld wrote:
> > This should return an error instead of panicing
> 
> I'm not sure - the callers ensure that if there's
> a parent id, there's a container type (otherwise
> they should be calling assignToMachine instead).
> 
> Done anyway.

[fwereade, 2013-11-27 16:13:35]

This is awesome, LGTM. Thank you very much.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode24
state/addmachine.go:24: // an instance for the machine.
These are the constraints *before* environment fallbacks are applied,right?
Would be worth mentioning that here.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode38
state/addmachine.go:38: // be associated with the machine.
This is only meaningful if InstanceId is set, I think?

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode52
state/addmachine.go:52: Jobs []MachineJob
I'd move jobs up above the optional parentid/containertype and
instanceid/nonce/hardware field groups.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:242: // given hardward characteristics.
s/hardward/hardware/

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:243: // The template must contain a valid instance id and
nonce.
Sucks that we don't require hardware characteristics, but looks like not all
providers actually return them :/. Would you add a bug for that if it doesn't
exist, please?

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:323: )
I'd just do `ops := []txn.Op{...}`, but as you like.

[rog, 2013-12-03 18:12:13]

Please take a look.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go
File state/addmachine.go (right):

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode24
state/addmachine.go:24: // an instance for the machine.
On 2013/11/27 16:13:36, fwereade wrote:
> These are the constraints *before* environment fallbacks are applied,right?
> Would be worth mentioning that here.

Done.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode38
state/addmachine.go:38: // be associated with the machine.
On 2013/11/27 16:13:36, fwereade wrote:
> This is only meaningful if InstanceId is set, I think?

Done.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcode52
state/addmachine.go:52: Jobs []MachineJob
On 2013/11/27 16:13:36, fwereade wrote:
> I'd move jobs up above the optional parentid/containertype and
> instanceid/nonce/hardware field groups.

Done.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:242: // given hardward characteristics.
On 2013/11/27 16:13:36, fwereade wrote:
> s/hardward/hardware/

Done.

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:243: // The template must contain a valid instance id and
nonce.
On 2013/11/27 16:13:36, fwereade wrote:
> Sucks that we don't require hardware characteristics, but looks like not all
> providers actually return them :/. Would you add a bug for that if it doesn't
> exist, please?

I'm not sure I understand this. Are you saying that we should check that some
minimal subset of h/w characteristics is set?

https://codereview.appspot.com/30390043/diff/60001/state/addmachine.go#newcod...
state/addmachine.go:323: )
On 2013/11/27 16:13:36, fwereade wrote:
> I'd just do `ops := []txn.Op{...}`, but as you like.

can't quite do that, as insertNewMachineOps returns a slice,
which can't go inside the above construct.
(we'd need the equivalent of the lisp ,@ operator :-])