From 1495fbefc6f86b0facacfb72dc98a9dcf8953657 Mon Sep 17 00:00:00 2001 From: Nader Ziada Date: Thu, 22 Nov 2018 17:43:54 -0500 Subject: [PATCH] Refactor TaskRun to support processing of TaskSpec --- .../v1alpha1/pipelinerun/pipelinerun_test.go | 2 +- .../taskrun/resources/input_resource_test.go | 5 +- .../taskrun/resources/input_resources.go | 15 +- pkg/reconciler/v1alpha1/taskrun/taskrun.go | 54 ++-- .../v1alpha1/taskrun/taskrun_test.go | 231 ++++++++++++++++++ pkg/reconciler/v1alpha1/taskrun/validate.go | 17 +- 6 files changed, 291 insertions(+), 33 deletions(-) diff --git a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go index 8f1acc28e26..fcc9e3cfaa1 100644 --- a/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go +++ b/pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go @@ -426,7 +426,7 @@ func TestUpdateTaskRunsState(t *testing.T) { }, Spec: v1alpha1.TaskRunSpec{ ServiceAccount: "test-sa", - TaskRef: v1alpha1.TaskRef{ + TaskRef: &v1alpha1.TaskRef{ Name: "unit-test-task", }, }, diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go index c80b382ee61..a926dcf0619 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resource_test.go @@ -218,10 +218,9 @@ func TestAddResourceToBuild(t *testing.T) { Name: "simpleTask", }, Inputs: v1alpha1.TaskRunInputs{ - Resources: []v1alpha1.TaskRunResource{{ ResourceRef: v1alpha1.PipelineResourceRef{ - Name: "the-git", + Name: "the-git-with-branch", }, Name: "workspace", }}, @@ -474,7 +473,7 @@ func TestAddResourceToBuild(t *testing.T) { }} { t.Run(c.desc, func(t *testing.T) { setUp() - got, err := AddInputResource(c.build, c.task, c.taskRun, pipelineResourceLister, logger) + got, err := AddInputResource(c.build, c.task.Name, c.task.Spec, c.taskRun, pipelineResourceLister, logger) if (err != nil) != c.wantErr { t.Errorf("Test: %q; NewControllerConfigFromConfigMap() error = %v, WantErr %v", c.desc, err, c.wantErr) } diff --git a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go index 0432597e09a..cfe7c326a7e 100644 --- a/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go +++ b/pkg/reconciler/v1alpha1/taskrun/resources/input_resources.go @@ -42,26 +42,27 @@ func getBoundResource(resourceName string, boundResources []v1alpha1.TaskRunReso // AddInputResource will update the input build with the input resource from the task func AddInputResource( build *buildv1alpha1.Build, - task *v1alpha1.Task, + taskName string, + taskSpec v1alpha1.TaskSpec, taskRun *v1alpha1.TaskRun, pipelineResourceLister listers.PipelineResourceLister, logger *zap.SugaredLogger, ) (*buildv1alpha1.Build, error) { - if task.Spec.Inputs == nil { + if taskSpec.Inputs == nil { return build, nil } var gitResource *v1alpha1.GitResource - for _, input := range task.Spec.Inputs.Resources { + for _, input := range taskSpec.Inputs.Resources { boundResource, err := getBoundResource(input.Name, taskRun.Spec.Inputs.Resources) if err != nil { return nil, fmt.Errorf("Failed to get bound resource: %s", err) } - resource, err := pipelineResourceLister.PipelineResources(task.Namespace).Get(boundResource.ResourceRef.Name) + resource, err := pipelineResourceLister.PipelineResources(taskRun.Namespace).Get(boundResource.ResourceRef.Name) if err != nil { - return nil, fmt.Errorf("task %q failed to Get Pipeline Resource: %q", task.Name, boundResource) + return nil, fmt.Errorf("task %q failed to Get Pipeline Resource: %q", taskName, boundResource) } switch resource.Spec.Type { @@ -69,7 +70,7 @@ func AddInputResource( { gitResource, err = v1alpha1.NewGitResource(resource) if err != nil { - return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", task.Name, boundResource.ResourceRef.Name) + return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", taskName, boundResource.ResourceRef.Name) } gitSourceSpec := &buildv1alpha1.GitSourceSpec{ Url: gitResource.URL, @@ -80,7 +81,7 @@ func AddInputResource( case v1alpha1.PipelineResourceTypeCluster: clusterResource, err := v1alpha1.NewClusterResource(resource) if err != nil { - return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", task.Name, boundResource.ResourceRef.Name) + return nil, fmt.Errorf("task %q invalid Pipeline Resource: %q", taskName, boundResource.ResourceRef.Name) } addClusterBuildStep(build, clusterResource) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun.go b/pkg/reconciler/v1alpha1/taskrun/taskrun.go index 0626eb5b31b..8f2d3be1bf2 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun.go @@ -207,12 +207,17 @@ func (c *Reconciler) reconcile(ctx context.Context, tr *v1alpha1.TaskRun) error build, err = c.createBuild(ctx, tr, pvc.Name) if err != nil { // This Run has failed, so we need to mark it as failed and stop reconciling it + var msg string + if tr.Spec.TaskRef != nil { + msg = fmt.Sprintf("References a Task %s that doesn't exist: ", fmt.Sprintf("%s/%s", tr.Namespace, tr.Spec.TaskRef.Name)) + } else { + msg = fmt.Sprintf("References a TaskSpec with missing information: ") + } tr.Status.SetCondition(&duckv1alpha1.Condition{ - Type: duckv1alpha1.ConditionSucceeded, - Status: corev1.ConditionFalse, - Reason: ReasonCouldntGetTask, - Message: fmt.Sprintf("References a Task %s that doesn't exist: %v", - fmt.Sprintf("%s/%s", tr.Namespace, tr.Spec.TaskRef.Name), err), + Type: duckv1alpha1.ConditionSucceeded, + Status: corev1.ConditionFalse, + Reason: ReasonCouldntGetTask, + Message: fmt.Sprintf("%s %v", msg, err), }) c.Recorder.Eventf(tr, corev1.EventTypeWarning, "BuildCreationFailed", "Failed to create build %q: %v", tr.Name, err) c.Logger.Errorf("Failed to create build for task %q :%v", err, tr.Name) @@ -317,19 +322,38 @@ func (c *Reconciler) createPVC(tr *v1alpha1.TaskRun) (*corev1.PersistentVolumeCl return v, nil } +func (c *Reconciler) getTaskSpec(tr *v1alpha1.TaskRun) (v1alpha1.TaskSpec, string, error) { + taskSpec := v1alpha1.TaskSpec{} + taskName := "" + if tr.Spec.TaskRef != nil && tr.Spec.TaskRef.Name != "" { + // Get related task for taskrun + t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) + if err != nil { + return taskSpec, taskName, fmt.Errorf("error when listing tasks %v", err) + } + taskSpec = t.Spec + taskName = t.Name + } else if tr.Spec.TaskSpec != nil { + taskSpec = *tr.Spec.TaskSpec + taskName = tr.Name + } else { + return taskSpec, taskName, fmt.Errorf("TaskRun %s not providing TaskRef or TaskSpec", tr.Name) + } + return taskSpec, taskName, nil +} + // createBuild creates a build from the task, using the task's buildspec // with pvcName as a volumeMount func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcName string) (*buildv1alpha1.Build, error) { - // Get related task for taskrun - t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) + ts, taskName, err := c.getTaskSpec(tr) if err != nil { - return nil, fmt.Errorf("error when listing tasks %v", err) + return nil, fmt.Errorf("taskRun %s has nil BuildSpec", tr.Name) } // TODO: Preferably use Validate on task.spec to catch validation error - bs := t.Spec.GetBuildSpec() + bs := ts.GetBuildSpec() if bs == nil { - return nil, fmt.Errorf("task %s has nil BuildSpec", t.Name) + return nil, fmt.Errorf("task %s has nil BuildSpec", taskName) } // For each step with no entrypoint set, try to populate it with the info @@ -352,25 +376,25 @@ func (c *Reconciler) createBuild(ctx context.Context, tr *v1alpha1.TaskRun, pvcN return nil, fmt.Errorf("couldn't create redirected Build: %v", err) } - build, err := resources.AddInputResource(b, t, tr, c.resourceLister, c.Logger) + build, err := resources.AddInputResource(b, taskName, ts, tr, c.resourceLister, c.Logger) if err != nil { c.Logger.Errorf("Failed to create a build for taskrun: %s due to input resource error %v", tr.Name, err) return nil, err } var defaults []v1alpha1.TaskParam - if t.Spec.Inputs != nil { - defaults = append(defaults, t.Spec.Inputs.Params...) + if ts.Inputs != nil { + defaults = append(defaults, ts.Inputs.Params...) } // Apply parameter templating from the taskrun. build = resources.ApplyParameters(build, tr, defaults...) // Apply bound resource templating from the taskrun. - build, err = resources.ApplyResources(build, tr.Spec.Inputs.Resources, c.resourceLister.PipelineResources(t.Namespace), "inputs") + build, err = resources.ApplyResources(build, tr.Spec.Inputs.Resources, c.resourceLister.PipelineResources(tr.Namespace), "inputs") if err != nil { return nil, fmt.Errorf("couldnt apply input resource templating: %s", err) } - build, err = resources.ApplyResources(build, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(t.Namespace), "outputs") + build, err = resources.ApplyResources(build, tr.Spec.Outputs.Resources, c.resourceLister.PipelineResources(tr.Namespace), "outputs") if err != nil { return nil, fmt.Errorf("couldnt apply output resource templating: %s", err) } diff --git a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go index 01316db2df1..9c746aa3ab2 100644 --- a/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go +++ b/pkg/reconciler/v1alpha1/taskrun/taskrun_test.go @@ -334,6 +334,33 @@ func TestReconcile(t *testing.T) { }, }, }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-taskSpec", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Params: []v1alpha1.TaskParam{{ + Name: "myarg", + Description: "mydesc", + Default: "mydefault", + }}, + }, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=${inputs.params.myarg}"}, + }, { + Name: "myothercontainer", + Image: "myotherimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-other-arg=${inputs.resources.git-resource.url}"}, + }}, + }, + }, }} d := test.Data{ @@ -945,3 +972,207 @@ func TestCreateRedirectedBuild(t *testing.T) { t.Errorf("services accounts do not match: %s should be %s", b.Spec.ServiceAccountName, tr.Spec.ServiceAccount) } } + +func TestTaskRunWithTaskSpec(t *testing.T) { + taskruns := []*v1alpha1.TaskRun{{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-taskspec", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + Inputs: v1alpha1.TaskRunInputs{ + Params: []v1alpha1.Param{ + { + Name: "myarg", + Value: "foo", + }, + }, + Resources: []v1alpha1.TaskRunResource{ + { + ResourceRef: v1alpha1.PipelineResourceRef{ + Name: gitResource.Name, + APIVersion: "a1", + }, + Name: "workspace", + }, + }, + }, + TaskSpec: &v1alpha1.TaskSpec{ + Inputs: &v1alpha1.Inputs{ + Resources: []v1alpha1.TaskResource{{ + Type: "git", + Name: "workspace", + }}, + Params: []v1alpha1.TaskParam{{ + Name: "myarg", + Description: "mydesc", + Default: "mydefault", + }}, + }, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=${inputs.params.myarg}"}, + }}, + }, + }, + }, { + ObjectMeta: metav1.ObjectMeta{ + Name: "test-taskrun-with-sources", + Namespace: "foo", + }, + Spec: v1alpha1.TaskRunSpec{ + TaskSpec: &v1alpha1.TaskSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }}, + Steps: []corev1.Container{{ + Name: "mycontainer", + Image: "myimage", + Command: []string{"/mycmd"}, + Args: []string{"--my-arg=foo"}, + }}, + }, + }, + }} + d := test.Data{ + TaskRuns: taskruns, + Tasks: []*v1alpha1.Task{simpleTask, saTask, templatedTask, defaultTemplatedTask}, + PipelineResources: []*v1alpha1.PipelineResource{gitResource, imageResource}, + } + testcases := []struct { + name string + taskRun *v1alpha1.TaskRun + wantedBuildSpec buildv1alpha1.BuildSpec + }{ + { + name: "success0", + taskRun: taskruns[0], + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Source: &buildv1alpha1.SourceSpec{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }, + Steps: []corev1.Container{ + entrypointCopyStep, + { + Name: "mycontainer", + Image: "myimage", + Command: []string{entrypointLocation}, + Args: []string{}, + Env: []corev1.EnvVar{ + { + Name: "ENTRYPOINT_OPTIONS", + Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, + }, + }, + VolumeMounts: []corev1.VolumeMount{toolsMount}, + }, + }, + Volumes: []corev1.Volume{ + getToolsVolume(taskruns[0].Name), + }, + }, + }, + { + name: "success1", + taskRun: taskruns[1], + wantedBuildSpec: buildv1alpha1.BuildSpec{ + Sources: []buildv1alpha1.SourceSpec{{ + Git: &buildv1alpha1.GitSourceSpec{ + Url: "https://foo.git", + Revision: "master", + }, + }}, + Steps: []corev1.Container{ + entrypointCopyStep, + { + Name: "mycontainer", + Image: "myimage", + Command: []string{entrypointLocation}, + Args: []string{}, + Env: []corev1.EnvVar{ + { + Name: "ENTRYPOINT_OPTIONS", + Value: `{"args":["/mycmd","--my-arg=foo"],"process_log":"/tools/process-log.txt","marker_file":"/tools/marker-file.txt"}`, + }, + }, + VolumeMounts: []corev1.VolumeMount{toolsMount}, + }, + }, + Volumes: []corev1.Volume{ + getToolsVolume(taskruns[1].Name), + }, + }, + }, + } + for _, tc := range testcases { + t.Run(tc.name, func(t *testing.T) { + c, _, clients := test.GetTaskRunController(d) + if err := c.Reconciler.Reconcile(context.Background(), getRunName(tc.taskRun)); err != nil { + t.Errorf("expected no error. Got error %v", err) + } + if len(clients.Build.Actions()) == 0 { + t.Errorf("Expected actions to be logged in the buildclient, got none") + } + // check error + build, err := clients.Build.BuildV1alpha1().Builds(tc.taskRun.Namespace).Get(tc.taskRun.Name, metav1.GetOptions{}) + if err != nil { + t.Fatalf("Failed to fetch build: %v", err) + } + if d := cmp.Diff(build.Spec, tc.wantedBuildSpec); d != "" { + t.Errorf("buildspec doesn't match, diff: %s", d) + } + + // This TaskRun is in progress now and the status should reflect that + condition := tc.taskRun.Status.GetCondition(duckv1alpha1.ConditionSucceeded) + if condition == nil || condition.Status != corev1.ConditionUnknown { + t.Errorf("Expected invalid TaskRun to have in progress status, but had %v", condition) + } + if condition != nil && condition.Reason != taskrun.ReasonRunning { + t.Errorf("Expected reason %q but was %s", taskrun.ReasonRunning, condition.Reason) + } + + namespace, name, err := cache.SplitMetaNamespaceKey(tc.taskRun.Name) + if err != nil { + t.Errorf("Invalid resource key: %v", err) + } + //Command, Args, Env, VolumeMounts + if len(clients.Kube.Actions()) == 0 { + t.Fatalf("Expected actions to be logged in the kubeclient, got none") + } + // 3. check that volume was created + pvc, err := clients.Kube.CoreV1().PersistentVolumeClaims(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to fetch build: %v", err) + } + + // get related TaskRun to populate expected PVC + tr, err := clients.Pipeline.PipelineV1alpha1().TaskRuns(namespace).Get(name, metav1.GetOptions{}) + if err != nil { + t.Errorf("Failed to fetch build: %v", err) + } + expectedVolume := getExpectedPVC(tr) + if d := cmp.Diff(pvc.Name, expectedVolume.Name); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if d := cmp.Diff(pvc.OwnerReferences, expectedVolume.OwnerReferences); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if d := cmp.Diff(pvc.Spec.AccessModes, expectedVolume.Spec.AccessModes); d != "" { + t.Errorf("pvc doesn't match, diff: %s", d) + } + if pvc.Spec.Resources.Requests["storage"] != expectedVolume.Spec.Resources.Requests["storage"] { + t.Errorf("pvc doesn't match, got: %v, expected: %v", + pvc.Spec.Resources.Requests["storage"], + expectedVolume.Spec.Resources.Requests["storage"]) + } + }) + } +} diff --git a/pkg/reconciler/v1alpha1/taskrun/validate.go b/pkg/reconciler/v1alpha1/taskrun/validate.go index c3a0ecca114..11ddadb4f76 100644 --- a/pkg/reconciler/v1alpha1/taskrun/validate.go +++ b/pkg/reconciler/v1alpha1/taskrun/validate.go @@ -24,14 +24,17 @@ import ( // validate all references in taskrun exist at runtime func validateTaskRun(c *Reconciler, tr *v1alpha1.TaskRun) error { - // verify task reference exists, all params are provided - // and all inputs/outputs are bound - t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) - if err != nil { - return fmt.Errorf("Error listing task ref %s: %v", - tr.Spec.TaskRef.Name, err) + if tr.Spec.TaskRef != nil { + // verify task reference exists, all params are provided + // and all inputs/outputs are bound + t, err := c.taskLister.Tasks(tr.Namespace).Get(tr.Spec.TaskRef.Name) + if err != nil { + return fmt.Errorf("Error listing task ref %s: %v", + tr.Spec.TaskRef.Name, err) + } + return validateTaskRunAndTask(c, *tr, t, tr.Namespace) } - return validateTaskRunAndTask(c, *tr, t, tr.Namespace) + return nil } //validateTaskRunTask validates task inputs, params and output matches taskrun