From 30540fc9a219097b468d2d4ca0e1f4171520ff4e Mon Sep 17 00:00:00 2001 From: Chitrang Patel Date: Thu, 23 Nov 2023 11:20:10 -0500 Subject: [PATCH] TEP-0142: Surface step results via sidecar logs Prior to this, we enabled surfacing step results via termination message. This PR does the same thing via sidecar logs. --- cmd/sidecarlogresults/main.go | 9 +- docs/stepactions.md | 5 - .../{no-ci => alpha}/stepaction-results.yaml | 2 + .../sidecarlogresults/sidecarlogresults.go | 90 ++++++++-- .../sidecarlogresults_test.go | 164 +++++++++++++++++- pkg/pod/pod.go | 39 ++++- pkg/pod/pod_test.go | 82 +++++++++ pkg/pod/status.go | 115 ++++++++---- pkg/pod/status_test.go | 63 +++++++ 9 files changed, 512 insertions(+), 57 deletions(-) rename examples/v1/taskruns/{no-ci => alpha}/stepaction-results.yaml (83%) diff --git a/cmd/sidecarlogresults/main.go b/cmd/sidecarlogresults/main.go index 70d8a7e9d76..c66ce1133e0 100644 --- a/cmd/sidecarlogresults/main.go +++ b/cmd/sidecarlogresults/main.go @@ -17,6 +17,7 @@ limitations under the License. package main import ( + "encoding/json" "flag" "log" "os" @@ -30,14 +31,20 @@ import ( func main() { var resultsDir string var resultNames string + var stepResultsStr string flag.StringVar(&resultsDir, "results-dir", pipeline.DefaultResultPath, "Path to the results directory. Default is /tekton/results") flag.StringVar(&resultNames, "result-names", "", "comma separated result names to expect from the steps running in the pod. eg. foo,bar,baz") + flag.StringVar(&stepResultsStr, "step-results", "", "json containing a map of step Name as key and list of result Names. eg. {\"stepName\":[\"foo\",\"bar\",\"baz\"]}") flag.Parse() if resultNames == "" { log.Fatal("result-names were not provided") } expectedResults := strings.Split(resultNames, ",") - err := sidecarlogresults.LookForResults(os.Stdout, pod.RunDir, resultsDir, expectedResults) + expectedStepResults := map[string][]string{} + if err := json.Unmarshal([]byte(stepResultsStr), &expectedStepResults); err != nil { + log.Fatal(err) + } + err := sidecarlogresults.LookForResults(os.Stdout, pod.RunDir, resultsDir, expectedResults, pipeline.StepsDir, expectedStepResults) if err != nil { log.Fatal(err) } diff --git a/docs/stepactions.md b/docs/stepactions.md index 65d954ce8c9..da46b8a9e8b 100644 --- a/docs/stepactions.md +++ b/docs/stepactions.md @@ -19,7 +19,6 @@ weight: 201 - [Specifying Remote StepActions](#specifying-remote-stepactions) - [Known Limitations](#known-limitations) - [Cannot pass Step Results between Steps](#cannot-pass-step-results-between-steps) - - [Cannot extract Step Results via Sidecar logs](#cannot-extract-step-results-via-sidecar-logs) ## Overview :warning: This feature is in a preview mode. @@ -430,7 +429,3 @@ The default resolver type can be configured by the `default-resolver-type` field ### Cannot pass Step Results between Steps It's not currently possible to pass results produced by a `Step` into following `Steps`. We are working on this feature and will be made available soon. - -### Cannot extract Step Results via Sidecar logs - -Currently, we only support Step Results via `Termination Message` (the default method of result extraction). The ability to extract `Step` results from `sidecar-logs` is not yet available. We are working on enabling this soon. diff --git a/examples/v1/taskruns/no-ci/stepaction-results.yaml b/examples/v1/taskruns/alpha/stepaction-results.yaml similarity index 83% rename from examples/v1/taskruns/no-ci/stepaction-results.yaml rename to examples/v1/taskruns/alpha/stepaction-results.yaml index f4a285d83db..7ea048830a7 100644 --- a/examples/v1/taskruns/no-ci/stepaction-results.yaml +++ b/examples/v1/taskruns/alpha/stepaction-results.yaml @@ -6,8 +6,10 @@ spec: image: alpine results: - name: result1 + - name: result2 script: | echo "I am a Step Action!!!" >> $(step.results.result1.path) + echo "I am a hidden step action!!!" >> $(step.results.result2.path) --- apiVersion: tekton.dev/v1 kind: TaskRun diff --git a/internal/sidecarlogresults/sidecarlogresults.go b/internal/sidecarlogresults/sidecarlogresults.go index c290aaadd48..12382d50458 100644 --- a/internal/sidecarlogresults/sidecarlogresults.go +++ b/internal/sidecarlogresults/sidecarlogresults.go @@ -25,6 +25,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/tektoncd/pipeline/pkg/apis/config" "github.com/tektoncd/pipeline/pkg/result" @@ -36,10 +37,19 @@ import ( // ErrSizeExceeded indicates that the result exceeded its maximum allowed size var ErrSizeExceeded = errors.New("results size exceeds configured limit") +type SidecarLogResultType string + +const ( + taskResultType SidecarLogResultType = "task" + stepResultType SidecarLogResultType = "step" + sidecarResultNameSeparator string = "." +) + // SidecarLogResult holds fields for storing extracted results type SidecarLogResult struct { Name string Value string + Type SidecarLogResultType } func fileExists(filename string) (bool, error) { @@ -89,10 +99,42 @@ func waitForStepsToFinish(runDir string) error { return nil } +func createSidecarResultName(stepName, resultName string) string { + return fmt.Sprintf("%s%s%s", stepName, sidecarResultNameSeparator, resultName) +} + +// ExtractStepAndResultFromSidecarResultName splits the result name to extract the step +// and result name from it. It only works if the format is . +func ExtractStepAndResultFromSidecarResultName(sidecarResultName string) (string, string, error) { + splitString := strings.SplitN(sidecarResultName, sidecarResultNameSeparator, 2) + if len(splitString) != 2 { + return "", "", fmt.Errorf("invalid string %s : expected somtthing that looks like .", sidecarResultName) + } + return splitString[0], splitString[1], nil +} + +func readResults(resultsDir, resultFile, stepName string, resultType SidecarLogResultType) (SidecarLogResult, error) { + value, err := os.ReadFile(filepath.Join(resultsDir, resultFile)) + if os.IsNotExist(err) { + return SidecarLogResult{}, nil + } else if err != nil { + return SidecarLogResult{}, fmt.Errorf("error reading the results file %w", err) + } + resultName := resultFile + if resultType == stepResultType { + resultName = createSidecarResultName(stepName, resultFile) + } + return SidecarLogResult{ + Name: resultName, + Value: string(value), + Type: resultType, + }, nil +} + // LookForResults waits for results to be written out by the steps // in their results path and prints them in a structured way to its // stdout so that the reconciler can parse those logs. -func LookForResults(w io.Writer, runDir string, resultsDir string, resultNames []string) error { +func LookForResults(w io.Writer, runDir string, resultsDir string, resultNames []string, stepResultsDir string, stepResults map[string][]string) error { if err := waitForStepsToFinish(runDir); err != nil { return fmt.Errorf("error while waiting for the steps to finish %w", err) } @@ -102,20 +144,39 @@ func LookForResults(w io.Writer, runDir string, resultsDir string, resultNames [ resultFile := resultFile g.Go(func() error { - value, err := os.ReadFile(filepath.Join(resultsDir, resultFile)) - if os.IsNotExist(err) { - return nil - } else if err != nil { - return fmt.Errorf("error reading the results file %w", err) + newResult, err := readResults(resultsDir, resultFile, "", taskResultType) + if err != nil { + return err } - newResult := SidecarLogResult{ - Name: resultFile, - Value: string(value), + if newResult.Name == "" { + return nil } results <- newResult return nil }) } + + for sName, sresults := range stepResults { + sresults := sresults + sName := sName + for _, resultName := range sresults { + resultName := resultName + stepResultsDir := filepath.Join(stepResultsDir, sName, "results") + + g.Go(func() error { + newResult, err := readResults(stepResultsDir, resultName, sName, stepResultType) + if err != nil { + return err + } + if newResult.Name == "" { + return nil + } + results <- newResult + return nil + }) + } + } + channelGroup := new(errgroup.Group) channelGroup.Go(func() error { if err := g.Wait(); err != nil { @@ -183,10 +244,19 @@ func parseResults(resultBytes []byte, maxResultLimit int) (result.RunResult, err if len(resultBytes) > maxResultLimit { return runResult, fmt.Errorf("invalid result \"%s\": %w of %d", res.Name, ErrSizeExceeded, maxResultLimit) } + var resultType result.ResultType + switch res.Type { + case taskResultType: + resultType = result.TaskRunResultType + case stepResultType: + resultType = result.StepResultType + default: + return result.RunResult{}, fmt.Errorf("invalid sidecar result type %v. Must be %v or %v", res.Type, taskResultType, stepResultType) + } runResult = result.RunResult{ Key: res.Name, Value: res.Value, - ResultType: result.TaskRunResultType, + ResultType: resultType, } return runResult, nil } diff --git a/internal/sidecarlogresults/sidecarlogresults_test.go b/internal/sidecarlogresults/sidecarlogresults_test.go index 7e078e26040..8543b780a62 100644 --- a/internal/sidecarlogresults/sidecarlogresults_test.go +++ b/internal/sidecarlogresults/sidecarlogresults_test.go @@ -46,9 +46,11 @@ func TestLookForResults_FanOutAndWait(t *testing.T) { results: []SidecarLogResult{{ Name: "foo", Value: "bar", + Type: "task", }, { Name: "foo2", Value: "bar2", + Type: "task", }}, }} { t.Run(c.desc, func(t *testing.T) { @@ -70,7 +72,7 @@ func TestLookForResults_FanOutAndWait(t *testing.T) { dir2 := t.TempDir() createRun(t, dir2, false) got := new(bytes.Buffer) - err := LookForResults(got, dir2, dir, resultNames) + err := LookForResults(got, dir2, dir, resultNames, "", map[string][]string{}) if err != nil { t.Fatalf("Did not expect any error but got: %v", err) } @@ -124,6 +126,7 @@ func TestLookForResults(t *testing.T) { result := SidecarLogResult{ Name: c.resultName, Value: c.resultValue, + Type: "task", } encodedResult, err := json.Marshal(result) if err != nil { @@ -135,7 +138,77 @@ func TestLookForResults(t *testing.T) { want = encodedResult } got := new(bytes.Buffer) - err := LookForResults(got, dir2, dir, []string{c.resultName}) + err := LookForResults(got, dir2, dir, []string{c.resultName}, "", map[string][]string{}) + if err != nil { + t.Fatalf("Did not expect any error but got: %v", err) + } + if d := cmp.Diff(want, got.Bytes()); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } + }) + } +} + +func TestLookForStepResults(t *testing.T) { + for _, c := range []struct { + desc string + stepName string + resultName string + resultValue string + createResult bool + stepError bool + }{{ + desc: "good result", + stepName: "step-foo", + resultName: "foo", + resultValue: "bar", + createResult: true, + stepError: false, + }, { + desc: "empty result", + stepName: "step-foo", + resultName: "foo", + resultValue: "", + createResult: true, + stepError: true, + }, { + desc: "missing result", + stepName: "step-foo", + resultName: "missing", + resultValue: "", + createResult: false, + stepError: false, + }} { + t.Run(c.desc, func(t *testing.T) { + dir := t.TempDir() + if c.createResult == true { + createStepResult(t, dir, c.stepName, c.resultName, c.resultValue) + } + dir2 := t.TempDir() + createRun(t, dir2, c.stepError) + + var want []byte + if c.createResult == true { + // This is the expected result + result := SidecarLogResult{ + Name: fmt.Sprintf("%s.%s", c.stepName, c.resultName), + Value: c.resultValue, + Type: "step", + } + encodedResult, err := json.Marshal(result) + if err != nil { + t.Error(err) + } + // encode adds a newline character at the end. + // We need to do the same before comparing + encodedResult = append(encodedResult, '\n') + want = encodedResult + } + got := new(bytes.Buffer) + stepResults := map[string][]string{ + c.stepName: {c.resultName}, + } + err := LookForResults(got, dir2, "", []string{}, dir, stepResults) if err != nil { t.Fatalf("Did not expect any error but got: %v", err) } @@ -151,9 +224,11 @@ func TestExtractResultsFromLogs(t *testing.T) { { Name: "result1", Value: "foo", + Type: "task", }, { Name: "result2", Value: "bar", + Type: "task", }, } podLogs := "" @@ -188,6 +263,7 @@ func TestExtractResultsFromLogs_Failure(t *testing.T) { { Name: "result1", Value: strings.Repeat("v", 4098), + Type: "task", }, } podLogs := "" @@ -208,12 +284,27 @@ func TestParseResults(t *testing.T) { { Name: "result1", Value: "foo", + Type: "task", }, { Name: "result2", Value: `{"IMAGE_URL":"ar.com", "IMAGE_DIGEST":"sha234"}`, + Type: "task", }, { Name: "result3", Value: `["hello","world"]`, + Type: "task", + }, { + Name: "step-foo.result1", + Value: "foo", + Type: "step", + }, { + Name: "step-foo.result2", + Value: `{"IMAGE_URL":"ar.com", "IMAGE_DIGEST":"sha234"}`, + Type: "step", + }, { + Name: "step-foo.result3", + Value: `["hello","world"]`, + Type: "step", }, } podLogs := []string{} @@ -233,6 +324,18 @@ func TestParseResults(t *testing.T) { Key: "result3", Value: `["hello","world"]`, ResultType: result.TaskRunResultType, + }, { + Key: "step-foo.result1", + Value: "foo", + ResultType: result.StepResultType, + }, { + Key: "step-foo.result2", + Value: `{"IMAGE_URL":"ar.com", "IMAGE_DIGEST":"sha234"}`, + ResultType: result.StepResultType, + }, { + Key: "step-foo.result3", + Value: `["hello","world"]`, + ResultType: result.StepResultType, }} stepResults := []result.RunResult{} for _, plog := range podLogs { @@ -247,11 +350,32 @@ func TestParseResults(t *testing.T) { } } +func TestParseResults_InvalidType(t *testing.T) { + results := []SidecarLogResult{{ + Name: "result1", + Value: "foo", + Type: "not task or step", + }} + podLogs := []string{} + for _, r := range results { + res, _ := json.Marshal(&r) + podLogs = append(podLogs, string(res)) + } + for _, plog := range podLogs { + _, err := parseResults([]byte(plog), 4096) + wantErr := fmt.Errorf("invalid sidecar result type not task or step. Must be task or step") + if d := cmp.Diff(wantErr.Error(), err.Error()); d != "" { + t.Fatal(diff.PrintWantGot(d)) + } + } +} + func TestParseResults_Failure(t *testing.T) { maxResultLimit := 4096 result := SidecarLogResult{ Name: "result2", Value: strings.Repeat("k", 4098), + Type: "task", } res1, _ := json.Marshal("result1 v1") res2, _ := json.Marshal(&result) @@ -325,6 +449,42 @@ func TestGetResultsFromSidecarLogs(t *testing.T) { } } +func TestExtractStepAndResultFromSidecarResultName(t *testing.T) { + sidecarResultName := "step-foo.resultName" + wantResult := "resultName" + wantStep := "step-foo" + gotStep, gotResult, err := ExtractStepAndResultFromSidecarResultName(sidecarResultName) + if err != nil { + t.Fatalf("did not expect an error but got: %v", err) + } + if gotStep != wantStep { + t.Fatalf("failed to extract step name from string %s. Expexted %s but got %s", sidecarResultName, wantStep, gotStep) + } + if gotResult != wantResult { + t.Fatalf("failed to extract result name from string %s. Expexted %s but got %s", sidecarResultName, wantResult, gotResult) + } +} + +func TestExtractStepAndResultFromSidecarResultName_Error(t *testing.T) { + sidecarResultName := "step-foo-resultName" + _, _, err := ExtractStepAndResultFromSidecarResultName(sidecarResultName) + wantErr := fmt.Errorf("invalid string step-foo-resultName : expected somtthing that looks like .") + if d := cmp.Diff(wantErr.Error(), err.Error()); d != "" { + t.Fatal(diff.PrintWantGot(d)) + } +} + +func createStepResult(t *testing.T, dir, stepName, resultName, resultValue string) { + t.Helper() + resultDir := filepath.Join(dir, stepName, "results") + _ = os.MkdirAll(resultDir, 0755) + resultFile := filepath.Join(resultDir, resultName) + err := os.WriteFile(resultFile, []byte(resultValue), 0644) + if err != nil { + t.Fatal(err) + } +} + func createResult(t *testing.T, dir string, resultName string, resultValue string) { t.Helper() resultFile := filepath.Join(dir, resultName) diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go index 04a0a2dec0e..d2af9a8cb23 100644 --- a/pkg/pod/pod.go +++ b/pkg/pod/pod.go @@ -18,6 +18,7 @@ package pod import ( "context" + "encoding/json" "fmt" "log" "math" @@ -190,7 +191,10 @@ func (b *Builder) Build(ctx context.Context, taskRun *v1.TaskRun, taskSpec v1.Ta windows := usesWindows(taskRun) if sidecarLogsResultsEnabled && taskSpec.Results != nil { // create a results sidecar - resultsSidecar := createResultsSidecar(taskSpec, b.Images.SidecarLogResultsImage, setSecurityContext, windows) + resultsSidecar, err := createResultsSidecar(taskSpec, b.Images.SidecarLogResultsImage, setSecurityContext, windows) + if err != nil { + return nil, err + } taskSpec.Sidecars = append(taskSpec.Sidecars, resultsSidecar) commonExtraEntrypointArgs = append(commonExtraEntrypointArgs, "-result_from", config.ResultExtractionMethodSidecarLogs) } @@ -568,26 +572,47 @@ func entrypointInitContainer(image string, steps []v1.Step, setSecurityContext, // based on the spec of the Task, the image that should run in the results sidecar, // whether it will run on a windows node, and whether the sidecar should include a security context // that will allow it to run in namespaces with "restricted" pod security admission. -func createResultsSidecar(taskSpec v1.TaskSpec, image string, setSecurityContext, windows bool) v1.Sidecar { +// It will also provide arguments to the binary that allow it to surface the step results. +func createResultsSidecar(taskSpec v1.TaskSpec, image string, setSecurityContext, windows bool) (v1.Sidecar, error) { names := make([]string, 0, len(taskSpec.Results)) for _, r := range taskSpec.Results { names = append(names, r.Name) } - securityContext := linuxSecurityContext - if windows { - securityContext = windowsSecurityContext - } resultsStr := strings.Join(names, ",") command := []string{"/ko-app/sidecarlogresults", "-results-dir", pipeline.DefaultResultPath, "-result-names", resultsStr} + + // create a map of container Name to step results + stepResults := map[string][]string{} + for i, s := range taskSpec.Steps { + if len(s.Results) > 0 { + stepName := StepName(s.Name, i) + stepResults[stepName] = make([]string, 0, len(s.Results)) + for _, r := range s.Results { + stepResults[stepName] = append(stepResults[stepName], r.Name) + } + } + } + + stepResultsBytes, err := json.Marshal(stepResults) + if err != nil { + return v1.Sidecar{}, err + } + if len(stepResultsBytes) > 0 { + command = append(command, "-step-results", string(stepResultsBytes)) + } sidecar := v1.Sidecar{ Name: pipeline.ReservedResultsSidecarName, Image: image, Command: command, } + securityContext := linuxSecurityContext + if windows { + securityContext = windowsSecurityContext + } if setSecurityContext { sidecar.SecurityContext = securityContext } - return sidecar + return sidecar, nil } // usesWindows returns true if the TaskRun will run on a windows node, diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go index 70dc7752b3f..68a9c79ffb5 100644 --- a/pkg/pod/pod_test.go +++ b/pkg/pod/pod_test.go @@ -1950,6 +1950,86 @@ _EOF_ "/tekton/results", "-result-names", "foo", + "-step-results", + "{}", + }, + Resources: corev1.ResourceRequirements{ + Requests: nil, + }, + VolumeMounts: append([]corev1.VolumeMount{ + {Name: "tekton-internal-bin", ReadOnly: true, MountPath: "/tekton/bin"}, + {Name: "tekton-internal-run-0", ReadOnly: true, MountPath: "/tekton/run/0"}, + }, implicitVolumeMounts...), + }}, + Volumes: append(implicitVolumes, binVolume, runVolume(0), downwardVolume, corev1.Volume{ + Name: "tekton-creds-init-home-0", + VolumeSource: corev1.VolumeSource{EmptyDir: &corev1.EmptyDirVolumeSource{Medium: corev1.StorageMediumMemory}}, + }), + ActiveDeadlineSeconds: &defaultActiveDeadlineSeconds, + }, + }, { + desc: "sidecar logs enabled with step results", + featureFlags: map[string]string{"results-from": "sidecar-logs"}, + ts: v1.TaskSpec{ + Results: []v1.TaskResult{{ + Name: "foo", + Type: v1.ResultsTypeString, + }}, + Steps: []v1.Step{{ + Name: "name", + Results: []v1.StepResult{{ + Name: "step-foo", + Type: v1.ResultsTypeString, + }}, + Image: "image", + Command: []string{"cmd"}, // avoid entrypoint lookup. + }}, + }, + want: &corev1.PodSpec{ + RestartPolicy: corev1.RestartPolicyNever, + InitContainers: []corev1.Container{ + entrypointInitContainer(images.EntrypointImage, []v1.Step{{Name: "name"}}, false /* setSecurityContext */, false /* windows */), + }, + Containers: []corev1.Container{{ + Name: "step-name", + Image: "image", + Command: []string{"/tekton/bin/entrypoint"}, + Args: []string{ + "-wait_file", + "/tekton/downward/ready", + "-wait_file_content", + "-post_file", + "/tekton/run/0/out", + "-termination_path", + "/tekton/termination", + "-step_metadata_dir", + "/tekton/run/0/status", + "-result_from", + "sidecar-logs", + "-step_results", + "step-foo", + "-results", + "foo", + "-entrypoint", + "cmd", + "--", + }, + VolumeMounts: append([]corev1.VolumeMount{binROMount, runMount(0, false), downwardMount, { + Name: "tekton-creds-init-home-0", + MountPath: "/tekton/creds", + }}, implicitVolumeMounts...), + TerminationMessagePath: "/tekton/termination", + }, { + Name: pipeline.ReservedResultsSidecarContainerName, + Image: "", + Command: []string{ + "/ko-app/sidecarlogresults", + "-results-dir", + "/tekton/results", + "-result-names", + "foo", + "-step-results", + "{\"step-name\":[\"step-foo\"]}", }, Resources: corev1.ResourceRequirements{ Requests: nil, @@ -2020,6 +2100,8 @@ _EOF_ "/tekton/results", "-result-names", "foo", + "-step-results", + "{}", }, Resources: corev1.ResourceRequirements{ Requests: nil, diff --git a/pkg/pod/status.go b/pkg/pod/status.go index 848c72ba05e..5fa850fb8ca 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -163,6 +163,49 @@ func MakeTaskRunStatus(ctx context.Context, logger *zap.SugaredLogger, tr v1.Tas return *trs, merr.ErrorOrNil() } +func createTaskResultsFromStepResults(stepRunRes []v1.TaskRunStepResult, neededStepResults map[string]string) []v1.TaskRunResult { + taskResults := []v1.TaskRunResult{} + for _, r := range stepRunRes { + // this result was requested by the Task + if _, ok := neededStepResults[r.Name]; ok { + taskRunResult := v1.TaskRunResult{ + Name: neededStepResults[r.Name], + Type: r.Type, + Value: r.Value, + } + taskResults = append(taskResults, taskRunResult) + } + } + return taskResults +} + +func getTaskResultsFromSidecarLogs(sidecarLogResults []result.RunResult) []result.RunResult { + taskResultsFromSidecarLogs := []result.RunResult{} + for _, slr := range sidecarLogResults { + if slr.ResultType == result.TaskRunResultType { + taskResultsFromSidecarLogs = append(taskResultsFromSidecarLogs, slr) + } + } + return taskResultsFromSidecarLogs +} + +func getStepResultsFromSidecarLogs(sidecarLogResults []result.RunResult, containerName string) ([]result.RunResult, error) { + stepResultsFromSidecarLogs := []result.RunResult{} + for _, slr := range sidecarLogResults { + if slr.ResultType == result.StepResultType { + stepName, resultName, err := sidecarlogresults.ExtractStepAndResultFromSidecarResultName(slr.Key) + if err != nil { + return []result.RunResult{}, err + } + if stepName == containerName { + slr.Key = resultName + stepResultsFromSidecarLogs = append(stepResultsFromSidecarLogs, slr) + } + } + } + return stepResultsFromSidecarLogs, nil +} + func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredLogger, stepStatuses []corev1.ContainerStatus, tr *v1.TaskRun, podPhase corev1.PodPhase, kubeclient kubernetes.Interface, ts *v1.TaskSpec) *multierror.Error { trs := &tr.Status var merr *multierror.Error @@ -178,24 +221,56 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL // Extract results from sidecar logs sidecarLogsResultsEnabled := config.FromContextOrDefaults(ctx).FeatureFlags.ResultExtractionMethod == config.ResultExtractionMethodSidecarLogs + sidecarLogResults := []result.RunResult{} if sidecarLogsResultsEnabled && tr.Status.TaskSpec.Results != nil { // extraction of results from sidecar logs - sidecarLogResults, err := sidecarlogresults.GetResultsFromSidecarLogs(ctx, kubeclient, tr.Namespace, tr.Status.PodName, pipeline.ReservedResultsSidecarContainerName, podPhase) + slr, err := sidecarlogresults.GetResultsFromSidecarLogs(ctx, kubeclient, tr.Namespace, tr.Status.PodName, pipeline.ReservedResultsSidecarContainerName, podPhase) if err != nil { merr = multierror.Append(merr, err) } - // populate task run CRD with results from sidecar logs - // since sidecar logs does not support step results yet, it is empty for now. - taskResults, _, _ := filterResults(sidecarLogResults, specResults, []v1.StepResult{}) - if tr.IsDone() { - trs.Results = append(trs.Results, taskResults...) - } + sidecarLogResults = append(sidecarLogResults, slr...) + } + // Populate Task results from sidecar logs + taskResultsFromSidecarLogs := getTaskResultsFromSidecarLogs(sidecarLogResults) + taskResults, _, _ := filterResults(taskResultsFromSidecarLogs, specResults, nil) + if tr.IsDone() { + trs.Results = append(trs.Results, taskResults...) } + // Continue with extraction of termination messages for _, s := range stepStatuses { // Avoid changing the original value by modifying the pointer value. state := s.State.DeepCopy() taskRunStepResults := []v1.TaskRunStepResult{} + + // Identify Step Results + stepResults := []v1.StepResult{} + if ts != nil { + for _, step := range ts.Steps { + if getContainerName(step.Name) == s.Name { + stepResults = append(stepResults, step.Results...) + } + } + } + // Identify StepResults needed by the Task Results + neededStepResults, err := findStepResultsFetchedByTask(s.Name, specResults) + if err != nil { + merr = multierror.Append(merr, err) + } + + // populate step results from sidecar logs + stepResultsFromSidecarLogs, err := getStepResultsFromSidecarLogs(sidecarLogResults, s.Name) + if err != nil { + merr = multierror.Append(merr, err) + } + _, stepRunRes, _ := filterResults(stepResultsFromSidecarLogs, specResults, stepResults) + if tr.IsDone() { + taskRunStepResults = append(taskRunStepResults, stepRunRes...) + // Set TaskResults from StepResults + trs.Results = append(trs.Results, createTaskResultsFromStepResults(stepRunRes, neededStepResults)...) + } + + // Parse termination messages if state.Terminated != nil && len(state.Terminated.Message) != 0 { msg := state.Terminated.Message @@ -215,35 +290,11 @@ func setTaskRunStatusBasedOnStepStatus(ctx context.Context, logger *zap.SugaredL merr = multierror.Append(merr, err) } - // Identify Step Results - stepResults := []v1.StepResult{} - if ts != nil { - for _, step := range ts.Steps { - if getContainerName(step.Name) == s.Name { - stepResults = append(stepResults, step.Results...) - } - } - } taskResults, stepRunRes, filteredResults := filterResults(results, specResults, stepResults) if tr.IsDone() { taskRunStepResults = append(taskRunStepResults, stepRunRes...) - // Identify StepResults needed by the Task Results - neededStepResults, err := findStepResultsFetchedByTask(s.Name, specResults) - if err != nil { - merr = multierror.Append(merr, err) - } // Set TaskResults from StepResults - for _, r := range stepRunRes { - // this result was requested by the Task - if _, ok := neededStepResults[r.Name]; ok { - taskRunResult := v1.TaskRunResult{ - Name: neededStepResults[r.Name], - Type: r.Type, - Value: r.Value, - } - taskResults = append(taskResults, taskRunResult) - } - } + taskResults = append(taskResults, createTaskResultsFromStepResults(stepRunRes, neededStepResults)...) trs.Results = append(trs.Results, taskResults...) } msg, err = createMessageFromResults(filteredResults) diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 4496651b661..64566f66f6b 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -29,6 +29,7 @@ import ( "github.com/tektoncd/pipeline/internal/sidecarlogresults" "github.com/tektoncd/pipeline/pkg/apis/config" v1 "github.com/tektoncd/pipeline/pkg/apis/pipeline/v1" + "github.com/tektoncd/pipeline/pkg/result" "github.com/tektoncd/pipeline/test/diff" corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" @@ -2229,3 +2230,65 @@ func TestSortPodContainerStatuses(t *testing.T) { } } } + +func TestGetStepResultsFromSidecarLogs(t *testing.T) { + stepName := "step-foo" + resultName := "step-res" + sidecarLogResults := []result.RunResult{{ + Key: fmt.Sprintf("%s.%s", stepName, resultName), + Value: "bar", + ResultType: result.StepResultType, + }, { + Key: "task-res", + Value: "task-bar", + ResultType: result.TaskRunResultType, + }} + want := []result.RunResult{{ + Key: resultName, + Value: "bar", + ResultType: result.StepResultType, + }} + got, err := getStepResultsFromSidecarLogs(sidecarLogResults, stepName) + if err != nil { + t.Errorf("did not expect an error but got: %v", err) + } + if d := cmp.Diff(want, got); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } +} + +func TestGetStepResultsFromSidecarLogs_Error(t *testing.T) { + stepName := "step-foo" + resultName := "step-res" + sidecarLogResults := []result.RunResult{{ + Key: fmt.Sprintf("%s-%s", stepName, resultName), + Value: "bar", + ResultType: result.StepResultType, + }} + _, err := getStepResultsFromSidecarLogs(sidecarLogResults, stepName) + wantErr := fmt.Errorf("invalid string %s-%s : expected somtthing that looks like .", stepName, resultName) + if d := cmp.Diff(wantErr.Error(), err.Error()); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } +} + +func TestGetTaskResultsFromSidecarLogs(t *testing.T) { + sidecarLogResults := []result.RunResult{{ + Key: "step-foo.step-res", + Value: "bar", + ResultType: result.StepResultType, + }, { + Key: "task-res", + Value: "task-bar", + ResultType: result.TaskRunResultType, + }} + want := []result.RunResult{{ + Key: "task-res", + Value: "task-bar", + ResultType: result.TaskRunResultType, + }} + got := getTaskResultsFromSidecarLogs(sidecarLogResults) + if d := cmp.Diff(want, got); d != "" { + t.Errorf(diff.PrintWantGot(d)) + } +}