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

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

Merged
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
48 changes: 27 additions & 21 deletions pkg/reconciler/taskrun/taskrun.go
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Copy link
Member

Choose a reason for hiding this comment

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

I think it might be worth adding a comment here on the conditions to avoid the conditions moving around again when the next change happens in the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

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
}
Comment on lines +501 to +507
Copy link
Member

Choose a reason for hiding this comment

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

Do these need to be computed each time, or does this only need to happen if pod == nil?

This might help clean things up a little bit if we can preserve the old structure so we can keep all the if pod == nil logic together - i.e.

if pod == nil {
  if tr.HasVolumeClaimTemplate() {
    ...
  }
 
  workspaceVolumes := workspace.CreateVolumes(tr.Spec.Workspaces)
  ...

  pod, err = c.createPod(ctx, ts, tr, rtr, workspaceVolumes)
  ...
}

tr.Status.TaskSpec = ts

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is that we need workspaceVolumes as input to applyParamsContextsResultsAndWorkspaces, and the whole issue here stemmed from that not getting called and tr.Status.TaskSpec not getting updated, so I think we do need to the createVolumes call every time. That's what was being done before 9cf7590

Copy link
Member

Choose a reason for hiding this comment

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

bleh - that's unfortunate. Might be something worth looking into refactoring (i.e. regrouping things that only need to be done once vs per reconcile), but won't hold up this PR for it.

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)
Expand Down
71 changes: 71 additions & 0 deletions pkg/reconciler/taskrun/taskrun_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm only testing for replacement of a param here because that's good enough to prove the point, and is simplest to do. Note that this issue does not show up with an inline tr.Spec.TaskSpec, interestingly.

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:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I initially tried this with a pre-set tr.Status.TaskSpec, but that kept passing when it shouldn't have. I'm guessing it might have eventually flaked into failure, but it's very possible that the specific conditions that triggered the flaky behavior would never actually show up in a unit test, so I just went with testing to make sure that a nil tr.Status.TaskSpec gets written properly.

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