Converts ResultType from string to int enum #4186

sm43 · 2021-08-23T18:33:29Z

This converts the ResultType in PipelineResourceResult from string
to int enum to curtail the size of json object emittted by steps.
ResultTypes are converted because they made JSON messages bigger, which in
turn limited the amount of space in termination messages for task results. String
support is maintained for backwards compatibility.

Closes #4150

Signed-off-by: Shivam Mukhade [email protected]

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in or deleted (only if no user facing changes)

Release Notes

ResultType in JSON blob of termination message is changed from String to Int. So, for ex previouly we used to have "type": "TaskRunResultType" in the terminaton message which now would appear as "type": 1 to curtail the size of json.

sm43 · 2021-08-23T18:34:19Z

@sbwsg Could you take a look and let me know if I am right track? 😅

ghost · 2021-08-23T19:07:55Z

One potential risk here is if a controller is upgraded while a TaskRun is mid-execution. If that would happen then the taskrun's pod could return "TaskRunResult" but the controller could be expecting 1. Is there a way we could work around this by implementing the Unmarshaller interface for ResultType? So if our custom UnmarshalJSON() detects a string then we convert it to its equivalent int?

ghost

Looks good to me so far!

pkg/apis/pipeline/v1beta1/task_types.go

sm43 · 2021-08-25T19:05:15Z

/kind cleanup

tekton-robot · 2021-08-25T19:05:27Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/resource_types.go	89.3%	84.1%	-5.2

tekton-robot · 2021-08-25T19:21:29Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/resource_types.go	89.3%	84.1%	-5.2

sm43 · 2021-08-25T19:34:01Z

@sbwsg updated the pr, ptal :)

sm43 · 2021-08-26T03:09:53Z

/retest

ghost · 2021-08-26T16:19:22Z

Hey @sm43 thanks a lot for this!

I think we might be better off removing the field name change ("type" -> "t") and only changing the value's type. The reason for that is then we wouldn't need the new pipelineResourceResult type which now appears in our openapi generated spec. Here's how this would look:

// PipelineResourceResult used to export the image name and digest as json
type PipelineResourceResult struct {
        // ... 
        ResultType  ResultType           `json:"type,omitempty"`
}

// ResultType used to find out whether a PipelineResourceResult is from a task result or not
type ResultType int

// UnmarshalJSON unmarshals either an int or a string into a ResultType. String
// ResultTypes were removed because they made JSON messages bigger, which in
// turn limited the amount of space in termination messages for task results. String
// support is maintained for backwards compatibility - the Pipelines controller could
// be stopped midway through TaskRun execution, updated with support for int in place
// of string, and then fail the running TaskRun because it doesn't know how to interpret
// the string value that the TaskRun's entrypoint will emit when it completes.
func (r *ResultType) UnmarshalJSON(data []byte) error {
        var asInt int
        var intErr error

        if err := json.Unmarshal(data, &asInt); err != nil {
                intErr = err
        } else {
                *r = ResultType(asInt)
                return nil
        }

        var asString string

        if err := json.Unmarshal(data, &asString); err != nil {
                return fmt.Errorf("unsupported value type, neither int nor string: %v", multierror.Append(intErr, err).ErrorOrNil())
        }

        switch asString {
        case "TaskRunResult":
                *r = TaskRunResultType
        case "PipelineResourceResult":
                *r = PipelineResourceResultType
        case "InternalTektonResult":
                *r = InternalTektonResultType
        default:
                *r = UnknownResultType
        }

        return nil
}

We still lose a few bytes because "type" remains but I think it's better not to introduce any new intermediary types to our openapi or swagger. What do you think?

I tested this using the unit tests you wrote, here's the diff for those:

