state: implement Unit.AssignToUnusedMachine

state/unit.go

 package state
 
 import (
         "errors"
         "fmt"
         "labix.org/v2/mgo/txn"
         "launchpad.net/juju-core/charm"
         "launchpad.net/juju-core/state/presence"
         "launchpad.net/juju-core/trivial"
         "sort"
@@ skipping 382 matching lines @@
         }
         if pudoc.MachineId == nil {
                 return 0, errors.New("unit not assigned to machine")
         }
         return *pudoc.MachineId, nil
 }
 
 var (
         machineDeadErr     = errors.New("machine is dead")
         unitDeadErr        = errors.New("unit is dead")
-        cannotAssignErr    = errors.New("unit cannot be assigned to machine")

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

This seems confusing, and luckily seems unused as well.

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

Done.

         alreadyAssignedErr = errors.New("unit is already assigned to a machine")
-        notUnusedErr       = errors.New("machine is not unused")

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

inUseErr = "machine is in use"

+        inUseErr           = errors.New("machine is not unused")

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

This was just half-done. Still has a double-negative on the message:

inUseErr = "machine is in use"

 )
 
 // assignToMachine is the internal version of AssignToMachine,
 // also used by AssignToUnusedMachine. It returns specific errors
 // in some cases:
 // - machineDeadErr when the machine is not alive.
 // - unitDeadErr when the unit is not alive.
 // - alreadyAssignedErr when the unit has already been assigned
-// - notUnusedErr when the machine already has a unit assigned (if onlyIfUnused is true)
-func (u *Unit) assignToMachine(m *Machine, onlyIfUnused bool) (err error) {

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

s/onlyIfUnused/unused/

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

much better.

+// - inUseErr when the machine already has a unit assigned (if unused is true)
+func (u *Unit) assignToMachine(m *Machine, unused bool) (err error) {
         if u.doc.MachineId != nil {
                 if *u.doc.MachineId != m.Id() {
                         return alreadyAssignedErr
                 }
                 return nil
         }
         if u.doc.Principal != "" {
                 return fmt.Errorf("unit is a subordinate")
         }
         assert := append(isAlive, D{
                 {"$or", []D{
                         D{{"machineid", nil}},
                         D{{"machineid", m.Id()}},
                 }},
         }...)
         massert := isAlive
-        if onlyIfUnused {
+        if unused {
                 massert = append(massert, D{{"principals", D{{"$size", 0}}}}...)
         }
         ops := []txn.Op{{
                 C:      u.st.units.Name,
                 Id:     u.doc.Name,
                 Assert: assert,
                 Update: D{{"$set", D{{"machineid", m.doc.Id}}}},
         }, {
                 C:      u.st.machines.Name,
                 Id:     m.doc.Id,
                 Assert: massert,
                 Update: D{{"$addToSet", D{{"principals", u.doc.Name}}}},
         }}
-        if err := u.st.runner.Run(ops, "", nil); err != nil {

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

if err == nil || err != txn.ErrAborted {
    return err
}

Dedent the rest.

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

Done.

-                if err != txn.ErrAborted {
-                        return err
-                }
-                u0, err := u.st.Unit(u.Name())
-                if err != nil {
-                        return err
-                }
-                m0, err := u.st.Machine(m.Id())
-                if err != nil {
-                        return err
-                }
-                switch {
-                case u0.Life() != Alive:
-                        return unitDeadErr
-                case m0.Life() != Alive:
-                        return machineDeadErr
-                case u0.doc.MachineId != nil:

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

|| !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.

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

good catch.

-                        return alreadyAssignedErr
-                }
-                return notUnusedErr
-        }
-        u.doc.MachineId = &m.doc.Id
-        return nil
+        err = u.st.runner.Run(ops, "", nil)
+        if err == nil {
+                u.doc.MachineId = &m.doc.Id
+                return nil
+        }
+        if err != txn.ErrAborted {
+                return err
+        }
+        u0, err := u.st.Unit(u.Name())
+        if err != nil {
+                return err
+        }
+        m0, err := u.st.Machine(m.Id())
+        if err != nil {
+                return err
+        }
+        switch {
+        case u0.Life() != Alive:
+                return unitDeadErr
+        case m0.Life() != Alive:
+                return machineDeadErr
+        case u0.doc.MachineId != nil || !unused:
+                return alreadyAssignedErr
+        }
+        return inUseErr
 }
 
 // AssignToMachine assigns this unit to a given machine.
 func (u *Unit) AssignToMachine(m *Machine) error {
         err := u.assignToMachine(m, false)
         if err != nil {
                 return fmt.Errorf("cannot assign unit %q to machine %v: %v", u, m, err)
         }
         return nil
 }
 
 var noUnusedMachines = errors.New("all machines in use")

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

This doesn't have to be a variable. Just inline it below.

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

it's used by the next branch.

 
 // AssignToUnusedMachine assigns u to a machine without other units.
 // If there are no unused machines besides machine 0, an error is returned.
 func (u *Unit) AssignToUnusedMachine() (m *Machine, err error) {
         // Select all machines with no principals except the bootstrap machine.
         sel := D{{"principals", D{{"$size", 0}}}, {"_id", D{{"$ne", 0}}}}
         // TODO use Batch(1). See https://bugs.launchpad.net/mgo/+bug/1053509
         iter := u.st.machines.Find(sel).Batch(2).Prefetch(0).Iter()
         var mdoc machineDoc
         for iter.Next(&mdoc) {
                 m := newMachine(u.st, &mdoc)
 
-                switch err := u.assignToMachine(m, true); err {

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

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(...)
}

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

Done.

-                case nil:
+                err := u.assignToMachine(m, true)
+                if err == nil {
                         return m, nil
-                case notUnusedErr, machineDeadErr:
-                default:
-                        return nil, fmt.Errorf("cannot assign unit %q to unused machine: %v", u.Name(), err)

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

s/u.Name()/u/

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

Done.

-                }
-        }
-        // TODO check to see if the unit has now been assigned,

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

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]

Done.

-        // and if so return a more descriptive error.
+                }
+                if err != inUseErr && err != machineDeadErr {
+                        return nil, fmt.Errorf("cannot assign unit %q to unused machine: %v", u, err)
+                }
+        }
         return nil, noUnusedMachines
 }
 
 // UnassignFromMachine removes the assignment between this unit and the
 // machine it's assigned to.
 func (u *Unit) UnassignFromMachine() (err error) {
         // TODO check local machine id and add an assert that the
         // machine id is as expected.
         ops := []txn.Op{{
                 C:      u.st.units.Name,
@@ skipping 120 matching lines @@
                 return p1.Protocol < p2.Protocol
         }
         return p1.Number < p2.Number
 }
 
 // SortPorts sorts the given ports, first by protocol,
 // then by number.
 func SortPorts(ports []Port) {
         sort.Sort(portSlice(ports))
 }