feat: add workloads to the plan #571
Conversation
| if len(data.Content) != 0 { | ||
| yml, err := yaml.Marshal(data) | ||
| if err != nil { | ||
| return nil, fmt.Errorf(`internal error: cannot marshal "workloads" section: %w`, err) |
There was a problem hiding this comment.
I've seen people quote with %q which is a single quote. Would it make sense to follow that convention using 'workloads' and just use normal double quotes on the outside ? Maybe I am missing something. It also applies to the next line.
There was a problem hiding this comment.
Maybe I did not understand correctly, but %q uses double quotation marks. I personally don't like mixing up single/double quotes, and also found this in existing code:
Lines 732 to 736 in e3b81e1
| s.config = config.Copy() // update service config from plan | ||
| // Update service config and workload from plan | ||
| s.config = config.Copy() | ||
| if s.config.Workload != "" { |
There was a problem hiding this comment.
If workload changed from "foo" to "" with the previous update, I think you do want to update s.workload to not contain a copy of the previous workload ?
There was a problem hiding this comment.
Also if the plan.Service is the same, and the workload changed, replan will not restart the service.
There was a problem hiding this comment.
I was going to comment the same thing as Fred: don't we want to also restart the service if the workload details have changed (its user/group), not just the service.workload name?
|
Sorry for the delay -- I hope to get to this tomorrow. |
There was a problem hiding this comment.
I realise this is still a draft PR, but this will also need documentation: for now, at least changes to the layer spec in docs/reference/layer-specification.md.
| @@ -253,6 +253,8 @@ func (m *PlanManager) updatePlanLayers(layers []*plan.Layer) (*plan.Plan, error) | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| // Validate workloads & services here? | |||
| // We can make a switch in personality for KernOS/Pebble | |||
There was a problem hiding this comment.
Wouldn't that be done in plan.Validate, that is, the p.Validate() call above? Or isn't that done automatically by the ValidatePlan method on the extension?
| @@ -191,6 +191,7 @@ type Service struct { | |||
| Requires []string `yaml:"requires,omitempty"` | |||
|
|
|||
| // Options for command execution | |||
| Workload string `yaml:"workload,omitempty"` | |||
There was a problem hiding this comment.
This will also need to be added to Service.Merge.
| return s.plan.Validate() | ||
| } | ||
|
|
||
| func makeptr[T any](v T) *T { |
There was a problem hiding this comment.
Nice. In client/identities_test.go we call this just ptr, rename to that for brevity + consistency?
| func makeptr[T any](v T) *T { | |
| func ptr[T any](v T) *T { |
|
|
||
| var _ plan.SectionExtension = (*WorkloadsSectionExtension)(nil) | ||
|
|
||
| type WorkloadsSectionExtension struct{} |
There was a problem hiding this comment.
I would argue this shouldn't be in servstate -- that just happens to be the only "consumer" of workloads for now. I think should should go in the plan package (internals/plan/workloads.go). Or maybe it's own workloads package, but I think plan is fine.
Naming suggestions:
- If it's in
planpackage, let's call itplan.WorkloadsExtension(section seems obvious/redundant there) - If it's in its own package, let's call it
workloads.PlanExtensionor similar
| if !ok { | ||
| return fmt.Errorf("internal error: invalid section type %T", ws) | ||
| } | ||
| workload, ok = ws.Entries[config.Workload] |
There was a problem hiding this comment.
Nit: I think there's too many things named ok in a short space. How about hasWorkloads for the first, and then ok for the other two, but scoped inside a one-liner if?
Alternatively, because ws.Entries values are *Workload you could just check != nil.
| for k, v := range other.Environment { | ||
| w.Environment[k] = v | ||
| } | ||
| if other.UserID != nil { |
There was a problem hiding this comment.
Here I think we do need the if as we only want to merge/copy if the other is set.
| func (w *Workload) merge(other *Workload) { | ||
| w.Environment = makeMapIfNil(w.Environment) | ||
| for k, v := range other.Environment { | ||
| w.Environment[k] = v |
There was a problem hiding this comment.
I'd prefer to do the makeMapIfNil inside the loop here (like Service.Merge and others do) so that if both maps are nil, the result is nil. Only allocate if we have to.
Or maybe we can use maps.Copy:
if len(other.Environment) > 0 {
w.Environment = makeMapIfNil(w.Environment)
maps.Copy(w.Environment, other.Environment)
}Then again, that dance is kind of annoying, so maybe a copyMap helper that does this. I'll give it some more thought separately to this PR.
| for name, workload := range ws.Entries { | ||
| if workload == nil { | ||
| return &plan.FormatError{ | ||
| Message: fmt.Sprintf("workload %q has a null value", name), |
There was a problem hiding this comment.
| Message: fmt.Sprintf("workload %q has a null value", name), | |
| Message: fmt.Sprintf("workload %q cannot have a null value", name), |
| default: | ||
| override: merge | ||
| environment: | ||
| "1": "2" |
There was a problem hiding this comment.
Should have one that replaces a dictionary key from above, eg a: z
| bar: baz | ||
| user-id: 1337 | ||
| user: alyssa | ||
| group-id: 1337 |
There was a problem hiding this comment.
Let's make the user, user-id and group, group-id different in these tests, to ensure the code isn't accidentally swapping the two.
Workloads are a way to optionally group services and apply shared run configuration (user, group, environment) to services, in a way that may be extended in the future to support confinement.