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(workloads): add plumbing to support workloads #558

Closed
wants to merge 3 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/daemon/daemon.go
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ import (
"github.com/canonical/pebble/internals/overlord/state"
"github.com/canonical/pebble/internals/reaper"
"github.com/canonical/pebble/internals/systemd"
"github.com/canonical/pebble/internals/workload"
)

var (
Expand Down Expand Up @@ -81,6 +82,9 @@ type Options struct {
// log output will be written to the writer.
ServiceOutput io.Writer

// Workloads contains information about the workloads present in the system.
Workloads map[string]workload.Workload

// OverlordExtension is an optional interface used to extend the capabilities
// of the Overlord.
OverlordExtension overlord.Extension
Expand Down Expand Up @@ -851,6 +855,7 @@ func New(opts *Options) (*Daemon, error) {
LayersDir: opts.LayersDir,
RestartHandler: d,
ServiceOutput: opts.ServiceOutput,
Workloads: opts.Workloads,
Extension: opts.OverlordExtension,
}

Expand Down
8 changes: 6 additions & 2 deletions internals/overlord/overlord.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import (
"github.com/canonical/pebble/internals/overlord/servstate"
"github.com/canonical/pebble/internals/overlord/state"
"github.com/canonical/pebble/internals/timing"
"github.com/canonical/pebble/internals/workload"
)

var (
Expand Down Expand Up @@ -74,6 +75,8 @@ type Options struct {
RestartHandler restart.Handler
// ServiceOutput is an optional output for the logging manager.
ServiceOutput io.Writer
// Workloads contain information about the workloads present in the system.
Workloads map[string]workload.Workload
// Extension allows extending the overlord with externally defined features.
Extension Extension
}
Expand Down Expand Up @@ -155,7 +158,7 @@ func New(opts *Options) (*Overlord, error) {
if layersDir == "" {
layersDir = filepath.Join(opts.PebbleDir, "layers")
}
o.planMgr, err = planstate.NewManager(layersDir)
o.planMgr, err = planstate.NewManager(opts.Workloads, layersDir)
if err != nil {
return nil, fmt.Errorf("cannot create plan manager: %w", err)
}
Expand All @@ -168,7 +171,8 @@ func New(opts *Options) (*Overlord, error) {
o.runner,
opts.ServiceOutput,
opts.RestartHandler,
o.logMgr)
o.logMgr,
opts.Workloads)
if err != nil {
return nil, fmt.Errorf("cannot create service manager: %w", err)
}
Expand Down
10 changes: 8 additions & 2 deletions internals/overlord/planstate/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"sync"

"github.com/canonical/pebble/internals/plan"
"github.com/canonical/pebble/internals/workload"
)

// LabelExists is the error returned by AppendLayer when a layer with that
Expand All @@ -40,12 +41,14 @@ type PlanManager struct {
plan *plan.Plan

changeListeners []PlanChangedFunc
workloads map[string]workload.Workload
}

func NewManager(layersDir string) (*PlanManager, error) {
func NewManager(workloads map[string]workload.Workload, layersDir string) (*PlanManager, error) {
manager := &PlanManager{
layersDir: layersDir,
plan: &plan.Plan{},
workloads: workloads,
}
return manager, nil
}
Expand All @@ -59,6 +62,9 @@ func (m *PlanManager) Load() error {
if err != nil {
return err
}
if err := plan.Validate(m.workloads); err != nil {
return err
}

m.planLock.Lock()
m.plan = plan
Expand Down Expand Up @@ -249,7 +255,7 @@ func (m *PlanManager) updatePlanLayers(layers []*plan.Layer) (*plan.Plan, error)
LogTargets: combined.LogTargets,
Sections: combined.Sections,
}
err = p.Validate()
err = p.Validate(m.workloads)
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a note for reviewers: Once services and checks are moved over to use the plan extension system, the workload definition can be supplied by the extension itself, and this signature change can be reverted so that the plan library is unaware of workloads.

if err != nil {
return nil, err
}
Expand Down
16 changes: 8 additions & 8 deletions internals/overlord/planstate/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import (

func (ps *planSuite) TestLoadInvalidPebbleDir(c *C) {
var err error
ps.planMgr, err = planstate.NewManager("/invalid/path")
ps.planMgr, err = planstate.NewManager(nil, "/invalid/path")
c.Assert(err, IsNil)
// Load the plan from the <pebble-dir>/layers directory
err = ps.planMgr.Load()
Expand Down Expand Up @@ -67,7 +67,7 @@ func (ps *planSuite) TestLoadLayers(c *C) {
plan.RegisterSectionExtension(testField, testExtension{})
defer plan.UnregisterSectionExtension(testField)
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)
// Write layers
for _, l := range loadLayers {
Expand Down Expand Up @@ -102,7 +102,7 @@ func (ps *planSuite) TestAppendLayers(c *C) {
plan.RegisterSectionExtension(testField, testExtension{})
defer plan.UnregisterSectionExtension(testField)
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

// Append a layer when there are no layers.
Expand Down Expand Up @@ -219,7 +219,7 @@ func (ps *planSuite) TestCombineLayers(c *C) {
plan.RegisterSectionExtension(testField, testExtension{})
defer plan.UnregisterSectionExtension(testField)
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

// "Combine" layer with no layers should just append.
Expand Down Expand Up @@ -405,7 +405,7 @@ test-field:

func (ps *planSuite) TestSetServiceArgs(c *C) {
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

// This is the original plan
Expand Down Expand Up @@ -447,7 +447,7 @@ services:
}

func (ps *planSuite) TestChangeListenerAndLocking(c *C) {
manager, err := planstate.NewManager(ps.layersDir)
manager, err := planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

calls := 0
Expand Down Expand Up @@ -514,7 +514,7 @@ func (ps *planSuite) TestAppendLayersWithoutInner(c *C) {
plan.RegisterSectionExtension(testField, testExtension{})
defer plan.UnregisterSectionExtension(testField)
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

layer := ps.parseLayer(c, 0, "foo/bar", "")
Expand All @@ -532,7 +532,7 @@ func (ps *planSuite) TestAppendLayersWithInner(c *C) {
plan.RegisterSectionExtension(testField, testExtension{})
defer plan.UnregisterSectionExtension(testField)
var err error
ps.planMgr, err = planstate.NewManager(ps.layersDir)
ps.planMgr, err = planstate.NewManager(nil, ps.layersDir)
c.Assert(err, IsNil)

appendLabels := []string{
Expand Down
23 changes: 19 additions & 4 deletions internals/overlord/servstate/handlers.go
Original file line number Diff line number Diff line change
Expand Up @@ -346,9 +346,16 @@ func (s *serviceData) startInternal() error {
s.cmd.Dir = s.config.WorkingDir

// Start as another user if specified in plan.
uid, gid, err := osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group)
if err != nil {
return err
w := s.manager.workloads[s.config.Workload]
Copy link
Contributor

@flotter flotter Jan 15, 2025

Choose a reason for hiding this comment

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

Should we deal the a failure lookup here, even though in theory validation already happened ? If no workloads are supplied, the map lookup will return nothing and w will be nil? Also, what happens if the service did not define workload (empty string)?

uid, gid := w.UID, w.GID
if (uid == nil) != (gid == nil) {
panic("both uid and gid must be provided by workload")
Copy link
Contributor

Choose a reason for hiding this comment

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

This panic will be triggered if either uid or gid is nil but not both. The panic text doesn't match.

}
if uid == nil {
uid, gid, err = osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group)
Copy link
Contributor

@paul-rodriguez paul-rodriguez Jan 15, 2025

Choose a reason for hiding this comment

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

Why reassign gid here if we know we have an explicitly set value already?

Edit: nvm, in this branch we know that uid == nil && gid == nil. The code feels a bit obfuscated.

if err != nil {
return err
}
}
if uid != nil && gid != nil {
isCurrent, err := osutil.IsCurrent(*uid, *gid)
Expand Down Expand Up @@ -378,12 +385,20 @@ func (s *serviceData) startInternal() error {
}
}

// Apply workload environment overrides.
if s.config.Workload != "" {
if w.Environment != nil {
for k, v := range w.Environment {
environment[k] = v
}
}
}

// Pass service description's environment variables to child process.
s.cmd.Env = os.Environ()
for k, v := range environment {
s.cmd.Env = append(s.cmd.Env, k+"="+v)
}

// Set up stdout and stderr to write to log ring buffer.
var outputIterator servicelog.Iterator
if s.manager.serviceOutput != nil {
Expand Down
10 changes: 8 additions & 2 deletions internals/overlord/servstate/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ import (
"github.com/canonical/pebble/internals/overlord/state"
"github.com/canonical/pebble/internals/plan"
"github.com/canonical/pebble/internals/servicelog"
"github.com/canonical/pebble/internals/workload"
)

// ServicesField is the top-level string key used in the Pebble plan.
const ServicesField = "services"

Comment on lines +19 to +21
Copy link
Contributor

Choose a reason for hiding this comment

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

Used any more ?

type ServiceManager struct {
state *state.State

Expand All @@ -30,7 +34,8 @@ type ServiceManager struct {
randLock sync.Mutex
rand *rand.Rand

logMgr LogManager
workloads map[string]workload.Workload
Copy link
Contributor

Choose a reason for hiding this comment

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

Since workloads are the mechanism which will evolve over time to provide service level access permissions and confinement features, for me it makes sense that the Service Manager owns the configuration.

logMgr LogManager
}

type LogManager interface {
Expand All @@ -41,14 +46,15 @@ type Restarter interface {
HandleRestart(t restart.RestartType)
}

func NewManager(s *state.State, runner *state.TaskRunner, serviceOutput io.Writer, restarter Restarter, logMgr LogManager) (*ServiceManager, error) {
func NewManager(s *state.State, runner *state.TaskRunner, serviceOutput io.Writer, restarter Restarter, logMgr LogManager, workloads map[string]workload.Workload) (*ServiceManager, error) {
manager := &ServiceManager{
state: s,
services: make(map[string]*serviceData),
serviceOutput: serviceOutput,
restarter: restarter,
rand: rand.New(rand.NewSource(time.Now().UnixNano())),
logMgr: logMgr,
workloads: workloads,
}

runner.AddHandler("start", manager.doStart, nil)
Expand Down
2 changes: 1 addition & 1 deletion internals/overlord/servstate/manager_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1778,7 +1778,7 @@ services:

func (s *S) newServiceManager(c *C) {
var err error
s.manager, err = servstate.NewManager(s.st, s.runner, s.logOutput, testRestarter{s.stopDaemon}, fakeLogManager{})
s.manager, err = servstate.NewManager(s.st, s.runner, s.logOutput, testRestarter{s.stopDaemon}, fakeLogManager{}, nil)
c.Assert(err, IsNil)
}

Expand Down
3 changes: 3 additions & 0 deletions internals/plan/extensions_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,9 @@ nexttest:

// Load the plan layer from disk (parse, combine and validate).
p, err := plan.ReadDir(layersDir)
if err == nil {
err = p.Validate(nil)
}
if testData.error != "" || err != nil {
// Expected error.
c.Assert(err, ErrorMatches, testData.error)
Expand Down
23 changes: 17 additions & 6 deletions internals/plan/plan.go
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,10 @@ import (
"github.com/canonical/x-go/strutil/shlex"
"gopkg.in/yaml.v3"

"github.com/canonical/pebble/cmd"
"github.com/canonical/pebble/internals/logger"
"github.com/canonical/pebble/internals/osutil"
"github.com/canonical/pebble/internals/workload"
)

// SectionExtension allows the plan layer schema to be extended without
Expand Down Expand Up @@ -191,6 +193,7 @@ type Service struct {
Requires []string `yaml:"requires,omitempty"`

// Options for command execution
Workload string `yaml:"workload,omitempty"`
Environment map[string]string `yaml:"environment,omitempty"`
UserID *int `yaml:"user-id,omitempty"`
User string `yaml:"user,omitempty"`
Expand Down Expand Up @@ -974,13 +977,25 @@ func (layer *Layer) Validate() error {

// Validate checks that the combined layers form a valid plan. See also
// Layer.Validate, which checks that the individual layers are valid.
func (p *Plan) Validate() error {
func (p *Plan) Validate(workloads map[string]workload.Workload) error {
for name, service := range p.Services {
if service.Command == "" {
return &FormatError{
Message: fmt.Sprintf(`plan must define "command" for service %q`, name),
}
}
if service.Workload != "" {
if workloads == nil {
return &FormatError{
Message: fmt.Sprintf(`service %q cannot run in workload %q because workloads are not supported in %v`, name, service.Workload, cmd.DisplayName),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this always an accurate message in this case? What if we just didn't specify any workload but are using a program that supports them?

}
}
if _, ok := workloads[service.Workload]; !ok {
return &FormatError{
Message: fmt.Sprintf(`service %q cannot run in non-existing workload %q`, name, service.Workload),
}
}
}
Comment on lines +993 to +998
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we enforce the rule here that if workloads are defined, uid, user, gid and group properties of the service must be unset ?

}

for name, check := range p.Checks {
Expand Down Expand Up @@ -1563,11 +1578,7 @@ func ReadDir(layersDir string) (*Plan, error) {
LogTargets: combined.LogTargets,
Sections: combined.Sections,
}
err = plan.Validate()
if err != nil {
return nil, err
}
return plan, err
return plan, nil
}

// MergeServiceContext merges the overrides on top of the service context
Expand Down
7 changes: 4 additions & 3 deletions internals/plan/plan_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1476,7 +1476,7 @@ func (s *S) TestParseLayer(c *C) {
LogTargets: result.LogTargets,
Sections: result.Sections,
}
err = p.Validate()
err = p.Validate(nil)
}
}
if err != nil || test.error != "" {
Expand Down Expand Up @@ -1519,7 +1519,7 @@ services:
LogTargets: combined.LogTargets,
Sections: combined.Sections,
}
err = p.Validate()
err = p.Validate(nil)
c.Assert(err, ErrorMatches, `services in before/after loop: .*`)
_, ok := err.(*plan.FormatError)
c.Assert(ok, Equals, true, Commentf("error must be *plan.FormatError, not %T", err))
Expand Down Expand Up @@ -1560,7 +1560,7 @@ services:
LogTargets: combined.LogTargets,
Sections: combined.Sections,
}
err = p.Validate()
err = p.Validate(nil)
c.Check(err, ErrorMatches, `plan must define "command" for service "srv1"`)
_, ok := err.(*plan.FormatError)
c.Check(ok, Equals, true, Commentf("error must be *plan.FormatError, not %T", err))
Expand Down Expand Up @@ -1627,6 +1627,7 @@ func (s *S) TestReadDir(c *C) {
}
}
}
err = sup.Validate(nil)
}
if err != nil || test.error != "" {
if test.error != "" {
Expand Down
20 changes: 20 additions & 0 deletions internals/workload/workload.go
Copy link
Contributor

@flotter flotter Jan 15, 2025

Choose a reason for hiding this comment

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

Perhaps, until this gets more flesh and there is a proven need (i.e. app armor and friends), this file could simply live inside servstate. However, looking at the dependency list, until we remove workloads from the plan into the servstate extension, this will not work as servstate imports plan already, and we would end up with a circular import between the plan and servstate.

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright (c) 2025 Canonical Ltd
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package workload

type Workload struct {
UID, GID *int
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems to me those aren't optional, so I don't see why we'd use a *int here instead of int.

Environment map[string]string
}
Loading