Skip to content

Commit

Permalink
Fix handling of case where non-existent resource is referenced
Browse files Browse the repository at this point in the history 
The helm end to end test was trying to bind a Pipeline’s resource to a resource that didn’t exist (my-special-resource). Debugging and fixing this revealed several problems and bugs which we fixed.

* If a PipelineRun didn’t bind the required resource, the reconcile function would continue on anyway (missing return statement!)
* If resolving a PipelineRun failed for any reason other than that a referenced Task couldn’t be find, the reconciler would pick up the PipelineRun again and continue trying to re-reconcile it. Now any resolution error will result in the PipelineRun being considered invalid and stopping.

The above all snuck in because @bobcatfish didn’t pay enough attention to covering the new functionality in integration tests at the reconcile level and has learned her lesson 😅

In the helm test itself, when the PipelineRun failed, we continued on, which led to tests taking much longer than needed to fail and also getting into a weird state (e.g. removing helm would fail because it was never set up). Now we stop the test immediately if the PipelineRun fails (though this could result in some desired cleanup in the cluster not occurring).

(It also looked like a test case at the reconciler level, which handled missing parameters, got wiped out at some point, so that’s back now.)

Co-authored-by: Nader Ziada <[email protected]>
  • Loading branch information
@bobcatfish @nader-ziada @knative-prow-robot
2 people authored and knative-prow-robot committed Jan 24, 2019
1 parent 3294a76 commit 65cf1cb
Show file tree
Hide file tree
Showing 5 changed files with 147 additions and 25 deletions.
29 changes: 25 additions & 4 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ const (
// ReasonCouldntGetTask indicates that the reason for the failure status is that the
// associated Pipeline's Tasks couldn't all be retrieved
ReasonCouldntGetTask = "CouldntGetTask"
// ReasonCouldntGetResource indicates that the reason for the failure status is that the
// associated PipelineRun's bound PipelineResources couldn't all be retrieved
ReasonCouldntGetResource = "CouldntGetResource"
// ReasonFailedValidation indicated that the reason for failure status is
// that pipelinerun failed runtime validation
ReasonFailedValidation = "PipelineValidationFailed"
Expand Down Expand Up @@ -200,6 +203,7 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
Message: fmt.Sprintf("PipelineRun %s doesn't bind Pipeline %s's PipelineResources correctly: %s",
fmt.Sprintf("%s/%s", pr.Namespace, pr.Name), fmt.Sprintf("%s/%s", pr.Namespace, pr.Spec.PipelineRef.Name), err),
})
return nil
}

pipelineState, err := resources.ResolvePipelineRun(
Expand All @@ -213,19 +217,36 @@ func (c *Reconciler) reconcile(ctx context.Context, pr *v1alpha1.PipelineRun) er
c.resourceLister.PipelineResources(pr.Namespace).Get,
p.Spec.Tasks, providedResources,
)

