Workspace "from" clauses ensure task ordering #1936

ghost · 2020-01-24T16:12:57Z

Changes

Workspaces allow users to wire PVCs through the tasks of theirpipelines. Unfortunately, multiple tasks all trying to mount a PVC at once can result in unpleasant conflicts.

To combat the situation where multiple Task pods are fighting over a PVC, "from" clauses have been introduced for workspaces. One task can explicitly declare that it will use a workspace from a previous task. When Tekton sees a from clause linking one task's workspace to another it will ensure that the sequence of those tasks is enforced.

Here's a relevant snippet of YAML to show how this PR currently operates:

  tasks:
    - name: task1
      taskRef:
        name: write-to-workspace
      workspaces:
        - name: output
          workspace: pipeline-ws1
    - name: task2
      taskRef:
        name: read-from-workspace
      workspaces:
        - name: src
          from:
            task: task1
            name: output

Here, task2 will run after task1 because task2 declares that its src workspace should be populated using whatever volume was used as the output workspace from task1.

Submitter Checklist

These are the criteria that every PR should meet, please check them off as you
review them:

Includes tests (if functionality changed/added)
Includes docs (if user facing)
Commit messages follow commit message best practices

See the contribution guide for more details.

Reviewer Notes

If API changes are included, additive changes must be approved by at least two OWNERS and backwards incompatible changes must be approved by more than 50% of the OWNERS, and they must first be added in a backwards compatible way.

Release Notes

Pipeline Tasks that need to share a workspace (for example a PVC to be written to and read from by multiple tasks) can now explicitly indicate the order in which they should run (and therefore access the workspace) by using "from" clauses.

pritidesai · 2020-01-25T00:29:56Z

How would it behave if a pipeline has three or more tasks?

tasks:
    - name: task1
      taskRef:
        name: write-to-workspace
      workspaces:
        - name: output
          workspace: pipeline-ws1
    - name: task2
      taskRef:
        name: read-from-workspace
      workspaces:
        - name: src
          workspace: $(task1.output)
    - name: task3
      taskRef:
        name: process-workspace
      workspaces:
        - name: src
          workspace: $(task1.output)

Would it enforce task2 is executed before task3? Or this use case is out of scope of this discussion and falls under runAfter?

IMO, $() is so far conceived as parameter interpolation and does not guarantee task execution order. It might create confusion in the beginning, if we choose to go with $(). I think from under workspaces makes ideal solution but we are adding one more field in the YAML.

ghost · 2020-01-27T14:31:30Z

Would it enforce task2 is executed before task3?

Hm, the way to ensure that currently would be to have task2 use workspace: $(task3.src). Then task2 would wait until task3 is done.

IMO, $() is so far conceived as parameter interpolation and does not guarantee task execution order. It might create confusion in the beginning, if we choose to go with $().

