feat(overlord): allow adding external managers

[paul-rodriguez]

This change enables an external application importing Pebble to extend the overlord functionality by adding additional managers.

Example overlord extension:

package ovldext

type OverlordExtension struct {
	fooManager *foostate.Manager
	barManager *barstate.Manager
}

func New() *OverlordExtension {
    return &OverlordExtension{}
}

func (oe *OverlordExtension) ExtraManagers(o *Overlord) ([]overlord.StateManager, error) {
    oe.fooManager, err := foostate.NewManager(o.State(), o.TaskRunner())
    if err != nil {
        return nil, err
    }
    oe.barManager, err = barstate.NewManager(o.State(), o.TaskRunner())
    if err != nil {
        return nil, err
    }
    return []overlord.StateManager{oe.fooManager, oe.barManager}, nil
}

func (oe *OverlordExtension) FooManager() *foostate.Manager { return oe.fooManager }

func (oe *OverlordExtension) BarManager() *barstate.Manager { return oe.barManager }

func Extension(o *overlord.Overlord) *OverlordExtension {
	return o.Extension().(*OverlordExtension)
}

Example startup code:

    dopts := daemon.Options{
        Dir:        pebbleDir,
        SocketPath: socketPath,

        // Supply overlord extension
        OverlordExtension: ovldext.New()            
    }
    
    d, err := daemon.New(&dopts)
    if err != nil {
        return err
    }

External HTTP request handler example:

    :  
    // We can now get access to the overlord
    o := d.Overlord()
    
    // Get access to the custom state manager to perform the HTTP request
    fooManager := ovldext.Extension(o).FooManager()
    :

[flotter]

Some brainstorming ideas for discussion.

[anpep]

I don't think the approach taken here is suitable for a public-facing API.

A simpler approach I suggest: having separate func New(...) (*Overlord, error) and func (*Overlord) Init() error functions, then have a func (*Overlord) AddManager(StateManager) error that we can simply call before o.Init().

What do you think?

[paul-rodriguez]

I don't think the approach taken here is suitable for a public-facing API.

A simpler approach I suggest: having separate func New(...) (*Overlord, error) and func (*Overlord) Init() error functions, then have a func (*Overlord) AddManager(StateManager) error that we can simply call before o.Init().

What do you think?

I don't like letting the user of that code be bitten by forgetting Init or calling AddManager after Init. Right now Overlord is more or less RAII, let's keep it that way if possible.

Here the only purpose of Init would be to ensure the TaskRunner is the last manager. With the changes I made to the StateEngine, Init is not necessary. You can AddManager in cmd_run as Fred suggested.

[flotter]

There appears to be a linter failure in one of the tests.

[anpep]

Excellent work on this @paul-rodriguez!

[benhoyt]

It's a bit awkward, but I understand why the task runner is special, and this seems reasonable to me -- it's definitely nice and simple!

[niemeyer]

Just for my own tracking, we've discussed this and the approach here is not going in a good direction as it's misusing the abstractions we have. Let's please get together and figure a better way to do this.

[flotter]

Looks great Paul. I think this is perfect for a first round of reviews, including a proposal discussion with Gustavo. I've taken the liberty to update the example in the PR description for a quick highlight of this scheme for understanding the proposal.

[flotter]

Current status: Further discussions required. Current design unlikely to progress in its current form.

[flotter]

Great! Added a few nitty and minor comments, as I think we may have time to do a detailed code review during our next meeting.

[flotter]

Great work Paul. Looks super nice.

Regarding the PR description example, feel free to remote it or change it if you want. I find it essential assisting reviewers get the complete picture for things talking about some hypothetical external use-case (especially when discussing in person), but do not feel like we have to keep it there, or that version of it.

[benhoyt]

This seems reasonable to me. Couple of comments about comments.

[anpep]

Stellar work on this Paul, thank you!

[niemeyer]

Thank you, that turned out well. Only documentation tuning suggested below.

[flotter]

Last two corrections added. Approved by Gustavo. Merging.