Skip to content

Commit

Permalink
WIP Sync the pipelinerun status from the informers
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@afrittoli
afrittoli committed May 8, 2020
1 parent e801a96 commit 66628f8
Show file tree
Hide file tree
Showing 2 changed files with 293 additions and 2 deletions.
69 changes: 69 additions & 0 deletions pkg/reconciler/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,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 @@ -195,6 +196,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 @@ -929,3 +938,63 @@ func storePipelineSpec(ctx context.Context, pr *v1alpha1.PipelineRun, ps *v1alph
}
return nil
}

func (c *Reconciler) updatePipelineRunStatusFromInformer(pr *v1alpha1.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
}
// Store a list of Condition TaskRuns for each PipelineTask (by name)
conditionTaskRuns := make(map[string][]*v1alpha1.TaskRun)
// Map PipelineTask names to TaskRun names that were already in the status
taskRunByPipelineTask := make(map[string]string)
// First loop over all the TaskRuns associated to Tasks
for _, taskrun := range taskRuns {
lbls := taskrun.GetLabels()
pipelineTask := lbls[pipeline.GroupName+pipeline.PipelineTaskLabelKey]
if _, ok := lbls[pipeline.GroupName+pipeline.ConditionCheckKey]; ok {
// Save condition for looping over them after this
if _, ok := conditionTaskRuns[pipelineTask]; !ok {
// If it's the first condition taskrun, initialise the slice
conditionTaskRuns[pipelineTask] = []*v1alpha1.TaskRun{}
}
conditionTaskRuns[pipelineTask] = append(conditionTaskRuns[pipelineTask], taskrun)
continue
}
// Non-condition taskruns only. Map pipeline task to taskrun name
taskRunByPipelineTask[pipelineTask] = taskrun.Name
if _, ok := pr.Status.TaskRuns[taskrun.Name]; !ok {
// This taskrun was missing from the status.
// Add it without conditions, which are handled in the next loop
pipelineTaskName := taskrun.Labels[pipeline.GroupName+pipeline.PipelineTaskLabelKey]
pr.Status.TaskRuns[taskrun.Name] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: pipelineTaskName,
Status: &taskrun.Status,
ConditionChecks: nil,
}
}
}
// Then loop over all the TaskRuns associated to Conditions
for pipelineTaskName, actualConditionTaskRuns := range conditionTaskRuns {
if taskRunName, ok := taskRunByPipelineTask[pipelineTaskName]; !ok {
// The pipelineTask associated to the conditions was not found. 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 := "__foobar__" // TBD use existing function to get a name
// conditionChecks := make(map[string]*v1alpha1.PipelineRunConditionCheckStatus)
pr.Status.TaskRuns[taskRunName] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: pipelineTaskName,
Status: nil,
ConditionChecks: nil, // TBD build the list here from actualConditionTaskRuns
}
} else {
foundConditionTaskRuns := pr.Status.TaskRuns[taskRunName]
// TBD compare found checks with actual ones and add anyone that is missing
fmt.Printf("Found: %#v Actual: %#v", foundConditionTaskRuns, actualConditionTaskRuns)
}
}
return nil
}
226 changes: 224 additions & 2 deletions pkg/reconciler/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -816,8 +816,8 @@ func TestReconcileOnCompletedPipelineRun(t *testing.T) {
t.Fatalf("Expected client to have updated the TaskRun status for a completed PipelineRun, but it did not")
}

actual := clients.Pipeline.Actions()[1].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
if actual == nil {
_, ok := clients.Pipeline.Actions()[1].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
if !ok {
t.Errorf("Expected a PipelineRun to be updated, but it wasn't.")
}
t.Log(clients.Pipeline.Actions())
Expand Down Expand Up @@ -2187,3 +2187,225 @@ func Test_storePipelineSpec(t *testing.T) {
t.Fatalf("-want, +got: %v", d)
}
}

