uniter: integrate filter type

worker/uniter/modes.go

 package uniter
 
 import (
         "errors"
         "fmt"
         "launchpad.net/juju-core/environs"
         "launchpad.net/juju-core/log"
         "launchpad.net/juju-core/state"
+        "launchpad.net/juju-core/worker"
         "launchpad.net/juju-core/worker/uniter/charm"
         "launchpad.net/juju-core/worker/uniter/hook"
         "launchpad.net/tomb"
 )
 
 // Mode defines the signature of the functions that implement the possible
 // states of a running Uniter.
 type Mode func(u *Uniter) (Mode, error)
 
 // ModeInit is the initial Uniter mode.
 func ModeInit(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeInit", &next, &err)()
+        defer modeContext("ModeInit", &err)()
         log.Printf("updating unit addresses")
         cfg, err := u.st.EnvironConfig()
         if err != nil {
                 return nil, err
         }
         provider, err := environs.Provider(cfg.Type())
         if err != nil {
                 return nil, err
         }
         if private, err := provider.PrivateAddress(); err != nil {
                 return nil, err
         } else if err = u.unit.SetPrivateAddress(private); err != nil {
                 return nil, err
         }
         if public, err := provider.PublicAddress(); err != nil {
                 return nil, err
         } else if err = u.unit.SetPublicAddress(public); err != nil {
                 return nil, err
         }
         return ModeContinue, nil
 }
 
 // ModeContinue determines what action to take based on persistent uniter state.
 func ModeContinue(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeContinue", &next, &err)()
+        defer modeContext("ModeContinue", &err)()
 
         // When no charm exists, install it.
         s, err := u.sf.Read()
         if err == ErrNoStateFile {
                 log.Printf("charm is not deployed")
                 sch, _, err := u.service.Charm()
                 if err != nil {
                         return nil, err
                 }
                 return ModeInstalling(sch), nil
         } else if err != nil {
                 return nil, err
         }
 
         // Filter out states not related to charm deployment.
         switch s.Op {
         case Abide:
                 log.Printf("continuing after %q hook", s.Hook.Kind)
                 switch s.Hook.Kind {
                 case hook.Install:
                         return ModeStarting, nil
                 case hook.Stop:
                         return ModeTerminating, nil
                 }
                 return ModeAbide, nil
         case RunHook:
                 if s.OpStep == Queued {
                         log.Printf("found queued %q hook", s.Hook.Kind)
-                        return nil, u.runHook(*s.Hook)
+                        if err = u.runHook(*s.Hook); err != nil && err != errHookFailed {
+                                return nil, err
+                        }
+                        return ModeContinue, nil
                 }
                 if s.OpStep == Done {
                         log.Printf("found uncommitted %q hook", s.Hook.Kind)
-                        return nil, u.commitHook(*s.Hook)
+                        if err = u.commitHook(*s.Hook); err != nil {
+                                return nil, err
+                        }
+                        return ModeContinue, nil
                 }
                 log.Printf("awaiting error resolution for %q hook", s.Hook.Kind)
                 return ModeHookError, nil
         }
 
         // Resume interrupted deployment operations.
         sch, err := u.st.Charm(s.CharmURL)
         if err != nil {
                 return nil, err
         }
         if s.Op == Install {
                 log.Printf("resuming charm install")
                 return ModeInstalling(sch), nil
         } else if s.Op == Upgrade {
                 log.Printf("resuming charm upgrade")
                 return ModeUpgrading(sch), nil
         }
         panic(fmt.Errorf("unhandled uniter operation %q", s.Op))
 }
 
 // ModeInstalling is responsible for the initial charm deployment.
 func ModeInstalling(sch *state.Charm) Mode {
+        name := fmt.Sprintf("ModeInstalling %s", sch.URL())
         return func(u *Uniter) (next Mode, err error) {
-                name := fmt.Sprintf("ModeInstalling %s", sch.URL())
-                defer modeContext(name, &next, &err)()
-                return nil, u.deploy(sch, Install)
+                defer modeContext(name, &err)()
+                if err = u.deploy(sch, Install); err != nil {
+                        return nil, err
+                }
+                return ModeContinue, nil
         }
 }
 
 // ModeUpgrading is responsible for upgrading the charm.
 func ModeUpgrading(sch *state.Charm) Mode {
+        name := fmt.Sprintf("ModeUpgrading %s", sch.URL())
         return func(u *Uniter) (next Mode, err error) {
-                name := fmt.Sprintf("ModeUpgrading %s", sch.URL())
-                defer modeContext(name, &next, &err)()
+                defer modeContext(name, &err)()
                 if err = u.deploy(sch, Upgrade); err == charm.ErrConflict {
                         return ModeConflicted(sch), nil
-                }
-                return nil, err
+                } else if err != nil {
+                        return nil, err
+                }
+                return ModeContinue, nil
         }
 }
 
 // ModeStarting runs the "start" hook.
 func ModeStarting(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeStarting", &next, &err)()
+        defer modeContext("ModeStarting", &err)()
         if err = u.unit.SetStatus(state.UnitInstalled, ""); err != nil {
                 return nil, err
         }
-        return nil, u.runHook(hook.Info{Kind: hook.Start})
+        if err := u.runHook(hook.Info{Kind: hook.Start}); err == errHookFailed {
+                return ModeHookError, nil
+        } else if err != nil {
+                return nil, err
+        }
+        return ModeContinue, nil
 }
 
 // ModeStopping runs the "stop" hook.
 func ModeStopping(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeStopping", &next, &err)()
-        return nil, u.runHook(hook.Info{Kind: hook.Stop})
+        defer modeContext("ModeStopping", &err)()
+        if err := u.runHook(hook.Info{Kind: hook.Stop}); err == errHookFailed {
+                return ModeHookError, nil
+        } else if err != nil {
+                return nil, err
+        }
+        return ModeContinue, nil
 }
 
 // ModeTerminating marks the unit dead and returns ErrDead.
 func ModeTerminating(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeTerminating", &next, &err)()
+        defer modeContext("ModeTerminating", &err)()
         if err = u.unit.SetStatus(state.UnitStopped, ""); err != nil {
                 return nil, err
         }
         if err = u.unit.EnsureDead(); err != nil {
                 return nil, err
         }
-        return nil, ErrDead
+        return nil, worker.ErrDead
 }
 
 // ModeAbide is the Uniter's usual steady state. It watches for and responds to:
 // * service configuration changes
 // * charm upgrade requests
 // * relation changes (not implemented)
 // * unit death
 func ModeAbide(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeAbide", &next, &err)()
+        defer modeContext("ModeAbide", &err)()
         s, err := u.sf.Read()
         if err != nil {
                 return nil, err
         }
         if s.Op != Abide {
                 return nil, fmt.Errorf("insane uniter state: %#v", s)

[rog, 2012/10/01 11:02:58]

"insane uniter state; expected Abide, got %#v", s) ?
nice to say what we're expecting to see. same applies
for similar error messages below.

         }
         if err = u.unit.SetStatus(state.UnitStarted, ""); err != nil {
                 return nil, err
         }
 
