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

Sync the pipelinerun status from the informers #2573

Merged
merged 1 commit into from
May 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
103 changes: 103 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"fmt"
"path/filepath"
"reflect"
"strconv"
"strings"
"time"

Expand All @@ -45,6 +46,7 @@ import (
"k8s.io/apimachinery/pkg/api/equality"
"k8s.io/apimachinery/pkg/api/errors"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/client-go/tools/cache"
"knative.dev/pkg/apis"
"knative.dev/pkg/configmap"
Expand Down Expand Up @@ -204,6 +206,14 @@ func (c *Reconciler) Reconcile(ctx context.Context, key string) error {
return err
}

// Make sure that the PipelineRun status is in sync with the actual TaskRuns
err = c.updatePipelineRunStatusFromInformer(pr)
if err != nil {
// This should not fail. Return the error so we can re-try later.
c.Logger.Errorf("Error while syncing the pipelinerun status: %v", err.Error())
return err
}

// Reconcile this copy of the pipelinerun and then write back any status or label
// updates regardless of whether the reconciliation errored out.
if err = c.reconcile(ctx, pr); err != nil {
Expand Down Expand Up @@ -935,3 +945,96 @@ func storePipelineSpec(ctx context.Context, pr *v1beta1.PipelineRun, ps *v1beta1
}
return nil
}

func (c *Reconciler) updatePipelineRunStatusFromInformer(pr *v1beta1.PipelineRun) error {
pipelineRunLabels := getTaskrunLabels(pr, "")
taskRuns, err := c.taskRunLister.TaskRuns(pr.Namespace).List(labels.SelectorFromSet(pipelineRunLabels))
if err != nil {
c.Logger.Errorf("could not list TaskRuns %#v", err)
return err
}
pr.Status = updatePipelineRunStatusFromTaskRuns(c.Logger, pr.Name, pr.Status, taskRuns)
return nil
}

func updatePipelineRunStatusFromTaskRuns(logger *zap.SugaredLogger, prName string, prStatus v1beta1.PipelineRunStatus, trs []*v1beta1.TaskRun) v1beta1.PipelineRunStatus {
// If no TaskRun was found, nothing to be done. We never remove taskruns from the status
if trs == nil || len(trs) == 0 {
return prStatus
}
// Store a list of Condition TaskRuns for each PipelineTask (by name)
conditionTaskRuns := make(map[string][]*v1beta1.TaskRun)
// Map PipelineTask names to TaskRun names that were already in the status
taskRunByPipelineTask := make(map[string]string)
if prStatus.TaskRuns != nil {
for taskRunName, pipelineRunTaskRunStatus := range prStatus.TaskRuns {
taskRunByPipelineTask[pipelineRunTaskRunStatus.PipelineTaskName] = taskRunName
}
}
// Loop over all the TaskRuns associated to Tasks
for _, taskrun := range trs {
lbls := taskrun.GetLabels()
pipelineTaskName := lbls[pipeline.GroupName+pipeline.PipelineTaskLabelKey]
if _, ok := lbls[pipeline.GroupName+pipeline.ConditionCheckKey]; ok {
// Save condition for looping over them after this
if _, ok := conditionTaskRuns[pipelineTaskName]; !ok {
// If it's the first condition taskrun, initialise the slice
conditionTaskRuns[pipelineTaskName] = []*v1beta1.TaskRun{}
}
conditionTaskRuns[pipelineTaskName] = append(conditionTaskRuns[pipelineTaskName], taskrun)
continue
}
if _, ok := prStatus.TaskRuns[taskrun.Name]; !ok {
// This taskrun was missing from the status.
// Add it without conditions, which are handled in the next loop
prStatus.TaskRuns[taskrun.Name] = &v1beta1.PipelineRunTaskRunStatus{
PipelineTaskName: pipelineTaskName,
Status: &taskrun.Status,
ConditionChecks: nil,
}
// Since this was recovered now, add it to the map, or it might be overwritten
taskRunByPipelineTask[pipelineTaskName] = taskrun.Name
}
}
// Then loop by pipelinetask name over all the TaskRuns associated to Conditions
for pipelineTaskName, actualConditionTaskRuns := range conditionTaskRuns {
Copy link
Member

@pritidesai pritidesai May 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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 😉

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

taskRunName, ok := taskRunByPipelineTask[pipelineTaskName]
if !ok {
// The pipelineTask associated to the conditions was not found in the pipelinerun
// status. This means that the conditions were orphaned, and never added to the
// status. In this case we need to generate a new TaskRun name, that will be used
// to run the TaskRun if the conditions are passed.
taskRunName = resources.GetTaskRunName(prStatus.TaskRuns, pipelineTaskName, prName)
prStatus.TaskRuns[taskRunName] = &v1beta1.PipelineRunTaskRunStatus{
PipelineTaskName: pipelineTaskName,
Status: nil,
ConditionChecks: nil,
}
}
// Build the map of condition checks for the taskrun
// If there were no other condition, initialise the map
conditionChecks := prStatus.TaskRuns[taskRunName].ConditionChecks
if conditionChecks == nil {
conditionChecks = make(map[string]*v1beta1.PipelineRunConditionCheckStatus)
}
for i, foundTaskRun := range actualConditionTaskRuns {
lbls := foundTaskRun.GetLabels()
if _, ok := conditionChecks[foundTaskRun.Name]; !ok {
// The condition check was not found, so we need to add it
// We only add the condition name, the status can now be gathered by the
// normal reconcile process
if conditionName, ok := lbls[pipeline.GroupName+pipeline.ConditionNameKey]; ok {
conditionChecks[foundTaskRun.Name] = &v1beta1.PipelineRunConditionCheckStatus{
ConditionName: fmt.Sprintf("%s-%s", conditionName, strconv.Itoa(i)),
}
} else {
// The condition name label is missing, so we cannot recover this
logger.Warnf("found an orphaned condition taskrun %#v with missing %s label",
foundTaskRun, pipeline.ConditionNameKey)
}
}
}
prStatus.TaskRuns[taskRunName].ConditionChecks = conditionChecks
}
return prStatus
}
Loading