diff --git a/pkg/apis/pipeline/v1beta1/resource_types_test.go b/pkg/apis/pipeline/v1beta1/resource_types_test.go
index 6cafbbe7d..3108c2ce6 100644
--- a/pkg/apis/pipeline/v1beta1/resource_types_test.go
+++ b/pkg/apis/pipeline/v1beta1/resource_types_test.go
@@ -15,7 +15,6 @@ package v1beta1_test

 import (
        "encoding/json"
-       "reflect"
        "testing"

        "github.com/google/go-cmp/cmp"
@@ -147,7 +146,6 @@ func TestApplyTaskModifier_AlreadyAdded(t *testing.T) {
 }

 func TestPipelineResourceResult_UnmarshalJSON(t *testing.T) {
-
        testcases := []struct {
                name string
                data string
@@ -163,7 +161,7 @@ func TestPipelineResourceResult_UnmarshalJSON(t *testing.T) {
                        pr:   v1beta1.PipelineResourceResult{Key: "resultName", Value: "", ResultType: v1beta1.InternalTektonResultType},
                }, {
                        name: "already defined as int",
-                       data: "{\"key\":\"resultName\",\"value\":\"\", \"t\": 1}",
+                       data: "{\"key\":\"resultName\",\"value\":\"\", \"type\": 1}",
                        pr:   v1beta1.PipelineResourceResult{Key: "resultName", Value: "", ResultType: v1beta1.TaskRunResultType},
                }}

@@ -173,8 +171,8 @@ func TestPipelineResourceResult_UnmarshalJSON(t *testing.T) {
                        if err := json.Unmarshal([]byte(tc.data), &pipRes); err != nil {
                                t.Errorf("Unexpected error when unmarshalling the json into PipelineResourceResult")
                        }
-                       if !reflect.DeepEqual(pipRes, tc.pr) {
-                               t.Errorf("Unexpected error: both objects are different")
+                       if d := cmp.Diff(tc.pr, pipRes); d != "" {
+                               t.Errorf(diff.PrintWantGot(d))
                        }
                })
        }

and

diff --git a/pkg/pod/status_test.go b/pkg/pod/status_test.go
index c451608a9..d18aa823a 100644
--- a/pkg/pod/status_test.go
+++ b/pkg/pod/status_test.go
@@ -549,7 +549,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Name: "step-bar",
 				State: corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
-						Message: `[{"key":"resultName","value":"resultValue", "t":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
+						Message: `[{"key":"resultName","value":"resultValue", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
 					},
 				},
 			}},
@@ -560,7 +560,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Steps: []v1beta1.StepState{{
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","t":1}]`,
+							Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":1}]`,
 						}},
 					Name:          "bar",
 					ContainerName: "step-bar",
@@ -587,7 +587,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Name: "step-banana",
 				State: corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
-						Message: `[{"key":"resultName","value":"resultValue", "t":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
+						Message: `[{"key":"resultName","value":"resultValue", "type":1}, {"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}}]`,
 					},
 				},
 			}},
@@ -598,7 +598,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Steps: []v1beta1.StepState{{
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","t":1}]`,
+							Message: `[{"key":"digest","value":"sha256:1234","resourceRef":{"name":"source-image"}},{"key":"resultName","value":"resultValue","type":1}]`,
 						}},
 					Name:          "banana",
 					ContainerName: "step-banana",
@@ -625,14 +625,14 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Name: "step-one",
 				State: corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
-						Message: `[{"key":"resultNameOne","value":"resultValueOne", "t":1}, {"key":"resultNameTwo","value":"resultValueTwo", "t":1}]`,
+						Message: `[{"key":"resultNameOne","value":"resultValueOne", "type":1}, {"key":"resultNameTwo","value":"resultValueTwo", "type":1}]`,
 					},
 				},
 			}, {
 				Name: "step-two",
 				State: corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
-						Message: `[{"key":"resultNameOne","value":"resultValueThree","t":1},{"key":"resultNameTwo","value":"resultValueTwo","t":1}]`,
+						Message: `[{"key":"resultNameOne","value":"resultValueThree","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`,
 					},
 				},
 			}},