Yeah that's a good point, I agree. It would also be confusing because variables are normally strings, but here the user isn't able to interpolate anything but other workspaces (i.e. they can't use a param to inject a workspace name). I think I'm going to change this to use a From field.

tekton-robot · 2020-01-28T17:10:21Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	70.0%

tekton-robot · 2020-01-28T17:33:22Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	70.0%

tekton-robot · 2020-01-28T17:38:52Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	70.0%

tekton-robot · 2020-01-28T18:14:55Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	100.0%

tekton-robot · 2020-01-28T21:25:52Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	100.0%

ghost · 2020-01-28T21:29:28Z

I've rewritten this to use "from" syntax instead and I think it's largely ready for review now. Removed WIP label.

tekton-robot · 2020-01-29T13:40:21Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	100.0%

pritidesai · 2020-01-29T21:46:36Z

thanks @sbwsg, validated one more use case with workspace from second task read-from-workspace:

apiVersion: tekton.dev/v1alpha1
kind: Pipeline
metadata:
  name: fetch-and-print-recipe
spec:
  workspaces:
  - name: password-vault
  - name: recipe-store
  - name: shared-data
  tasks:
  - name: fetch-the-recipe
    taskRef:
      name: fetch-secure-data
    workspaces:
    - name: super-secret-password
      workspace: password-vault
    - name: secure-store
      workspace: recipe-store
    - name: filedrop
      workspace: shared-data
  - name: print-the-recipe
    taskRef:
      name: print-data
    params:
    - name: filename
      value: recipe.txt
    workspaces:
    - name: storage
      from:
        task: fetch-the-recipe
        name: filedrop
  - name: one-more-print-the-recipe
    taskRef:
      name: print-data
    params:
      - name: filename
        value: recipe.txt
    workspaces:
      - name: storage
        from:
          task: print-the-recipe
          name: storage

Follows the sequential order as specified:

kubectl get pipelinerun recipe-time-1 -o json | jq .status.taskRuns[].status.startTime
"2020-01-29T21:43:22Z"
"2020-01-29T21:43:43Z"
"2020-01-29T21:43:32Z"

/lgtm

ghost · 2020-01-30T13:36:51Z

ah, thank you for checking it out @pritidesai! I think I might add an example that does this as well to make sure multiple "from"s chain correctly.

tekton-robot · 2020-01-30T14:37:18Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	100.0%

pritidesai · 2020-01-30T20:52:02Z

/lgtm

Would also like to hear @skaegi's comments on this which we couldn't get to during the demo in working group.

tekton-robot · 2020-02-04T20:47:43Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	86.5%	-0.2
test/builder/workspace.go	Do not exist	100.0%

ghost · 2020-02-05T13:26:40Z

Ah no, I missed the reconciler test. Adding today!

ghost · 2020-02-05T13:54:52Z

Oh yeah one more thought, maybe you have an example that handles this, but what happens when you fan-out, i.e. use the same workspace in 2+ parallel tasks? can all the tasks safely write to it? in PipelineResources we actually copied the data to and from a local directory vs having ppl write to the PVC directly. (still would have had race conditions tho if multiple parallel tasks are trying to update the same resource :S)

Support for this is dictated by the volume I think. Some types don't support multiple simultaneous writers while some do. GKE's persistent volume implementation doesn't support multiple simultaneous writers. I believe the platform will complain through Pod errors if one tries to do that. We don't perform any copies of the data as we shuffle the PVCs around.

tekton-robot · 2020-02-05T14:44:13Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	87.3%	0.7
test/builder/task.go	81.1%	80.6%	-0.5
test/builder/workspace.go	Do not exist	90.5%

ghost · 2020-02-05T14:51:51Z

aaand reconciler tests have been added

pritidesai · 2020-02-06T00:23:43Z

examples/pipelineruns/workspaces-multiple-from.yaml

+  generateName: fib-
+spec:
+  pipelineRef:
+    name: horrible-fibonacci


@sbwsg I think you need to change this to worlds-slowest-but-greatest-fibonacci 🤔

this pipelinerun is failing with:

kubectl get pipelinerun NAME SUCCEEDED REASON STARTTIME COMPLETIONTIME fib-pipelinerun False CouldntGetPipeline 26s 26s

@sbwsg I might not have pulled all your code to test with (because of the conflict), please ignore it if you are not hitting this but the pods are stuck in pending:

kubectl get pods NAME READY STATUS RESTARTS AGE fib-pipelinerun-init-1-5rglh-pod-zvznr 0/1 Pending 0 19m fib-pipelinerun-init-2-nnq6v-pod-bq8xs 0/1 Pending 0 19m

with a message:

"message": "pod status \"PodScheduled\":\"False\"; message: \"pod has unbound immediate PersistentVolumeClaims\"",

amazing, great catch. I updated the pipelineref.

There are some remaining issues related to using regional PVCs in the example yaml. I'm getting volume node affinity errors when I run it locally and it appears that similar issues are occurring here in the CI cluster as well. Can't quite put my finger on what's going wrong but still looking into it.

ghost · 2020-02-06T13:13:11Z

/hold

This feature is still a bit contentious and so I'm still in the process of discussing whether it's desirable / needed / a future-us problem / etc...

Workspaces allow users to wire PVCs through the tasks of their pipelines. Unfortunately, multiple tasks all trying to mount a PVC at once can result in unpleasant conflicts. To combat the situation where multiple Task pods are fighting over a PVC, "from" clauses have been introduced for workspaces. One task can explicitly declare that it will use a workspace from a previous task. When Tekton sees a from clause linking one task's workspace to another it will ensure that the sequence of those tasks is enforced.

tekton-robot · 2020-02-06T14:25:31Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	87.3%	0.7
test/builder/task.go	81.1%	80.6%	-0.5
test/builder/workspace.go	Do not exist	90.5%

ghost · 2020-02-06T19:42:52Z

Given that we're no longer going with "from" syntax for task results, and that the requirement for this feature remains under debate I'm going to close the pull request. We can reopen at some point if we decide this is more than a nice-to-have.

ghost · 2020-02-10T16:25:45Z

OK we're going to revisit this PR but using a variable interpolation syntax instead.

tekton-robot · 2020-02-10T16:28:03Z

The following is the coverage report on pkg/.
Say /test pull-tekton-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
test/builder/pipeline.go	86.6%	87.3%	0.7
test/builder/task.go	81.1%	80.6%	-0.5
test/builder/workspace.go	Do not exist	90.5%

tekton-robot · 2020-02-10T17:02:36Z

@sbwsg: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-tekton-pipeline-integration-tests	`5bb5e07`	link	`/test pull-tekton-pipeline-integration-tests`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

ghost · 2020-04-06T11:29:38Z

Closing for now, intend to revisit at a later date.

…ion 👷‍♀️ This is a super minor change to rename WorkspacePipelineDeclaration to PipelineWorkspaceDeclaration. WorkspacePipelineDeclaration sounds like it's a pipeline declaration inside of a workspace, but actually its meant to be a workspace declaration inside of a pipeline! I'm trying to pickup some of the work started in tektoncd#1936 and this seemed like a reasonable improvement to carve off and merge on its own :D Co-authored-by: Scott <[email protected]>

…ion 👷‍♀️ This is a super minor change to rename WorkspacePipelineDeclaration to PipelineWorkspaceDeclaration. WorkspacePipelineDeclaration sounds like it's a pipeline declaration inside of a workspace, but actually its meant to be a workspace declaration inside of a pipeline! I'm trying to pickup some of the work started in #1936 and this seemed like a reasonable improvement to carve off and merge on its own :D Co-authored-by: Scott <[email protected]>

ghost requested a review from bobcatfish January 24, 2020 16:12

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Jan 24, 2020

tekton-robot requested review from imjasonh and vdemeester January 24, 2020 16:13

tekton-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 24, 2020

ghost changed the title ~~Workspace Variable Substitutions ensure task ordering~~ WIP Workspace Variable Substitutions ensure task ordering Jan 24, 2020

tekton-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 24, 2020

ghost added the kind/design Categorizes issue or PR as related to design. label Jan 24, 2020

ghost changed the title ~~WIP Workspace Variable Substitutions ensure task ordering~~ WIP Workspace "from" clauses ensure task ordering Jan 28, 2020

ghost changed the title ~~WIP Workspace "from" clauses ensure task ordering~~ Workspace "from" clauses ensure task ordering Jan 28, 2020

tekton-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 28, 2020

ghost removed the kind/design Categorizes issue or PR as related to design. label Jan 28, 2020

tekton-robot assigned pritidesai Jan 29, 2020

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 29, 2020

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2020

ghost mentioned this pull request Jan 30, 2020

Support referencing task results from task params in a pipeline #1978

Closed

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2020

pritidesai reviewed Feb 6, 2020

View reviewed changes

tekton-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 6, 2020

ghost closed this Feb 6, 2020

This was referenced Feb 7, 2020

Link workspaces together through a pipeline. #2017

Closed

Enforce task ordering in pipeline when they share PVC workspace #1878

Closed

ghost reopened this Feb 10, 2020

bobcatfish mentioned this pull request Feb 21, 2020

Make it obvious from the docs that emptyDir workspaces are not passed from task to task #2081

Closed

ghost closed this Apr 6, 2020

bobcatfish mentioned this pull request May 29, 2020

Use PipelineWorkspaceDeclaration instead of WorkspacePipelineDeclaration 👷‍♀️ #2717

Merged

1 task

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Workspace "from" clauses ensure task ordering #1936

Workspace "from" clauses ensure task ordering #1936

ghost commented Jan 24, 2020 •

edited by ghost

Loading

pritidesai commented Jan 25, 2020 •

edited

Loading

ghost commented Jan 27, 2020 •

edited by ghost

Loading

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

ghost commented Jan 28, 2020

tekton-robot commented Jan 29, 2020

pritidesai commented Jan 29, 2020 •

edited

Loading

ghost commented Jan 30, 2020

tekton-robot commented Jan 30, 2020

pritidesai commented Jan 30, 2020

tekton-robot commented Feb 4, 2020

ghost commented Feb 5, 2020

ghost commented Feb 5, 2020 •

edited by ghost

Loading

tekton-robot commented Feb 5, 2020

ghost commented Feb 5, 2020

pritidesai Feb 6, 2020

pritidesai Feb 6, 2020

pritidesai Feb 6, 2020

ghost Feb 6, 2020

ghost commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

ghost commented Feb 6, 2020

ghost commented Feb 10, 2020

tekton-robot commented Feb 10, 2020

tekton-robot commented Feb 10, 2020

ghost commented Apr 6, 2020

Workspace "from" clauses ensure task ordering #1936

Workspace "from" clauses ensure task ordering #1936

Conversation

ghost commented Jan 24, 2020 • edited by ghost Loading

Changes

Submitter Checklist

Reviewer Notes

Release Notes

pritidesai commented Jan 25, 2020 • edited Loading

ghost commented Jan 27, 2020 • edited by ghost Loading

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

tekton-robot commented Jan 28, 2020

ghost commented Jan 28, 2020

tekton-robot commented Jan 29, 2020

pritidesai commented Jan 29, 2020 • edited Loading

ghost commented Jan 30, 2020

tekton-robot commented Jan 30, 2020

pritidesai commented Jan 30, 2020

tekton-robot commented Feb 4, 2020

ghost commented Feb 5, 2020

ghost commented Feb 5, 2020 • edited by ghost Loading

tekton-robot commented Feb 5, 2020

ghost commented Feb 5, 2020

pritidesai Feb 6, 2020

Choose a reason for hiding this comment

pritidesai Feb 6, 2020

Choose a reason for hiding this comment

pritidesai Feb 6, 2020

Choose a reason for hiding this comment

ghost Feb 6, 2020

Choose a reason for hiding this comment

ghost commented Feb 6, 2020

tekton-robot commented Feb 6, 2020

ghost commented Feb 6, 2020

ghost commented Feb 10, 2020

tekton-robot commented Feb 10, 2020

tekton-robot commented Feb 10, 2020

ghost commented Apr 6, 2020

ghost commented Jan 24, 2020 •

edited by ghost

Loading

pritidesai commented Jan 25, 2020 •

edited

Loading

ghost commented Jan 27, 2020 •

edited by ghost

Loading

pritidesai commented Jan 29, 2020 •

edited

Loading

ghost commented Feb 5, 2020 •

edited by ghost

Loading