state.Unit: don't reuse existing machines

state.Unit: don't reuse existing machines

Two main changes here:
 * A new AssignNew policy has been added that doesn't reuse machines
 * A function to AssignToNewMachine for a Unit

We don't want to reuse machines right now as existing machines
that may have had units deployed on them may well be dirty, as we
cannot ensure that the charms clean up after themselves.  Perhaps
in the not too distant future, we may be able to deploy charms
into containers that would mean that when we removed the unit we
can be sure that the machine is clean, but until that is the
case, we don't want to reuse machines.  However I didn't want to
remove functionality that we would be wanting again soon(ish), so
a new policy has been created, and the providers updated to use
it.

To make the operations to construct a new machine easily
available, a new function was added to State to get the required
operations.

Unit gets a new function to AssignToNewMachine.  This creates the
new machine and sets the assignment in one transaction.
Preconditions are much easier here as we know that the machine
doesn't have any current principals, and is of the correct
series.

https://code.launchpad.net/~thumper/juju-core/start-instance-tool-selection/+merge/155688

(do not edit description out of merge proposal)

[thumper, 2013-03-27 09:52:50]

Please take a look.

[fwereade, 2013-03-27 11:06:40]

Looks very nice, just a few comments; LGTM with those addressed.

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode425
state/assign_test.go:425: unused, err := s.State.AddMachine("series",
state.JobHostUnits)
assert err

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode449
state/assign_test.go:449: func (s *AssignSuite)
TestAssignUnitToNewMachineDyingService(c *C) {
s/Service/Unit/

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode459
state/assign_test.go:459: func (s *AssignSuite)
TestAssignUnitToNewMachineRemovedService(c *C) {
s/Service/Unit/

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode470
state/assign_test.go:470: 
Please just add one more test for passing AssignNew into AssignUnit itself.

https://codereview.appspot.com/7845048/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7845048/diff/1/state/state.go#newcode870
state/state.go:870: return u.AssignToNewMachine()
fallthrough? Not sure, YMMV

https://codereview.appspot.com/7845048/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode726
state/unit.go:726: mdoc, ops, err := u.st.addMachineOps(mdoc)
Check error

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode732
state/unit.go:732: }...)
I think we can just make this:

isUnassigned := D{{"machineid", ""}}

...

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode736
state/unit.go:736: Assert: assert,
and this:

Assert: append(isAliveDoc, isUnassigned...),

...but this bit's a matter of taste, I guess.

Regardless, I don't think we need the $or -- there's no way the unit could
already be assigned to the machine we just created, is there?

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode746
state/unit.go:746: }
My preference would be to smoosh the error handling from Run into a single
if/else if chain, but as you prefer.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode748
state/unit.go:748: // would fail (due to the machine id that it has is unique),
then the only
s/due to/because/

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode750
state/unit.go:750: // alive, or it has been assigned to a different machine.
The last couple of lines feel a little awkward, rephrase a little if possible.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode757
state/unit.go:757: return unitDeadErr
It might just be Dying, not necessarily dead or removed. Not sure whether this
is a matter of returning a different error, or just of fixing/renaming the
existing one.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode761
state/unit.go:761: // Error better than a panic... maybe.
Definitely better to error than panic here, I think.

[thumper, 2013-03-27 20:23:24]

About to push changes from this review.

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode425
state/assign_test.go:425: unused, err := s.State.AddMachine("series",
state.JobHostUnits)
On 2013/03/27 11:06:40, fwereade wrote:
> assert err

Done.

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode449
state/assign_test.go:449: func (s *AssignSuite)
TestAssignUnitToNewMachineDyingService(c *C) {
On 2013/03/27 11:06:40, fwereade wrote:
> s/Service/Unit/

Done.

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode459
state/assign_test.go:459: func (s *AssignSuite)
TestAssignUnitToNewMachineRemovedService(c *C) {
On 2013/03/27 11:06:40, fwereade wrote:
> s/Service/Unit/

Done.

https://codereview.appspot.com/7845048/diff/1/state/assign_test.go#newcode470
state/assign_test.go:470: 
On 2013/03/27 11:06:40, fwereade wrote:
> Please just add one more test for passing AssignNew into AssignUnit itself.

Done.

https://codereview.appspot.com/7845048/diff/1/state/state.go
File state/state.go (right):

https://codereview.appspot.com/7845048/diff/1/state/state.go#newcode870
state/state.go:870: return u.AssignToNewMachine()
On 2013/03/27 11:06:40, fwereade wrote:
> fallthrough? Not sure, YMMV

I think I'd rather keep it explicit than fallthrough.

https://codereview.appspot.com/7845048/diff/1/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode726
state/unit.go:726: mdoc, ops, err := u.st.addMachineOps(mdoc)
On 2013/03/27 11:06:40, fwereade wrote:
> Check error

Done.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode732
state/unit.go:732: }...)
On 2013/03/27 11:06:40, fwereade wrote:
> I think we can just make this:
> 
> isUnassigned := D{{"machineid", ""}}
> 
> ...

