From 045723f11166d26f0fb5a3e9b7a1323541d2a091 Mon Sep 17 00:00:00 2001 From: Andrea Frittoli Date: Thu, 7 May 2020 15:44:23 +0100 Subject: [PATCH] WIP Sync the pipelinerun status from the informers 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. --- pkg/reconciler/pipelinerun/pipelinerun.go | 36 +++ .../pipelinerun/pipelinerun_test.go | 226 +++++++++++++++++- 2 files changed, 260 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/pipelinerun/pipelinerun.go b/pkg/reconciler/pipelinerun/pipelinerun.go index d84b9800a17..5c57583f94e 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun.go +++ b/pkg/reconciler/pipelinerun/pipelinerun.go @@ -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" @@ -188,6 +189,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 { @@ -930,3 +939,30 @@ 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 + } + for _, taskrun := range taskRuns { + lbls := taskrun.GetLabels() + if _, ok := lbls[pipeline.GroupName+pipeline.ConditionCheckKey]; ok { + // Condition checks are not in the TaskRuns map directly + continue + } + if pr.Status.TaskRuns[taskrun.Name] == nil { + pipelineTaskName := taskrun.Labels[pipeline.GroupName+pipeline.PipelineTaskLabelKey] + conditionChecks := make(map[string]*v1alpha1.PipelineRunConditionCheckStatus) + // TBD rebuild the condition checks from the lost of taskRuns with the right labels + pr.Status.TaskRuns[taskrun.Name] = &v1alpha1.PipelineRunTaskRunStatus{ + PipelineTaskName: pipelineTaskName, + Status: &taskrun.Status, + ConditionChecks: conditionChecks, // TBD handling ConditionChecks + } + } + } + return nil +} diff --git a/pkg/reconciler/pipelinerun/pipelinerun_test.go b/pkg/reconciler/pipelinerun/pipelinerun_test.go index cb246c49444..e78c8966439 100644 --- a/pkg/reconciler/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/pipelinerun/pipelinerun_test.go @@ -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()) @@ -2185,3 +2185,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) + } +}