stateengine: make DryStart() required

 * Add StateEngine.dryStarted check on Ensure() in order to make
   DryStart() call actually required.
 * Call DryStart() on overlord and state engine tests
 * Remove noopStateBackend: not required since --dry run is cut
   after the DryStart() call, so no state is ever persisted.
   Furthermore, this allows for potential dry-run of state patches.
 * Remove Overlord.addManager(), since StateEngine.AddManager()
   already exists for this purpose, and Overlord.AddManager() as well.
 * Don't plan.ReadDir() on ServiceManager.DryStart(), acquire plan
   instead so that we actually initialize the manager instead.

internals/daemon/daemon.go

@@ -73,7 +73,7 @@ type Options struct {
 	// log output will be written to the writer.
 	ServiceOutput io.Writer
 
-	// Dry will only perform initialization tasks that don't have system-wide side effects.
+	// Dry will only perform initialization tasks that don't have unwanted side effects.
 	Dry bool
 }
 

internals/overlord/backend.go

@@ -34,15 +34,3 @@ func (osb *overlordStateBackend) Checkpoint(data []byte) error {
 func (osb *overlordStateBackend) EnsureBefore(d time.Duration) {
 	osb.ensureBefore(d)
 }
-
-type noopStateBackend struct {
-	ensureBefore func(d time.Duration)
-}
-
-func (nsb *noopStateBackend) Checkpoint(data []byte) error {
-	return nil
-}
-
-func (nsb *noopStateBackend) EnsureBefore(d time.Duration) {
-	nsb.ensureBefore(d)
-}

internals/overlord/overlord.go

@@ -89,16 +89,9 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
 	}
 	statePath := filepath.Join(pebbleDir, ".pebble.state")
 
-	var backend state.Backend
-	if dry {
-		backend = &noopStateBackend{
-			ensureBefore: o.ensureBefore,
-		}
-	} else {
-		backend = &overlordStateBackend{
-			path:         statePath,
-			ensureBefore: o.ensureBefore,
-		}
+	backend := &overlordStateBackend{
+		path:         statePath,
+		ensureBefore: o.ensureBefore,
 	}
 
 	s, err := loadState(statePath, restartHandler, backend)
@@ -116,16 +109,16 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
 	o.runner.AddOptionalHandler(matchAnyUnknownTask, handleUnknownTask, nil)
 
 	o.logMgr = logstate.NewLogManager()
-	o.addManager(o.logMgr)
+	o.stateEng.AddManager(o.logMgr)
 
 	o.serviceMgr, err = servstate.NewManager(s, o.runner, o.pebbleDir, serviceOutput, restartHandler, o.logMgr)
 	if err != nil {
 		return nil, err
 	}
-	o.addManager(o.serviceMgr)
+	o.stateEng.AddManager(o.serviceMgr)
 
 	o.commandMgr = cmdstate.NewManager(o.runner)
-	o.addManager(o.commandMgr)
+	o.stateEng.AddManager(o.commandMgr)
 
 	o.checkMgr = checkstate.NewManager()
 
@@ -149,10 +142,6 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
 	return o, nil
 }
 