func TestReconcileOutOfSyncPipelineRun(t *testing.T) {
// It may happen that a PipelineRun creates one or more TaskRuns during reconcile
// but it fails to sync the update on the status back. This test verifies that
// the reconciler is able to coverge back to a consistent state with the orphaned
// TaskRuns back in the PipelineRun status.
// For more details, see https://github.com/tektoncd/pipeline/issues/2558
prOutOfSyncName := "test-pipeline-run-out-of-sync"
helloWorldTask := tb.Task("hello-world", tb.TaskNamespace("foo"))

// Condition checks for the third task
prccs3 := make(map[string]*v1alpha1.PipelineRunConditionCheckStatus)
conditionCheckName3 := prOutOfSyncName + "hello-world-3-always-true-xxxyyy"
prccs3[conditionCheckName3] = &v1alpha1.PipelineRunConditionCheckStatus{
ConditionName: "always-true-0",
Status: &v1alpha1.ConditionCheckStatus{},
}
// Condition checks for the fourth task
prccs4 := make(map[string]*v1alpha1.PipelineRunConditionCheckStatus)
conditionCheckName4 := prOutOfSyncName + "hello-world-4-always-true-xxxyyy"
prccs4[conditionCheckName4] = &v1alpha1.PipelineRunConditionCheckStatus{
ConditionName: "always-true-0",
Status: &v1alpha1.ConditionCheckStatus{},
}
testPipeline := tb.Pipeline("test-pipeline", tb.PipelineNamespace("foo"), tb.PipelineSpec(
tb.PipelineTask("hello-world-1", helloWorldTask.Name),
tb.PipelineTask("hello-world-2", helloWorldTask.Name),
tb.PipelineTask("hello-world-3", helloWorldTask.Name, tb.PipelineTaskCondition("always-true")),
tb.PipelineTask("hello-world-4", helloWorldTask.Name, tb.PipelineTaskCondition("always-true"))))

// This taskrun is in the pipelinerun status. It completed successfully.
taskRunDone := tb.TaskRun("test-pipeline-run-out-of-sync-hello-world-1",
tb.TaskRunNamespace("foo"),
tb.TaskRunOwnerReference("PipelineRun", prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineLabelKey, testPipeline.Name),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineRunLabelKey, prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineTaskLabelKey, "hello-world-1"),
tb.TaskRunSpec(tb.TaskRunTaskRef("hello-world")),
tb.TaskRunStatus(
tb.StatusCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionTrue,
}),
),
)

// This taskrun is *not* in the pipelinerun status. It's still running.
taskRunOrphaned := tb.TaskRun("test-pipeline-run-out-of-sync-hello-world-2",
tb.TaskRunNamespace("foo"),
tb.TaskRunOwnerReference("PipelineRun", prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineLabelKey, testPipeline.Name),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineRunLabelKey, prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineTaskLabelKey, "hello-world-2"),
tb.TaskRunSpec(tb.TaskRunTaskRef("hello-world")),
tb.TaskRunStatus(
tb.StatusCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
}),
),
)

// This taskrun has a condition attached. The condition is in the pipelinerun, but the taskrun
// itself is *not* in the pipelinerun status. It's still running.
taskRunWithCondition := tb.TaskRun("test-pipeline-run-out-of-sync-hello-world-3",
tb.TaskRunNamespace("foo"),
tb.TaskRunOwnerReference("PipelineRun", prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineLabelKey, testPipeline.Name),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineRunLabelKey, prOutOfSyncName),
tb.TaskRunLabel(pipeline.GroupName+pipeline.PipelineTaskLabelKey, "hello-world-3"),
tb.TaskRunSpec(tb.TaskRunTaskRef("hello-world")),
tb.TaskRunStatus(
tb.StatusCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
}),
),
)

// This taskrun has a condition attached. The condition is *not* the pipelinerun, and it's still
// running. The taskrun itself was not created yet.
taskRunWithOrphanedConditionName := "test-pipeline-run-out-of-sync-hello-world-4"