@@ -643,14 +643,14 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Steps: []v1beta1.StepState{{
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"resultNameOne","value":"resultValueOne","t":1},{"key":"resultNameTwo","value":"resultValueTwo","t":1}]`,
+							Message: `[{"key":"resultNameOne","value":"resultValueOne","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`,
 						}},
 					Name:          "one",
 					ContainerName: "step-one",
 				}, {
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"resultNameOne","value":"resultValueThree","t":1},{"key":"resultNameTwo","value":"resultValueTwo","t":1}]`,
+							Message: `[{"key":"resultNameOne","value":"resultValueThree","type":1},{"key":"resultNameTwo","value":"resultValueTwo","type":1}]`,
 						}},
 					Name:          "two",
 					ContainerName: "step-two",
@@ -727,7 +727,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Name: "step-pear",
 				State: corev1.ContainerState{
 					Terminated: &corev1.ContainerStateTerminated{
-						Message: `[{"key":"resultNameOne","value":"","t":2}, {"key":"resultNameTwo","value":"","t":3}, {"key":"resultNameThree","value":"","t":1}]`},
+						Message: `[{"key":"resultNameOne","value":"","type":2}, {"key":"resultNameTwo","value":"","type":3}, {"key":"resultNameThree","value":"","type":1}]`},
 				},
 			}},
 		},
@@ -737,7 +737,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Steps: []v1beta1.StepState{{
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"resultNameOne","value":"","t":2},{"key":"resultNameThree","value":"","t":1}]`,
+							Message: `[{"key":"resultNameOne","value":"","type":2},{"key":"resultNameThree","value":"","type":1}]`,
 						}},
 					Name:          "pear",
 					ContainerName: "step-pear",
