wip: e2e: safer timeout test (less flakey 🙏)

[vdemeester]

Changes

• Do not start two go routines sweat, my bad, I messed up a rebase on my
  part brought an additional timeout goroutine bowing_man (23af2de).

• Use a channel (started) to make sure we start the timeout timer in
  time at the time we issue the go … call.

  When using the go keyword to start a goroutines, there is no
  guarantee the code inside the go routine will start right away. The
  scheduler might (and most likely will) wait for the main
  goroutine (or the caller goroutine) to have a waiting/sleeping time,
  to start working in the issued go routine.

  This means, that before that fix, we have no guarantee we started
  the timer at the right time — especially if the controller is very
  busy.

  Passing a channel and waiting for it to be closed just after the go … call forces the scheduler to sleep and run the goroutine's
  code. Which, in our case, that we started the timeout timer at the
  right time.

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

See the contribution guide
for more details.

Release Notes

Fix timeout handling : in the case of a busy controller, it was possible that the timeout timer wouldn't start at the right time — this should be fixed now

[tekton-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vdemeester

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

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

[vdemeester]

This line is duplicated with this one — see #674 for reference

[vdemeester]

/test pull-tekton-pipeline-integration-tests

[vdemeester]

/test pull-tekton-pipeline-integration-tests

[vdemeester]

/test pull-tekton-pipeline-integration-tests

[vdemeester]

Damn, it failed once still… 😓

/test pull-tekton-pipeline-integration-tests

[bobcatfish]

/test pull-tekton-pipeline-integration-tests

[bobcatfish]

I tried to help out with this but I couldn't get to the bottom of it either! I noticed some things that might help tho

Ideas:

• Maybe we can encapsulate some of this functionality in functions to make it a bit clearer exactly what's happening (unless this is an anti-pattern!)
• Can we add some log statements when the timeouts are setup etc.? might help with debugging

Some other things I noticed were wrong:

• We use different "resync" periods in the PipelineRun and TaskRun controllers, TaskRun uses GetTrackerLease, PipelineRun uses 30 minutes
• I don't think there is any reason to be en-queuing PipelineRuns on timeout, i.e. i think we could completely get rid of the PipelineRun timeout handling logic: the TaskRun handling logic works b/c checkTimeout is called at the TaskRun level on every reconcile, but as far as I can tell, there is no PipelineRun equivalent - all timeouts are handled at the TaskRun level, so we are just re-enqueuing these forever (and I did this locally and the tests continued passing!)

Questions:

• Why do we use the status lock whenever we're touching the status of an object? the actual updating doesn't happen when the values are changed, so even with locks, we can still overwrite changes

[bobcatfish]

i think that using WaitPipelineRun could be a little less error prone maybe if we wrapped it, something like this:

In the reconciler:

if !pr.HasStarted() {
    c.timeoutHandler.StartPipelineRunWait()
}

In timeout handler:

func (t *TimeoutSet) StartPipelineRunWait(pr *v1alpha1.PipelineRun, started chan struct{}) {
		started := make(chan struct{})
		go t.waitPipelineRun(pr, started)
		<-started
}

[bobcatfish]

(i wouldnt be surprised if this is an anti-pattern - hiding the fact that we're creating a go routine! - im a bit of a channel / goroutine noob)

[vdemeester]

@bobcatfish why would it be an anti-pattern ? context.Context and other packages uses that (aka "hide" goroutine/channel usage as much as then can)

[bobcatfish]

awesome!! that's perfect then :D :D :D

[bobcatfish]

could this comment be updated to include a description of the new param? 😇 as a channel noob I couldn't tell right away whether WaitPipelineRun or the caller were writing to started 😇

[bobcatfish]

another option here (instead of passing in started) could be to pass the timeAfter into this goroutine? e.g.

func (t *TimeoutSet) WaitPipelineRun(pr *v1alpha1.PipelineRun, timeAfter <-chan Time) {
   // Then we wouldn't need to write to `started`, we could use `timeAfter` directly in the `case`?
}

This would mean the caller would have to create timeAfter - maybe we could wrap WaitPipelineRun and create timeAfter in the wrapping function

[vdemeester]

interesting, that might look better even 👼

[tekton-robot]

@vdemeester: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-integration-tests	dcefc7e	link	/test pull-tekton-pipeline-integration-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.

[dlorenc]

Is this PR still relevant? I think it can probably be closed now right?

[vdemeester]

oh good point @dlorenc, it's not relevant anymore 😅
/close

[tekton-robot]

@vdemeester: Closing this PR.

In response to this:

oh good point @dlorenc, it's not relevant anymore 😅
/close

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.