variable substitution in finally section

[pritidesai]

Changes

Variables under params section were not substituted with the values for
tasks in finally section. Fixing the parameter interpolation for final tasks.

Fixes #2906

/cc @bobcatfish @vdemeester

/kind bug

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

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

See the contribution guide for more details.

Double check this list of stuff that's easy to miss:

• If you are adding a new binary/image to the cmd dir, please update
  the release Task to build and release this image.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Variables from params section are now substituted with specified values for final tasks.

[tekton-robot]

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

[tekton-robot]

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

[tekton-robot]

This PR cannot be merged: expecting exactly one kind/ label

Available kind/ labels are:

kind/question: Issues or PRs that are questions around the project or a particular feature
kind/bug: Categorizes issue or PR as related to a bug.
kind/flake: Categorizes issue or PR as related to a flakey test
kind/cleanup: Categorizes issue or PR as related to cleaning up code, process, or technical debt.
kind/design: Categorizes issue or PR as related to design.
kind/documentation: Categorizes issue or PR as related to documentation.
kind/feature: Categorizes issue or PR as related to a new feature.
kind/misc: Categorizes issue or PR as a miscellaneuous one.

[pritidesai]

how come go coverage stats are missing? 🤔

[afrittoli]

Thanks for the prompt fix.
Just a NIT, but I think we can also merge it as it is.
/approve

[afrittoli]

/approve

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli, jlpettersson

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 [afrittoli]

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

[ghost]

/lgtm

[ghost]

@pritidesai Unfortunately it looks like the EasyCLA bot might require a kick to wake it up, which means making a null edit (adding an extra space or something to the commit message should work) and then a force-push. Apologies for that.

[bobcatfish]

how come go coverage stats are missing? 🤔

Our coverage tool is unreliable :( tektoncd/plumbing#165 I think we should switch to using something like coveralls

[ghost]

/lgtm

[pritidesai]

/test pull-tekton-pipeline-build-tests

[jlpettersson]

I have bitten myself in the foot a few times using append(). I know that the way @pritidesai initially wrote is safe. This may also work - but it may also introduce problems that is hard to debug since it may mutate p.Tasks

I don't have deep enough understanding about this, there are some articles that may elaborate about this, e.g. https://appliedgo.net/slices/

In general I prefer functional programming languages that never mutate slices in a dangerous way :)

[pritidesai]

yup and that's one of the reasons its not directly ranging over p.Tasks instead its assigned to a new object/variable tasks:

