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.

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

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

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions internals/cli/cmd_run.go
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 sharedRunEnterArgsHelp = 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 @@ -162,6 +164,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
11 changes: 8 additions & 3 deletions internals/daemon/daemon_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,13 @@ func (h *fakeHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
}

type fakeManager struct {
id string
ensureCalls int
id string
ensureCalls, dryStartCalls int
}

func (m *fakeManager) DryStart() error {
m.dryStartCalls++
return nil
}

func (m *fakeManager) Ensure() error {
Expand All @@ -111,7 +116,7 @@ type fakeExtension struct {
}

func (f *fakeExtension) ExtraManagers(o *overlord.Overlord) ([]overlord.StateManager, error) {
f.mgr = fakeManager{id: "expected", ensureCalls: 0}
f.mgr = fakeManager{id: "expected", ensureCalls: 0, dryStartCalls: 0}
result := []overlord.StateManager{&f.mgr}
return result, nil
}
Expand Down
5 changes: 5 additions & 0 deletions internals/overlord/cmdstate/manager.go
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
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ func (m *LogManager) ServiceStarted(service *plan.Service, buffer *servicelog.Ri
}
}

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

// Ensure implements overlord.StateManager.
func (m *LogManager) Ensure() error {
return nil
Expand Down
16 changes: 10 additions & 6 deletions internals/overlord/overlord.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
package overlord

import (
"errors"
"fmt"
"io"
"os"
Expand Down Expand Up @@ -94,7 +95,6 @@ type Overlord struct {

// New creates an Overlord with all its state managers.
func New(opts *Options) (*Overlord, error) {

o := &Overlord{
pebbleDir: opts.PebbleDir,
loopTomb: new(tomb.Tomb),
Expand Down Expand Up @@ -175,6 +175,10 @@ func New(opts *Options) (*Overlord, error) {
// before it.
o.stateEng.AddManager(o.runner)

// 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
}
return o, nil
}

Expand Down Expand Up @@ -358,17 +362,16 @@ func (o *Overlord) settle(timeout time.Duration, beforeCleanups func()) error {
var errs []error
for !done {
if timeout > 0 && time.Since(t0) > timeout {
err := fmt.Errorf("Settle is not converging")
if len(errs) != 0 {
return &ensureError{append(errs, err)}
return newMultiError("settle is not converging", errs)
}
return err
return errors.New("settle is not converging")
}
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 @@ -395,7 +398,7 @@ func (o *Overlord) settle(timeout time.Duration, beforeCleanups func()) error {
}
}
if len(errs) != 0 {
return &ensureError{errs}
return newMultiError("state ensure errors", errs)
}
return nil
}
Expand Down Expand Up @@ -474,6 +477,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 Down
10 changes: 9 additions & 1 deletion internals/overlord/overlord_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,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 Down Expand Up @@ -562,6 +566,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 @@ -621,7 +629,7 @@ func (ovs *overlordSuite) TestSettleNotConverging(c *C) {
err := o.Settle(250 * time.Millisecond)
s.Lock()

c.Check(err, ErrorMatches, `Settle is not converging`)
c.Check(err, ErrorMatches, `settle is not converging`)

}

Expand Down
6 changes: 6 additions & 0 deletions internals/overlord/servstate/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,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
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
83 changes: 75 additions & 8 deletions internals/overlord/stateengine.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
package overlord

import (
"bytes"
"errors"
"fmt"
"strings"
"sync"

"github.com/canonical/pebble/internals/logger"
Expand All @@ -25,6 +28,10 @@ import (
// StateManager is implemented by types responsible for observing
// the system and manipulating it to reflect the desired state.
type StateManager interface {
// DryStart prepares the manager to run its activities without
// incurring unwanted side effects.
DryStart() error

// Ensure forces a complete evaluation of the current state.
// See StateEngine.Ensure for more details.
Ensure() error
Expand Down Expand Up @@ -53,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
Expand All @@ -72,12 +80,68 @@ func (se *StateEngine) State() *state.State {
return se.state
}

type ensureError struct {
errs []error
// multiError collects multiple errors that affected an operation.
type multiError struct {
anpep marked this conversation as resolved.
Show resolved Hide resolved
header string
errs []error
}

// newMultiError returns a new multiError struct initialized with
// the given format string that explains what operation potentially
// went wrong. multiError can be nested and will render correctly
// in these cases.
func newMultiError(header string, errs []error) error {
return &multiError{header: header, errs: errs}
}

func (e *ensureError) Error() string {
return fmt.Sprintf("state ensure errors: %v", e.errs)
// Error formats the error string.
func (me *multiError) Error() string {
return me.nestedError(0)
}

// helper to ensure formating of nested multiErrors works.
func (me *multiError) nestedError(level int) string {
indent := strings.Repeat(" ", level)
buf := bytes.NewBufferString(fmt.Sprintf("%s:\n", me.header))
if level > 8 {
return "circular or too deep error nesting (max 8)?!"
}
for i, err := range me.errs {
switch v := err.(type) {
case *multiError:
fmt.Fprintf(buf, "%s- %v", indent, v.nestedError(level+1))
default:
fmt.Fprintf(buf, "%s- %v", indent, err)
}
if i < len(me.errs)-1 {
fmt.Fprintf(buf, "\n")
}
}
return buf.String()
}

var errStateEngineStopped = errors.New("state engine already stopped")

// DryStart ensures all managers are ready to run their activities
// before the first call to Ensure() is made.
func (se *StateEngine) DryStart() error {
se.mgrLock.Lock()
defer se.mgrLock.Unlock()
if se.stopped {
return errStateEngineStopped
}
var errs []error
for _, m := range se.managers {
err := m.DryStart()
if err != nil {
errs = append(errs, err)
}
}
if len(errs) != 0 {
return newMultiError("dry-start failed", errs)
}
se.dryStarted = true
return nil
}

// Ensure asks every manager to ensure that they are doing the necessary
Expand All @@ -91,8 +155,11 @@ func (e *ensureError) Error() string {
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 {
anpep marked this conversation as resolved.
Show resolved Hide resolved
return fmt.Errorf("state engine already stopped")
return errStateEngineStopped
}
var errs []error
for _, m := range se.managers {
Expand All @@ -103,7 +170,7 @@ func (se *StateEngine) Ensure() error {
}
}
if len(errs) != 0 {
return &ensureError{errs}
return newMultiError("state ensure errors", errs)
}
return nil
}
Expand Down
Loading
Loading