Done.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode736
state/unit.go:736: Assert: assert,
On 2013/03/27 11:06:40, fwereade wrote:
> and this:
> 
> Assert: append(isAliveDoc, isUnassigned...),
> 
> ...but this bit's a matter of taste, I guess.
> 
> Regardless, I don't think we need the $or -- there's no way the unit could
> already be assigned to the machine we just created, is there? 

No.  I'm still getting my head around how we handle the ops for the
transactions.  But you are right, and I've adjusted the code.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode746
state/unit.go:746: }
On 2013/03/27 11:06:40, fwereade wrote:
> My preference would be to smoosh the error handling from Run into a single
> if/else if chain, but as you prefer.

Done.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode748
state/unit.go:748: // would fail (due to the machine id that it has is unique),
then the only
On 2013/03/27 11:06:40, fwereade wrote:
> s/due to/because/

Done.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode750
state/unit.go:750: // alive, or it has been assigned to a different machine.
On 2013/03/27 11:06:40, fwereade wrote:
> The last couple of lines feel a little awkward, rephrase a little if possible.

Done.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode757
state/unit.go:757: return unitDeadErr
On 2013/03/27 11:06:40, fwereade wrote:
> It might just be Dying, not necessarily dead or removed. Not sure whether this
> is a matter of returning a different error, or just of fixing/renaming the
> existing one.

This error behaviour has been copied from the other Assign methods.  I think as
far as the caller is concerned, Dying is as good as Dead.  I think we may want
to consider renaming the error in a different branch, if at all.

https://codereview.appspot.com/7845048/diff/1/state/unit.go#newcode761
state/unit.go:761: // Error better than a panic... maybe.
On 2013/03/27 11:06:40, fwereade wrote:
> Definitely better to error than panic here, I think.

Cool.  Wasn't entirely sure.

[thumper, 2013-03-27 20:28:32]

Please take a look.

[wallyworld, 2013-03-27 21:33:17]

Nice branch, good tests.
LGTM, but best to get a more informed opinion on the actual addMachineOps logic.

[dave_cheney.net, 2013-03-27 22:47:08]

Super close, just a few things to polish up.

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go#n...
environs/dummy/environs.go:511: // Updated to be consistent with ec2 and
openstack.
Remove this line.

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go#n...
environs/dummy/environs.go:516: // to assign back to unused machines.
This is a really important comment block, but it feels out of place on the dummy
provider. Sure dummy should follow the conservative policies of ec2 and
openstack, but maybe the comment should read.

// Although dummy does not actually start instances, it must respect the 
// conservative assignment policy for the providers that do instantiate 
// machines. 

Then take what you have written here and move to to ec2 and openstack (assuming
it isn't there already, I haven't got that far yet)

https://codereview.appspot.com/7845048/diff/6001/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/7845048/diff/6001/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:82: c.Assert(policy, FitsTypeOf, state.AssignNew)
please test this in the same way as in local_test, they are probably both
correct, but pick one.

https://codereview.appspot.com/7845048/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7845048/diff/6001/juju/conn.go#newcode290
juju/conn.go:290: // State.
+1, please push a card onto the backlog for this.

https://codereview.appspot.com/7845048/diff/6001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/7845048/diff/6001/state/assign_test.go#newcode500
state/assign_test.go:500: _, err := s.State.AddMachine("series",
state.JobHostUnits) // available machine
worst constant ever. state.MaxUnitsPerHost ?

