Add support for crun hook stdout/stderr annotations in precreate hooks - #1089
Add support for crun hook stdout/stderr annotations in precreate hooks#1089ajoshua2004 wants to merge 1 commit into
Conversation
| runStderr = stderrFile | ||
| } | ||
|
|
||
| hookErr, err = RunWithOptions(ctx, RunOptions{Hook: &hook, Dir: options.Dir, State: data, Stdout: runStdout, Stderr: runStderr, PostKillTimeout: options.PostKillTimeout}) |
There was a problem hiding this comment.
unless I am missing something Stderr: stderrFile should work here without having to decalre or do a nil check above. If stderrFile is nil then we can just pass it as nil
There was a problem hiding this comment.
tried this and it being a nil os.File breaks it. passing it as a nil causes os/exec to close the stderr fd instead of redirecting to /dev/null. added a regression test and it fails without the check
| } | ||
|
|
||
| func TestRuntimeConfigFilterOutputRedirection(t *testing.T) { | ||
| ctx := context.Background() |
There was a problem hiding this comment.
use t.Context() for a per test context which is cancelled at the end of a test in case something leaks
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
we already use github.com/stretchr/testify/assert so this should use github.com/stretchr/testify/require and then call require.NoError() for nicer error messages, same in all the other places here
| info, err := os.Stat(stdoutPath) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } |
There was a problem hiding this comment.
well you should read the file and make it is empty for better coverage.
| input := &spec.Spec{ | ||
| Version: "1.0.0", | ||
| Root: &spec.Root{Path: "rootfs"}, | ||
| Annotations: map[string]string{AnnotationHookStdout: stdoutPath}, | ||
| } | ||
| expectedJSON, err := json.Marshal(input) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
|
|
||
| hooks := []spec.Hook{{Path: path, Args: []string{"sh", "-c", "cat"}}} | ||
| hookErr, err := RuntimeConfigFilterWithOptions(ctx, RuntimeConfigFilterOptions{Hooks: hooks, Config: input, PostKillTimeout: DefaultPostKillTimeout}) | ||
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| if hookErr != nil { | ||
| t.Fatal(hookErr) | ||
| } |
There was a problem hiding this comment.
there is a lot of duplication for these tests, I think this would be written a lot better as table driven test, i.e. a loop over all test cases defined as struct where the input/output are defined once. TestRuntimeConfigFilter can serve as example of what I mean.
Precreate hooks run directly by Podman before the OCI runtime is invoked, so they never benefit from crun's run.oci.hooks.stdout/ run.oci.hooks.stderr annotations. This adds the same support to RuntimeConfigFilterWithOptions, opening the annotated file(s) in append mode (creating if missing, mode 0700 to match crun) and wiring hook stdout/stderr into them. Signed-off-by: Joshua Arrevillaga <2004jarrevillaga@gmail.com>
bda30fb to
d459229
Compare
2 participants
OCI hooks can redirect their output to a file via 2 crun annotations
run.oci.hooks.stdout=FILE/run.oci.hooks.stderr=FILEThis works for every hook type except precreatehooks, since those are executed directly by podman before the OCI runtime is invoked so crun doesnt get a chance to honor the annotations. This adds the same annotation support toRuntimeConfigFilterWithOptionsso precreate hook output anc be redirected too. PR in podman will be needed to bump vendor dependency.Test
go test ./pkg/hooks/exec/... -run TestRuntimeConfigFilterOutputRedirection -v