stage1: move more logic out of AppUnit #3496
Conversation
|
Failures are known flake (TestAppUserGroup) |
| } | ||
|
|
||
| // getAppResources processes any isolators that manipulate cgroups. | ||
| func getAppResources(isolators types.Isolators) (appResources, error) { |
| } | ||
|
|
||
| if !ok { | ||
| fmt.Fprintf(os.Stderr, "warning: resource/%s isolator set but support disabled in the kernel, skipping\n", name) |
There was a problem hiding this comment.
Let's return a named or typed Error and do the warn printing in the stage1 entrypoint code, not here in common.
There was a problem hiding this comment.
Hmm. This is tricky because this could possibly be the case for multiple isolators. What's the idiomatic way of representing multiple warnings?
There was a problem hiding this comment.
// ErrIsolatorUnsupported is the error that determines whether the specified isolator is supported in the kernel.
type ErrIsolatorUnsupported struct {
Isolator string
}
func (e ErrIsolatorUnsupported) Error() string {
return "unsupported isolator "+e.Isolator
}
The consumer would then do a type assertion if it is interested which isolator is unsupported:
if unsupportedErr, ok := err.(ErrIsolatorUnsupported); ok {
fmt.Println(unsupportedErr.Isolator)
}
| const ( | ||
| ModeBlacklist filterType = iota | ||
| ModeWhitelist filterType = iota | ||
| ) |
There was a problem hiding this comment.
nit:
const (
ModeBlacklist filterType = iota
ModeWhitelist
)
| filterPrefix := sdBlacklistPrefix | ||
| if sf.mode == ModeWhitelist { | ||
| filterPrefix = sdWhitelistPrefix | ||
| } |
There was a problem hiding this comment.
nit: let's make a switch statement here and err out on unknown mode.
| @@ -284,134 +283,121 @@ func (uw *UnitWriter) Error() error { | |||
| } | |||
|
|
|||
| func (uw *UnitWriter) AppUnit(ra *schema.RuntimeApp, binPath string, opts ...*unit.UnitOption) { | |||
| if uw.err != nil { | |||
There was a problem hiding this comment.
if uw.err != nil { was removed here, can you elaborate why?
| uw.err = errwrap.Wrap(errors.New("unable to write environment file for systemd"), err) | ||
| opts = uw.AppSystemdUnit(pa, binPath, opts) | ||
|
|
||
| if uw.err != nil { |
There was a problem hiding this comment.
The if uw.err != nil check must be at the beginning of the function to exit immediately and not execute any code in case a previous err happened.
There was a problem hiding this comment.
Not in this case - AppSystemdUnit can also apply err, in which case we should exit.
I'm not sure why this is structured this way, but it was like this when I got here.
There was a problem hiding this comment.
This was structured the "errWriter" way, see https://blog.golang.org/errors-are-values, applied to the unitWriter.
The reasoning is that the caller can call unitWriter methods sequentially and check the error only once at the end of the sequence to avoid long if err != nil ... repetitions to the unit writer.
If we don't put the if uw.err != nil check at the top of the function, we break this semantic. I should and will add a reference to the godoc to the above mentioned article.
There was a problem hiding this comment.
I see what you mean - though there is only one caller in this case. ::shrugs::
| } | ||
|
|
||
| // AppSystemdUnit sets up an application for isolation via systemd | ||
| func (uw *UnitWriter) AppSystemdUnit(pa *preparedApp, binPath string, opts []*unit.UnitOption) []*unit.UnitOption { |
There was a problem hiding this comment.
This function should check if any previous error occured using if uw.err != nil { return nil } and not execute any code.
There was a problem hiding this comment.
That is not needed. There is nothing that could set err before this function.
There was a problem hiding this comment.
A previous UnitWriter method invocation could cause to set the err before this function, hence it is needed. You don't know if the UnitWriter is not in errornous state already.
|
@s-urbaniak updated, thanks! |
| } | ||
|
|
||
| if !ok { | ||
| fmt.Fprintf(os.Stderr, "warning: resource/%s isolator set but support disabled in the kernel, skipping\n", name) |
There was a problem hiding this comment.
ok, this is still here, and it was there also in the old code. I'll submit an issue to get rid of all fmt.Fprintf occurrences and replace them with respective injectable warnf loggers.
|
This got a conflict in the meanwhile. It looks fine in general, I'll do a last pass after the rebase. |
|
@squeed bump |
|
Rebased, thanks! |
|
This PR got hit by a github downtime, so it is in a kind for weird state. @squeed Mind rebasing and pushing again? |
|
Rebased - let's go for green! |
|
fail :-( |
|
Neat, it printed the byte sequence. The error message is
I'll dig in to this. |
Try and simplify app unit generation in a few ways. This is in
preparation for pluggable stage2 isolation engines.
- generate a struct with runtime information, rather than generating
unit directives directly from isolators, etc.
- Split systemd-specific and more generic unit code
|
Bad merge - OnFailure snuck back in. Fixed. |
Try and simplify app unit generation in a few ways This is in preparation for pluggable stage2 isolation.