Skip to content

Commit

Permalink
fix(cmdstate): don't store execSetup and environment in state
Browse files Browse the repository at this point in the history
Per #411, the environment (and other task data) can get quite large for
exec objects. Given that exec commands are one-shot and only relevant
for the current run of Pebble, there's no need to persist them.

Note that none of this data is in "api-data" so it's not accessible via
the API at all right now, and so this is a non-breaking change.

Later if we want to make exec tasks a bit more introspectable we can
add the command line or other fields to "api-data" (but we shouldn't
add the entire environment).

Fixes #411.
  • Loading branch information
benhoyt committed Aug 9, 2024
1 parent 81f57da commit fc6ef32
Show file tree
Hide file tree
Showing 3 changed files with 21 additions and 5 deletions.
8 changes: 4 additions & 4 deletions internals/overlord/cmdstate/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,13 +70,13 @@ type execution struct {
}

func (m *CommandManager) doExec(task *state.Task, tomb *tomb.Tomb) error {
var setup execSetup
st := task.State()
st.Lock()
err := task.Get("exec-setup", &setup)
setupObj := st.Cached(execSetupKey{task.ID()})
st.Unlock()
if err != nil {
return fmt.Errorf("cannot get exec setup object for task %q: %v", task.ID(), err)
setup, ok := setupObj.(*execSetup)
if !ok || setup == nil {
return fmt.Errorf("internal error: cannot get exec setup object for task %q", task.ID())
}

// Set up the object that will track the execution.
Expand Down
16 changes: 16 additions & 0 deletions internals/overlord/cmdstate/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import (
"net/http"
"sync"

"gopkg.in/tomb.v2"

"github.com/canonical/pebble/internals/overlord/state"
)

Expand All @@ -34,9 +36,23 @@ func NewManager(runner *state.TaskRunner) *CommandManager {
executionsCond: sync.NewCond(&sync.Mutex{}),
}
runner.AddHandler("exec", manager.doExec, nil)

// Delete the in-memory execSetup object when the exec is done.
runner.AddCleanup("exec", func(task *state.Task, tomb *tomb.Tomb) error {
st := task.State()
st.Lock()
defer st.Unlock()
st.Cache(execSetupKey{task.ID()}, nil)
return nil
})

return manager
}

type execSetupKey struct {
taskID string
}

// Ensure is part of the overlord.StateManager interface.
func (m *CommandManager) Ensure() error {
return nil
Expand Down
2 changes: 1 addition & 1 deletion internals/overlord/cmdstate/request.go
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ func Exec(st *state.State, args *ExecArgs) (*state.Task, ExecMetadata, error) {
GroupID: args.GroupID,
WorkingDir: workingDir,
}
task.Set("exec-setup", &setup)
st.Cache(execSetupKey{task.ID()}, &setup)

metadata := ExecMetadata{
TaskID: task.ID(),
Expand Down

0 comments on commit fc6ef32

Please sign in to comment.