-
Notifications
You must be signed in to change notification settings - Fork 58
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
uid, gid := w.UID, w.GID | ||
if (uid == nil) != (gid == nil) { | ||
panic("both uid and gid must be provided by workload") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This panic will be triggered if either |
||
} | ||
if uid == nil { | ||
uid, gid, err = osutil.NormalizeUidGid(s.config.UserID, s.config.GroupID, s.config.User, s.config.Group) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why reassign Edit: nvm, in this branch we know that |
||
if err != nil { | ||
return err | ||
} | ||
} | ||
if uid != nil && gid != nil { | ||
isCurrent, err := osutil.IsCurrent(*uid, *gid) | ||
|
@@ -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 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used any more ? |
||
type ServiceManager struct { | ||
state *state.State | ||
|
||
|
@@ -30,7 +34,8 @@ type ServiceManager struct { | |
randLock sync.Mutex | ||
rand *rand.Rand | ||
|
||
logMgr LogManager | ||
workloads map[string]workload.Workload | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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"` | ||
|
@@ -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), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
|
@@ -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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
Environment map[string]string | ||
} |
There was a problem hiding this comment.
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.