Reduce PipelineResource bindings #320

bobcatfish · 2018-12-08T01:29:54Z

Expected Behavior

Users should be able to effectively say something like this in a PipelineRun:

“For the Pipeline, use git resource W, image resources Y + X, and deploy to cluster Z.”

Actual Behavior

The current reality is that users have to provide theses values repeatedly, more like:

“For Task A use git resource W and image X, for Task B use git resource W and image Y, for Task C use git resource W and image Y and image X and cluster Z…”

For example in this PipelineRun, we provide bindings for build-skaffold-web’s workspace and builtImage PipelineResources, and for deploy-web’s workspace and image PipelineResources.

apiVersion: pipeline.knative.dev/v1alpha1
kind: PipelineRun
metadata:
  name: demo-pipeline-run-1
spec:
  pipelineRef:
    name: demo-pipeline
  triggerRef:
    type: manual
  resources:
  - name: build-skaffold
    inputs:
    - name: workspace
      resourceRef:
        name: skaffold-git
    outputs:
    - name: image1
      resourceRef:
        name: skaffold-image-leeroy-web
    - name: image2
      resourceRef:
        name: skaffold-image-leeroy-app
   - name: deploy-web
    inputs:
    - name: workspace
      resourceRef:
        name: skaffold-git
    - name: image
      resourceRef:
        name: skaffold-image-leeroy-web

However, when a Pipeline uses the providedBy field to link the output of one Task to the input of another, it should be possible to infer the PipelineResource to use for the Task from the providedBy field.

For example in this Pipeline, we bind the output of the build-skaffold-web Task to the input PipelineResource called image for the deploy-web Task.

apiVersion: pipeline.knative.dev/v1alpha1
kind: Pipeline
metadata:
  name: demo-pipeline
spec:
  tasks:
  - name: build-skaffold
    taskRef:
      name: build-push-images
  - name: deploy-web
    taskRef:
      name: demo-deploy-kubectl
    resources:
    - name: image
      providedBy:
      - build-skaffold-web

Additional Info

This is related to these issues:

Design Output handling #124 - Designing output handling
Replace Pipeline linear execution with graph based execution #168 - Implementing DAG execution

The text was updated successfully, but these errors were encountered:

bobcatfish · 2018-12-08T01:31:37Z

This doc has some proposed solutions(note doc visible to members of knative-dev@googlegroups.com)

I'm currently leaning toward alternative #1, but it means significantly changing providedBy (second place to the "proposal" itself).

Very interested in ppl's thoughts + @shashwathi @pivotal-nader-ziada @imjasonh @tejal29 @aaron-prindle

@dlorenc

While working on creating an end to end test for tektoncd#168, @dlorenc and I had a very hard time determining why our pipeline wasn't working as expected - it turned out that we were using the wrong resource name in our `providedBy` clauses. While adding this validation I _really_ struggled to understand how the `providedBy` clause actually works. I've updated the docs with my best understanding of how it is currently working. Fortunately in tektoncd#320 we will be simplifying how this works! This is follow-up work for #124

@dlorenc

While working on creating an end to end test for tektoncd#168, @dlorenc and I had a very hard time determining why our pipeline wasn't working as expected - it turned out that we were using the wrong resource name in our `providedBy` clauses. While adding this validation I _really_ struggled to understand how the `providedBy` clause actually works. I've updated the docs with my best understanding of how it is currently working. Fortunately in tektoncd#320 we will be simplifying how this works! This is follow-up work for #124

@dlorenc

While working on creating an end to end test for #168, @dlorenc and I had a very hard time determining why our pipeline wasn't working as expected - it turned out that we were using the wrong resource name in our `providedBy` clauses. While adding this validation I _really_ struggled to understand how the `providedBy` clause actually works. I've updated the docs with my best understanding of how it is currently working. Fortunately in #320 we will be simplifying how this works! This is follow-up work for #124

@vdemeester

