-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Split /tekton/run directories into separate volumes. #4278
Conversation
The following is the coverage report on the affected files.
|
Would it be worthwhile to add either an e2e test or an example yaml that exercises the RO behaviour of the volumes? E.g. something like: kind: TaskRun
apiVersion: tekton.dev/v1beta1
spec:
taskSpec:
steps:
- image: # image with a shell
script: |
set +e # dont fail the script on error
printf "hello world" > /tekton/steps/run/1/foo.txt
if [ $? -eq 0 ] ; then
echo "able to write to run directory of non-current step"
exit 1
fi
- image: # this step exists only to have the run/1 volumemount in the first step
script: echo "step 2" |
4d43364
to
bd98730
Compare
Done. (I wish there was a separate test-only directory, but added to examples for now) |
The following is the coverage report on the affected files.
|
This changes splits each `/tekton/run` volume into separate volumes so that steps can only mutate their own runtime information. This prevents steps from unexpectedly interfering with other step execution. To do this, we repurpose the `/tekton/run/#` path into a step-specific directory. Since this was previously used by the entrypoint for the post/wait files, we now use `/tekton/run/#/out` as the post/wait filepath instead. This does not change behavior of the directory, it enforces expected behavior of steps. `/tekton/run` is considered an internal implementation detail and is not covered by the API compatibility policy, so it is safe to make changes to the behavior of these files/paths. This does not stop user execution from writing the step's own `/tekton/run/#` folder. This needs more discussion/design - additional changes (if needed) will be made in another commit. This change is only focused on `/tekton/run` to reduce PR complexity. We will likely want to make a similar change to /tekton/steps in another commit. We may also look to consolidate all per-step volumes into a single source (i.e. creds-init does something similar as well). AFAICT, Ephemeral Volumes (i.e. EmptyDir) are exempt from Node Volume limits (https://kubernetes.io/docs/concepts/storage/storage-limits/) - spot checked this with a TaskRun with 100+ steps on both kind and GKE.
bd98730
to
bde910f
Compare
The following is the coverage report on the affected files.
|
Cheers! /lgtm |
/test pull-tekton-pipeline-alpha-integration-tests pull-tekton-pipeline-integration-tests |
/test pull-tekton-pipeline-integration-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/retest
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Changes
This changes splits each
/tekton/run
volume into separate volumesso that steps can only mutate their own runtime information. This
prevents steps from unexpectedly interfering with other step execution.
To do this, we repurpose the
/tekton/run/#
path into astep-specific directory. Since this was previously used by the
entrypoint for the post/wait files, we now use
/tekton/run/#/out
asthe post/wait filepath instead.
This does not change behavior of the directory, it enforces expected
behavior of steps.
/tekton/run
is considered an internal implementation detail and is notcovered by the API compatibility policy, so it is safe to make changes
to the behavior of these files/paths.
This does not stop user execution from writing the step's own
/tekton/run/#
folder. This needs more discussion/design - additional changes (if
needed) will be made in another commit.
This change is only focused on
/tekton/run
to reduce PR complexity. We willlikely want to make a similar change to /tekton/steps in another commit.
We may also look to consolidate all per-step volumes into a single
source (i.e. creds-init does something similar as well).
AFAICT, Ephemeral Volumes (i.e. EmptyDir) are exempt from Node Volume
limits (https://kubernetes.io/docs/concepts/storage/storage-limits/) -
spot checked this with a TaskRun with 100+ steps on both kind and GKE.
Part of #4227
/kind cleanup
Performance
Not very rigorous/scientific, but I ran 50 TaskRuns against this commit and v0.28.1 on a kind cluster, each with 100+ steps of:
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
functionality, content, code)
Release Notes
Internal only changes.