Sync the pipelinerun status from the informers #2573
Conversation
tekton-robot
added
the
do-not-merge/work-in-progress
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
afrittoli
force-pushed
the
issues/2558
branch
2 times, most recently
from
66628f8 to
55216e6
Compare
afrittoli
changed the title
tekton-robot
removed
the
do-not-merge/work-in-progress
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
|
/test pull-tekton-pipeline-integration-tests |
|
In response to this:
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. |
|
[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:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
afrittoli
force-pushed
the
issues/2558
branch
3 times, most recently
from
32df491 to
fd37ed4
Compare
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
@vdemeester perhaps we are at the time limit with alpha + beta + the new test? Shall I increase the timeout a bit? |
|
/test pull-tekton-pipeline-integration-tests |
|
I wonder if the new pool could be related to the timeout @bobcatfish? |
|
/test pull-tekton-pipeline-integration-tests |
afrittoli
added
kind/bug
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/test pull-tekton-pipeline-integration-tests |
|
The integration tests are failing way too often on this PR, perhaps it's better to hold this one until I get a better understanding of the reason for that. |
tekton-robot
added
the
do-not-merge/hold
| } | ||
| } | ||
| // Then loop by pipelinetask name over all the TaskRuns associated to Conditions | ||
| for pipelineTaskName, actualConditionTaskRuns := range conditionTaskRuns { |
There was a problem hiding this comment.
@afrittoli pr.Status.TaskRuns holds an entry for the pipelineTask associated to a condition (could be more than one condition as well) with nil TaskRunStatus and one or more checks under ConditionChecks as in pr.Status.TaskRuns[PipelineTaskRunName].ConditionChecks.
Condition containers are always under ConditionChecks and not part of the map pr.Status.TaskRuns.
For example, for a pipelineTask with two conditions, conditionTaskRuns contains two containers, one for each condition and after these condition containers are created, this query returns them as valid taskRuns associated with the current pipeline:
c.taskRunLister.TaskRuns(pr.Namespace).List(labels.SelectorFromSet(pipelineRunLabels))
so in the looping over those tasks, as they are conditionalChecks, line 974 is not executed and assumes the pipelineTask was not found in line 987 and creates a new taskRun name.
Hope its making sense 😉
There was a problem hiding this comment.
Thank you so much for looking into this!
L974 is designed explicitly to only track TaskRun not associated to conditions, but I agree the logic is broken there. It works fine in case of orphaned conditions, but it doesn't in case of non-orphaned ones.
If the conditions are created and still running, there will be TaskRuns for them in the list, they can be identified as condition ones from the labels. The main TaskRun does not exists yet, however its name, under normal conditions, will already be in the PipelineRun status so that it may contain the status of the conditions:
taskrun-name (not yet created)
PipelineTaskName: pipelineTaskName,
Status: nil,
ConditionChecks: [array of condition checks],
Since my loops are based on what I get from the TaskRun list, I do not discover the existing TaskRunName with no real TaskRun under which conditions are hosted. I create a new one, and I end up having two TaskRuns with the same conditions - I think.
I will work on fixing this. I now separated my function in two parts, one which doesn't need the controller and takes the list of taskruns as input, so that it should be easier to write extra tests.
There was a problem hiding this comment.
I fixed the issue now by initializing taskRunByPipelineTask from the known PR status, and adding to it as orphaned TaskRuns are recovered.
I added much more test coverage, hopefully it will work ok this time.
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
|
/hold cancel |
tekton-robot
removed
the
do-not-merge/hold
When we reconcile a pipelinerun, we should ensure that the pipelinerun status is always in sync with the actual list of taskruns that can be provided by the taskrun informer. The only way to filter taskruns is by labels tekton.dev/pipelinerun. In case an orphaned taskrun is found, we can use the other labels on the taskrun to reconstruct the missing entry in the pipelinerun status, whether it's a missing taskrun or a missing condition check.
|
The following is the coverage report on the affected files.
|
|
/kind feature |
tekton-robot
added
the
kind/feature
|
This PR cannot be merged: expecting exactly one kind/ label Available
|
|
/test pull-tekton-pipeline-integration-tests |
|
@pritidesai @bobcatfish this should be ready now - CI is green and test coverage much improved now. |
|
sorry @afrittoli havent got chance to review it, its in my todo list for tomorrow :) |
|
thanks @afrittoli |
Changes
When we reconcile a pipelinerun, we should ensure that the
pipelinerun status is always in sync with the actual list of taskruns
that can be provided by the taskrun informer.
The only way to filter taskruns is by labels tekton.dev/pipelinerun.
In case an orphaned taskrun is found, we can use the other labels
on the taskrun to reconstruct the missing entry in the pipelinerun
status.
Fixes #2558
Submitter Checklist
These are the criteria that every PR should meet, please check them off as you
review them:
See the contribution guide for more details.
Double check this list of stuff that's easy to miss:
cmddir, please updatethe 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.