In tektoncd#342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for tektoncd#320 and tektoncd#168 (maybe) so I'm inclined to wait - open to other ideas tho!

@vdemeester

In tektoncd#342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for tektoncd#320 and tektoncd#168 (maybe) so I'm inclined to wait - open to other ideas tho!

bobcatfish · 2019-01-04T00:16:50Z

Thanks for all the input on the doc! So right now it's looking like alternative #1.b is the way to go.

The distinct changes that will be required:

Adding a resources section to the Pipeline spec, which will be the values we bind to in PipelineRun (Declare resources in pipeline 😇 #415)
Changing providedBy to from (Rename providedBy to from #437)
Changing triggerRef to trigger (maybe also changing pipelineRef in PipelineRun to pipeline and taskRef in taskRun to task but I'm going to leave those for now) (Use trigger instead of triggerRef #366)
~~Adding runAfter as a way to express ordering when linking resources isn't appropriate~~ I'm going to open a separate issue/ update Replace Pipeline linear execution with graph based execution #168 about this

@sebgoa

We use the syntax `somethingRef` in several fields on our specs because we want to be able to use ducktyping down the line to switch out other types (see tektoncd#238). However `triggerRef` is a bit weird because: - If the type is `manual`, there is no actual CRD to reference - We don't know what other sorts of types we might need (e.g. when created via events) - It was inconsistent b/w TaskRun and PipelineRun (TaskRun had an extra level of indirection) This commit applies @sebgoa's suggestion to simplify this to just `trigger`. This was part of the discussion in tektoncd#320 about simplifying the interfaace.

@vdemeester

In #342 I added docs and validation for the `providedBy` field, but I missed some feedback from @vdemeester before the PR was merged, so I'm addressing it now! The change to use the builders, even just for `PipelineResources`, removes a surprising amount of boilerplate! It doesn't seem like I can really use the builders for constructing the `state` object (slice of `ResolvedPipelineRunTask`) unless we add builder for `ResolvedPipelineRunTask`. That might be a good idea, however I'm planning to iterate on some of this for #320 and #168 (maybe) so I'm inclined to wait - open to other ideas tho!

@sebgoa

We use the syntax `somethingRef` in several fields on our specs because we want to be able to use ducktyping down the line to switch out other types (see #238). However `triggerRef` is a bit weird because: - If the type is `manual`, there is no actual CRD to reference - We don't know what other sorts of types we might need (e.g. when created via events) - It was inconsistent b/w TaskRun and PipelineRun (TaskRun had an extra level of indirection) This commit applies @sebgoa's suggestion to simplify this to just `trigger`. This was part of the discussion in #320 about simplifying the interfaace.

Now resources will be bound to a "name" which is used in the `Pipeline` to refer to them. This will simplify the binding declarations in `PipelineRun` and also make the `Pipeline` definition solely responsible for linking resources to Tasks. Toward tektoncd#320

This updates the API and supporting logic to allow Pipelines to declare the resources they will use, and provide placeholder names for them, which PipelineRuns can bind to. This significantly simplifies PipelineRun binding logic. This is toward fixing tektoncd#320

Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see tektoncd#168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of tektoncd#320

Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of tektoncd#320

Now resources will be bound to a "name" which is used in the `Pipeline` to refer to them. This will simplify the binding declarations in `PipelineRun` and also make the `Pipeline` definition solely responsible for linking resources to Tasks. Toward tektoncd#320

This updates the API and supporting logic to allow Pipelines to declare the resources they will use, and provide placeholder names for them, which PipelineRuns can bind to. This significantly simplifies PipelineRun binding logic. This is toward fixing tektoncd#320

Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see tektoncd#168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of tektoncd#320

Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of tektoncd#320

Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see tektoncd#168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of tektoncd#320

Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of tektoncd#320

Before tektoncd#320, `providedBy` would be declared on "resources" in general, and so it was possible that this could be declared on an output. Now the schema will only allow `providedBy` to be declared on inputs and so it is no longer possible to get into this position.

This updates the API and supporting logic to allow Pipelines to declare the resources they will use, and provide placeholder names for them, which PipelineRuns can bind to. This significantly simplifies PipelineRun binding logic. This is toward fixing tektoncd#320

Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see tektoncd#168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of tektoncd#320

Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of tektoncd#320

Before tektoncd#320, `providedBy` would be declared on "resources" in general, and so it was possible that this could be declared on an output. Now the schema will only allow `providedBy` to be declared on inputs and so it is no longer possible to get into this position.

The PipelineRun controller creates TaskResourceBindings by iterating over maps, and iteration over maps is non-deterministic. The order of these values in the result doesn't matter, so this commit updates the tests to compare them regardless of order. Part of tektoncd#320

Now resources will be bound to a "name" which is used in the `Pipeline` to refer to them. This will simplify the binding declarations in `PipelineRun` and also make the `Pipeline` definition solely responsible for linking resources to Tasks. Toward #320

This updates the API and supporting logic to allow Pipelines to declare the resources they will use, and provide placeholder names for them, which PipelineRuns can bind to. This significantly simplifies PipelineRun binding logic. This is toward fixing #320

Tasks should only rely on outputs of tasks that execute before them, they shouldn't be able to rely on outputs of future tasks. This also means we no longer need the `canRunTask` function: since Tasks currently execute linearly (see #168), the only way `canRunTask` could return false would be if `Tasks` in `providedBy` actually executed _after_ the one currently being evaluated, which would result in an unrunnable and invalid Pipeline. Now we will catch this on Pipeline creation. Part of #320

Now that we're declaring Resources, we can do some extra validation on creation that used to need to wait until only runtime. Also generalized the logic for looking for missing and extra values - though this will probably make the error messages a bit less helpful. Part of #320

Before #320, `providedBy` would be declared on "resources" in general, and so it was possible that this could be declared on an output. Now the schema will only allow `providedBy` to be declared on inputs and so it is no longer possible to get into this position.

The PipelineRun controller creates TaskResourceBindings by iterating over maps, and iteration over maps is non-deterministic. The order of these values in the result doesn't matter, so this commit updates the tests to compare them regardless of order. Part of #320

@tejal29

We completed the 3 user studies we intended to do, so we don't need to maintain these exmaples any longer. (I realized that I had missed the examples with some of the API changes from tektoncd#320, and after talking with @tejal29 it seems reasonable to remove these now instead of continuing to maintain them.)

@sebgoa

`providedBy` was a way for users to indicate when a Tasks's input should come from the output of another Task - including any changes that Task had made to it (e.g. adding files to a checkout of a git repo by building a binary). We have struggled with an intuitive way to describe it - orginally this was called `passedConstraint`. In the design doc for tektoncd#320, @sebgoa suggested we call this `from` which seems like the clearest name yet! Fixes tektoncd#320 (remaining item in tektoncd#320 to add `runAfter` will be moved to another issue)

@tejal29

We completed the 3 user studies we intended to do, so we don't need to maintain these exmaples any longer. (I realized that I had missed the examples with some of the API changes from #320, and after talking with @tejal29 it seems reasonable to remove these now instead of continuing to maintain them.)

@sebgoa

`providedBy` was a way for users to indicate when a Tasks's input should come from the output of another Task - including any changes that Task had made to it (e.g. adding files to a checkout of a git repo by building a binary). We have struggled with an intuitive way to describe it - orginally this was called `passedConstraint`. In the design doc for tektoncd#320, @sebgoa suggested we call this `from` which seems like the clearest name yet! Fixes tektoncd#320 (remaining item in tektoncd#320 to add `runAfter` will be moved to another issue)

@sebgoa

`providedBy` was a way for users to indicate when a Tasks's input should come from the output of another Task - including any changes that Task had made to it (e.g. adding files to a checkout of a git repo by building a binary). We have struggled with an intuitive way to describe it - orginally this was called `passedConstraint`. In the design doc for #320, @sebgoa suggested we call this `from` which seems like the clearest name yet! Fixes #320 (remaining item in #320 to add `runAfter` will be moved to another issue)

Three caveats to this integration test: - It was created before tektoncd#320, so the resource binding is not correct - It was created before tektoncd#387, so it relies on the log PVC which will no longer exist (can work around this by mounting a PVC explicitly in the test and writing directly to it instead of echoing?) - It doesn't exercise `runAfter` functionality

This tests DAG functionality by defining a pipeline with both fan in and fan out. The idea is that each Task echoes the current time, so after the pipeline compeletes, we can look at which Task echoes which which time to make sure they run in order. The tasks are also declared in the Pipeline in the wrong order on purpose, to make sure that the order of declaration doesn't affect how they are run (the opposite of the current functionality) Three caveats to this integration test: - It was created before tektoncd#320, so the resource binding is not correct - It was created before tektoncd#387, so it relies on the log PVC which will no longer exist (can work around this by mounting a PVC explicitly in the test and writing directly to it instead of echoing?) - It doesn't exercise `runAfter` functionality

bobcatfish added this to the (0.3 Alpha release) Docs + Output Resources milestone Dec 8, 2018

bobcatfish self-assigned this Dec 8, 2018

nader-ziada mentioned this issue Dec 11, 2018

Initial attempt at a Getting Started tutorial #327

Merged

bobcatfish mentioned this issue Dec 20, 2018

Add docs and validation for providedBy #342

Merged

bobcatfish mentioned this issue Jan 3, 2019

Use test builders and fix providedBy example 👷‍♀️ #365

Merged

bobcatfish mentioned this issue Jan 4, 2019

Use trigger instead of triggerRef #366

Merged

bobcatfish added design This task is about creating and discussing a design meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given labels Jan 10, 2019

bobcatfish mentioned this issue Jan 10, 2019

Audit and update docs to only contain functionality that actually works for 0.4 release #386

Closed

bobcatfish mentioned this issue Jan 22, 2019

Declare resources in pipeline 😇 #415

Merged

bobcatfish changed the title ~~Reduce PipelineResource bindings~~ Reduce PipelineResource bindings 😇 Jan 22, 2019

bobcatfish changed the title ~~Reduce PipelineResource bindings 😇~~ Reduce PipelineResource bindings Jan 22, 2019

bobcatfish modified the milestones: (0.3 Alpha release) Docs + Output Resources, 0.4 Knative Release Jan 23, 2019

bobcatfish added a commit to bobcatfish/pipeline that referenced this issue Jan 24, 2019

Update output examples to use new API (tektoncd#320)

c0f80e9

knative-prow-robot pushed a commit that referenced this issue Jan 24, 2019

Update output examples to use new API (#320)

d34c72d

bobcatfish mentioned this issue Jan 24, 2019

Replace Pipeline linear execution with graph based execution #168

Closed

bobcatfish mentioned this issue Jan 24, 2019

Remove user study examples #436

Merged

bobcatfish mentioned this issue Jan 24, 2019

Rename providedBy to from #437

Merged

knative-prow-robot closed this as completed in #437 Jan 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reduce PipelineResource bindings #320

Reduce PipelineResource bindings #320

bobcatfish commented Dec 8, 2018

bobcatfish commented Dec 8, 2018

bobcatfish commented Jan 4, 2019 •

edited

Loading

Reduce PipelineResource bindings #320

Reduce PipelineResource bindings #320

Comments

bobcatfish commented Dec 8, 2018

Expected Behavior

Actual Behavior

Additional Info

bobcatfish commented Dec 8, 2018

bobcatfish commented Jan 4, 2019 • edited Loading

bobcatfish commented Jan 4, 2019 •

edited

Loading