prOutOfSync := tb.PipelineRun(prOutOfSyncName,
tb.PipelineRunNamespace("foo"),
tb.PipelineRunSpec(testPipeline.Name, tb.PipelineRunServiceAccountName("test-sa")),
tb.PipelineRunStatus(tb.PipelineRunStatusCondition(apis.Condition{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
Reason: "",
Message: "",
}),
tb.PipelineRunTaskRunsStatus(taskRunDone.Name, &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-1",
Status: &v1alpha1.TaskRunStatus{},
}),
tb.PipelineRunTaskRunsStatus(taskRunWithCondition.Name, &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-3",
Status: nil,
ConditionChecks: prccs,
}),
),
)
prs := []*v1alpha1.PipelineRun{prOutOfSync}
ps := []*v1alpha1.Pipeline{testPipeline}
ts := []*v1alpha1.Task{helloWorldTask}
trs := []*v1alpha1.TaskRun{taskRunDone, taskRunOrphaned, taskRunWithCondition}
cs := []*v1alpha1.Condition{
tb.Condition("always-true", tb.ConditionNamespace("foo"), tb.ConditionSpec(
tb.ConditionSpecCheck("", "foo", tb.Args("bar")),
)),
}
d := test.Data{
PipelineRuns: prs,
Pipelines: ps,
Tasks: ts,
TaskRuns: trs,
Conditions: cs,
}

testAssets, cancel := getPipelineRunController(t, d)
defer cancel()
c := testAssets.Controller
clients := testAssets.Clients

if err := c.Reconciler.Reconcile(context.Background(), "foo/"+prOutOfSync.Name); err != nil {
t.Fatalf("Error reconciling: %s", err)
}

// if len(clients.Pipeline.Actions()) != 2 {
// t.Fatalf("Expected client to have updated the TaskRun status for a completed PipelineRun, but it did not")
// }

_, ok := clients.Pipeline.Actions()[1].(ktesting.UpdateAction).GetObject().(*v1alpha1.PipelineRun)
if !ok {
t.Errorf("Expected a PipelineRun to be updated, but it wasn't.")
}
t.Log(clients.Pipeline.Actions())
actions := clients.Pipeline.Actions()
pipelineUpdates := 0
for _, action := range actions {
if action != nil {
switch {
case action.Matches("create", "taskruns"):
t.Fatalf("Expected client to not have created a TaskRun, but it did")
case action.Matches("update", "pipelineruns"):
pipelineUpdates++
default:
continue
}
}
}
if pipelineUpdates != 2 {
// If only the pipelinerun status changed, we expect one update
t.Fatalf("Expected client to have updated the pipelinerun once, but it did %d times", pipelineUpdates)
}

// Check that the PipelineRun was reconciled correctly
reconciledRun, err := clients.Pipeline.TektonV1alpha1().PipelineRuns("foo").Get(prOutOfSync.Name, metav1.GetOptions{})
if err != nil {
t.Fatalf("Somehow had error getting completed reconciled run out of fake client: %s", err)
}

// This PipelineRun should still be running and the status should reflect that
if !reconciledRun.Status.GetCondition(apis.ConditionSucceeded).IsUnknown() {
t.Errorf("Expected PipelineRun status to be running, but was %v", reconciledRun.Status.GetCondition(apis.ConditionSucceeded))
}

expectedTaskRunsStatus := make(map[string]*v1alpha1.PipelineRunTaskRunStatus)
// taskRunDone did not change
expectedTaskRunsStatus[taskRunDone.Name] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-1",
Status: &v1alpha1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{
{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionTrue,
},
},
},
},
}
// taskRunOrphaned was recovered into the status
expectedTaskRunsStatus[taskRunOrphaned.Name] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-2",
Status: &v1alpha1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{
{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
},
},
},
},
}
// taskRunWithCondition was recovered into the status. The condition did not change.
expectedTaskRunsStatus[taskRunWithCondition.Name] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-3",
Status: &v1alpha1.TaskRunStatus{
Status: duckv1beta1.Status{
Conditions: []apis.Condition{
{
Type: apis.ConditionSucceeded,
Status: corev1.ConditionUnknown,
},
},
},
},
ConditionChecks: prccs3,
}
// taskRunWithOrphanedConditionName had the condition recovered into the status. No taskrun.
expectedTaskRunsStatus[taskRunWithOrphanedConditionName] = &v1alpha1.PipelineRunTaskRunStatus{
PipelineTaskName: "hello-world-4",
ConditionChecks: prccs4,
}

if d := cmp.Diff(reconciledRun.Status.TaskRuns, expectedTaskRunsStatus); d != "" {
t.Fatalf("Expected PipelineRun status to match TaskRun(s) status, but got a mismatch: %s", d)
}
}

0 comments on commit 66628f8

Please sign in to comment.