state: machine units watcher reports unassignment

state/watcher.go

 package state
 
 import (
         "labix.org/v2/mgo"
         "launchpad.net/juju-core/environs/config"
         "launchpad.net/juju-core/state/watcher"
         "launchpad.net/tomb"
         "strings"
 )
 
@@ skipping 1285 matching lines @@
         }
         return pending, nil
 }
 
 func (w *MachineUnitsWatcher) merge(pending []string, unit string) (new []string, err error) {
         doc := unitDoc{}
         err = w.st.units.FindId(unit).One(&doc)
         if err != nil && err != mgo.ErrNotFound {
                 return nil, err
         }
+        found := err != mgo.ErrNotFound
         life, known := w.known[unit]
-        if err == mgo.ErrNotFound {
-                if known {
-                        w.st.watcher.Unwatch(w.st.units.Name, unit, w.in)
-                        if life != Dead {
-                                return append(pending, unit), nil
-                        }
+        isprincipal := doc.Principal == ""

[niemeyer, 2012/10/19 00:10:25]

If it wasn't found, that's not right.

+        w.known[unit] = doc.Life

[niemeyer, 2012/10/19 00:10:25]

If it wasn't found, that's certainly not right either.

+        if !known {
+                if _, ok := w.known[doc.Principal]; !ok && !isprincipal {

[niemeyer, 2012/10/19 00:10:25]

Why are we looking for doc.Principal when it can be ""?

+                        return pending, nil
                 }
-                return pending, nil
-        }
-        if !known {
                 w.st.watcher.Watch(w.st.units.Name, unit, doc.TxnRevno, w.in)

[niemeyer, 2012/10/19 00:10:25]

If the document is !known && !found, we'll get here.

                 pending = append(pending, unit)

[niemeyer, 2012/10/19 00:10:25]

So if the document was previously unknown, we don't care about the fact it was
just unassigned, or assigned to the wrong machine after we decided to merge it?

-        } else if life != doc.Life {
-                found := false
-                for _, v := range pending {
-                        if v == unit {
-                                found = true
+        } else {
+                var unassigned bool
+                if isprincipal {
+                        unassigned = (doc.MachineId != nil) && *doc.MachineId != w.machine.doc.Id || doc.MachineId == nil

[niemeyer, 2012/10/19 00:10:25]

doc.MachineId == nil || *doc.MachineId != w.machine.doc.Id

+                } else if _, ok := w.known[doc.Principal]; !ok {
+                        unassigned = true
+                }
+                if !found || unassigned {
+                        delete(w.known, unit)
+                        w.st.watcher.Unwatch(w.st.units.Name, unit, w.in)
+                        if doc.Life != Dead && !hasString(pending, unit) {

[niemeyer, 2012/10/19 00:10:25]

By now I'm finding pretty hard to recognize all the ins and outs of the
implementation. I can't tell with certainty if this is free from the kind of
issue I described above.

+                                pending = append(pending, unit)
                         }
+                        for _, subunit := range doc.Subordinates {
+                                pending, err = w.merge(pending, subunit)
+                                if err != nil {
+                                        return nil, err
+                                }
+                        }
+                        return pending, nil
                 }
-                if !found {
+                if doc.Life != life && !hasString(pending, unit) {
                         pending = append(pending, unit)
                 }
         }
-        w.known[unit] = doc.Life
         for _, subunit := range doc.Subordinates {
                 if _, ok := w.known[subunit]; !ok {
                         pending, err = w.merge(pending, subunit)
                         if err != nil {
                                 return nil, err
                         }
                 }
         }
         return pending, nil
 }
@@ skipping 34 matching lines @@
                         if len(changes) > 0 {
                                 out = w.out
                         }
                 case out <- changes:
                         out = nil
                         changes = nil
                 }
         }
         panic("unreachable")
 }