From 885b1af61789d42d43634d063d1b5bdae321a88a Mon Sep 17 00:00:00 2001 From: Alexander Misdorp <6093240+Peaorl@users.noreply.github.com> Date: Tue, 25 Aug 2020 18:59:44 +0200 Subject: [PATCH] Introduce InternalTektonResultType as a ResultType In light of #3087 the need for a ResultType that is not exposed as a TaskRunResult or PipelineResourceResult arises. In #3087, a Step can emit a result indicating a Step timeout has occurred. This is a result that should not be exposed hence the need for a new ResultType called InternalTektonResultType. This commit ensures results of this type are filtered out. Introducing an InternalTektonResultType ensures a future proof solution to internal results that should not be exposed. Aside from the example in #3087, a present candidate is the result written out by a Step containing a "StartedAt" key. Currently this result is filtered out with a specific function. Marking it as an InternalTektonResultTypes now allows for this result to automatically be filtered out. Additionally this commit brings about refactoring (and sometimes renaming) of functions deemed related to taskrun status updates from pkg/reconciler/taskrun/taskrun.go to pkg/pod/status/status.go. This is accompanied with moving test cases for unexported functions in taskrun.go to test cases for exported functions in status.go that now rely on these unexported functions. --- pkg/apis/pipeline/v1beta1/task_types.go | 2 + pkg/entrypoint/entrypointer.go | 10 +- pkg/pod/status.go | 120 +++++--- pkg/pod/status_test.go | 371 ++++++++++++++++++++++-- pkg/reconciler/taskrun/taskrun.go | 62 +--- pkg/reconciler/taskrun/taskrun_test.go | 272 ----------------- 6 files changed, 439 insertions(+), 398 deletions(-) diff --git a/pkg/apis/pipeline/v1beta1/task_types.go b/pkg/apis/pipeline/v1beta1/task_types.go index 34fe2c3bb87..405527f8b32 100644 --- a/pkg/apis/pipeline/v1beta1/task_types.go +++ b/pkg/apis/pipeline/v1beta1/task_types.go @@ -26,6 +26,8 @@ const ( TaskRunResultType ResultType = "TaskRunResult" // PipelineResourceResultType default pipeline result value PipelineResourceResultType ResultType = "PipelineResourceResult" + // InternalTektonResultType default internal tekton result value + InternalTektonResultType ResultType = "InternalTektonResult" // UnknownResultType default unknown result type value UnknownResultType ResultType = "" ) diff --git a/pkg/entrypoint/entrypointer.go b/pkg/entrypoint/entrypointer.go index 188344a8fe7..6c9ed4edbc4 100644 --- a/pkg/entrypoint/entrypointer.go +++ b/pkg/entrypoint/entrypointer.go @@ -102,8 +102,9 @@ func (e Entrypointer) Go() error { // *but* we write postfile to make next steps bail too. e.WritePostFile(e.PostFile, err) output = append(output, v1beta1.PipelineResourceResult{ - Key: "StartedAt", - Value: time.Now().Format(timeFormat), + Key: "StartedAt", + Value: time.Now().Format(timeFormat), + ResultType: v1beta1.InternalTektonResultType, }) return err @@ -114,8 +115,9 @@ func (e Entrypointer) Go() error { e.Args = append([]string{e.Entrypoint}, e.Args...) } output = append(output, v1beta1.PipelineResourceResult{ - Key: "StartedAt", - Value: time.Now().Format(timeFormat), + Key: "StartedAt", + Value: time.Now().Format(timeFormat), + ResultType: v1beta1.InternalTektonResultType, }) err := e.Runner.Run(e.Args...) diff --git a/pkg/pod/status.go b/pkg/pod/status.go index f7d9ad66625..a60aa6efdfd 100644 --- a/pkg/pod/status.go +++ b/pkg/pod/status.go @@ -17,7 +17,6 @@ limitations under the License. package pod import ( - "encoding/json" "fmt" "sort" "strings" @@ -97,27 +96,51 @@ func SidecarsReady(podStatus corev1.PodStatus) bool { } // MakeTaskRunStatus returns a TaskRunStatus based on the Pod's status. -func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod, taskSpec v1beta1.TaskSpec) v1beta1.TaskRunStatus { +func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev1.Pod, taskSpec v1beta1.TaskSpec) (v1beta1.TaskRunStatus, error) { trs := &tr.Status if trs.GetCondition(apis.ConditionSucceeded) == nil || trs.GetCondition(apis.ConditionSucceeded).Status == corev1.ConditionUnknown { // If the taskRunStatus doesn't exist yet, it's because we just started running MarkStatusRunning(trs, v1beta1.TaskRunReasonRunning.String(), "Not all Steps in the Task have finished executing") } + // Complete if we did not find a step that is not complete, or the pod is in a definitely complete phase + complete := areStepsComplete(pod) || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed + + if complete { + updateCompletedTaskRun(trs, pod) + } else { + updateIncompleteTaskRun(trs, pod) + } + trs.PodName = pod.Name trs.Steps = []v1beta1.StepState{} trs.Sidecars = []v1beta1.SidecarState{} + var err error + for _, s := range pod.Status.ContainerStatuses { if IsContainerStep(s.Name) { if s.State.Terminated != nil && len(s.State.Terminated.Message) != 0 { - message, time, err := removeStartInfoFromTerminationMessage(s) + + var results []v1beta1.PipelineResourceResult + results, err = termination.ParseMessage(s.State.Terminated.Message) if err != nil { - logger.Errorf("error setting the start time of step %q in taskrun %q: %w", s.Name, tr.Name, err) - } - if time != nil { - s.State.Terminated.StartedAt = *time - s.State.Terminated.Message = message + logger.Errorf("termination message could not be parsed as JSON: %v", err) + } else { + //Further processing if the termination message is JSON formatted + var time *metav1.Time + time, err = extractStartedAtTimeFromResults(results) + if err != nil { + logger.Errorf("error setting the start time of step %q in taskrun %q: %v", s.Name, tr.Name, err) + } + if time != nil { + s.State.Terminated.StartedAt = *time + } + if tr.IsSuccessful() { + taskResults, pipelineResourceResults := filterResultsAndResources(results) + trs.TaskRunResults = append(trs.TaskRunResults, taskResults...) + trs.ResourcesResult = append(trs.ResourcesResult, pipelineResourceResults...) + } } } trs.Steps = append(trs.Steps, v1beta1.StepState{ @@ -135,51 +158,70 @@ func MakeTaskRunStatus(logger *zap.SugaredLogger, tr v1beta1.TaskRun, pod *corev }) } } - - // Complete if we did not find a step that is not complete, or the pod is in a definitely complete phase - complete := areStepsComplete(pod) || pod.Status.Phase == corev1.PodSucceeded || pod.Status.Phase == corev1.PodFailed - - if complete { - updateCompletedTaskRun(trs, pod) - } else { - updateIncompleteTaskRun(trs, pod) - } + trs.TaskRunResults = removeDuplicateResults(trs.TaskRunResults) // Sort step states according to the order specified in the TaskRun spec's steps. trs.Steps = sortTaskRunStepOrder(trs.Steps, taskSpec.Steps) - return *trs + return *trs, err +} + +func filterResultsAndResources(results []v1beta1.PipelineResourceResult) ([]v1beta1.TaskRunResult, []v1beta1.PipelineResourceResult) { + var taskResults []v1beta1.TaskRunResult + var pipelineResourceResults []v1beta1.PipelineResourceResult + for _, r := range results { + switch r.ResultType { + case v1beta1.TaskRunResultType: + taskRunResult := v1beta1.TaskRunResult{ + Name: r.Key, + Value: r.Value, + } + taskResults = append(taskResults, taskRunResult) + case v1beta1.InternalTektonResultType: + // Internal messages are ignored because they're not used as external result + continue + case v1beta1.PipelineResourceResultType: + fallthrough + default: + pipelineResourceResults = append(pipelineResourceResults, r) + } + } + + return taskResults, pipelineResourceResults } -// removeStartInfoFromTerminationMessage searches for a result called "StartedAt" in the JSON-formatted -// termination message of a step and returns the values to use for sets State.Terminated if it's -// found. The "StartedAt" result is also removed from the list of results in the container status. -func removeStartInfoFromTerminationMessage(s corev1.ContainerStatus) (string, *metav1.Time, error) { - r, err := termination.ParseMessage(s.State.Terminated.Message) - if err != nil { - return "", nil, fmt.Errorf("termination message could not be parsed as JSON: %w", err) +func removeDuplicateResults(taskRunResult []v1beta1.TaskRunResult) []v1beta1.TaskRunResult { + if len(taskRunResult) == 0 { + return nil + } + + uniq := make([]v1beta1.TaskRunResult, 0) + latest := make(map[string]v1beta1.TaskRunResult, 0) + for _, res := range taskRunResult { + if _, seen := latest[res.Name]; !seen { + uniq = append(uniq, res) + } + latest[res.Name] = res + } + for i, res := range uniq { + uniq[i] = latest[res.Name] } - for index, result := range r { + return uniq +} + +func extractStartedAtTimeFromResults(results []v1beta1.PipelineResourceResult) (*metav1.Time, error) { + + for _, result := range results { if result.Key == "StartedAt" { t, err := time.Parse(timeFormat, result.Value) if err != nil { - return "", nil, fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) + return nil, fmt.Errorf("could not parse time value %q in StartedAt field: %w", result.Value, err) } - message := "" startedAt := metav1.NewTime(t) - // remove the entry for the starting time - r = append(r[:index], r[index+1:]...) - if len(r) == 0 { - message = "" - } else if bytes, err := json.Marshal(r); err != nil { - return "", nil, fmt.Errorf("error marshalling remaining results back into termination message: %w", err) - } else { - message = string(bytes) - } - return message, &startedAt, nil + return &startedAt, nil } } - return "", nil, nil + return nil, nil } func updateCompletedTaskRun(trs *v1beta1.TaskRunStatus, pod *corev1.Pod) { diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go index 11a6913315d..87ad6fe7ed4 100644 --- a/pkg/pod/status_test.go +++ b/pkg/pod/status_test.go @@ -33,13 +33,20 @@ import ( var ignoreVolatileTime = cmp.Comparer(func(_, _ apis.VolatileTime) bool { return true }) +var conditionRunning apis.Condition = apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionUnknown, + Reason: v1beta1.TaskRunReasonRunning.String(), + Message: "Not all Steps in the Task have finished executing", +} +var conditionSucceeded apis.Condition = apis.Condition{ + Type: apis.ConditionSucceeded, + Status: corev1.ConditionTrue, + Reason: v1beta1.TaskRunReasonSuccessful.String(), + Message: "All Steps have completed executing", +} + func TestMakeTaskRunStatus(t *testing.T) { - conditionRunning := apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionUnknown, - Reason: v1beta1.TaskRunReasonRunning.String(), - Message: "Not all Steps in the Task have finished executing", - } for _, c := range []struct { desc string podStatus corev1.PodStatus @@ -604,6 +611,335 @@ func TestMakeTaskRunStatus(t *testing.T) { }}, }, }, + }, { + desc: "image resource updated", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-foo", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:12345","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"digest","value":"sha256:12345","resourceRef":{"name":"source-image"}}]`, + }}, + Name: "foo", + ContainerName: "step-foo", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:12345", + ResourceRef: v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test result with pipeline result", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }}, + Name: "bar", + ContainerName: "step-bar", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:1234", + ResourceRef: v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultName", + Value: "resultValue", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "test result with pipeline result - no result type", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-banana", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, + }}, + Name: "banana", + ContainerName: "step-banana", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "digest", + Value: "sha256:1234", + ResourceRef: v1beta1.PipelineResourceRef{Name: "source-image"}, + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultName", + Value: "resultValue", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "two test results", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-one", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + }, + }, + }, { + Name: "step-two", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueThree", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + }}, + Name: "one", + ContainerName: "step-one", + }, { + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"resultValueThree", "type": "TaskRunResult"}, {"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, + }}, + Name: "two", + ContainerName: "step-two", + }}, + Sidecars: []v1beta1.SidecarState{}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultNameOne", + Value: "resultValueThree", + }, { + Name: "resultNameTwo", + Value: "resultValueTwo", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "update task results when task fails", + podStatus: corev1.PodStatus{ + Phase: corev1.PodFailed, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-mango", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"name":"source-image","digest":"sha256:1234"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{Conditions: []apis.Condition{{ + Reason: "Failed", + Message: "build failed for unspecified reasons.", + Type: apis.ConditionSucceeded, + Status: corev1.ConditionFalse, + }}}, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"name":"source-image","digest":"sha256:1234"}]`, + }}, + Name: "mango", + ContainerName: "step-mango", + }}, + Sidecars: []v1beta1.SidecarState{}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }, { + desc: "filter internaltektonresult", + podStatus: corev1.PodStatus{ + Phase: corev1.PodSucceeded, + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-pear", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"", "type": "PipelineResourceResult"}, {"key":"resultNameTwo","value":"", "type": "InternalTektonResult"}, {"key":"resultNameThree","value":"", "type": "TaskRunResult"}]`, + }, + }, + }}, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"resultNameOne","value":"", "type": "PipelineResourceResult"}, {"key":"resultNameTwo","value":"", "type": "InternalTektonResult"}, {"key":"resultNameThree","value":"", "type": "TaskRunResult"}]`, + }}, + Name: "pear", + ContainerName: "step-pear", + }}, + Sidecars: []v1beta1.SidecarState{}, + ResourcesResult: []v1beta1.PipelineResourceResult{{ + Key: "resultNameOne", + Value: "", + ResultType: "PipelineResourceResult", + }}, + TaskRunResults: []v1beta1.TaskRunResult{{ + Name: "resultNameThree", + Value: "", + }}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, + }} { + t.Run(c.desc, func(t *testing.T) { + now := metav1.Now() + pod := &corev1.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pod", + Namespace: "foo", + CreationTimestamp: now, + }, + Status: c.podStatus, + } + startTime := time.Date(2010, 1, 1, 1, 1, 1, 1, time.UTC) + tr := v1beta1.TaskRun{ + ObjectMeta: metav1.ObjectMeta{ + Name: "task-run", + Namespace: "foo", + }, + Status: v1beta1.TaskRunStatus{ + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + StartTime: &metav1.Time{Time: startTime}, + }, + }, + } + + logger, _ := logging.NewLogger("", "status") + got, err := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) + + // Common traits, set for test case brevity. + c.want.PodName = "pod" + c.want.StartTime = &metav1.Time{Time: startTime} + + ensureTimeNotNil := cmp.Comparer(func(x, y *metav1.Time) bool { + if x == nil { + return y == nil + } + return y != nil + }) + if err != nil { + t.Errorf("MakeTaskRunResult: %s", err) + } + if d := cmp.Diff(c.want, got, ignoreVolatileTime, ensureTimeNotNil); d != "" { + t.Errorf("Diff %s", diff.PrintWantGot(d)) + } + if tr.Status.StartTime.Time != c.want.StartTime.Time { + t.Errorf("Expected TaskRun startTime to be unchanged but was %s", tr.Status.StartTime) + } + }) + } +} + +func TestMakeRunStatusErrors(t *testing.T) { + for _, c := range []struct { + desc string + podStatus corev1.PodStatus + taskSpec v1beta1.TaskSpec + want v1beta1.TaskRunStatus + }{{ + desc: "malformed StartedAt time", + podStatus: corev1.PodStatus{ + ContainerStatuses: []corev1.ContainerStatus{{ + Name: "step-bar", + State: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"StartedAt", "value":"invalid", "type":"InternalTektonResult"}]`, + }, + }, + }}, + Phase: corev1.PodSucceeded, + }, + want: v1beta1.TaskRunStatus{ + Status: duckv1beta1.Status{ + Conditions: []apis.Condition{conditionSucceeded}, + }, + TaskRunStatusFields: v1beta1.TaskRunStatusFields{ + Steps: []v1beta1.StepState{{ + ContainerState: corev1.ContainerState{ + Terminated: &corev1.ContainerStateTerminated{ + Message: `[{"key":"StartedAt", "value":"invalid", "type":"InternalTektonResult"}]`, + }}, + Name: "bar", + ContainerName: "step-bar", + }}, + Sidecars: []v1beta1.SidecarState{}, + // We don't actually care about the time, just that it's not nil + CompletionTime: &metav1.Time{Time: time.Now()}, + }, + }, }, { desc: "non-json-termination-message-with-steps-afterwards-shouldnt-panic", taskSpec: v1beta1.TaskSpec{ @@ -664,7 +1000,6 @@ func TestMakeTaskRunStatus(t *testing.T) { ExitCode: 1, Message: "this is a non-json termination message. dont panic!", }}, - Name: "non-json", ContainerName: "step-non-json", ImageID: "image", @@ -694,34 +1029,24 @@ func TestMakeTaskRunStatus(t *testing.T) { }, }} { t.Run(c.desc, func(t *testing.T) { - now := metav1.Now() pod := &corev1.Pod{ ObjectMeta: metav1.ObjectMeta{ - Name: "pod", - Namespace: "foo", - CreationTimestamp: now, + Name: "pod", + Namespace: "foo", }, Status: c.podStatus, } - startTime := time.Date(2010, 1, 1, 1, 1, 1, 1, time.UTC) tr := v1beta1.TaskRun{ ObjectMeta: metav1.ObjectMeta{ Name: "task-run", Namespace: "foo", }, - Status: v1beta1.TaskRunStatus{ - TaskRunStatusFields: v1beta1.TaskRunStatusFields{ - StartTime: &metav1.Time{Time: startTime}, - }, - }, } logger, _ := logging.NewLogger("", "status") - got := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) + got, err := MakeTaskRunStatus(logger, tr, pod, c.taskSpec) - // Common traits, set for test case brevity. c.want.PodName = "pod" - c.want.StartTime = &metav1.Time{Time: startTime} ensureTimeNotNil := cmp.Comparer(func(x, y *metav1.Time) bool { if x == nil { @@ -729,12 +1054,12 @@ func TestMakeTaskRunStatus(t *testing.T) { } return y != nil }) + if err == nil { + t.Error("Expected error, got nil") + } if d := cmp.Diff(c.want, got, ignoreVolatileTime, ensureTimeNotNil); d != "" { t.Errorf("Diff %s", diff.PrintWantGot(d)) } - if tr.Status.StartTime.Time != c.want.StartTime.Time { - t.Errorf("Expected TaskRun startTime to be unchanged but was %s", tr.Status.StartTime) - } }) } } diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go index c5b12318a96..67d25816469 100644 --- a/pkg/reconciler/taskrun/taskrun.go +++ b/pkg/reconciler/taskrun/taskrun.go @@ -41,7 +41,6 @@ import ( "github.com/tektoncd/pipeline/pkg/reconciler/events/cloudevent" "github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources" "github.com/tektoncd/pipeline/pkg/reconciler/volumeclaim" - "github.com/tektoncd/pipeline/pkg/termination" "github.com/tektoncd/pipeline/pkg/timeout" "github.com/tektoncd/pipeline/pkg/workspace" corev1 "k8s.io/api/core/v1" @@ -397,9 +396,8 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, } // Convert the Pod's status to the equivalent TaskRun Status. - tr.Status = podconvert.MakeTaskRunStatus(logger, *tr, pod, *taskSpec) - - if err := updateTaskRunResourceResult(tr, *pod); err != nil { + tr.Status, err = podconvert.MakeTaskRunStatus(logger, *tr, pod, *taskSpec) + if err != nil { return err } @@ -601,62 +599,6 @@ func (c *Reconciler) createPod(ctx context.Context, tr *v1beta1.TaskRun, rtr *re type DeletePod func(podName string, options *metav1.DeleteOptions) error -func updateTaskRunResourceResult(taskRun *v1beta1.TaskRun, pod corev1.Pod) error { - podconvert.SortContainerStatuses(&pod) - - if taskRun.IsSuccessful() { - for idx, cs := range pod.Status.ContainerStatuses { - if cs.State.Terminated != nil { - msg := cs.State.Terminated.Message - r, err := termination.ParseMessage(msg) - if err != nil { - return fmt.Errorf("parsing message for container status %d: %v", idx, err) - } - taskResults, pipelineResourceResults := getResults(r) - taskRun.Status.TaskRunResults = append(taskRun.Status.TaskRunResults, taskResults...) - taskRun.Status.ResourcesResult = append(taskRun.Status.ResourcesResult, pipelineResourceResults...) - } - } - taskRun.Status.TaskRunResults = removeDuplicateResults(taskRun.Status.TaskRunResults) - } - return nil -} - -func getResults(results []v1beta1.PipelineResourceResult) ([]v1beta1.TaskRunResult, []v1beta1.PipelineResourceResult) { - var taskResults []v1beta1.TaskRunResult - var pipelineResourceResults []v1beta1.PipelineResourceResult - for _, r := range results { - switch r.ResultType { - case v1beta1.TaskRunResultType: - taskRunResult := v1beta1.TaskRunResult{ - Name: r.Key, - Value: r.Value, - } - taskResults = append(taskResults, taskRunResult) - case v1beta1.PipelineResourceResultType: - fallthrough - default: - pipelineResourceResults = append(pipelineResourceResults, r) - } - } - return taskResults, pipelineResourceResults -} - -func removeDuplicateResults(taskRunResult []v1beta1.TaskRunResult) []v1beta1.TaskRunResult { - uniq := make([]v1beta1.TaskRunResult, 0) - latest := make(map[string]v1beta1.TaskRunResult, 0) - for _, res := range taskRunResult { - if _, seen := latest[res.Name]; !seen { - uniq = append(uniq, res) - } - latest[res.Name] = res - } - for i, res := range uniq { - uniq[i] = latest[res.Name] - } - return uniq -} - func isExceededResourceQuotaError(err error) bool { return err != nil && k8serrors.IsForbidden(err) && strings.Contains(err.Error(), "exceeded quota") } diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go index 7ad27d2f3f3..6f1f1ddeddd 100644 --- a/pkg/reconciler/taskrun/taskrun_test.go +++ b/pkg/reconciler/taskrun/taskrun_test.go @@ -53,7 +53,6 @@ import ( ktesting "k8s.io/client-go/testing" "k8s.io/client-go/tools/record" "knative.dev/pkg/apis" - duckv1beta1 "knative.dev/pkg/apis/duck/v1beta1" "knative.dev/pkg/configmap" "knative.dev/pkg/controller" "knative.dev/pkg/logging" @@ -2318,277 +2317,6 @@ func TestReconcileCloudEvents(t *testing.T) { } } -func TestUpdateTaskRunResourceResult(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "image resource updated", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, - }, - }, - }}, - }, - }, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResult(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "test result with pipeline result", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}, "type": "PipelineResourceResult"}]`, - }, - }, - }}, - }, - }, - wantResults: []v1beta1.TaskRunResult{{ - Name: "resultName", - Value: "resultValue", - }}, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - ResultType: "PipelineResourceResult", - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.wantResults, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult TaskRunResults %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult ResourcesResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResult2(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "test result with pipeline result - no result type", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`, - }, - }, - }}, - }, - }, - wantResults: []v1beta1.TaskRunResult{{ - Name: "resultName", - Value: "resultValue", - }}, - want: []resourcev1alpha1.PipelineResourceResult{{ - Key: "digest", - Value: "sha256:1234", - ResourceRef: resourcev1alpha1.PipelineResourceRef{Name: "source-image"}, - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.wantResults, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.want, tr.Status.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResultTwoResults(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []v1beta1.TaskRunResult - }{{ - desc: "two test results", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueOne", "type": "TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, - }, - }, - }, { - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultNameOne","value":"resultValueThree", "type": "TaskRunResult"},{"key":"resultNameTwo","value":"resultValueTwo", "type": "TaskRunResult"}]`, - }, - }, - }}, - }, - }, - want: []v1beta1.TaskRunResult{{ - Name: "resultNameOne", - Value: "resultValueThree", - }, { - Name: "resultNameTwo", - Value: "resultValueTwo", - }}, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - tr := &v1beta1.TaskRun{} - tr.Status.SetCondition(&apis.Condition{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }) - if err := updateTaskRunResourceResult(tr, c.pod); err != nil { - t.Errorf("updateTaskRunResourceResult: %s", err) - } - if d := cmp.Diff(c.want, tr.Status.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResultWhenTaskFailed(t *testing.T) { - for _, c := range []struct { - desc string - podStatus corev1.PodStatus - taskRunStatus *v1beta1.TaskRunStatus - wantResults []v1beta1.TaskRunResult - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "update task results when task fails", - podStatus: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `[{"key":"resultName","value":"resultValue", "type": "TaskRunResult"}, {"name":"source-image","digest":"sha256:1234"}]`, - }, - }, - }}, - }, - taskRunStatus: &v1beta1.TaskRunStatus{ - Status: duckv1beta1.Status{Conditions: []apis.Condition{{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionFalse, - }}}, - }, - wantResults: nil, - want: nil, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - if d := cmp.Diff(c.want, c.taskRunStatus.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult resources %s", diff.PrintWantGot(d)) - } - if d := cmp.Diff(c.wantResults, c.taskRunStatus.TaskRunResults); d != "" { - t.Errorf("updateTaskRunResourceResult results %s", diff.PrintWantGot(d)) - } - }) - } -} - -func TestUpdateTaskRunResourceResult_Errors(t *testing.T) { - for _, c := range []struct { - desc string - pod corev1.Pod - taskRunStatus *v1beta1.TaskRunStatus - want []resourcev1alpha1.PipelineResourceResult - }{{ - desc: "image resource exporter with malformed json output", - pod: corev1.Pod{ - Status: corev1.PodStatus{ - ContainerStatuses: []corev1.ContainerStatus{{ - State: corev1.ContainerState{ - Terminated: &corev1.ContainerStateTerminated{ - Message: `MALFORMED JSON{"digest":"sha256:1234"}`, - }, - }, - }}, - }, - }, - taskRunStatus: &v1beta1.TaskRunStatus{ - Status: duckv1beta1.Status{Conditions: []apis.Condition{{ - Type: apis.ConditionSucceeded, - Status: corev1.ConditionTrue, - }}}, - }, - want: nil, - }} { - t.Run(c.desc, func(t *testing.T) { - names.TestingSeed() - if err := updateTaskRunResourceResult(&v1beta1.TaskRun{Status: *c.taskRunStatus}, c.pod); err == nil { - t.Error("Expected error, got nil") - } - if d := cmp.Diff(c.want, c.taskRunStatus.ResourcesResult); d != "" { - t.Errorf("updateTaskRunResourceResult %s", diff.PrintWantGot(d)) - } - }) - } -} - func TestReconcile_Single_SidecarState(t *testing.T) { runningState := corev1.ContainerStateRunning{StartedAt: metav1.Time{Time: time.Now()}} taskRun := tb.TaskRun("test-taskrun-sidecars",