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.

Sign up for GitHub

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

Jump to bottom

Initial design ideas for Source #39

Merged
merged 10 commits into from
Sep 20, 2018
Merged

Initial design ideas for Source #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
1557bd9
initial design ideas for source
nader-ziada Sep 20, 2018
d13a27d
changes to implement Resource concept
nader-ziada Sep 20, 2018
3d7f61c
remove extra function in resource interface
nader-ziada Sep 20, 2018
d092a46
Update JSON struct tag to pass go vet
nader-ziada Sep 20, 2018
498ed55
changes to make tests work
nader-ziada Sep 20, 2018
35c8873
minor readme changes
nader-ziada Sep 20, 2018
f216482
remove resource ref, update configs and samples
nader-ziada Sep 20, 2018
87a357a
Add resources as CRD
nader-ziada Sep 20, 2018
b21141f
Rename task inputs & outputs
nader-ziada Sep 20, 2018
77352df
Merge branch 'master' into design
bobcatfish Sep 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ The CRDs involved are:
* [PipelineParams](#pipelineparams)
* [TaskRun](#taskrun)
* [PipelineRun](#pipelinerun)
* [Resources](#resources)

High level details of this design:

Expand All @@ -41,6 +42,7 @@ High level details of this design:
easily is powerful (e.g. see failures easily, dig into logs, e.g. like
[the Jenkins test analyzer plugin](https://wiki.jenkins.io/display/JENKINS/Test+Results+Analyzer+Plugin))
* [Tasks](#tasks) can depend on artifacts, output and parameters created by other tasks.
* [Resources](#resources) are the artifacts used as inputs and outputs of TaskRuns.

## Task

Expand All @@ -57,9 +59,7 @@ with additional input types and clearly defined outputs.

`Pipeline` describes a graph of [Tasks](#task) to execute. It defines the DAG
and expresses how all inputs (including [PipelineParams](#pipelineparams) and outputs
from previous `Tasks`) feed into each `Task`. It allows for fan in and fan out, and
ordering can be expressed explicitly using `prev` and `next`, or it can be inferred
from a `Task’s` inputs.
from previous `Tasks`) feed into each `Task`.

Dependencies between parameters or inputs/outputs are expressed as references to k8s objects.

Expand All @@ -70,9 +70,7 @@ can be invoked with many different instances of `PipelineParams`, which can allo
for scenarios such as running against PRs and against a user’s personal setup.
`PipelineParams` can control:

* What **sources** the `Pipeline` runs against
* Which **serviceAccount** to use (provided to all tasks)
* What **artifact** stores are used (e.g. Docker registries)
* Where **results** are stored (e.g. in GCS)

## TaskRun
Expand Down Expand Up @@ -129,3 +127,16 @@ completes (or fails).
When the `PipelineRun` has completed, the `taskRuns` field will contain
references to all `TaskRuns` which were executed and their next and
previous `TaskRuns`.

### Resources

`Resources` in a pipelines are the set of objects that are going to be used
as inputs and outputs of a `TaskRun`.

* `Resources` is created directly in a pipeline configuration and bound
to `TaskRun` as an input and/or output source.
* The (optional) `passedConstraint` key on an `input source` defines a set of previous task names.
* When the `passedConstraint` key is specified on an input source, only the version of
Copy link
Contributor

@tejal29 tejal29 Sep 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Having a hard time wrapping my head around “passedConstraints” bound to “source”
Example: https://github.com/knative/build-pipeline/blob/cf8546cc88b6ee298f99b6f64b63d5db8acb78c2/examples/pipelines/guestbook.yaml#L79
Specifically in this case, we have 1 source key but 2 passedConstraints.
Which source version do i pick?

I see a CI Pipeline defined like this:
Where, sources are input to a pipeline and then something comes out of the pipeline. It could be a production ready image or an app.
Pipeline consists of tasks and each task executes some steps on the source and then produce resources like (images, jars, test results) etc.
The pipeline will be run, managed by a controller. Let us say we call it PipelineController.

In my head Pipeline looks like this
Each PipelineRun will have a RunContext which will store the state of the pipeline.
The PipelineController will add the inputs and outputs produced along the way are added to RunContext along with their versions.
Now, when Task1 completes, the PipelineController should invoke Task2. It will refer the versions of the inputs and outputs in the context and pass the required artifacts.
This behaviour should be “implicit”.
We do not want different versions of the same source to be running in the same pipeline run.
Adding a passingConstraint which is optional at the source level may actually lead to such scenarios.
Hence, i feel moving passedConstraint to task level makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

The thinking behind passedConstraint in the source is the following:

  • We are trying to make it explicit that the version of the resource stays the same across multiple tasks in a pipelineRun. By adding the passedConstraint key on the InputSource that means that the input of that task has to be the same version that passed the previous task(s) mentioned in constraint.
  • This allows for the option of another resource (for example a test only repo) that always wants to get latest in each run and doesn't need the same version
  • From our experience, this is a common use case when we have a set of acceptance tests or integrations tests in a separate repo and therefore in a different input source
  • This gives the user defining the pipeline a little more flexibility
  • We would not have different versions of the same resource running with different versions since we said the minimum restart unit is the pipelineRun, but the passedConstraint allows a PipelineController to validate that the test task is running with the image generated from the build task and not latest that was already in the pipeline context from another previous task or in the register from a previous pipelineRun.

@tejal29 what do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clarifying @pivotal-nader-ziada

We are trying to make it explicit that the version of the resource stays the same across multiple tasks in a pipelineRun. By adding the passedConstraint key on the InputSource that means that the input of that task has to be the same version that passed the previous task(s) mentioned in constraint.
This allows for the option of another resource (for example a test only repo) that always wants to get latest in each run and doesn't need the same version

Ok, this makes things more clear now. I think, we just have defaults flipped. If you have a resource defined like this, i was more inclined towards overriding the resource version.

resource:
    - name: guestbook
      type: git
      url: github.com/kubernetes/examples
      serviceAccount: githubServiceAccount
      revision: HEAD

e.g. The first task in the pipeline pulling in guestbook will use revision HEAD and lock its sha. The next tasks in pipeline which depend on resource guestbook will override the revision with the locked sha built by 1st task in the pipeline.
I like your approach where you explicitly specify the passedConstraint.
However, we are now relied on pipeline owners to make sure they correctly configure it. We can work around it by adding some "intelligence" in validators and spinning out warning if we see patterns where pipeline-creators are pulling in the same resource in two tasks but not using passedConstraint.

the resource that passed through the defined list of tasks is used.
* The `passedConstraint` allows for `Tasks` to fan in and fan out, and ordering can be expressed explicitly
using this key since a task needing a resource from a another task would have to run after.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any interest in updating the README Pipeline overview image to the one you presented today @shashwathi @pivotal-nader-ziada ? And adding the resources v. pipelineparams image?

(I'm happy to add these if you'd prefer)

Copy link
Member Author

Choose a reason for hiding this comment

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

The same diagrams are in the PR branch. Should be updated. Feel free to add them in any other location you prefer

71 changes: 38 additions & 33 deletions config/crds/pipeline_v1beta1_pipeline.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,40 +22,60 @@ spec:
type: object
spec:
properties:
resources:
items:
properties:
name:
type: string
resourceRef:
properties:
apiVersion:
type: string
name:
type: string
required:
- name
type: object
required:
- name
- resourceRef
type: object
type: array
tasks:
items:
properties:
artifactStoreBindings:
inputSourceBindings:
items:
properties:
storeKey:
name:
type: string
storeName:
passedConstraints:
items:
type: string
type: array
sourceKey:
type: string
required:
- storeName
- storeKey
- name
- sourceKey
type: object
type: array
name:
type: string
nextTasks:
items:
type: string
type: array
paramBindings:
outputSourceBindings:
items:
properties:
inputName:
type: string
taskName:
name:
type: string
taskOutputName:
passedConstraints:
items:
type: string
type: array
sourceKey:
type: string
required:
- inputName
- taskName
- taskOutputName
- name
- sourceKey
type: object
type: array
params:
Expand All @@ -70,22 +90,6 @@ spec:
- value
type: object
type: array
prevTasks:
items:
type: string
type: array
sourceBindings:
items:
properties:
inputName:
type: string
sourceKey:
type: string
required:
- inputName
- sourceKey
type: object
type: array
taskRef:
properties:
apiVersion:
Expand All @@ -102,6 +106,7 @@ spec:
type: array
required:
- tasks
- resources
type: object
status:
type: object
Expand Down
39 changes: 0 additions & 39 deletions config/crds/pipeline_v1beta1_pipelineparams.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,6 @@ spec:
type: object
spec:
properties:
artifactStores:
items:
properties:
name:
type: string
type:
type: string
url:
type: string
required:
- name
- type
- url
type: object
type: array
results:
properties:
logs:
Expand Down Expand Up @@ -84,32 +69,8 @@ spec:
type: object
serviceAccount:
type: string
sources:
items:
properties:
branch:
type: string
commit:
type: string
name:
type: string
serviceAccount:
type: string
type:
type: string
url:
type: string
required:
- name
- type
- url
- branch
type: object
type: array
required:
- serviceAccount
- sources
- artifactStores
- results
type: object
status:
Expand Down
19 changes: 19 additions & 0 deletions config/crds/pipeline_v1beta1_pipelinerun.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ spec:
- lastTransitionTime
type: object
type: array
resourceVersion:
items:
properties:
resourceRef:
properties:
apiVersion:
type: string
name:
type: string
required:
- name
type: object
version:
type: string
required:
- resourceRef
- version
type: object
type: array
taskRuns:
items:
properties:
Expand Down
61 changes: 61 additions & 0 deletions config/crds/pipeline_v1beta1_resource.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
apiVersion: apiextensions.k8s.io/v1beta1
kind: CustomResourceDefinition
metadata:
creationTimestamp: null
labels:
controller-tools.k8s.io: "1.0"
name: resources.pipeline.knative.dev
spec:
group: pipeline.knative.dev
names:
kind: Resource
plural: resources
scope: Namespaced
validation:
openAPIV3Schema:
properties:
apiVersion:
type: string
kind:
type: string
metadata:
type: object
spec:
properties:
resources:
items:
properties:
name:
type: string
params:
items:
properties:
name:
type: string
value:
type: string
required:
- name
- value
type: object
type: array
type:
type: string
required:
- name
- type
- params
type: object
type: array
required:
- resources
type: object
status:
type: object
type: object
version: v1beta1
status:
acceptedNames:
kind: ""
plural: ""
conditions: null
34 changes: 24 additions & 10 deletions config/crds/pipeline_v1beta1_task.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,38 +59,52 @@ spec:
properties:
name:
type: string
type:
value:
type: string
required:
- name
- type
- value
type: object
type: array
sources:
resources:
items:
properties:
name:
type: string
resourceRef:
properties:
apiVersion:
type: string
name:
type: string
required:
- name
type: object
required:
- name
- resourceRef
type: object
type: array
type: object
outputs:
properties:
artifacts:
resources:
items:
properties:
name:
type: string
storeKey:
type: string
type:
type: string
resourceRef:
properties:
apiVersion:
type: string
name:
type: string
required:
- name
type: object
required:
- name
- type
- storeKey
- resourceRef
type: object
type: array
results:
Expand Down
Loading