https://codereview.appspot.com/7845048/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7845048/diff/6001/state/unit.go#newcode42
state/unit.go:42: AssignNew AssignmentPolicy = "new"
This is going to drive you insane, but these typed constants don't do what you
think.

http://play.golang.org/p/hW9AL7YsTH

[thumper, 2013-03-28 00:43:35]

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go
File environs/dummy/environs.go (right):

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go#n...
environs/dummy/environs.go:511: // Updated to be consistent with ec2 and
openstack.
On 2013/03/27 22:47:09, dfc wrote:
> Remove this line.

Done.

https://codereview.appspot.com/7845048/diff/6001/environs/dummy/environs.go#n...
environs/dummy/environs.go:516: // to assign back to unused machines.
On 2013/03/27 22:47:09, dfc wrote:
> This is a really important comment block, but it feels out of place on the
dummy
> provider. Sure dummy should follow the conservative policies of ec2 and
> openstack, but maybe the comment should read.
> 
> // Although dummy does not actually start instances, it must respect the 
> // conservative assignment policy for the providers that do instantiate 
> // machines. 
> 
> Then take what you have written here and move to to ec2 and openstack
(assuming
> it isn't there already, I haven't got that far yet)

Done.

https://codereview.appspot.com/7845048/diff/6001/environs/jujutest/tests.go
File environs/jujutest/tests.go (right):

https://codereview.appspot.com/7845048/diff/6001/environs/jujutest/tests.go#n...
environs/jujutest/tests.go:82: c.Assert(policy, FitsTypeOf, state.AssignNew)
On 2013/03/27 22:47:09, dfc wrote:
> please test this in the same way as in local_test, they are probably both
> correct, but pick one.

This test is bogus.  Removing it.

In order for the provider to implement the interface, it has to provide this
method, and it has to be the right type.

https://codereview.appspot.com/7845048/diff/6001/juju/conn.go
File juju/conn.go (right):

https://codereview.appspot.com/7845048/diff/6001/juju/conn.go#newcode290
juju/conn.go:290: // State.
On 2013/03/27 22:47:09, dfc wrote:
> +1, please push a card onto the backlog for this.

https://canonical.leankit.com/Boards/View/103148069/104402685

https://codereview.appspot.com/7845048/diff/6001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/7845048/diff/6001/state/assign_test.go#newcode500
state/assign_test.go:500: _, err := s.State.AddMachine("series",
state.JobHostUnits) // available machine
On 2013/03/27 22:47:09, dfc wrote:
> worst constant ever. state.MaxUnitsPerHost ?

I agree it isn't a good constant, but it is one used a lot already.  And the
constant means "this machine has the job of hosting units".

https://codereview.appspot.com/7845048/diff/6001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/7845048/diff/6001/state/unit.go#newcode42
state/unit.go:42: AssignNew AssignmentPolicy = "new"
On 2013/03/27 22:47:09, dfc wrote:
> This is going to drive you insane, but these typed constants don't do what you
> think.
> 
> http://play.golang.org/p/hW9AL7YsTH

I know, but just following what the others are doing.

Obviously if we want a distinct type we'd go:

type AssignmentPolicy struct {
  string
}

Or something.

[dave_cheney.net, 2013-03-28 00:44:31]

LGTM. Fire at will.

[thumper, 2013-03-28 00:45:07]

Please take a look.

[thumper, 2013-03-28 00:52:20]

*** Submitted:

state.Unit: don't reuse existing machines

Two main changes here:
 * A new AssignNew policy has been added that doesn't reuse machines
 * A function to AssignToNewMachine for a Unit

We don't want to reuse machines right now as existing machines
that may have had units deployed on them may well be dirty, as we
cannot ensure that the charms clean up after themselves.  Perhaps
in the not too distant future, we may be able to deploy charms
into containers that would mean that when we removed the unit we
can be sure that the machine is clean, but until that is the
case, we don't want to reuse machines.  However I didn't want to
remove functionality that we would be wanting again soon(ish), so
a new policy has been created, and the providers updated to use
it.

To make the operations to construct a new machine easily
available, a new function was added to State to get the required
operations.

Unit gets a new function to AssignToNewMachine.  This creates the
new machine and sets the assignment in one transaction.
Preconditions are much easier here as we know that the machine
doesn't have any current principals, and is of the correct
series.

R=fwereade, wallyworld, dfc
CC=
https://codereview.appspot.com/7845048