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) + } +}