@@ -775,7 +775,7 @@ func TestMakeTaskRunStatus(t *testing.T) {
 				Steps: []v1beta1.StepState{{
 					ContainerState: corev1.ContainerState{
 						Terminated: &corev1.ContainerStateTerminated{
-							Message: `[{"key":"resultNameOne","value":"","t":2},{"key":"resultNameThree","value":"","t":1}]`,
+							Message: `[{"key":"resultNameOne","value":"","type":2},{"key":"resultNameThree","value":"","type":1}]`,
 						}},
 					Name:          "pear",
 					ContainerName: "step-pear",

ghost · 2021-08-26T16:21:52Z

pkg/apis/pipeline/v1beta1/resource_types_test.go

+			if err := json.Unmarshal([]byte(tc.data), &pipRes); err != nil {
+				t.Errorf("Unexpected error when unmarshalling the json into PipelineResourceResult")
+			}
+			if !reflect.DeepEqual(pipRes, tc.pr) {


Suggest using cmp.Diff here so that devs can see which fields are incorrect. Here's example usage:

if d := cmp.Diff(tc.pr, pipRes); d != "" { t.Errorf(diff.PrintWantGot(d)) }

ScrapCodes · 2021-08-27T09:51:27Z

pkg/apis/pipeline/v1beta1/task_types.go

 	// UnknownResultType default unknown result type value
-	UnknownResultType ResultType = ""
+	UnknownResultType = 4


Q: Can we set "UnknownResultType" to a higher value, e.g. 10 or 100? So that, in future more types can be added.

tekton-robot · 2021-08-29T16:19:00Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/resource_types.go	89.3%	86.0%	-3.2

This converts the ResultType in PipelineResourceResult from string to int enum to curtail the size of json object emittted by steps. ResultTypes were removed because they made JSON messages bigger, which in turn limited the amount of space in termination messages for task results. String support is maintained for backwards compatibility. Closes tektoncd#4150 Signed-off-by: Shivam Mukhade <[email protected]>

tekton-robot · 2021-08-29T16:22:58Z

The following is the coverage report on the affected files.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/apis/pipeline/v1beta1/resource_types.go	89.3%	86.0%	-3.2

sm43 · 2021-08-29T16:40:17Z

@sbwsg yeah that make sense to not change type tot. Thank you adding the code snippet.
Updated the pr with changes.
@ScrapCodes Updated UnknownResultType to have value 10 let me know if that is good enough
ptal :)

ghost

Fantastic, thanks a lot @sm43 !

ghost · 2021-08-31T14:44:56Z

pkg/apis/pipeline/v1beta1/task_types.go

 	// UnknownResultType default unknown result type value
-	UnknownResultType ResultType = ""
+	UnknownResultType = 10


tekton-robot · 2021-08-31T14:47:29Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbwsg

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbwsg]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

vdemeester

A very naive question, isn't this kind-of breaking the API ? Even though pipeline doesn't consume it, there might be tools that are consuming this and used to see strings (like TaskRunResult) and will see an int now, isn't it ?

ghost · 2021-09-13T13:13:57Z

@vdemeester the way we pass messages from Tasks to the controller is an internal implementation detail isn't it?

vdemeester · 2021-09-13T14:26:03Z

@vdemeester the way we pass messages from Tasks to the controller is an internal implementation detail isn't it?

Well, isn't this "exposed" as part of the status field of a TaskRun or PipelineRun ? If yes, then this means a user that was looking at those (for whatever reason) will have a "breakage" as the type changed (using to be a string, and is a number now). This is, I think, also affecting the tkn cli 🙃 (cc @piyush-garg) as we display results.

ghost · 2021-09-13T14:34:20Z

It's exposed at the following location in taskruns:

status:
  steps:
  - container: step-print-date-human-readable
    terminated:
      message: '[{"key":"current-date-human-readable","value":"Mon Sep 13 14:28:59
        UTC 2021\n","type":"TaskRunResult"},{"key":"current-date-unix-timestamp","value":"1631543338\n","type":"TaskRunResult"}]'

To be super explicit: we're talking about the stringified JSON blob in the termination message field. The "type" field in that stringified JSON blob will change from a string ("TaskRunResult") to an int.

You're saying that consumers of our API are parsing the stringified JSON blob for this data rather than looking in the status.taskResults list for the value?

vdemeester · 2021-09-13T15:03:39Z

You're saying that consumers of our API are parsing the stringified JSON blob for this data rather than looking in the status.taskResults list for the value?

They might but yes, if it's only exposed there, then I think we are safe 😛

pritidesai · 2021-09-14T04:16:26Z

I kind of agree with @vdemeester, whoever is looking at the terminated JSON blob will have a message with int instead of TaskRunResult. But I don't think it was designed to consume it in that way anyways so I am all good with these changes.

I would like to see the changes in the release notes to be on the safer side. @sm43 will you be able to please add the release notes with the status JSON blob change.

afrittoli

Thanks for this!
I agree with @pritidesai, we need to call this out in the release notes of this PR and of next release. We might want to document that the termination message is not meant as a stable API, even if it's exposed within the status.
Once the release notes are updated this lgtm.

ghost · 2021-09-14T14:10:53Z

Nice idea @afrittoli I'll make a PR to document the lack of guarantees around the termination message.

ghost · 2021-09-14T14:47:29Z

Created #4226 to capture termination message note.

sm43 · 2021-09-15T05:06:52Z

@sbwsg @pritidesai added release notes, ptal if it looks okay.

pritidesai · 2021-09-15T16:40:30Z

thanks @sm43

/lgtm

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Aug 23, 2021

tekton-robot requested review from imjasonh and a user August 23, 2021 18:33

tekton-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Aug 23, 2021

ghost reviewed Aug 23, 2021

View reviewed changes

pkg/apis/pipeline/v1beta1/task_types.go Outdated Show resolved Hide resolved

sm43 force-pushed the result-type-to-int-enum branch from cca4451 to 15fe177 Compare August 25, 2021 19:03

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 25, 2021

tekton-robot added the kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. label Aug 25, 2021

sm43 force-pushed the result-type-to-int-enum branch from 15fe177 to e38dd57 Compare August 25, 2021 19:19

sm43 requested a review from a user August 25, 2021 19:19

ghost reviewed Aug 26, 2021

View reviewed changes

ScrapCodes reviewed Aug 27, 2021

View reviewed changes

sm43 force-pushed the result-type-to-int-enum branch from e38dd57 to 14818ab Compare August 29, 2021 16:17

sm43 force-pushed the result-type-to-int-enum branch from 14818ab to d21e087 Compare August 29, 2021 16:20

ghost approved these changes Aug 31, 2021

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2021

ghost added this to the Pipelines v0.28 milestone Aug 31, 2021

pritidesai mentioned this pull request Sep 1, 2021

Changing the way Result Parameters are stored. #4012

Open

vdemeester reviewed Sep 9, 2021

View reviewed changes

afrittoli reviewed Sep 14, 2021

View reviewed changes

tekton-robot assigned pritidesai Sep 15, 2021

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 15, 2021

tekton-robot merged commit db62a61 into tektoncd:main Sep 15, 2021

sm43 deleted the result-type-to-int-enum branch September 15, 2021 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Converts ResultType from string to int enum #4186

Converts ResultType from string to int enum #4186

sm43 commented Aug 23, 2021 •

edited

Loading

sm43 commented Aug 23, 2021

ghost commented Aug 23, 2021

ghost left a comment

sm43 commented Aug 25, 2021

tekton-robot commented Aug 25, 2021

tekton-robot commented Aug 25, 2021

sm43 commented Aug 25, 2021

sm43 commented Aug 26, 2021

ghost commented Aug 26, 2021 •

edited by ghost

Loading

ghost Aug 26, 2021

ScrapCodes Aug 27, 2021

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

sm43 commented Aug 29, 2021

ghost left a comment

ghost Aug 31, 2021

tekton-robot commented Aug 31, 2021

vdemeester left a comment

ghost commented Sep 13, 2021

vdemeester commented Sep 13, 2021

ghost commented Sep 13, 2021

vdemeester commented Sep 13, 2021

pritidesai commented Sep 14, 2021

afrittoli left a comment

ghost commented Sep 14, 2021

ghost commented Sep 14, 2021

sm43 commented Sep 15, 2021

pritidesai commented Sep 15, 2021

Converts ResultType from string to int enum #4186

Converts ResultType from string to int enum #4186

Conversation

sm43 commented Aug 23, 2021 • edited Loading

Submitter Checklist

Release Notes

sm43 commented Aug 23, 2021

ghost commented Aug 23, 2021

ghost left a comment

Choose a reason for hiding this comment

sm43 commented Aug 25, 2021

tekton-robot commented Aug 25, 2021

tekton-robot commented Aug 25, 2021

sm43 commented Aug 25, 2021

sm43 commented Aug 26, 2021

ghost commented Aug 26, 2021 • edited by ghost Loading

ghost Aug 26, 2021

Choose a reason for hiding this comment

ScrapCodes Aug 27, 2021

Choose a reason for hiding this comment

tekton-robot commented Aug 29, 2021

tekton-robot commented Aug 29, 2021

sm43 commented Aug 29, 2021

ghost left a comment

Choose a reason for hiding this comment

ghost Aug 31, 2021

Choose a reason for hiding this comment

tekton-robot commented Aug 31, 2021

vdemeester left a comment

Choose a reason for hiding this comment

ghost commented Sep 13, 2021

vdemeester commented Sep 13, 2021

ghost commented Sep 13, 2021

vdemeester commented Sep 13, 2021

pritidesai commented Sep 14, 2021

afrittoli left a comment

Choose a reason for hiding this comment

ghost commented Sep 14, 2021

ghost commented Sep 14, 2021

sm43 commented Sep 15, 2021

pritidesai commented Sep 15, 2021

sm43 commented Aug 23, 2021 •

edited

Loading

ghost commented Aug 26, 2021 •

edited by ghost

Loading