Replace Pipeline linear execution with graph based execution #168
Comments
tejal29
added
the
meaty-juicy-coding-work
|
@bobcatfish, in #160, we did not get rid of linear execution. I had a chat with @pivotal-nader-ziada and we decided to remove the logic in another PR. |
I just want to make sure that when we implement this task, we still support linear execution. This task as it's currently worded sounds like we will only be able to run tasks in parallel, which would be a step backward from where we are now. We don't want to get rid of linear execution, we want to support more use cases |
|
The plan is to use |
|
+1 to what @pivotal-nader-ziada said. It respects |
Sometimes you want to run Tasks in sequence, and you would do this with I'm just saying that saying we are "removing" linear execution is very confusing if you don't already know intimately how the controller is implemented. But this is a moot point since @tejal29 is the one implementing it and knows what this means :) |
|
@bobcatfish if the tasks are not related at all, is there a need to run them in sequence? if yes, we might need a task to identify that in the yaml |
|
concourse has an optional |
|
I think we are talking about 2 different things:
I'm not saying we need (1), as far as I'm concerned this is just a syntax thing, not really a feature. After this Task, we'll still have (2), which is the only thing I'm saying is important. I want to add features, not remove them. |
|
@bobcatfish i think you got confused by the title "Remove linear Execution". I will change it to "Remove Default Linear Pipeline Execution" |
bobcatfish
changed the title
|
The initial implementation of the DAG is available in https://github.com/knative/build-pipeline/blob/master/pkg/reconciler/v1alpha1/pipeline/resources/dag.go - the next step will be to update the Pipeline to actually use this. |
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. This is the first of a couple follow up commits to add more validation: this one makes sure that extra resources are not provided (e.g. if you typo-ed a param with a default value, you may never know your param was being ignored). (Shout out to @vdemeester - I updated the reconciler tests both before and after his test refactoring, and it was nearly impossible to understand the tests before his builders were added: the builders made it waaaaay easier! 🙏 Follow up on #124
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
bobcatfish
changed the title
|
I haven't gotten to this in ages so I'm taking myself off of it - the one thing I did tho was create an end to end test for the functionality which I'll try to push in a branch if I ever get around to it. If someone reading this wants to work on it, ping me if you'd like to see the end to end test (np if you want to start fresh). Note that in #320 we talked about also adding |
|
@bobcatfish 👋 #always-interested |
|
hahaha o really @vdemeester , v exciting!!!! ❤️ |
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
|
Updated description of Task to include |
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
|
Okay @vdemeester my test is at bobcatfish@39f7664 but it's pretty rough and there are a few caveats :D (ps should we assign this one to you? 😜 ) |
|
/assign |
|
It might make sense to limit the maximum number of parallel tasks to be executed at any point in time for one pipeline. |
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of #168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of #168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of #168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of #168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of tektoncd#168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Update the documentation such that Tasks no longer execute in the order they are declared in the Pipeline, order is now controlled by `from` AND `runAfter`. We need to add `runAfter` as part of #168 because without it, all ordering must be expressed with `from`, which would make our examples really gross temporarily (not to mention be a lot of work and slow down test execution) just to remove it as soon as we add `runAfter`, so we might as well do it all at once.
Add link to release-0.7.0 👼
Expected Behavior
Tasks in pipeline can run in parallel if:
fromfield)runAfter- not yet in API, see description ofrunAfterin design doc (note doc visible to members of knative-dev@googlegroups.com)Actual Behavior
Right now, all Tasks execute linearly in the order they are defined in in the
PipelineSteps to Reproduce the Problem
Parallel case:
fromcase:fromanother, and declare them in the opposite order they should run in (i.e. put the Task the resource isfromlast)runAftercaserunAfteranother and declare them in the opposite order they should run in (i.e. put the Task that should berunAfterlast)And combining these cases!
Additional Info
The text was updated successfully, but these errors were encountered: