From ca362d8d0f90c90893eff5a6cc9726517aeba98d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81ngel=20P=C3=A9rez?= Date: Mon, 12 Jun 2023 13:37:59 +0200 Subject: [PATCH] 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 | 2 +- internals/overlord/backend.go | 11 ---------- internals/overlord/overlord.go | 26 +++++++---------------- internals/overlord/servstate/manager.go | 2 +- internals/overlord/stateengine.go | 12 +++++++---- internals/overlord/stateengine_test.go | 28 +++++++++++++++++++------ 6 files changed, 40 insertions(+), 41 deletions(-) diff --git a/internals/daemon/daemon.go b/internals/daemon/daemon.go index d028c494f..df82ddcd2 100644 --- a/internals/daemon/daemon.go +++ b/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 } diff --git a/internals/overlord/backend.go b/internals/overlord/backend.go index 9ad5c1466..3c7ba17a6 100644 --- a/internals/overlord/backend.go +++ b/internals/overlord/backend.go @@ -35,14 +35,3 @@ 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) -} diff --git a/internals/overlord/overlord.go b/internals/overlord/overlord.go index 6befc35bc..50ca1a313 100644 --- a/internals/overlord/overlord.go +++ b/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 { diff --git a/internals/overlord/servstate/manager.go b/internals/overlord/servstate/manager.go index 535724228..3be455f48 100644 --- a/internals/overlord/servstate/manager.go +++ b/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 } diff --git a/internals/overlord/stateengine.go b/internals/overlord/stateengine.go index 00417e735..5575357c8 100644 --- a/internals/overlord/stateengine.go +++ b/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 } diff --git a/internals/overlord/stateengine_test.go b/internals/overlord/stateengine_test.go index 72e8d3ce2..8de824c69 100644 --- a/internals/overlord/stateengine_test.go +++ b/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") }