Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert "InternalTektonResult" from string to int enum #4150

Closed
ghost opened this issue Aug 12, 2021 · 2 comments · Fixed by #4186
Closed

Convert "InternalTektonResult" from string to int enum #4150

ghost opened this issue Aug 12, 2021 · 2 comments · Fixed by #4186
Assignees
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.

Comments

@ghost
Copy link

ghost commented Aug 12, 2021

Every Step that Pipelines runs emits a JSON object with metadata about its execution: the content of task results, a StartedAt timestamp, step exit codes etc etc.

The JSON object is passed back to the Tekton Controller by way of the "Termination Message", a Kubernetes feature that allows a Container to write a short amount of content that ends up back in the Pod's YAML. The emphasis here is on "short" - the termination message is limited to only a few thousand bytes, which in turn means that the JSON object Steps return needs to fit within this.

Several fields in the emitted JSON object are "InternalTektonResults", meaning that they are metadata which the Tekton Controller needs but that doesn't need to be surfaced in typically user-facing fields like Task Results. Each of these "internal-only" fields is given a "type" member in the Termination Message that looks like this:

{"type": "InternalTektonResult"}

Each entry in the JSON object that uses this type consumes, at minimum, 28 bytes of the termination message allotment (6 for "type" and 22 for "InternalTektonResult"). As we add more metadata which needs to be passed around this overhead will grow.

For the longer term we've discussed methods of avoiding the Termination Message altogether. In the short term we could easily curtail this wastage by changing "type" from a string to an int. e.g. {"type": 0}. The int would then map to an enum of possible types (of which there are few). This would drop the overhead of each internal result from 28 bytes to 7.

@ghost ghost added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt. labels Aug 12, 2021
@sm43
Copy link
Member

sm43 commented Aug 21, 2021

/assign

sm43 pushed a commit to sm43/tektoncd-pipeline that referenced this issue Aug 23, 2021
This converts the ResultType from string to int enum to curtail
the size of json object emittted by steps.

Closes tektoncd#4150

Signed-off-by: Shivam Mukhade <[email protected]>
sm43 pushed a commit to sm43/tektoncd-pipeline that referenced this issue Aug 25, 2021
This converts the ResultType in PipelineResourceResult from string
to int enum to curtail the size of json object emittted by steps.

Closes tektoncd#4150

Signed-off-by: Shivam Mukhade <[email protected]>
sm43 pushed a commit to sm43/tektoncd-pipeline that referenced this issue Aug 25, 2021
This converts the ResultType in PipelineResourceResult from string
to int enum to curtail the size of json object emittted by steps.

Closes tektoncd#4150

Signed-off-by: Shivam Mukhade <[email protected]>
@ghost
Copy link
Author

ghost commented Aug 27, 2021

We could drop this even further by switching from "type" to (for example) "_" or "t".

Revisiting this particular idea - I don't think it's possible to achieve a field rename without modifying our swagger or openapi generated files. This complicates the problem a little bit - the termination message json object is not a public-facing api (not intended for external consumption) so it's a bit weird that it even shows up in these places at all.

Anyway, long story short: I don't think changing the "type" field to be named "t" or "_" is a good idea anymore so I'm going to remove that bit from the Issue description. I should have checked what the impact of this would be before suggesting it.

sm43 pushed a commit to sm43/tektoncd-pipeline that referenced this issue Aug 29, 2021
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]>
sm43 pushed a commit to sm43/tektoncd-pipeline that referenced this issue Aug 29, 2021
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 pushed a commit that referenced this issue Sep 15, 2021
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 #4150

Signed-off-by: Shivam Mukhade <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. kind/cleanup Categorizes issue or PR as related to cleaning up code, process, or technical debt.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant