Add "service context" support for exec and health checks #246
Conversation
Fixes canonical#158 (in preparation for canonical#159)
This is Pebble PR canonical/pebble#246 Also add working_dir field to pebble.Service and flesh out types for HttpDict, TcpDict, and ExecDict.
| @@ -65,7 +65,7 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) { | |||
| ctx, cancel := context.WithCancel(context.Background()) | |||
| check := &checkData{ | |||
| config: config, | |||
| checker: newChecker(config), | |||
| checker: newChecker(config, p), | |||
| ctx: ctx, | |||
| cancel: cancel, | |||
There was a problem hiding this comment.
This is not about this PR, but looking at this logic makes me think there's a bug here that needs to be addressed separately. The cancel here is a request. Right above we're calling cancel() as if this would be a synchronous operation, but it's not. The goroutine may take its time to run. This may cause very awkward outcomes, such as an old check running after a new check. We need to use tomb or something similar to make the stopping of checkers become synchronous.
There was a problem hiding this comment.
Indeed! @flotter Actually fixed this exact bug a couple of weeks ago. It was causing intermittent issues in CI, but of course as you point out it could be a problem in production too.
@flotter Note that in future (perhaps for your servstate fix :-) we should probably use tombs for this rather than wait groups, to be consistent with other parts of Pebble. Pebble already uses tombs extensively elsewhere, so it won't be a new dependency. I should have mentioned that in the code review.
| Group: config.Exec.Group, | ||
| WorkingDir: config.Exec.WorkingDir, | ||
| } | ||
| merged, err := plan.MergeServiceContext(p, config.Exec.Context, overrides) |
There was a problem hiding this comment.
That's a very clean implementation. Thank you.
internals/plan/plan.go
Outdated
| @@ -486,6 +487,9 @@ func (c *ExecCheck) Merge(other *ExecCheck) { | |||
| if other.Command != "" { | |||
| c.Command = other.Command | |||
| } | |||
| if other.Context != "" { | |||
| c.Context = other.Context | |||
There was a problem hiding this comment.
The only thing I'm not sure about here is if we want to support a context: default variant to allow a new layer to reset the context value back to the default (don't use any service context). Otherwise if one layer sets it, there's no way to unset it.
My inclination is that it's very unlikely to be used and we should leave it out for now.
|
|
||
| // MergeServiceContext merges the overrides on top of the service context | ||
| // specified by serviceName, returning a new ContextOptions value. | ||
| func MergeServiceContext(p *Plan, serviceName string, overrides ContextOptions) (ContextOptions, error) { |
There was a problem hiding this comment.
This is the shared helper function for both exec (one-shot commands) and exec-type health checks, used to merge the specified overrides on top of the service context values.
| @@ -65,7 +65,7 @@ func (m *CheckManager) PlanChanged(p *plan.Plan) { | |||
| ctx, cancel := context.WithCancel(context.Background()) | |||
| check := &checkData{ | |||
| config: config, | |||
| checker: newChecker(config), | |||
| checker: newChecker(config, p), | |||
| ctx: ctx, | |||
| cancel: cancel, | |||
There was a problem hiding this comment.
Indeed! @flotter Actually fixed this exact bug a couple of weeks ago. It was causing intermittent issues in CI, but of course as you point out it could be a problem in production too.
@flotter Note that in future (perhaps for your servstate fix :-) we should probably use tombs for this rather than wait groups, to be consistent with other parts of Pebble. Pebble already uses tombs extensively elsewhere, so it won't be a new dependency. I should have mentioned that in the code review.
|
Code review changes address. This is done now, but I'll wait till after Tuesday to merge in case we decide on a different name than "context" during the bikeshed. |
|
Updated "context" to "service-context" per discussion. |
#15909 Relevant changes for use of Pebble in Juju: * fix: exec and checks inherit environment from daemon (canonical/pebble#234) * Allow non-root to specify user/group to itself (canonical/pebble#239) * Add support for services to configure working directory (canonical/pebble#245) * Improve error messages with exec, especially if working dir missing (canonical/pebble#236) * Add "service context" support for exec and health checks (canonical/pebble#246) Full list of changes [here](canonical/pebble@5842ea6...00bcd1f). Note that this updates the version of `canonical/x-go` as well as switching to Canonical fork of `go-flags`.
This is Pebble PR canonical/pebble#246 Also add working_dir field to pebble.Service and flesh out types for HttpDict, TcpDict, and ExecDict. Bump up to latest Pyright version while we're at it It will be supported on Juju 3.1.6+ and 3.2.1+ and 3.3+
This adds the ability to run exec commands (one-shot commands) and exec-based health checks using the context from an existing service, specifically, reusing its environment variables, user/group, and working directory. Any options in the exec command or health check, if present, override the service context (and the environment variables are merged).
This is based on spec OP034.
Once this is merged, we should update the Pebble version in Juju. Update: done in juju/juju#15909