state: implement Unit.AssignToUnusedMachine

state: implement Unit.AssignToUnusedMachine



https://code.launchpad.net/~rogpeppe/juju-core/075-assign-to-unused/+merge/125527

(do not edit description out of merge proposal)

[rog, 2012-09-20 16:38:52]

Please take a look.

[TheMue, 2012-09-20 16:58:45]

On 2012/09/20 16:38:52, rog wrote:
> Please take a look.

LGTM

[rog, 2012-09-20 17:09:59]

Please take a look.

[niemeyer, 2012-09-20 17:15:37]

This looks very good, actually.

Some minor comments below. Will look at the tests tomorrow.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode403
state/unit.go:403: cannotAssignErr    = errors.New("unit cannot be assigned to
machine")
This seems confusing, and luckily seems unused as well.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode405
state/unit.go:405: notUnusedErr       = errors.New("machine is not unused")
inUseErr = "machine is in use"

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode415
state/unit.go:415: func (u *Unit) assignToMachine(m *Machine, onlyIfUnused bool)
(err error) {
s/onlyIfUnused/unused/

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode446
state/unit.go:446: if err := u.st.runner.Run(ops, "", nil); err != nil {
if err == nil || err != txn.ErrAborted {
    return err
}

Dedent the rest.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode463
state/unit.go:463: case u0.doc.MachineId != nil:
|| !unused

Otherwise below will yield "no unused machines" if someone won the race and
assigned the unit beforehand, even when this method is called to assign to
specific machine, which makes the "no unused" error quite confusing.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode481
state/unit.go:481: var noUnusedMachines = errors.New("all machines in use")
This doesn't have to be a variable. Just inline it below.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode494
state/unit.go:494: switch err := u.assignToMachine(m, true); err {
This switch seems to be making the logic less readable. This is the same as:

err := assign
if err == nil {
    return m, nil
}
if err != inUseErr && err != machineDeadErr {
    return nil, fmt.Errorf(...)
}

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode499
state/unit.go:499: return nil, fmt.Errorf("cannot assign unit %q to unused
machine: %v", u.Name(), err)
s/u.Name()/u/

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode502
state/unit.go:502: // TODO check to see if the unit has now been assigned,
I'd not fix this and drop the TODO. We can keep looping here forever with
someone else changing the state under our feet and the error message would
always be wrong.

[rog, 2012-09-21 08:35:19]

Please take a look.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode403
state/unit.go:403: cannotAssignErr    = errors.New("unit cannot be assigned to
machine")
On 2012/09/20 17:15:37, niemeyer wrote:
> This seems confusing, and luckily seems unused as well.

Done.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode415
state/unit.go:415: func (u *Unit) assignToMachine(m *Machine, onlyIfUnused bool)
(err error) {
On 2012/09/20 17:15:37, niemeyer wrote:
> s/onlyIfUnused/unused/

much better.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode446
state/unit.go:446: if err := u.st.runner.Run(ops, "", nil); err != nil {
On 2012/09/20 17:15:37, niemeyer wrote:
> if err == nil || err != txn.ErrAborted {
>     return err
> }
> 
> Dedent the rest.

Done.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode463
state/unit.go:463: case u0.doc.MachineId != nil:
On 2012/09/20 17:15:37, niemeyer wrote:
> || !unused
> 
> Otherwise below will yield "no unused machines" if someone won the race and
> assigned the unit beforehand, even when this method is called to assign to
> specific machine, which makes the "no unused" error quite confusing.

good catch.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode481
state/unit.go:481: var noUnusedMachines = errors.New("all machines in use")
On 2012/09/20 17:15:37, niemeyer wrote:
> This doesn't have to be a variable. Just inline it below.

it's used by the next branch.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode494
state/unit.go:494: switch err := u.assignToMachine(m, true); err {
On 2012/09/20 17:15:37, niemeyer wrote:
> This switch seems to be making the logic less readable. This is the same as:
> 
> err := assign
> if err == nil {
>     return m, nil
> }
> if err != inUseErr && err != machineDeadErr {
>     return nil, fmt.Errorf(...)
> }

Done.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode499
state/unit.go:499: return nil, fmt.Errorf("cannot assign unit %q to unused
machine: %v", u.Name(), err)
On 2012/09/20 17:15:37, niemeyer wrote:
> s/u.Name()/u/

Done.

https://codereview.appspot.com/6549045/diff/2001/state/unit.go#newcode502
state/unit.go:502: // TODO check to see if the unit has now been assigned,
On 2012/09/20 17:15:37, niemeyer wrote:
> I'd not fix this and drop the TODO. We can keep looping here forever with
> someone else changing the state under our feet and the error message would
> always be wrong.

Done.

[rog, 2012-09-21 08:40:26]

*** Submitted:

state: implement Unit.AssignToUnusedMachine

R=TheMue, niemeyer
CC=
https://codereview.appspot.com/6549045

[niemeyer, 2012-09-21 09:28:13]

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcode21
state/assign_test.go:21: s.service, err = s.State.AddService("wordpress",
s.charm)
I think the fact we have suite-provided data here is both confusing tests and
making them more fragile. The test TestAssignUnitToUnusedMachineOnlyZero for
example, assumes that there's an unused machine zero, which means that changing
SetUpTest in an apparently innocent way could end up disabling the test
inadvertently.

I suggest we drop the data here, and make every test create their own state, so
that we have all in context. So if we have multiple
machines/services/units/whatever, they are all visible in the test itself.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:156: func (s *AssignSuite) TestAssignMachineWhenDying(c *C)
{
This test is really multiple tests bundled into a single one. Splitting them off
into sub-test would be great.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:242: // Add some units to another service and allocate them
to machines
Having the entities being created locally will help this test too.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:264: // TODO s.unit.Die
?

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:303: c.Assert(err, ErrorMatches, `all machines in use`)
The sequence in this test is a nice one.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:306: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithChangingService(c *C) {
s/Changing/Removed/

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:307: // Check for a 'state changed' error if a service is
manipulated
// Fail if service is removed.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:320: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithChangingUnit(c *C) {
s/Changing/Removed/

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:322: // during reuse.
// Fail if unit is removed.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:334: func (s *AssignSuite)
TestAssignUnitToUnusedMachineOnlyZero(c *C) {
As mentioned above, we definitely need to have some logic here that verifies
that we do have machine 0 unused.  Otherwise the harness can be modified and
this test would be inadvertently disabled.

https://codereview.appspot.com/6549045/diff/12001/state/unit.go
File state/unit.go (right):

https://codereview.appspot.com/6549045/diff/12001/state/unit.go#newcode404
state/unit.go:404: inUseErr           = errors.New("machine is not unused")
This was just half-done. Still has a double-negative on the message:

inUseErr = "machine is in use"

[rog, 2012-09-21 10:34:34]

Comments addressed in https://codereview.appspot.com/6550052

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go
File state/assign_test.go (right):

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcode21
state/assign_test.go:21: s.service, err = s.State.AddService("wordpress",
s.charm)
On 2012/09/21 09:28:13, niemeyer wrote:
> I think the fact we have suite-provided data here is both confusing tests and
> making them more fragile. The test TestAssignUnitToUnusedMachineOnlyZero for
> example, assumes that there's an unused machine zero, which means that
changing
> SetUpTest in an apparently innocent way could end up disabling the test
> inadvertently.
> 
> I suggest we drop the data here, and make every test create their own state,
so
> that we have all in context. So if we have multiple
> machines/services/units/whatever, they are all visible in the test itself.

done. this also has the nice effect that we also test AssignUnit to machine 0
which should not be special in that context.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:156: func (s *AssignSuite) TestAssignMachineWhenDying(c *C)
{
On 2012/09/21 09:28:13, niemeyer wrote:
> This test is really multiple tests bundled into a single one. Splitting them
off
> into sub-test would be great.

Done.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:264: // TODO s.unit.Die
On 2012/09/21 09:28:13, niemeyer wrote:
> ?

yeah. did the TODO :-)

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:303: c.Assert(err, ErrorMatches, `all machines in use`)
On 2012/09/21 09:28:13, niemeyer wrote:
> The sequence in this test is a nice one.

cool.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:306: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithChangingService(c *C) {
On 2012/09/21 09:28:13, niemeyer wrote:
> s/Changing/Removed/

Done.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:307: // Check for a 'state changed' error if a service is
manipulated
On 2012/09/21 09:28:13, niemeyer wrote:
> // Fail if service is removed.

Done.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:320: func (s *AssignSuite)
TestAssignUnitToUnusedMachineWithChangingUnit(c *C) {
On 2012/09/21 09:28:13, niemeyer wrote:
> s/Changing/Removed/

Done.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:322: // during reuse.
On 2012/09/21 09:28:13, niemeyer wrote:
> // Fail if unit is removed.

Done.

https://codereview.appspot.com/6549045/diff/12001/state/assign_test.go#newcod...
state/assign_test.go:334: func (s *AssignSuite)
TestAssignUnitToUnusedMachineOnlyZero(c *C) {
On 2012/09/21 09:28:13, niemeyer wrote:
> As mentioned above, we definitely need to have some logic here that verifies
> that we do have machine 0 unused.  Otherwise the harness can be modified and
> this test would be inadvertently disabled.

Done.