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

Implement simple PipelineRun #61

Closed
bobcatfish opened this issue Sep 21, 2018 · 1 comment · Fixed by #128
Closed

Implement simple PipelineRun #61

bobcatfish opened this issue Sep 21, 2018 · 1 comment · Fixed by #128
Labels
meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given

Comments

@bobcatfish
Copy link
Collaborator

Expected Behavior

A user should be able to create a PipelineRun which will look at the Tasks in the referenced Pipeline and for each of those, create a TaskRun.

If #59 is done before this, creation of the TaskRun would also trigger running of the Task, however if not, nothing would actually be executed but the TaskRuns would exist.

Out of scope of this issue (coming in later issues):

  • Providing inputs to the Tasks
  • Doing something with produced outputs

Requirements:

Actual Behavior

If you create a PipelineRun, nothing will happen.

Steps to Reproduce the Problem

  1. Deploy the Pipeline CRD + related CRDs to your cluster (note: should be docs on how to do this post kubebuilder)
  2. Create a Task like this:
apiVersion: pipeline.knative.dev/v1beta1
kind: Task
metadata:
  name: helloworld-task
  namespace: default
spec:
    buildSpec:
        steps:
            - name: helloworld
              image: busybox
              args: ['echo', 'hello', 'build']
  1. Create a Pipeline like this:
apiVersion: pipeline.knative.dev/v1beta1
kind: Pipeline
metadata:
  name: hello-pipeline
  namespace: default
spec:
    tasks:
        - name: helloworld-task-1
          taskRef:
              name: helloworld-task
        - name: helloworld-task-2
          taskRef:
              name: helloworld-task
        - name: helloworld-task-3
          taskRef:
              name: helloworld-task
  1. Create a PipelineRun like this:
apiVersion: pipeline.knative.dev/v1beta1
kind: PipelineRun
metadata:
  name: hello-pipeline-run-1
  namespace: default
spec:
    pipelineRef:
        name: hello-pipeline
    triggerRef:
        type: manual
  1. Creating the PipelineRun should have created TaskRuns, so we should then be able to see:

a. The status of the PipelineRun should be updated (see example), check with kubectl get pipelineRuns
b. There should be 3 TaskRuns created, each with a unique name and a reference to the PipelineRun (check with kubectl get taskRuns), for example:

apiVersion: pipeline.knative.dev/v1beta1
kind: TaskRun
metadata:
  name: helloworld-task-run-1-1
spec:
    taskRef:
        name: helloworldTask
    trigger:
        triggerRef:
            type: manual