tasks := p.Tasks
for i := range tasks {

I have been struggling to avoid append completely, will give it one more try.

[jlpettersson]

yup and that's one of the reasons its not directly ranging over p.Tasks instead its assigned to a new object/variable tasks:

tasks := p.Tasks
for i := range tasks {

I have been struggling to avoid append completely, will give it one more try.

I mean, as you originally wrote:

tasks := append(tasks, p.Tasks...)
tasks := append(tasks, p.Finally...)
for i := range tasks {

is safe - because tasks is local here.

However, the optimization:

tasks := append(p.Tasks, p.Finally...)
for i := range tasks {

may (I am not sure - but I have struggled with this earlier) be problematic, as append() here "mutates" p.Tasks and even if you range tasks here, there may be problem in other places in the code - since p.Tasks is mutated now.

I don't know - but my Go experience tells me that this may be problematic.

[bobcatfish]

may (I am not sure - but I have struggled with this earlier) be problematic, as append() here "mutates" p.Tasks and even if you range tasks here, there may be problem in other places in the code - since p.Tasks is mutated now.

My understanding of this is that p.Tasks will not be mutated - my understanding of slices comes and goes depending on how recently I've read https://blog.golang.org/slices but my understanding is that append returns a new slice and sometimes allocates new memory to do it, but the slice passed in as the first argument is not changed

[bobcatfish]

@jlpettersson i didnt see the article you linked above (https://appliedgo.net/slices/) before replying but i think there's an important distinction:

Especially the second one can cause confusion, because after an append(), sometimes the original array has been changed, and sometimes a new array has been created, and the original one stays the same. If the original array was referenced by different parts of the code, one reference then may point to stale data.

The array afaik is the underlying storage; it might be mutated but that doesn't change the slice that was passed in as an arg.

Changes to slices can cause unexpected side effects to other slices, e.g.: https://play.golang.org/p/zY0WJztAt_z but afaik that append function could change the array underlying p.Tasks but wouldn't change the slice. (95% sure? 😅 )

[pritidesai]

I am still trying to understand as well if the original p.Tasks gets changed 😓

Wrote a sample program with three slices (slice1, slice2, and slice3) where slice3 := append(slice1, slice2...) and this kind of append does not change the original slice slice1 but for some reason I am not 100% convinced and feel the original changes were safe 😭

https://play.golang.org/p/T28YUyMXQka

[pritidesai]

and build shows failed when its not finished 😭 :

[bobcatfish]

but for some reason I am not 100% convinced and feel the original changes were safe 😭

one of the things that makes this hard is that the choice to allocate more memory or not will depend on the specifics of the machine it's being run on, so its hard to be certain about it.

afaik the underlying array can be mutated by append but not the slice (which is why append requires you to assign the new slice)

For example: https://play.golang.org/p/SmRt2_3eJVG

No matter how many times we append to the original slice, it doesn't change. The underlying memory might, because we append to it and the slices that are created as a result have to increase in size and eventually capacity, but the original is the same.

[bobcatfish]

/lgtm

[pritidesai]

Couldn't resist, sorry for commenting on closed PR 🙏, this is interesting article:

https://medium.com/@Jarema./golang-slice-append-gotcha-e9020ff37374

The changes in this PR wouldn't be impacted since the original slice is not modified but sharing in general. It causes issues when more than one slices are created using append and one single original slice:

Results in:

initial slice:  [0 1 2 3 4 5 6 7 8 9 10]
i: [0 1 2 3 4 5 6 7 8 9 10]
j: [0 1 2 3 4 5 6 7 8 9 10 102]
g: [0 1 2 3 4 5 6 7 8 9 10 102]
h:[0 1 2 3 4 5 6 7 8 9 10 102]

All the slices j, g, and h has 102 as the last element even when j and g are appending 100 and 101 respectively.

It depends on how the original slice is initialized.

[jlpettersson]

Couldn't resist, sorry for commenting on closed PR 🙏, this is interesting article:

https://medium.com/@Jarema./golang-slice-append-gotcha-e9020ff37374

From the article

What’s the solution?

• use append only to append new value to given slice, not to create new one:
  someSlice = append(someSlice, newElement)

This is the pattern I follow, to avoid my earlier mistakes, and therefore suggested

tasks := append(tasks, p.Tasks...)
tasks := append(tasks, p.Finally...)

but as written now, probably works in this case - just be careful with that pattern in other situations.

[bobcatfish]

well that is definitely not what i expected!! it makes sense after staring at it for a while but i think you're both right that there are more gotchas here than I realized!

It depends on how the original slice is initialized.

Specifically it depends on the size of the underlying array - when the slice is initalized in this loop:

func create(iterations int) []int {
    a := make([]int, 0)
    for i := 0; i < iterations; i++ {
        a = append(a, i)
    }
    return a
}

The slice is created with size 0 so many of the append calls need to allocate more space in the array - if you look at the len and cap as it runs, in the playground the space is doubled every time more is needed, so when the loop returns, the slice has capacity 16, while len is 11.

When the slice is initialized with i := []int{0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10} both the size and the capacity are set to 11 immediately. This means that the append calls need to create a new array each time and so j, g and h are pointing at different arrays.

In the cases where the behavior is as expected, all the slices are pointing at different memory - in the cases where the behavior is not as expected, all the slices are pointing at the same memory.

https://play.golang.org/p/woTJPqN8E6Q

Also note that printing i, the original slice, continues to return the same data because the length and capacity of that slice are unchanged.

So appending to p.Tasks would not actually change p.Tasks - what it might change is memory beyond the end of the slice pointed at by p.Tasks.

[bobcatfish]

If you tried to do something like:

go foo(append(p.Tasks, something))
go bar(append(p.Tasks, somethingElse))
go baz(append(p.Tasks, someOtherThing))

Then depending on the capacity of p.Tasks you might see something crazy! i.e. all 3 functions might be operating on a slice with p.Tasks and someOtherThing :O

Or if you did something like foo(append(p.Tasks, something)) and then saved the arg to some cache or something - by the time you get back to it it might have changed!

[pritidesai]

Yup I absolutely agree @bobcatfish @jlpettersson

And the way we initialize p.Tasks is through make 😞

pipeline/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go

Lines 764 to 769 in 568efa4

	if in.Tasks != nil {
	in, out := &in.Tasks, &out.Tasks
	*out = make([]PipelineTask, len(*in))
	for i := range *in {
	(*in)[i].DeepCopyInto(&(*out)[i])
	}

ps.Tasks slice is created using make and length is set to the number of tasks without any capacity which by default sets the capacity to the length of tasks. And same for ps.Finally so append(ps.Tasks, ps.Finally...) ends up creating new slice with new memory allocation. Making ps.Tasks using make works fine since the expectation is not updating it after its created except variable substitutions 🤔

I am trying to convert our observations here into tests to confirm pipeline is not impacted in general.

Wrote two tests with two pipelines:

• one dag task and one final task in a pipeline
• five dag tasks and one final tasks in a pipeline

https://github.com/tektoncd/pipeline/compare/master...pritidesai:append?expand=1#diff-c9627eaed950dc49af457b127fc6da6f

Without anything in the pipeline (which is not the ideal case), memory allocations of tasks and finally are

• one dag task and one final task with Sizeof(v1beta1.PipelineTask) being 152 bytes.

DAG tasks: 0xc000454c90
Final Tasks:  0xc000454cc0
Appended slice: 0xc000431a40

• five dag tasks and one final task

DAG tasks: 0xc000454e40 - 0xc000454f00 
Final Tasks:   0xc000454f30
Appended slice: 0xc00042b800

Also, note the length and capacity of a new slice created using Append with 5 DAG tasks and 1 final task is set to - Length 6 and Capacity 10 (double of the length of original slice).

 go test ./pkg/apis/pipeline/v1beta1/... -tags=unit -run=TestPipeline_Copy -v
=== RUN   TestPipeline_Copy
    TestPipeline_Copy: pipeline_types_test.go:26: Tasks: [{dag-task-1 0xc000454c90 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy: pipeline_types_test.go:27: Length of Tasks:  1
    TestPipeline_Copy: pipeline_types_test.go:28: Capacity of Tasks:  1
    TestPipeline_Copy: pipeline_types_test.go:30: Finally:  [{final-task-1 0xc000454cc0 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy: pipeline_types_test.go:31: Length of Finally:  1
    TestPipeline_Copy: pipeline_types_test.go:32: Capacity of Finally:  1
    TestPipeline_Copy: pipeline_types_test.go:35:
    TestPipeline_Copy: pipeline_types_test.go:36: Append Tasks and Finally:  [{dag-task-1 0xc000454c90 <nil> [] 0 [] <nil> [] [] nil} {final-task-1 0xc000454cc0 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy: pipeline_types_test.go:37: Address: 0xc000431a40
    TestPipeline_Copy: pipeline_types_test.go:38: Length:  2
    TestPipeline_Copy: pipeline_types_test.go:39: Capacity:  2
--- PASS: TestPipeline_Copy (0.00s)
=== RUN   TestPipeline_Copy_Muliple_Tasks
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:61: Tasks: [{dag-task-1 0xc000454e40 <nil> [] 0 [] <nil> [] [] nil} {dag-task-2 0xc000454e70 <nil> [] 0 [] <nil> [] [] nil} {dag-task-3 0xc000454ea0 <nil> [] 0 [] <nil> [] [] nil} {dag-task-4 0xc000454ed0 <nil> [] 0 [] <nil> [] [] nil} {dag-task-5 0xc000454f00 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:62: Length of Tasks:  5
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:63: Capacity of Tasks:  5
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:65: Finally:  [{final-task-1 0xc000454f30 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:66: Length of Finally:  1
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:67: Capacity of Finally:  1
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:70:
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:71: Append Tasks and Finally:  [{dag-task-1 0xc000454e40 <nil> [] 0 [] <nil> [] [] nil} {dag-task-2 0xc000454e70 <nil> [] 0 [] <nil> [] [] nil} {dag-task-3 0xc000454ea0 <nil> [] 0 [] <nil> [] [] nil} {dag-task-4 0xc000454ed0 <nil> [] 0 [] <nil> [] [] nil} {dag-task-5 0xc000454f00 <nil> [] 0 [] <nil> [] [] nil} {final-task-1 0xc000454f30 <nil> [] 0 [] <nil> [] [] nil}]
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:72: Address: 0xc00042b800
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:73: Length:  6
    TestPipeline_Copy_Muliple_Tasks: pipeline_types_test.go:74: Capacity:  10
--- PASS: TestPipeline_Copy_Muliple_Tasks (0.00s)
PASS
ok  	github.com/tektoncd/pipeline/pkg/apis/pipeline/v1beta1	3.169s

I don't have any outcome of this research yet but wanted to share (1) the way ps.Tasks is being initialized and (2) how ps.Finally is created and allocated right where ps.Tasks ends (may be because the test pipeline doesn't have any thing beyond that). 🤔

[bobcatfish]

Thanks for doing this research @pritidesai !

I would to be clear that I think the danger of any impact to our controllers in general is minimal - this is something that's worth keeping in mind but I think the circumstances that lead to this gotcha are unlikely. (famous last words haha XD)

[bobcatfish]

I'd like to add a reassurance to everyone as well:

We can all try our best to write bug free code, but it's never going to be perfect. As much as possible we want everyone to feel safe to create contributions without having to agonize over edge cases - we have the test automation to support this. Of course we want to think things through, but we can only do so much.

I think being educated about this kind of behavior will be most useful if:

1. We discover a bug one day that is something like "hey it seems like something else is modifying this memory, I wonder what it could be"
2. If we start adding more go routines or global state. In the case of global state: we better have a really good reasons because the edge cases and problems it causes are numerous! In the case of go routines: this is something we always need to do with care anyway. No matter how easy go makes this, there is a lot of reasoning that needs to be done around accessing memory safely. (And side note, if there was anywhere to look for impacts caused by this kind of code, our existing go routines, such as our timeout handler, are probably the place to look - but in no way do I expect anyone on this PR to do that! Unless they feel like it :)

[bobcatfish]

And the way we initialize p.Tasks is through make 😞

pipeline/pkg/apis/pipeline/v1beta1/zz_generated.deepcopy.go

Lines 764 to 769 in 568efa4

	if in.Tasks != nil {
	in, out := &in.Tasks, &out.Tasks
	*out = make([]PipelineTask, len(*in))
	for i := range *in {
	(*in)[i].DeepCopyInto(&(*out)[i])
	}

@pritidesai I think that might in fact be good news!

The problem with the example code above was that make was being used with an initial size of 0:

a := make([]int, 0)

In the call to make above, we're making the underlying array exactly the size that we need. I'm not sure we can guarantee it, but I think that makes the chances very good that any attempts to append to that memory will require creation of a new array. (Which is what we want, b/c changes to that new array will not impact the original one.)

[bobcatfish]

Circling back here @jlpettersson you may be interested to hear that THIS VERY LINE DID END UP BITING US: #3244 ( + @jerop @pritidesai )

On the upside 😅 I think we can now express why this is a problem, and it wasn't because of it mutating the underlying memory, it was because:

• Sometimes append would return a new slice and sometimes it wouldn't; when it did, it would copy everything from the original to the new slice. So the resulting logic would sometimes operate on the original, and sometimes on a copy which led to inconsistent behavior - which was then hard to reason about due to slices within the object being copied such that the pointer would still point at the original memory @_@ so the resulting structure was like a hybrid (some attributes are copies and updating them does not update the original; some were pointers at the same memory and updating them DOES update the original)

whew!!!