-func (o *Overlord) addManager(mgr StateManager) {
-	o.stateEng.AddManager(mgr)
-}
-
 func loadState(statePath string, restartHandler restart.Handler, backend state.Backend) (*state.State, error) {
 	timings := timing.Start("", "", map[string]string{"startup": "load-state"})
 
@@ -445,6 +434,7 @@ func FakeWithState(handleRestart func(restart.RestartType)) *Overlord {
 	s := state.New(fakeBackend{o: o})
 	o.stateEng = NewStateEngine(s)
 	o.runner = state.NewTaskRunner(s)
+	o.stateEng.DryStart()
 	return o
 }
 
@@ -454,7 +444,7 @@ func (o *Overlord) AddManager(mgr StateManager) {
 	if o.inited {
 		panic("internal error: cannot add managers to a fully initialized Overlord")
 	}
-	o.addManager(mgr)
+	o.stateEng.AddManager(mgr)
 }
 
 type fakeBackend struct {

internals/overlord/servstate/manager.go

@@ -239,7 +239,7 @@ func (m *ServiceManager) acquirePlan() (release func(), err error) {
 
 // DryStart implements StateManager.DryStart.
 func (m *ServiceManager) DryStart() error {
-	_, err := plan.ReadDir(m.pebbleDir)
+	_, err := m.Plan()
 	return err
 }
 

internals/overlord/stateengine.go

@@ -29,8 +29,7 @@ var errStateEngineStopped = errors.New("state engine already stopped")
 // the system and manipulating it to reflect the desired state.
 type StateManager interface {
 	// DryStart prepares the manager to run its activities without
-	// incurring side effects that might affect the system
-	// state.
+	// incurring unwanted side effects.
 	DryStart() error
 
 	// Ensure forces a complete evaluation of the current state.
@@ -61,8 +60,9 @@ type StateStopper interface {
 // cope with Ensure calls in any order, coordinating among themselves
 // solely via the state.
 type StateEngine struct {
-	state   *state.State
-	stopped bool
+	state      *state.State
+	dryStarted bool
+	stopped    bool
 	// managers in use
 	mgrLock  sync.Mutex
 	managers []StateManager
@@ -114,6 +114,7 @@ func (se *StateEngine) DryStart() error {
 	if len(errs) != 0 {
 		return &multiError{errs}
 	}
+	se.dryStarted = true
 	return nil
 }
 
@@ -128,6 +129,9 @@ func (se *StateEngine) DryStart() error {
 func (se *StateEngine) Ensure() error {
 	se.mgrLock.Lock()
 	defer se.mgrLock.Unlock()
+	if !se.dryStarted {
+		return errors.New("state engine did not dry-start")
+	}
 	if se.stopped {
 		return errStateEngineStopped
 	}

internals/overlord/stateengine_test.go

@@ -97,10 +97,20 @@ func (ses *stateEngineSuite) TestDryStartError(c *C) {
 	se.AddManager(mgr2)
 
 	err := se.DryStart()
-	c.Check(err.Error(), DeepEquals, "multiple errors: boom1; boom2")
+	c.Check(err, ErrorMatches, `multiple errors: boom1; boom2`)
 	c.Check(calls, DeepEquals, []string{"drystart:mgr1", "drystart:mgr2"})
 }
 
+func (ses *stateEngineSuite) TestEnsureFailsNoDryStart(c *C) {
+	s := state.New(nil)
+	se := overlord.NewStateEngine(s)
+	mgr := &fakeManager{name: "mgr"}
+	se.AddManager(mgr)
+
+	err := se.Ensure()
+	c.Check(err, ErrorMatches, `state engine did not dry-start`)
+}
+
 func (ses *stateEngineSuite) TestEnsureError(c *C) {
 	s := state.New(nil)
 	se := overlord.NewStateEngine(s)
@@ -116,9 +126,12 @@ func (ses *stateEngineSuite) TestEnsureError(c *C) {
 	se.AddManager(mgr1)
 	se.AddManager(mgr2)
 
-	err := se.Ensure()
+	err := se.DryStart()
+	c.Check(err, IsNil)
+
+	err = se.Ensure()
 	c.Check(err.Error(), DeepEquals, "multiple errors: boom1; boom2")
-	c.Check(calls, DeepEquals, []string{"ensure:mgr1", "ensure:mgr2"})
+	c.Check(calls, DeepEquals, []string{"drystart:mgr1", "drystart:mgr2", "ensure:mgr1", "ensure:mgr2"})
 }
 
 func (ses *stateEngineSuite) TestStop(c *C) {
@@ -133,11 +146,14 @@ func (ses *stateEngineSuite) TestStop(c *C) {
 	se.AddManager(mgr1)
 	se.AddManager(mgr2)
 
+	err := se.DryStart()
+	c.Check(err, IsNil)
+
 	se.Stop()
-	c.Check(calls, DeepEquals, []string{"stop:mgr1", "stop:mgr2"})
+	c.Check(calls, DeepEquals, []string{"drystart:mgr1", "drystart:mgr2", "stop:mgr1", "stop:mgr2"})
 	se.Stop()
-	c.Check(calls, HasLen, 2)
+	c.Check(calls, HasLen, 4)
 
-	err := se.Ensure()
+	err = se.Ensure()
 	c.Check(err, ErrorMatches, "state engine already stopped")
 }