(I left the PipelineParams reference out of the PipelineRun, but if it turns out that we need a ServiceAccount to make this work, we'll need to specify it in the PipelineParams and reference it in the PipelineRun as well)

@tejal29
Copy link
Contributor

tejal29 commented Sep 27, 2018

@bobcatfish Was looking at if there is a way to divide this further.
Few subtasks i am thinking of:

  • Add unit tests for task_controller.go
  • Add unit tests for task_run_controller.go

On the branch, i went ahead and added all the methods

  • runWorker
  • processNextWorkItem
  • enqueueBuild
  • syncHandler
    etc..

We could potentially see if
https://github.com/knative/build-pipeline/blob/master/pkg/controller/taskrun/controller.go#L275 has some tasks recorded.
We can pair on adding these tests in pipeline controller and then add them in the other controllers.

@tejal29 tejal29 moved this from In progress to Needs review in Pipeline CRD Sep 28, 2018
@tejal29 tejal29 moved this from Needs review to In progress in Pipeline CRD Oct 4, 2018
bobcatfish referenced this issue in bobcatfish/pipeline Oct 9, 2018
Added the beginnings of a skeleton of an integration test for
PipelineRun for #61. In doing this I realized that I couldn't creating
PipelineParam objects with the clients b/c the plural name was wrong -
it needs to be `pipelineparamses` like I'm Smeagol.

Meanwhile refactored the existing controller code to break some code out
of the Reconciler, so it can be instantiated without an entire
controller (just depends on the objects it needs). The decision about
what TaskRuns to create has been separated from the logic to retrieve
existing ones, and the logic to create them. (`GetTasks`,
`getNextPipelineRunTaskRun`)

The tests were refactored such that the success cases are in separate
tests from the failure cases, so the table driven tests dont have to
handle both and it's more clear what the tests are doing.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 9, 2018
Added the beginnings of a skeleton of an integration test for
PipelineRun for #61. In doing this I realized that I couldn't create
PipelineParam objects with the clients b/c the plural name was wrong -
it needs to be `pipelineparamses` like I'm Smeagol. I tried to work
around this and stumbled on
kubernetes/code-generator#53 so it doesn't
seem to be possible without changing the code-generator code :(

Meanwhile refactored the existing controller code to break some code out
of the Reconciler, so it can be instantiated without an entire
controller (instead depends only on the objects it needs). The decision about
what TaskRuns to create has been separated from the logic to retrieve
existing ones, and the logic to create them. (`GetTasks`,
`getNextPipelineRunTaskRun`)

The tests were refactored such that the success cases are in separate
tests from the failure cases, so the table driven tests dont have to
handle both and it's more clear what the tests are doing.
knative-prow-robot pushed a commit that referenced this issue Oct 10, 2018
Added the beginnings of a skeleton of an integration test for
PipelineRun for #61. In doing this I realized that I couldn't create
PipelineParam objects with the clients b/c the plural name was wrong -
it needs to be `pipelineparamses` like I'm Smeagol. I tried to work
around this and stumbled on
kubernetes/code-generator#53 so it doesn't
seem to be possible without changing the code-generator code :(

Meanwhile refactored the existing controller code to break some code out
of the Reconciler, so it can be instantiated without an entire
controller (instead depends only on the objects it needs). The decision about
what TaskRuns to create has been separated from the logic to retrieve
existing ones, and the logic to create them. (`GetTasks`,
`getNextPipelineRunTaskRun`)

The tests were refactored such that the success cases are in separate
tests from the failure cases, so the table driven tests dont have to
handle both and it's more clear what the tests are doing.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added logic to check statuses of other TaskRuns when deciding if a new
one should be started for #61
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added logic to check statuses of other TaskRuns when deciding if a new
one should be started for #61
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
PipelineRun status will be based on the condition of the TaskRuns which
it has created, for #61. If any TaskRuns have failed, the PipelineRun
has failed. If all are successful, it is successful. If any are in
progress, it is in progress.

This is assuming a linear Pipeline, we will have to tweak this a bit
when we implement the graph (for #65)
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added logic to check statuses of other TaskRuns when deciding if a new
one should be started for #61
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
PipelineRun status will be based on the condition of the TaskRuns which
it has created, for #61. If any TaskRuns have failed, the PipelineRun
has failed. If all are successful, it is successful. If any are in
progress, it is in progress.

This is assuming a linear Pipeline, we will have to tweak this a bit
when we implement the graph (for #65)
bobcatfish referenced this issue in bobcatfish/pipeline Oct 11, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
@bobcatfish bobcatfish moved this from In progress to Needs review in Pipeline CRD Oct 12, 2018
bobcatfish referenced this issue in bobcatfish/pipeline Oct 12, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
@bobcatfish bobcatfish added the meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given label Oct 12, 2018
bobcatfish referenced this issue in bobcatfish/pipeline Oct 12, 2018
Added logic to check statuses of other TaskRuns when deciding if a new
one should be started for #61
bobcatfish referenced this issue in bobcatfish/pipeline Oct 12, 2018
PipelineRun status will be based on the condition of the TaskRuns which
it has created, for #61. If any TaskRuns have failed, the PipelineRun
has failed. If all are successful, it is successful. If any are in
progress, it is in progress.

This is assuming a linear Pipeline, we will have to tweak this a bit
when we implement the graph (for #65)
bobcatfish referenced this issue in bobcatfish/pipeline Oct 12, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
Pipeline CRD automation moved this from Needs review to Done Oct 13, 2018
knative-prow-robot pushed a commit that referenced this issue Oct 13, 2018
Added logic to check statuses of other TaskRuns when deciding if a new
one should be started for #61
knative-prow-robot pushed a commit that referenced this issue Oct 13, 2018
PipelineRun status will be based on the condition of the TaskRuns which
it has created, for #61. If any TaskRuns have failed, the PipelineRun
has failed. If all are successful, it is successful. If any are in
progress, it is in progress.

This is assuming a linear Pipeline, we will have to tweak this a bit
when we implement the graph (for #65)
knative-prow-robot pushed a commit that referenced this issue Oct 13, 2018
Added the Task reference to the TaskRun so that when a PipelineRun
creates a TaskRun, it actually executes! (For #61)

While running the integration test, noticed that the PipelineRuns
weren't getting reconciled quickly enough, but adding a tracker which
will invoke reconcile when the created TaskRuns are updated fixed this -
however it did still take > 1 minute to create 3 helloworld TaskRuns and
wait for them to complete, so since 3 was arbitrary, reduced to 2.

Also cleaned up the TaskRun controller a bit: using the Logger object on
the controller/reconciler itself, made the log messages a bit more
descriptive.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given
Projects
No open projects
Pipeline CRD
  
Done
Development

Successfully merging a pull request may close this issue.

2 participants