Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

ghost · 2021-03-25T15:17:28Z

Changes

I've added everyone that I've spoken to about this as reviewers on the PR. Sorry
for the spam but I want to widely publicise it before moving ahead.

Prior to this commit our conversion of pipelines from v1beta1 to v1alpha1
did not account for fields that were solely present in the v1beta1
pipeline spec. This meant, for example, that WhenExpressions in PipelineTasks
were quietly dropped when moving from v1beta1 to v1alpha1. If a user
subsequently re-applied that v1alpha1 resource to the cluster then
the resource would be upgraded back to v1beta1 for storage without
those when expressions.

Why would this down-conversion happen? Users with older clients that
don't know about v1beta1 yet will still be requesting resources using the v1alpha1
apiVersion.

Over time this problem would only be exacerbated - any new features added to the
v1beta1 spec would be lost on conversion unless we ensured that all new fields were
also added to the v1alpha1 spec (or accounted for in the conversion webhook).

So, to account for all the fields that might be in v1beta1 but not in v1alpha1 this commit
serializes the entire v1beta1 pipeline spec into an annotation
on Pipelines and PipelineRuns (when the nested pipelineSpec field is present). This will
protect the entire set of beta-only fields from being lost in translation through this conversion
process. This annotation only exists in the YAML that is returned to the user. This annotation never
gets stored in etcd (unless I'm missing something!)

Unfortunately this approach does also have drawbacks: if a user makes changes
to the v1alpha1 YAML and then re-applies it then those changes
will be lost, overwritten by the contents of the annotation. I'm not sure if
there's a great way to determine, for all fields, how to successfully "merge"
the beta annotation with the contents of the modified v1alpha1 spec when
up-converting. There's no clear signal, as far as I can tell, that any given field has or
has not been modified (beyond performing a cmp.Diff? Would that be an acceptable option?)

/hold

Submitter Checklist

Includes tests (if functionality changed/added)
Commit messages follow commit message best practices
Release notes block has been filled in or deleted (only if no user facing changes)

Release Notes

Users requesting the v1alpha1 version of Pipelines or PipelineRuns with nested PipelineSpec will now find an entire copy of the v1beta1 PipelineSpec stored as an annotation in the returned data. This is to work around a bug where fields that are beta-specific could be lost if the v1alpha1 document is re-applied to the cluster. No further action is required but you may notice larger-than-expected documents (approx. 2x larger) being returned if your clients are still requesting v1alpha1 Pipelines or PipelineRuns.

It's strongly recommended to upgrade your clients to newer versions that support v1beta1 Tasks and Pipelines.

ghost · 2021-03-25T15:18:01Z

This should probably get some more thorough testing as well if we decide to move forward with it.

tekton-robot · 2021-03-25T15:19:26Z

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/v1alpha1/pipeline_conversion.go	91.6%	83.5%	-8.1
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

ghost · 2021-03-31T17:41:15Z

Writing a test for this exposed another field we're silently dropping on conversion: Pipeline's spec.results are not preserved on round-trips.

I'm updating this branch to fix that too.

tekton-robot · 2021-03-31T17:57:51Z

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/v1alpha1/pipeline_conversion.go	91.6%	83.8%	-7.7
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

tekton-robot · 2021-03-31T18:20:45Z

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/v1alpha1/pipeline_conversion.go	91.6%	83.8%	-7.7
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

tekton-robot · 2021-03-31T18:32: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/v1alpha1/pipeline_conversion.go	91.6%	90.9%	-0.7
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

ghost · 2021-03-31T18:35:26Z

Remaining TODOs:

Write table test for error cases in pkg/apis/pipeline/v1alpha1/pipeline_conversion.go
Add unit test for legacy finally annotation in PipelineRuns

tekton-robot · 2021-04-01T14:57:37Z

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/v1alpha1/pipeline_conversion.go	91.6%	95.0%	3.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

tekton-robot · 2021-04-01T15:01:34Z

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/v1alpha1/pipeline_conversion.go	91.6%	95.0%	3.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.5%	-4.3

tekton-robot · 2021-04-01T15:54:04Z

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/v1alpha1/pipeline_conversion.go	91.6%	95.0%	3.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.8%	-4.0

tekton-robot · 2021-04-01T15:54: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/v1alpha1/pipeline_conversion.go	91.6%	95.0%	3.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	82.8%	-4.0

vdemeester · 2021-04-01T16:02:12Z

So, to account for all the fields that might be in v1beta1 but not in v1alpha1 this commit
serializes the entire v1beta1 pipeline spec into an annotation
on Pipelines and PipelineRuns (when the nested pipelineSpec field is present). This will
protect the entire set of beta-only fields from being lost in translation through this conversion
process. This annotation only exists in the YAML that is returned to the user. This annotation never
gets stored in etcd (unless I'm missing something!)

Unfortunately this approach does also have drawbacks: if a user makes changes
to the v1alpha1 YAML and then re-applies it then those changes
will be lost, overwritten by the contents of the annotation. I'm not sure if
there's a great way to determine, for all fields, how to successfully "merge"
the beta annotation with the contents of the modified v1alpha1 spec when
up-converting. There's no clear signal, as far as I can tell, that any given field has or
has not been modified (beyond performing a cmp.Diff? Would that be an acceptable option?)

I think that's something to test. If a user apply a v1alpha1 object that already exists, kubectl will get it in v1alpha1, thus will get the annotated object and then merge the given one with the new one. This should mean, we have the annotation at that point and hopefully, this means we should be good to go. That won't work for replace but well… we can't do anything here.

I am not sure about service side apply though, but might work in a very similar fashion 🐼

tekton-robot · 2021-04-01T16:26:38Z

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/v1alpha1/pipeline_conversion.go	91.6%	94.0%	2.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	89.7%	2.9

ghost · 2021-04-01T16:40:41Z

I think that's something to test. If a user apply a v1alpha1 object that already exists, kubectl will get it in v1alpha1, thus will get the annotated object and then merge the given one with the new one. This should mean, we have the annotation at that point and hopefully, this means we should be good to go. That won't work for replace but well… we can't do anything here.

I am not sure about service side apply though, but might work in a very similar fashion 🐼

Looking at an example Pipeline, let's say it starts as a v1beta1 document:

kind: Pipeline
apiVersion: tekton.dev/v1beta1
metadata:
  name: p
spec:
  tasks:
  - name: t
    taskRef:
      name: t
    when:
    - input: "w"
      operator: "in"
      values: ["x"]

On conversion to v1alpha1 we get this:

kind: PipelineRun
apiVersion: tekton.dev/v1alpha1
metadata:
  name: p
  annotations:
    tekton.dev/v1beta1PipelineSpec: `{ tasks: [{ "name": "t", "taskRef": {"name": "t"} "when": [{ "input": "w", "operator": "in", "values": ["x"] }] }] }`
spec:
  tasks:
  - name: t
    taskRef:
      name: t

Now, let's say the user updates the v1alpha1 yaml file to change the task name to "u":

kind: PipelineRun
apiVersion: tekton.dev/v1alpha1
metadata:
  name: p
  annotations:
    tekton.dev/v1beta1PipelineSpec: `{ tasks: [{ "name": "t", "taskRef": {"name": "t"} "when": [{ "input": "w", "operator": "in", "values": ["x"] }] }] }`
spec:
  tasks:
  - name: u
    taskRef:
      name: t

^ Our conversion code can no longer figure out which pipelinetask is the right one to merge the when section into. So we just replace the whole spec with the original v1beta1 version, which means the user loses their pipelinetask name change. I don't think there's any way around this - WDYT?

The round-tripped Pipeline ends up reverted to this:

kind: Pipeline
apiVersion: tekton.dev/v1beta1
metadata:
  name: p
spec:
  tasks:
  - name: t
    taskRef:
      name: t
    when:
    - input: "w"
      operator: "in"
      values: ["x"]

Prior to this commit our conversion of pipelines from v1beta1 to v1alpha1 did not account for fields that were solely present in the v1beta1 pipeline spec. This meant, for example, that When Expressions in PipelineTasks were quietly dropped when moving from v1beta1 to v1alpha1. If a user subsequently re-applied that v1alpha1 resource to the cluster then the resource would be upgraded back to v1beta1 prior to storage without the missing when expressions. This problem would only be exacerbated further over time - any new features added to the v1beta1 spec would be lost on conversion. This commit serializes the entire v1beta1 pipeline spec into an annotation on Pipelines and PipelineRuns (for nested pipelineSpec fields). This will protect the entire set of beta-only fields from being lost in translation. Unfortunately this approach does also have drawbacks: if a user makes changes to the v1alpha1 YAML and then re-applies it then those changes will be lost - overwritten by the contents of the annotation. I'm not sure if there's a great way to determine, for all fields, how to successfully "merge" the beta annotation with the contents of the modified v1alpha1 spec when up-converting. An e2e test has been added to send a v1beta1 pipeline through the entire roundtrip flow and then compare the before/after for differences. One more fix is also included here: prior to this commit we were not preserving pipeline results when converting pipelines from v1beta1 to v1alpha1. This commit corrects that by copying pipeline results to the v1alpha1 pipeline and back.

tekton-robot · 2021-04-01T16:58:46Z

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/v1alpha1/pipeline_conversion.go	91.6%	94.0%	2.4
pkg/apis/pipeline/v1alpha1/pipelinerun_conversion.go	86.8%	89.7%	2.9

tekton-robot · 2021-04-01T17:23:21Z

@sbwsg: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-integration-tests	`08f7692`	link	`/test pull-tekton-pipeline-integration-tests`
pull-tekton-pipeline-unit-tests	`08f7692`	link	`/test tekton-pipeline-unit-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

bobcatfish

excellent commit message 🎉

Just a couple random nits and thoughts:

looks like the unit test coverage has gone down :O (looks like some error conditions not being tested?) (maybe that's covered by the TODO you've already got)
i guess we'll still need to follow this same approach for our other beta types ... and add more of this when we go to v1 as well (i wonder if its worth starting to write a list somewhere of things like this we need to make sure not to forget when we go to v1 - or even just making a note on the v1 issue)

/approve

bobcatfish · 2021-04-01T20:50:18Z

test/pipeline_beta_alpha_roundtrip_test.go

+
+	if _, ok := pV1alpha1.Annotations[v1alpha1.V1Beta1PipelineSpecSerializedAnnotationKey]; !ok {
+		t.Fatal("the serialized v1beta1 object is not present in the v1alpha1 object's annotations")
+	}


do we also want to assert against the serialized object? (or is it that we feel this is adequately covered by unit tests? might be worth asserting against something in this object just to be sure the contents are at least approximately what we expect)

bobcatfish · 2021-04-01T20:52:23Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

-const FinallyFieldName = "finally"
+// This annotation was used to serialize and deserialize Finally tasks in the
+// 0.22 Pipelines release. Keep it around to continue supporting any documents
+// converted during that time.


would it make sense to add this to our doc tracking deprecations?

bobcatfish · 2021-04-01T20:52:53Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

@@ -107,6 +122,7 @@ func (sink *PipelineSpec) ConvertFrom(ctx context.Context, source v1beta1.Pipeli
 	sink.Params = source.Params
 	sink.Workspaces = source.Workspaces
 	sink.Description = source.Description
+	sink.Results = source.Results


im curious why this was missing before?

We didn't preserve Pipeline Results on the roundtrip to v1alpha1 and back :S so users were potentially losing this part of their pipeline specs prior to this commit. 🐛

bobcatfish · 2021-04-01T20:54:18Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

+	if meta.Annotations != nil {
+		if str, ok := meta.Annotations[V1Beta1PipelineSpecSerializedAnnotationKey]; ok {
+			if err := json.Unmarshal([]byte(str), spec); err != nil {
+				return fmt.Errorf("error deserializing v1beta1 document from annotation: %w", err)


maybe im overthinking this but im wondering about the impact of this annotation becoming malformed; i.e. do we want to have an error or just ignore it? (i guess having an error is safer in the long run?)

bobcatfish · 2021-04-01T20:55:02Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion.go

+			delete(meta.Annotations, V1Beta1PipelineSpecSerializedAnnotationKey)
+			if len(meta.Annotations) == 0 {
+				meta.Annotations = nil
+			}


im curious why we are explicitly setting this to nil? (maybe a comment would help)

bobcatfish · 2021-04-01T21:00:05Z

pkg/apis/pipeline/v1alpha1/pipeline_conversion_test.go

+		return false
+	}
+
+	structField, ok = p.Index(-1).(cmp.StructField)


the -1 and -2 to me feel like they could use a comment or variable to add some more context

tekton-robot · 2021-04-01T21:01:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

vdemeester · 2021-04-02T13:12:16Z

Now, let's say the user updates the v1alpha1 yaml file to change the task name to "u":
kind: PipelineRun
apiVersion: tekton.dev/v1alpha1
metadata:
  name: p
  annotations:
    tekton.dev/v1beta1PipelineSpec: `{ tasks: [{ "name": "t", "taskRef": {"name": "t"} "when": [{ "input": "w", "operator": "in", "values": ["x"] }] }] }`
spec:
  tasks:
  - name: u
    taskRef:
      name: t
^ Our conversion code can no longer figure out which pipelinetask is the right one to merge the when section into. So we just replace the whole spec with the original v1beta1 version, which means the user loses their pipelinetask name change. I don't think there's any way around this - WDYT?

The round-tripped Pipeline ends up reverted to this:
kind: Pipeline
apiVersion: tekton.dev/v1beta1
metadata:
  name: p
spec:
  tasks:
  - name: t
    taskRef:
      name: t
    when:
    - input: "w"
      operator: "in"
      values: ["x"]

Right, I think this is where we need to define "precendence" of things. Anything that is present in the spec should take over the annotation I feel.

ghost · 2021-04-02T13:21:22Z

Right, I think this is where we need to define "precendence" of things. Anything that is present in the spec should take over the annotation I feel.

Can you go into a bit more detail here because I'm not sure it's possible for nested objects like PipelineTask. Works o.k. for fields at the top of the pipeline spec but not for nested tasks and finally entries I don't think?

afrittoli · 2021-04-12T17:51:14Z

Assumption: I think it would be fair to allow users upgrade the API version in their resources (only via PUT, never via PATCH), but we could explicitly disallow downgrading that way - i.e. to downgrade the API version of a resource the only way would be do DELETE and POST.

With that assumption in mind we could store in an annotation the api version used at create time, and decide what we allow based on that:

allow read / writes when the API version in the URL (/apis/tekton.dev/<api-version>) matches the annotation
allow reads via any API version, use the annotations with the fully serialised stored version when the API version is different from the original one
allow writes (only via PUT) is the API version in the URL is more recent than the original one
disallow writes in any case if the API version in the URL is older than the original one

My concern is that I've never seen k8s APIs behave this way - and it would be better to give users a consistent experience in terms of API versioning, unless there is a really good reason to behave differently.

ghost · 2021-06-10T14:43:44Z

I'm going to close this for the time being. The next step for anyone picking this up later would be to write a TEP to try and capture the different alternative approaches / tradeoffs and then build a community consensus on best next steps.

ghost added the kind/bug Categorizes issue or PR as related to a bug. label Mar 25, 2021

ghost requested review from vdemeester, pritidesai, imjasonh, afrittoli, dibyom and jerop March 25, 2021 15:17

tekton-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Apr 1, 2021

bobcatfish reviewed Apr 1, 2021

View reviewed changes

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

ghost mentioned this pull request Apr 5, 2021

Losing When Expressions converting from v1beta1 to v1alpha1 #3815

Closed

ghost closed this Jun 10, 2021

ghost mentioned this pull request Feb 3, 2022

[TEP-0096] Add Proposal for V1 Features tektoncd/community#587

Merged

lbernick mentioned this pull request Feb 4, 2022

Handle deprecated fields during conversion from v1beta1 to v1 #4546

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

ghost commented Mar 25, 2021 •

edited by ghost

Loading

ghost commented Mar 25, 2021

tekton-robot commented Mar 25, 2021

ghost commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

ghost commented Mar 31, 2021 •

edited by ghost

Loading

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

vdemeester commented Apr 1, 2021 •

edited

Loading

tekton-robot commented Apr 1, 2021

ghost commented Apr 1, 2021 •

edited by ghost

Loading

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

bobcatfish left a comment

bobcatfish Apr 1, 2021

bobcatfish Apr 1, 2021

bobcatfish Apr 1, 2021

ghost Apr 2, 2021 •

edited by ghost

Loading

bobcatfish Apr 1, 2021

bobcatfish Apr 1, 2021

bobcatfish Apr 1, 2021

tekton-robot commented Apr 1, 2021

vdemeester commented Apr 2, 2021

ghost commented Apr 2, 2021

afrittoli commented Apr 12, 2021

ghost commented Jun 10, 2021

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

Serialize entire v1beta1 PipelineSpec when converting to v1alpha #3854

Conversation

ghost commented Mar 25, 2021 • edited by ghost Loading

Changes

Submitter Checklist

Release Notes

ghost commented Mar 25, 2021

tekton-robot commented Mar 25, 2021

ghost commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

tekton-robot commented Mar 31, 2021

ghost commented Mar 31, 2021 • edited by ghost Loading

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

vdemeester commented Apr 1, 2021 • edited Loading

tekton-robot commented Apr 1, 2021

ghost commented Apr 1, 2021 • edited by ghost Loading

tekton-robot commented Apr 1, 2021

tekton-robot commented Apr 1, 2021

bobcatfish left a comment

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

ghost Apr 2, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

bobcatfish Apr 1, 2021

Choose a reason for hiding this comment

tekton-robot commented Apr 1, 2021

vdemeester commented Apr 2, 2021

ghost commented Apr 2, 2021

afrittoli commented Apr 12, 2021

ghost commented Jun 10, 2021

ghost commented Mar 25, 2021 •

edited by ghost

Loading

ghost commented Mar 31, 2021 •

edited by ghost

Loading

vdemeester commented Apr 1, 2021 •

edited

Loading

ghost commented Apr 1, 2021 •

edited by ghost

Loading

ghost Apr 2, 2021 •

edited by ghost

Loading