if err != nil {
if errors.IsNotFound(err) {
// This Run has failed, so we need to mark it as failed and stop reconciling it
// This Run has failed, so we need to mark it as failed and stop reconciling it
switch err := err.(type) {
case *resources.TaskNotFoundError:
pr.Status.SetCondition(&duckv1alpha1.Condition{
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetTask,
Message: fmt.Sprintf("Pipeline %s can't be Run; it contains Tasks that don't exist: %s",
fmt.Sprintf("%s/%s", p.Namespace, p.Name), err),
})
case *resources.ResourceNotFoundError:
pr.Status.SetCondition(&duckv1alpha1.Condition{
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonCouldntGetResource,
Message: fmt.Sprintf("PipelineRun %s can't be Run; it tries to bind Resources that don't exist: %s",
fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err),
})
default:
pr.Status.SetCondition(&duckv1alpha1.Condition{
Type: duckv1alpha1.ConditionSucceeded,
Status: corev1.ConditionFalse,
Reason: ReasonFailedValidation,
Message: fmt.Sprintf("PipelineRun %s can't be Run; couldn't resolve all references: %s",
fmt.Sprintf("%s/%s", p.Namespace, pr.Name), err),
})
return nil
}
return fmt.Errorf("error getting Tasks for Pipeline %s: %s", p.Name, err)
return nil
}

if err := resources.ValidateProvidedBy(pipelineState); err != nil {
Expand Down
38 changes: 34 additions & 4 deletions pkg/reconciler/v1alpha1/pipelinerun/pipelinerun_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,19 +229,33 @@ func TestReconcile(t *testing.T) {
}

func TestReconcile_InvalidPipelineRuns(t *testing.T) {
ts := []*v1alpha1.Task{tb.Task("a-task-that-exists", "foo")}
ts := []*v1alpha1.Task{
tb.Task("a-task-that-exists", "foo"),
tb.Task("a-task-that-needs-params", "foo", tb.TaskSpec(
tb.TaskInputs(tb.InputsParam("some-param")))),
}
ps := []*v1alpha1.Pipeline{
tb.Pipeline("pipeline-missing-tasks", "foo", tb.PipelineSpec(
tb.PipelineTask("myspecialtask", "sometask"),
)),
tb.Pipeline("a-pipeline-without-params", "foo", tb.PipelineSpec(
tb.PipelineTask("some-task", "a-task-that-needs-params"))),
tb.Pipeline("a-fine-pipeline", "foo", tb.PipelineSpec(
tb.PipelineTask("some-task", "a-task-that-exists"),
)),
tb.PipelineDeclaredResource("a-resource", v1alpha1.PipelineResourceTypeGit),
tb.PipelineTask("some-task", "a-task-that-exists",
tb.PipelineTaskInputResource("needed-resource", "a-resource")))),
tb.Pipeline("a-pipeline-that-should-be-caught-by-admission-control", "foo", tb.PipelineSpec(
tb.PipelineTask("some-task", "a-task-that-exists",
tb.PipelineTaskInputResource("needed-resource", "a-resource")))),
}
prs := []*v1alpha1.PipelineRun{
tb.PipelineRun("invalid-pipeline", "foo", tb.PipelineRunSpec("pipeline-not-exist")),
tb.PipelineRun("pipelinerun-missing-tasks", "foo", tb.PipelineRunSpec("pipeline-missing-tasks")),
tb.PipelineRun("pipeline-params-dont-exist", "foo", tb.PipelineRunSpec("a-fine-pipeline")),
tb.PipelineRun("pipeline-params-dont-exist", "foo", tb.PipelineRunSpec("a-pipeline-without-params")),
tb.PipelineRun("pipeline-resources-not-bound", "foo", tb.PipelineRunSpec("a-fine-pipeline")),
tb.PipelineRun("pipeline-resources-dont-exist", "foo", tb.PipelineRunSpec("a-fine-pipeline",
tb.PipelineRunResourceBinding("a-resource", tb.PipelineResourceBindingRef("missing-resource")))),
tb.PipelineRun("pipeline-resources-not-declared", "foo", tb.PipelineRunSpec("a-pipeline-that-should-be-caught-by-admission-control")),
}
d := test.Data{
Tasks: ts,
Expand All @@ -261,6 +275,22 @@ func TestReconcile_InvalidPipelineRuns(t *testing.T) {
name: "invalid-pipeline-run-missing-tasks-shd-stop-reconciling",
pipelineRun: prs[1],
reason: ReasonCouldntGetTask,
}, {
name: "invalid-pipeline-run-params-dont-exist-shd-stop-reconciling",
pipelineRun: prs[2],
reason: ReasonFailedValidation,
}, {
name: "invalid-pipeline-run-resources-not-bound-shd-stop-reconciling",
pipelineRun: prs[3],
reason: ReasonInvalidBindings,
}, {
name: "invalid-pipeline-run-missing-resource-shd-stop-reconciling",
pipelineRun: prs[4],
reason: ReasonCouldntGetResource,
}, {
name: "invalid-pipeline-missing-declared-resource-shd-stop-reconciling",
pipelineRun: prs[5],
reason: ReasonFailedValidation,
},
}

Expand Down
28 changes: 24 additions & 4 deletions pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,25 @@ func getPipelineRunTaskResources(pt v1alpha1.PipelineTask, providedResources map
return inputs, outputs, nil
}

// TaskNotFoundError indicates that the resolution failed because a referenced Task couldn't be retrieved
type TaskNotFoundError struct {
Name string
Msg string
}

func (e *TaskNotFoundError) Error() string {
return fmt.Sprintf("Couldn't retrieve Task %q: %s", e.Name, e.Msg)
}

// ResourceNotFoundError indicates that the resolution failed because a referenced PipelineResource couldn't be retrieved
type ResourceNotFoundError struct {
Msg string
}

func (e *ResourceNotFoundError) Error() string {
return fmt.Sprintf("Couldn't retrieve PipelineResource: %s", e.Msg)
}

// ResolvePipelineRun retrieves all Tasks instances which are reference by tasks, getting
// instances from getTask. If it is unable to retrieve an instance of a referenced Task, it
// will return an error, otherwise it returns a list of all of the Tasks retrieved.
Expand All @@ -157,9 +176,10 @@ func ResolvePipelineRun(prName string, getTask resources.GetTask, getClusterTask
t, err = getTask(pt.TaskRef.Name)
}
if err != nil {
// If the Task can't be found, it means the PipelineRun is invalid. Return the same error
// type so it can be used by the caller.
return nil, err
return nil, &TaskNotFoundError{
Name: pt.TaskRef.Name,
Msg: err.Error(),
}
}

// Get all the resources that this task will be using, if any
Expand All @@ -171,7 +191,7 @@ func ResolvePipelineRun(prName string, getTask resources.GetTask, getClusterTask
spec := t.TaskSpec()
rtr, err := resources.ResolveTaskResources(&spec, t.TaskMetadata().Name, inputs, outputs, getResource)
if err != nil {
return nil, fmt.Errorf("couldn't resolve task resources for task %q: %v", t.TaskMetadata().Name, err)
return nil, &ResourceNotFoundError{Msg: err.Error()}
}
rprt.ResolvedTaskResources = rtr

Expand Down
64 changes: 57 additions & 7 deletions pkg/reconciler/v1alpha1/pipelinerun/resources/pipelinerunresolution_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -449,15 +449,17 @@ func TestResolvePipelineRun_TaskDoesntExist(t *testing.T) {
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return nil, fmt.Errorf("should not get called") }

_, err := ResolvePipelineRun("pipelinerun", getTask, getClusterTask, getResource, pts, providedResources)
if err == nil {
switch err := err.(type) {
case nil:
t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.Name)
}
if !errors.IsNotFound(err) {
t.Fatalf("Expected same error type returned by func for non-existent Task for Pipeline %s but got %s", p.Name, err)
case *TaskNotFoundError:
// expected error
default:
t.Fatalf("Expected specific error type returned by func for non-existent Task for Pipeline %s but got %s", p.Name, err)
}
}

func TestResolvePipelineRun_ResourcesDontExist(t *testing.T) {
func TestResolvePipelineRun_ResourceBindingsDontExist(t *testing.T) {
tests := []struct {
name string
p *v1alpha1.Pipeline
Expand All @@ -483,17 +485,65 @@ func TestResolvePipelineRun_ResourcesDontExist(t *testing.T) {

getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil }
getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return clustertask, nil }
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return nil, fmt.Errorf("should not get called") }
getResource := func(name string) (*v1alpha1.PipelineResource, error) { return nil, fmt.Errorf("shouldnt be called") }

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := ResolvePipelineRun("pipelinerun", getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
if err == nil {
t.Fatalf("Expected error getting non-existent Tasks for Pipeline %s but got none", p.Name)
t.Fatalf("Expected error when bindings are in incorrect state for Pipeline %s but got none", p.Name)
}
})
}
}

func TestResolvePipelineRun_ResourcesDontExist(t *testing.T) {
tests := []struct {
name string
p *v1alpha1.Pipeline
}{
{
name: "input doesnt exist",
p: tb.Pipeline("pipelines", "namespace", tb.PipelineSpec(
tb.PipelineTask("mytask1", "task",
tb.PipelineTaskInputResource("input1", "git-resource"),
),
)),
},
{
name: "output doesnt exist",
p: tb.Pipeline("pipelines", "namespace", tb.PipelineSpec(
tb.PipelineTask("mytask1", "task",
tb.PipelineTaskOutputResource("input1", "git-resource"),
),
)),
},
}
providedResources := map[string]v1alpha1.PipelineResourceRef{
"git-resource": v1alpha1.PipelineResourceRef{
Name: "doesnt-exist",
},
}

getTask := func(name string) (v1alpha1.TaskInterface, error) { return task, nil }
getClusterTask := func(name string) (v1alpha1.TaskInterface, error) { return clustertask, nil }
getResource := func(name string) (*v1alpha1.PipelineResource, error) {
return nil, errors.NewNotFound(v1alpha1.Resource("pipelineresource"), name)
}

for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
_, err := ResolvePipelineRun("pipelinerun", getTask, getClusterTask, getResource, tt.p.Spec.Tasks, providedResources)
switch err := err.(type) {
case nil:
t.Fatalf("Expected error getting non-existent Resources for Pipeline %s but got none", p.Name)
case *ResourceNotFoundError:
// expected error
default:
t.Fatalf("Expected specific error type returned by func for non-existent Resource for Pipeline %s but got %s", p.Name, err)
}
})
}
}

func TestResolveTaskRuns_AllStarted(t *testing.T) {
Expand Down
13 changes: 7 additions & 6 deletions test/helm_task_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ func TestHelmDeployPipelineRun(t *testing.T) {
}

logger.Infof("Creating Task %s", createImageTaskName)
if _, err := c.TaskClient.Create(getCreateImageTask(namespace, t)); err != nil {
if _, err := c.TaskClient.Create(getCreateImageTask(namespace, t, logger)); err != nil {
t.Fatalf("Failed to create Task `%s`: %s", createImageTaskName, err)
}

Expand Down Expand Up @@ -96,6 +96,7 @@ func TestHelmDeployPipelineRun(t *testing.T) {
CollectBuildLogs(c, tr.Status.PodName, namespace, logger)
}
}
t.Fatalf("PipelineRun execution failed; helm may or may not have been installed :(")
}

logger.Info("Waiting for service to get external IP")
Expand Down Expand Up @@ -145,7 +146,7 @@ func getGoHelloworldGitResource(namespace string) *v1alpha1.PipelineResource {
))
}

func getCreateImageTask(namespace string, t *testing.T) *v1alpha1.Task {
func getCreateImageTask(namespace string, t *testing.T, logger *logging.BaseLogger) *v1alpha1.Task {
// according to knative/test-infra readme (https://github.com/knative/test-infra/blob/13055d769cc5e1756e605fcb3bcc1c25376699f1/scripts/README.md)
// the KO_DOCKER_REPO will be set with according to the project where the cluster is created
// it is used here to dynamically get the docker registry to push the image to
Expand All @@ -155,7 +156,7 @@ func getCreateImageTask(namespace string, t *testing.T) *v1alpha1.Task {
}

imageName = fmt.Sprintf("%s/%s", dockerRepo, AppendRandomString(sourceImageName))
t.Logf("Image to be pusblished: %s", imageName)
logger.Infof("Image to be pusblished: %s", imageName)

return tb.Task(createImageTaskName, namespace, tb.TaskSpec(
tb.TaskInputs(tb.InputsResource("gitsource", v1alpha1.PipelineResourceTypeGit)),
Expand Down Expand Up @@ -193,8 +194,8 @@ func getHelmDeployPipeline(namespace string) *v1alpha1.Pipeline {
tb.PipelineTaskInputResource("gitsource", "git-repo"),
),
tb.PipelineTask("helm-deploy", helmDeployTaskName,
tb.PipelineTaskInputResource("gitsource", "git-repo", tb.ProvidedBy(createImageTaskName)),
tb.PipelineTaskParam("pathToHelmCharts", "/workspace/test/gohelloworld/gohelloworld-chart"),
tb.PipelineTaskInputResource("gitsource", "git-repo"),
tb.PipelineTaskParam("pathToHelmCharts", "/workspace/gitsource/test/gohelloworld/gohelloworld-chart"),
tb.PipelineTaskParam("chartname", "gohelloworld"),
tb.PipelineTaskParam("image", imageName),
),
Expand All @@ -204,7 +205,7 @@ func getHelmDeployPipeline(namespace string) *v1alpha1.Pipeline {
func getHelmDeployPipelineRun(namespace string) *v1alpha1.PipelineRun {
return tb.PipelineRun(helmDeployPipelineRunName, namespace, tb.PipelineRunSpec(
helmDeployPipelineName,
tb.PipelineRunResourceBinding("git-repo", tb.PipelineResourceBindingRef("my-special-resource")),
tb.PipelineRunResourceBinding("git-repo", tb.PipelineResourceBindingRef(sourceResourceName)),
))
}

Expand Down

0 comments on commit 65cf1cb

Please sign in to comment.