feat(plan): add plan section support

[flotter]

The plan library was originally written as a configuration schema for the services manager (servstate). Over time the need arose to support configurations for other managers such as checks (checkstate) and log-targets (logstate). The configuration schema for these managers, closely related to the services manager, has since also been built in to the plan library.

The services manager and its related checks and log-targets functionality will always be part of the Pebble core. However, as Pebble is getting more functionality (additional managers) and also used as a core in derivative projects, a more modular and dynamic approach to extending the schema is needed.

Add an the SectionExtension interface for use by managers who wishes to register a schema extension during Pebble startup.

Inside each layer, top level entries are now referred to as sections (built-in sections includes summary, description, services, log-targets and checks).

Each section has an associated field that is the top level key, and if supplied by an extension, an opaque backing type Section.

SectionExtension interface:

// SectionExtension allows the plan layer schema to be extended without
// adding centralised schema knowledge to the plan library.
type SectionExtension interface {
	// ParseSection returns a newly allocated concrete type containing the
	// unmarshalled section content.
	ParseSection(data yaml.Node) (LayerSection, error)

	// CombineSections returns a newly allocated concrete type containing the
	// result of combining the supplied sections in order.
	CombineSections(sections ...LayerSection) (LayerSection, error)

	// ValidatePlan takes the complete plan as input, and allows the
	// extension to validate the plan. This can be used for cross section
	// dependency validation.
	ValidatePlan(plan *Plan) error
}

type Section interface {
	// Validate checks whether the section is valid, returning an error if not.
	Validate() error
	
        // IsZero reports whether the section is empty.
	IsZero() bool
}

Example usage:

// New SectionExtension type
type fooExtension struct{}
func (f *fooExtension) ParseSection(data yaml.Node) (LayerSection, error) {...}
func (f *fooExtension) CombineSections(sections ...LayerSection) (LayerSection, error) {...}
func (f *fooExtension) ValidatePlan(plan *Plan) error {...}

// New Section type
type FooSection struct {
    Entries map[string]Bar `yaml:",inline,omitempty"`
}
type Bar struct {
    Name string `yaml:"name,omitempty"`
}
func (s *FooSection) Validate() error {...}
func (s *FooSection) IsZero() bool {...}

// Early startup
plan.RegisterExtension("foo", &fooExtension{})
:
// Load layers containing new section
newPlan := plan.ReadDir(layersDir)
:
// Show plan
yaml.Marshal(newPlan)

Example YAML output:

foo:
    bar1:
           name: test1
    bar2:
           name: test2          

[benhoyt]

I really like this direction -- thanks. Left a few comments.

[benhoyt]

One other thing: it'd be quite interesting to me if instead of having builtin sections we can just implement "services" etc using RegisterExtension (and register those sections in init() by default). That would really prove the concept and avoid the special-casing. Would probably be best in a follow-up PR though to avoid too much churn here.

[flotter]

One other thing: it'd be quite interesting to me if instead of having builtin sections we can just implement "services" etc using RegisterExtension (and register those sections in init() by default). That would really prove the concept and avoid the special-casing. Would probably be best in a follow-up PR though to avoid too much churn here.

This is definitely the next step, and should not be difficult to do. I would be super happy to make this happen in followup PRs. I do feel confident (after implementing a quite complex extension), that the existing structures will be trivial to map to extensions.

[hpidcock]

Thanks @flotter for putting this together. I'm not totally a fan of this method of extending pebble. I understand that this is probably the cleanest way to achieve this currently. My fear is that we need to have additional extensions in other parts of pebble for different super projects and their needs. We can probably cross that bridge when we get to it, hence why I am approving this now.

My initial thoughts on an alternative path to achieve the same thing would be through the use of a stub extensions module. Pebble would import "github.com/canonical/pebble/extensions", this would be a separate go module.

Then through composition, we can achieve something similar.

type Layer struct {
	extensions.Layer
	Order       int                   `yaml:"-"`
	Label       string                `yaml:"-"`
	Summary     string                `yaml:"summary,omitempty"`
	Description string                `yaml:"description,omitempty"`
	Services    map[string]*Service   `yaml:"services,omitempty"`
	Checks      map[string]*Check     `yaml:"checks,omitempty"`
	LogTargets  map[string]*LogTarget `yaml:"log-targets,omitempty"`
}

The "github.com/canonical/pebble/extensions" module on the pebble project would be a series of empty structs, interfaces and noop functions. Pebble would embed these structs and interfaces as well as call the stub functions in key places.

Then in the super project, you can use go work/go mod replace directives to achieve redirecting "github.com/canonical/pebble/extensions" to the super project's implementation.

There are obviously a host of issues with this approach (mostly around testing), but that's the best I could come up with to challenge what this PR is proposing.

[niemeyer]

Thanks for this, Fred. Here is a final pass from my side with details only. The overall aspect looks good, and I'm happy once you and the team are happy.

[niemeyer]

This seems a bit awkward to read. I realize it's mimicking what was there before, with Services, Checks, etc, but now it seems even more weird as we have Layers, Sections, and individual Sections in the same level. This is okay if it's temporary, but is it? And if so, we need a comment in here properly spelling out the plan so the next person can make sense of the current state. Can we do better so it's more sensible meanwhile?

[flotter]

I previously played with having:

type Plan struct {
	Layers     []*Layer              `yaml:"-"`
	combined *Layer               `yaml:",inline"
}

The main issue I observed was that you lose the ability to simply access content from the plan directly, unless you add helper methods, and this propagated far into the codebase. Also, we currently do not marshal summary and description so you would require a custom MarshalYAML (which we need in any case):

func (p *Plan) Services() map[string]*Service {
    return p.combined.Services
}

If we do that, then perhaps waiting until we migrated the 3 built-in sections out, so that it looks like this:

type Plan struct {
	Layers     []*Layer              `yaml:"-"`
	Sections map[string]LayerSection `yaml:",inline"`
}

@benhoyt Perhaps we can brainstorm what could be nice here?

[benhoyt]

Thanks Harry and Gustavo for your reviews. @flotter I spoke a bit further with Harry about his concerns, as I'd misunderstood. He's basically concerned that we'll start to accumulate more and more dynamic hooks and ways to extend Pebble, which don't actually help Pebble itself, and will make the codebase less simple/static and hence harder to understood. There are other ways to skin the cat (that still require complexity, but they might push it from dynamic/runtime complexity to version control and workspace complexity) that we may want to think about in future.

I do share this concern, but we both agreed that this is reasonable for now, and that we'll keep an eye on it over time.

[benhoyt]

This looks good, thanks. A few minor comments, but I'm happy with the approach/naming/implementation overall.

[benhoyt]

Thanks! Let's go ahead and merge this -- we can have further discussion about planstate in #489.

[benhoyt]

Actually one more thing from Gustavo above. In the renaming thread, he said:

The function name should be renamed too, as RegisterSectionExtension, so we have the same terminology throughout the surface.

I think that's reasonable to keep things consistent -- so please make that tweak and then merge this.