-        // Execute an initial config-changed hook regardless of state.

[niemeyer, 2012/10/01 22:47:33]

s/regardless of state//; We only got here after quite a bit of state-checking.

[fwereade, 2012/10/02 07:01:35]

SGTM

+        // Prime the filter by requesting a config event, then handling it. This
+        // has two purposes: (1) run a config-change hook before doing anything
+        // else and (2) ensure that subsequent config events received actually
+        // correspond to changes relative to the state at the time we first ran
+        // the hook.
         cc := hook.Info{Kind: hook.ConfigChanged}
-        u.wantConfigEvent()
+        u.f.WantConfigEvent()
         select {
         case <-u.Dying():
                 return nil, tomb.ErrDying
-        case <-u.configEvents():

[niemeyer, 2012/10/01 22:47:33]

It'd be good to have a comment explaining why we have the logic here in that
shape. I suspect the reasoning is that we might have a config event either way,
and we want to catch it and discard it, but it's not obvious why we:

1) Say we want an event above

2) Receive the event

3) Ignore it completely

[fwereade, 2012/10/02 07:01:35]

The rationale is that we want to run one config-changed event here, and only run
further ones in the upcoming loop when they correspond to a different config. So
we prime the watcher to deliver a guaranteed event here, and can then rest
assured that it won't send any more until it gets one newer than the one it sent
in this loop, where we handled it.

-                if err = u.runHook(cc); err != nil {
-                        return nil, err
-                }
-        }
-
-        // Watch for everything else (including further config changes).

[niemeyer, 2012/10/01 22:47:33]

d

It's not watching everything else. It's watching the relevant channels for the
context, as usual.

[fwereade, 2012/10/02 07:01:35]

Ha, yes.

-        u.wantCharmEvent()
+        case <-u.f.ConfigEvents():
+                if err = u.runHook(cc); err == errHookFailed {
+                        return ModeHookError, nil
+                } else if err != nil {
+                        return nil, err
+                }
+        }
+
+        url, err := charm.ReadCharmURL(u.charm)
+        if err != nil {
+                return nil, err
+        }
+        u.f.WantUpgradeEvent(url, false)
         for {
                 select {
                 case <-u.Dying():
                         return nil, tomb.ErrDying
-                case <-u.unitDying():
+                case <-u.f.UnitDying():
                         // TODO don't stop until all relations broken.
                         return ModeStopping, nil
-                case <-u.configEvents():
-                        if err = u.runHook(cc); err != nil {
-                                return nil, err
-                        }
-                case ch := <-u.charmEvents():
-                        upgrade, err := u.getUpgrade(ch, false)
-                        if err == errNoUpgrade {
-                                continue
+                case <-u.f.ConfigEvents():
+                        if err = u.runHook(cc); err == errHookFailed {
+                                return ModeHookError, nil
                         } else if err != nil {
                                 return nil, err
                         }
+                case upgrade := <-u.f.UpgradeEvents():
                         return ModeUpgrading(upgrade), nil
                 }
         }
         panic("unreachable")
 }
 
 // ModeHookError is responsible for watching and responding to:
 // * user resolution of hook errors
 // * charm upgrade requests
 func ModeHookError(u *Uniter) (next Mode, err error) {
-        defer modeContext("ModeHookError", &next, &err)()
+        defer modeContext("ModeHookError", &err)()
         s, err := u.sf.Read()
         if err != nil {
                 return nil, err
         }
         if s.Op != RunHook || s.OpStep != Pending {
                 return nil, fmt.Errorf("insane uniter state: %#v", s)
         }
         msg := fmt.Sprintf("hook failed: %q", s.Hook.Kind)
         if err = u.unit.SetStatus(state.UnitError, msg); err != nil {
                 return nil, err
         }
-        resolveHook := getResolveHook(*s.Hook)
-        u.wantResolvedEvent()
-        u.wantCharmEvent()
+        url, err := charm.ReadCharmURL(u.charm)
+        if err != nil {
+                return nil, err
+        }
+        u.f.WantResolvedEvent()
+        u.f.WantUpgradeEvent(url, true)
         for {
                 select {
                 case <-u.Dying():
                         return nil, tomb.ErrDying
-                case rm := <-u.resolvedEvents():
-                        if success, err := u.resolveError(*rm, resolveHook); success {
-                                return ModeContinue, nil
-                        } else if err != nil && err != errHookFailed {
-                                return nil, err
-                        }
-                case ch := <-u.charmEvents():
-                        upgrade, err := u.getUpgrade(ch, true)
-                        if err == errNoUpgrade {
+                case rm := <-u.f.ResolvedEvents():
+                        switch rm {
+                        case state.ResolvedRetryHooks:
+                                err = u.runHook(*s.Hook)
+                        case state.ResolvedNoHooks:
+                                err = u.commitHook(*s.Hook)
+                        default:
+                                return nil, fmt.Errorf("unknown resolved mode %q", rm)
+                        }
+                        if e := u.unit.ClearResolved(); e != nil {
+                                return nil, e
+                        }
+                        if err == errHookFailed {
                                 continue
                         } else if err != nil {
                                 return nil, err
                         }
+                        return ModeContinue, nil
+                case upgrade := <-u.f.UpgradeEvents():
                         return ModeUpgrading(upgrade), nil
                 }
         }
         panic("unreachable")
 }
 
 // ModeConflicted is responsible for watching and responding to:
 // * user resolution of charm upgrade conflicts
 // * forced charm upgrade requests
 func ModeConflicted(sch *state.Charm) Mode {
         return func(u *Uniter) (next Mode, err error) {
-                defer modeContext("ModeConflicted", &next, &err)()
+                defer modeContext("ModeConflicted", &err)()
                 if err = u.unit.SetStatus(state.UnitError, "upgrade failed"); err != nil {
                         return nil, err
                 }
-                u.wantResolvedEvent()
-                u.wantCharmEvent()
+                u.f.WantResolvedEvent()
+                u.f.WantUpgradeEvent(sch.URL(), true)
                 for {
                         select {
                         case <-u.Dying():
                                 return nil, tomb.ErrDying
-                        case rm := <-u.resolvedEvents():
-                                if success, err := u.resolveError(*rm, resolveConflict); success {
-                                        return ModeUpgrading(sch), nil
-                                } else if err != nil {
+                        case <-u.f.ResolvedEvents():
+                                err = u.charm.Snapshotf("Upgrade conflict resolved.")
+                                if e := u.unit.ClearResolved(); e != nil {
+                                        return nil, e
+                                }
+                                if err != nil {
                                         return nil, err
                                 }
-                        case ch := <-u.charmEvents():
-                                upgrade, err := u.getUpgrade(ch, true)
-                                if err != nil {
-                                        if err == errNoUpgrade {
-                                                continue
-                                        }
-                                        return nil, err
-                                }
+                                return ModeUpgrading(sch), nil
+                        case upgrade := <-u.f.UpgradeEvents():
                                 if err := u.charm.Revert(); err != nil {
                                         return nil, err
                                 }
                                 return ModeUpgrading(upgrade), nil
                         }
                 }
                 panic("unreachable")
         }
 }
 
 // modeContext returns a function that implements logging and common error
 // manipulation for Mode funcs.
-func modeContext(name string, next *Mode, err *error) func() {

[rog, 2012/10/01 11:02:58]

as discussed online, most/all of this could be moved into the main loop.

[niemeyer, 2012/10/01 22:47:33]

+1 on turning this into straightforward code. Some of that shouldn't go to the
main loop, though, as it's magically manipulating the contract of the modes.
More details below.

+func modeContext(name string, err *error) func() {
         log.Printf(name + " starting")
         return func() {
                 log.Debugf(name + " exiting")
                 switch *err {
-                case nil:
-                        if *next == nil {

[niemeyer, 2012/10/01 22:47:33]

This seems to obscure the logic without benefit. When we want ModeContinue, we
should return ModeContinue.

-                                *next = ModeContinue
-                        }
-                case errHookFailed:
-                        *next, *err = ModeHookError, nil

[niemeyer, 2012/10/01 22:47:33]

Same idea. The state machine is being hidden behind the magic. If you want to
avoid typing if err == foo too often, it's trivial to make this with forward
logic by having:

    return runHook(...)

Where this function returns (next Mode, err error).

This may sound silly, but a person reading this code will understand what it is
doing, while a person that reads "return nil, runHook(...)", has to dig what the
heck is going on when we return nil, and why does it not stop running, and why
in certain cases the error doesn't actually stop the state machine.

This is all avoided if we keep the convention simple and strong:

1) Errors stop the state machine

2) First parameter has the next mode

[fwereade, 2012/10/02 07:01:35]

Good points all. Will drop the ModeContinue bits entirely; would anyone object
to putting the errHookFailed handling explicitly in he main loop?

[niemeyer, 2012/10/02 14:12:59]

I'd much prefer having it close to the logic that handles the error instead, and
holding the conventions mentioned above true. There's nothing special about this
error as far as the state machine is concerned.

-                case tomb.ErrDying, ErrDead:
-                        log.Printf(name + " shutting down")
+                case nil, tomb.ErrDying, worker.ErrDead:
                 default:
                         *err = errors.New(name + ": " + (*err).Error())
                 }
         }
 }