Write TaskRun.Status.TaskSpec with replaced spec on every reconcile run

fixes #5574 With #5537, the call to `applyParamsContextsResultsAndWorkspaces` and setting of `tr.Status.TaskSpec` to the output of that call was moved from happening with every call to `reconcile` to only happening if there wasn't already a pod created. This didn't cause any problems with execution, but it sometimes resulted in `tr.Status.TaskSpec` not getting set properly or getting reset to `nil` on a subsequent `reconcile` run, with the end result that, sometimes, `tr.Status.TaskSpec` would contain the original `TaskSpec` without parameter, result, context, and workspace references being replaced with the corresponding value. This caused flaky failures in the `TestPipelineRunStatusSpec/pipeline_status_spec_updated` e2e test, and integration test failures in Chains (tektoncd/chains#577). This change moves the PVC creation out of the `if pod == nil {` block that creates the pod if needed, while still checking if `pod` is `nil` before creating the PVC, and brings the `applyParamsContextsResultsAndWorkspaces` call and setting of `tr.Status.TaskSpec` back out of the pod creation block, but after the possible PVC creation. Signed-off-by: Andrew Bayer <[email protected]>
tektoncd · Sep 29, 2022 · 8bb5e52 · 8bb5e52
1 parent 0362c6b
commit 8bb5e52
Show file tree

Hide file tree

Showing 2 changed files with 98 additions and 21 deletions.
diff --git a/pkg/reconciler/taskrun/taskrun.go b/pkg/reconciler/taskrun/taskrun.go
@@ -481,31 +481,37 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1beta1.TaskRun, rtr *re
 		}
 	}
 
-	if pod == nil {
-		if tr.HasVolumeClaimTemplate() {
-			if err := c.pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, tr.Spec.Workspaces, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil {
-				logger.Errorf("Failed to create PVC for TaskRun %s: %v", tr.Name, err)
-				tr.Status.MarkResourceFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
-					fmt.Errorf("Failed to create PVC for TaskRun %s workspaces correctly: %s",
-						fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err))
-				return controller.NewPermanentError(err)
-			}
-
-			taskRunWorkspaces := applyVolumeClaimTemplates(tr.Spec.Workspaces, *kmeta.NewControllerRef(tr))
-			// This is used by createPod below. Changes to the Spec are not updated.
-			tr.Spec.Workspaces = taskRunWorkspaces
+	// Please note that this block is required to run before `applyParamsContextsResultsAndWorkspaces` is called the first time,
+	// and that `applyParamsContextsResultsAndWorkspaces` _must_ be called on every reconcile.
+	if pod == nil && tr.HasVolumeClaimTemplate() {
+		if err := c.pvcHandler.CreatePersistentVolumeClaimsForWorkspaces(ctx, tr.Spec.Workspaces, *kmeta.NewControllerRef(tr), tr.Namespace); err != nil {
+			logger.Errorf("Failed to create PVC for TaskRun %s: %v", tr.Name, err)
+			tr.Status.MarkResourceFailed(volumeclaim.ReasonCouldntCreateWorkspacePVC,
+				fmt.Errorf("Failed to create PVC for TaskRun %s workspaces correctly: %s",
+					fmt.Sprintf("%s/%s", tr.Namespace, tr.Name), err))
+			return controller.NewPermanentError(err)
 		}
 
-		// Get the randomized volume names assigned to workspace bindings
-		workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces)
+		taskRunWorkspaces := applyVolumeClaimTemplates(tr.Spec.Workspaces, *kmeta.NewControllerRef(tr))
+		// This is used by createPod below. Changes to the Spec are not updated.
+		tr.Spec.Workspaces = taskRunWorkspaces
+	}
 
-		ts, err := applyParamsContextsResultsAndWorkspaces(ctx, tr, rtr, workspaceVolumes)
-		if err != nil {
-			logger.Errorf("Error updating task spec parameters, contexts, results and workspaces: %s", err)
-			return err
-		}
-		tr.Status.TaskSpec = ts
+	// Get the randomized volume names assigned to workspace bindings
+	workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces)
 
+	ts, err := applyParamsContextsResultsAndWorkspaces(ctx, tr, rtr, workspaceVolumes)
+	if err != nil {
+		logger.Errorf("Error updating task spec parameters, contexts, results and workspaces: %s", err)
+		return err
+	}
+	tr.Status.TaskSpec = ts
+
+	if len(tr.Status.TaskSpec.Steps) > 0 {
+		logger.Infof("set taskspec for %s/%s - script: %s", tr.Namespace, tr.Name, tr.Status.TaskSpec.Steps[0].Script)
+	}
+
+	if pod == nil {
 		pod, err = c.createPod(ctx, ts, tr, rtr, workspaceVolumes)
 		if err != nil {
 			newErr := c.handlePodCreationError(tr, err)

diff --git a/pkg/reconciler/taskrun/taskrun_test.go b/pkg/reconciler/taskrun/taskrun_test.go
@@ -4925,3 +4925,74 @@ status:
 		})
 	}
 }
+
+func TestReconcile_ReplacementsInStatusTaskSpec(t *testing.T) {
+	task := parse.MustParseTask(t, `
+metadata:
+  name: test-task-with-replacements
+  namespace: foo
+spec:
+  params:
+  - default: mydefault
+    name: myarg
+    type: string
+  steps:
+  - script: echo $(inputs.params.myarg)
+    image: myimage
+    name: mycontainer
+`)
+	tr := parse.MustParseTaskRun(t, `
+metadata:
+  name: test-taskrun-with-replacements
+  namespace: foo
+spec:
+  params:
+  - name: myarg
+    value: foo
+  taskRef:
+    name: test-task-with-replacements
+status:
+  podName: the-pod
+`)
+
+	expectedStatusSpec := &v1beta1.TaskSpec{
+		Params: []v1beta1.ParamSpec{{
+			Name:    "myarg",
+			Default: v1beta1.NewStructuredValues("mydefault"),
+			Type:    v1beta1.ParamTypeString,
+		}},
+		Steps: []v1beta1.Step{{
+			Script: "echo foo",
+			Image:  "myimage",
+			Name:   "mycontainer",
+		}},
+	}
+
+	d := test.Data{
+		TaskRuns: []*v1beta1.TaskRun{tr},
+		Tasks:    []*v1beta1.Task{task},
+		Pods: []*corev1.Pod{{
+			ObjectMeta: metav1.ObjectMeta{
+				Namespace: "foo",
+				Name:      "the-pod",
+			},
+		}},
+	}
+	testAssets, cancel := getTaskRunController(t, d)
+	defer cancel()
+
+	if err := testAssets.Controller.Reconciler.Reconcile(testAssets.Ctx, getRunName(tr)); err == nil {
+		t.Error("Wanted a wrapped requeue error, but got nil.")
+	} else if ok, _ := controller.IsRequeueKey(err); !ok {
+		t.Errorf("expected no error. Got error %v", err)
+	}
+
+	updatedTR, err := testAssets.Clients.Pipeline.TektonV1beta1().TaskRuns(tr.Namespace).Get(testAssets.Ctx, tr.Name, metav1.GetOptions{})
+	if err != nil {
+		t.Fatalf("getting updated taskrun: %v", err)
+	}
+
+	if d := cmp.Diff(expectedStatusSpec, updatedTR.Status.TaskSpec); d != "" {
+		t.Errorf("expected Status.TaskSpec to match, but differed: %s", diff.PrintWantGot(d))
+	}
+}