Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(cli): add --dry option to pebble run #214

Closed
wants to merge 6 commits into from
Closed

feat(cli): add --dry option to pebble run #214

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8ecc565
Add dry run on daemon and managers
anpep Jun 8, 2023
901dcd4
Minor fixes
anpep Jun 9, 2023
f615dbf
stateengine: make DryStart() required
anpep Jun 12, 2023
782783f
cli: import `multiError` implementation from snapd
anpep Jun 26, 2023
73954cf
Merge branch 'master' into overlord-prevalidation
anpep Nov 10, 2023
e973894
Merge branch 'master' into overlord-prevalidation
anpep Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions internals/cli/cmd_run.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ type sharedRunEnterOpts struct {
Hold bool `long:"hold"`
HTTP string `long:"http"`
Verbose bool `short:"v" long:"verbose"`
Dry bool `long:"dry"`
Args [][]string `long:"args" terminator:";"`
}

Expand All @@ -56,6 +57,7 @@ var sharedRunEnterOptsHelp = map[string]string{
"hold": "Do not start default services automatically",
"http": `Start HTTP API listening on this address (e.g., ":4000")`,
"verbose": "Log all output from services to stdout",
"dry": `Attempt to run without actually running`,
"args": `Provide additional arguments to a service`,
}

Expand Down Expand Up @@ -144,6 +146,7 @@ func runDaemon(rcmd *cmdRun, ch chan os.Signal, ready chan<- func()) error {
dopts := daemon.Options{
Dir: pebbleDir,
SocketPath: socketPath,
Dry: rcmd.Dry,
}
if rcmd.Verbose {
dopts.ServiceOutput = os.Stdout
Expand All @@ -154,6 +157,9 @@ func runDaemon(rcmd *cmdRun, ch chan os.Signal, ready chan<- func()) error {
if err != nil {
return err
}
if rcmd.Dry {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this go all the way to the sanityCheck below, right before Start?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've extensively discussed this with @flotter, and one of his (rightful) concerns where around system-wide side effects caused by e.g. listening on a port, since it's an "external" side effect (i.e. contributes to Pebble initialization by persisting some state externally, be it listening on a socket or calling .Checkpoint() on the state backend).

The idea is for the validation to be contained in the .DryStart() methods, and also sanityCheck() is a no-op for now anyway.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, there's definitely a limit that is worth considering, so @flotter is right on track. The question here is the cost-benefit: what are we leaving out by following a bit further, and what are the issues that can happen. For example, if we just open the port, it means we've validated that the port may be opened at all. Given that benefit, what's the cost? Will any messages be handled, or maybe not because that's done further down?

return nil
}
if err := d.Init(); err != nil {
return err
}
Expand Down
5 changes: 4 additions & 1 deletion internals/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,9 @@ type Options struct {
// ServiceOuput is an optional io.Writer for the service log output, if set, all services
// log output will be written to the writer.
ServiceOutput io.Writer

// Dry will only perform initialization tasks that don't have unwanted side effects.
Dry bool
}

// A Daemon listens for requests and routes them to the right command
Expand Down Expand Up @@ -790,7 +793,7 @@ func New(opts *Options) (*Daemon, error) {
httpAddress: opts.HTTPAddress,
}

ovld, err := overlord.New(opts.Dir, d, opts.ServiceOutput)
ovld, err := overlord.New(opts.Dir, d, opts.ServiceOutput, opts.Dry)
if err == errExpectedReboot {
// we proceed without overlord until we reach Stop
// where we will schedule and wait again for a system restart.
Expand Down
5 changes: 5 additions & 0 deletions internals/overlord/cmdstate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func NewManager(runner *state.TaskRunner) *CommandManager {
return manager
}

// DryStart is part of the overlord.StateManager interface.
func (m *CommandManager) DryStart() error {
return nil
}

// Ensure is part of the overlord.StateManager interface.
func (m *CommandManager) Ensure() error {
return nil
Expand Down
5 changes: 5 additions & 0 deletions internals/overlord/logstate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,11 @@ func (m *LogManager) ServiceStarted(serviceName string, buffer *servicelog.RingB
// TODO: implement
}

// DryStart implements overlord.StateManager.
func (m *LogManager) DryStart() error {
return nil
}

// Ensure implements overlord.StateManager.
func (m *LogManager) Ensure() error {
return nil
Expand Down
2 changes: 1 addition & 1 deletion internals/overlord/managers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func (s *mgrsSuite) SetUpTest(c *C) {

s.dir = c.MkDir()

o, err := overlord.New(s.dir, nil, nil)
o, err := overlord.New(s.dir, nil, nil, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the reason why I'm not a fan of boolean args: the call site is not readable unless you know the function prototype by heart.

I'm even less of a fan of providing nil as a valid value to anything, but that's idiomatic Go.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function signature is just asking for a refactor of its arguments to a struct:

Suggested change
o, err := overlord.New(s.dir, nil, nil, false)
o, err := overlord.New(&overlord.Opts{
PebbleDir: s.dir,
RestartHandler: nil,
ServiceOutput: nil,
Dry: false,
})

But that's unrelated to this specific PR.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method has remained quite simple, with those couple of arguments, for several years now. But every few weeks we have a PR that tries to add more parameters to it, and if we had allowed these to go on we'd have a struct with 50 different options by now. Not everything that needs to be passed down a chain of objects needs to be in a constructor. In the case at hand, per the other comment, we should not do DryStart on the Init, but rather inside Start itself and only if it has not yet run externally. On the Daemon, we can call a DryStart on the overlord itself when we're doing a dry run.

c.Assert(err, IsNil)
s.o = o
}
Expand Down
27 changes: 15 additions & 12 deletions internals/overlord/overlord.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ type Overlord struct {

// New creates a new Overlord with all its state managers.
// It can be provided with an optional restart.Handler.
func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writer) (*Overlord, error) {
func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writer, dry bool) (*Overlord, error) {
o := &Overlord{
pebbleDir: pebbleDir,
loopTomb: new(tomb.Tomb),
Expand All @@ -93,6 +93,7 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
path: statePath,
ensureBefore: o.ensureBefore,
}

s, err := loadState(statePath, restartHandler, backend)
if err != nil {
return nil, err
Expand All @@ -108,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()

Expand All @@ -133,11 +134,12 @@ func New(pebbleDir string, restartHandler restart.Handler, serviceOutput io.Writ
// the shared task runner should be added last!
o.stateEng.AddManager(o.runner)

return o, nil
}
// Dry start all managers.
if err := o.stateEng.DryStart(); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this only happen if dry is true?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really; the dry start mechanism, as discussed on KO026, always runs in order to run initialization tasks that do not incur in unwanted side-effects.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, but it's a bit awkward to see this here at the end of Init.

I think this shoud be run when starting, or on a specific DryStart method that only does the dry-starting. This way we also don't need to provide further arguments into the constructor here, and no bools either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue of running this when starting (e.g. at the top of overlord.Loop()) is that Loop() might be called too late and other components (e.g. the daemon) might have already done some state-mutating operations.

The issue of requiring callers to call an overlord.DryStart() method manually is that it's easy to forget, and thus misuse the API. Also, DryStart() is not an operation you do on the overlord, but rather on the each of the managers. Holding the caller responsible for initializing the managers doesn't seem right to me: the overlord should be the one doing this very internal operation on the managers.

I think running this at the bottom of overlord.New() is the sweet spot for these reasons. Also, we don't need any kind of arguments (that argument from previous commits was clearly a bug and a misunderstanding of the problem on my side), because DryStart() is meant to be called always.

return nil, err
}

func (o *Overlord) addManager(mgr StateManager) {
o.stateEng.AddManager(mgr)
return o, nil
}

func loadState(statePath string, restartHandler restart.Handler, backend state.Backend) (*state.State, error) {
Expand Down Expand Up @@ -318,15 +320,15 @@ func (o *Overlord) settle(timeout time.Duration, beforeCleanups func()) error {
if timeout > 0 && time.Since(t0) > timeout {
err := fmt.Errorf("Settle is not converging")
if len(errs) != 0 {
return &ensureError{append(errs, err)}
return &multiError{append(errs, err)}
}
return err
}
next := o.ensureTimerReset()
err := o.stateEng.Ensure()
switch ee := err.(type) {
case nil:
case *ensureError:
case *multiError:
anpep marked this conversation as resolved.
Show resolved Hide resolved
errs = append(errs, ee.errs...)
default:
errs = append(errs, err)
Expand All @@ -353,7 +355,7 @@ func (o *Overlord) settle(timeout time.Duration, beforeCleanups func()) error {
}
}
if len(errs) != 0 {
return &ensureError{errs}
return &multiError{errs}
}
return nil
}
Expand Down Expand Up @@ -432,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
}

Expand All @@ -441,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 {
Expand Down
36 changes: 22 additions & 14 deletions internals/overlord/overlord_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (ovs *overlordSuite) TestNew(c *C) {
restore := patch.Fake(42, 2, nil)
defer restore()

o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)
c.Check(o, NotNil)

Expand All @@ -83,7 +83,7 @@ func (ovs *overlordSuite) TestNewWithGoodState(c *C) {
err := ioutil.WriteFile(ovs.statePath, fakeState, 0600)
c.Assert(err, IsNil)

o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

state := o.State()
Expand Down Expand Up @@ -111,7 +111,7 @@ func (ovs *overlordSuite) TestNewWithInvalidState(c *C) {
err := ioutil.WriteFile(ovs.statePath, fakeState, 0600)
c.Assert(err, IsNil)

_, err = overlord.New(ovs.dir, nil, nil)
_, err = overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, ErrorMatches, "cannot read state: EOF")
}

Expand All @@ -130,7 +130,7 @@ func (ovs *overlordSuite) TestNewWithPatches(c *C) {
err := ioutil.WriteFile(ovs.statePath, fakeState, 0600)
c.Assert(err, IsNil)

o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

state := o.State()
Expand Down Expand Up @@ -163,6 +163,10 @@ type witnessManager struct {
ensureCallback func(s *state.State) error
}

func (wm *witnessManager) DryStart() error {
return nil
}

func (wm *witnessManager) Ensure() error {
if wm.expectedEnsure--; wm.expectedEnsure == 0 {
close(wm.ensureCalled)
Expand All @@ -175,7 +179,7 @@ func (wm *witnessManager) Ensure() error {
}

func (ovs *overlordSuite) TestTrivialRunAndStop(c *C) {
o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

o.Loop()
Expand All @@ -185,7 +189,7 @@ func (ovs *overlordSuite) TestTrivialRunAndStop(c *C) {
}

func (ovs *overlordSuite) TestUnknownTasks(c *C) {
o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

// unknown tasks are ignored and succeed
Expand Down Expand Up @@ -486,7 +490,7 @@ func (ovs *overlordSuite) TestCheckpoint(c *C) {
oldUmask := syscall.Umask(0)
defer syscall.Umask(oldUmask)

o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

s := o.State()
Expand Down Expand Up @@ -554,6 +558,10 @@ func newSampleManager(s *state.State, runner *state.TaskRunner) *sampleManager {
return sm
}

func (sm *sampleManager) DryStart() error {
return nil
}

func (sm *sampleManager) Ensure() error {
if sm.ensureCallback != nil {
sm.ensureCallback()
Expand Down Expand Up @@ -727,7 +735,7 @@ func (ovs *overlordSuite) TestSettleExplicitEnsureBefore(c *C) {
}

func (ovs *overlordSuite) TestRequestRestartNoHandler(c *C) {
o, err := overlord.New(ovs.dir, nil, nil)
o, err := overlord.New(ovs.dir, nil, nil, false)
c.Assert(err, IsNil)

st := o.State()
Expand Down Expand Up @@ -760,7 +768,7 @@ func (rb *testRestartHandler) RebootIsMissing(_ *state.State) error {
func (ovs *overlordSuite) TestRequestRestartHandler(c *C) {
rb := &testRestartHandler{}

o, err := overlord.New(ovs.dir, rb, nil)
o, err := overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, IsNil)

st := o.State()
Expand All @@ -779,7 +787,7 @@ func (ovs *overlordSuite) TestVerifyRebootNoPendingReboot(c *C) {

rb := &testRestartHandler{}

_, err = overlord.New(ovs.dir, rb, nil)
_, err = overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, IsNil)

c.Check(rb.rebootState, Equals, "as-expected")
Expand All @@ -792,7 +800,7 @@ func (ovs *overlordSuite) TestVerifyRebootOK(c *C) {

rb := &testRestartHandler{}

_, err = overlord.New(ovs.dir, rb, nil)
_, err = overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, IsNil)

c.Check(rb.rebootState, Equals, "as-expected")
Expand All @@ -806,7 +814,7 @@ func (ovs *overlordSuite) TestVerifyRebootOKButError(c *C) {
e := errors.New("boom")
rb := &testRestartHandler{rebootVerifiedErr: e}

_, err = overlord.New(ovs.dir, rb, nil)
_, err = overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, Equals, e)

c.Check(rb.rebootState, Equals, "as-expected")
Expand All @@ -822,7 +830,7 @@ func (ovs *overlordSuite) TestVerifyRebootIsMissing(c *C) {

rb := &testRestartHandler{}

_, err = overlord.New(ovs.dir, rb, nil)
_, err = overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, IsNil)

c.Check(rb.rebootState, Equals, "did-not-happen")
Expand All @@ -839,7 +847,7 @@ func (ovs *overlordSuite) TestVerifyRebootIsMissingError(c *C) {
e := errors.New("boom")
rb := &testRestartHandler{rebootVerifiedErr: e}

_, err = overlord.New(ovs.dir, rb, nil)
_, err = overlord.New(ovs.dir, rb, nil, false)
c.Assert(err, Equals, e)

c.Check(rb.rebootState, Equals, "did-not-happen")
Expand Down
6 changes: 6 additions & 0 deletions internals/overlord/servstate/manager.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,12 @@ func (m *ServiceManager) acquirePlan() (release func(), err error) {
return release, nil
}

// DryStart implements StateManager.DryStart.
func (m *ServiceManager) DryStart() error {
_, err := m.Plan()
return err
}

// Ensure implements StateManager.Ensure.
func (m *ServiceManager) Ensure() error {
return nil
Expand Down
4 changes: 4 additions & 0 deletions internals/overlord/state/taskrunner.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,6 +331,10 @@ func (r *TaskRunner) tryUndo(t *Task) {
}
}

func (r *TaskRunner) DryStart() error {
return nil
}

// Ensure starts new goroutines for all known tasks with no pending
// dependencies.
// Note that Ensure will lock the state.
Expand Down
Loading