Initialize working dirs inside workspace dir #726

dicarlo2 · 2019-04-04T19:25:34Z

Changes

In order to allow steps to define workingDirs that are subdirectories of the workspace directory we need to make sure they have been created first since they will not exist on startup.

Fixes #725

Note: this PR does not solve the general problem of ensuring all valid combinations of workingDir + volumes + volume mounts will not fail, instead opting to keep things simple and solve the most common case as a first pass - setting the working directory to the cloned git repository (or other input resource folder). We also do not check that the workingDir path actually refers to the implicit workspace volume (as opposed to a user defined one that they've mounted at the same path) because it keeps the logic simple and the worst case is we have an additional empty directory in the implicit workspace volume.

Adds an additional parameter to the controller config for the image to use for this container. We could have reused the other bash image parameter, but it makes for a nice extension point to tekton to keep them separate IMO since it allows overriding specific bits of functionality should the user desire.

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.

Release Notes

Fix setting workingDirs that are subdirectories of the default workspace directory

dwnusbaum · 2019-04-04T19:26:26Z

/ok-to-test

dicarlo2 · 2019-04-04T20:15:46Z

/retest

vdemeester · 2019-04-05T07:29:00Z

config/controller.yaml

@@ -35,6 +35,7 @@ spec:
          "-git-image", "github.com/tektoncd/pipeline/cmd/git-init",
          "-nop-image", "github.com/tektoncd/pipeline/cmd/nop",
          "-bash-noop-image", "github.com/tektoncd/pipeline/cmd/bash",
+          "-bash-working-dir-image", "github.com/tektoncd/pipeline/cmd/bash",


Why not using bash-noop-image instead of having a new flag ? (we were also thinking of renaming this one as the noop make it more scarier than it is)

We could use the noop image, though my thinking was that it's nice to have separate arguments for users to override if necessary. I.e. if for some reason in a user's setup they need to override the image that's used for creating working directories, e.g. to have their own custom logic run, but don't want to change it for the current use of bash-noop-image (inside of the artifact setup). Thoughts?

That SGTM @dicarlo2

(besides @vdemeester I think we can actually get rid of the noop-image now that we're not using init containers right? i'm having deja vu like we already discussed this... 🤔🤔🤔)

(ahahahaa that was a different image and you already removed it @dicarlo2 🤣 🤣 🤣 #715 thank you 🙏 ... meanwhile we have got to get noop out of the name of that flag so ppl like me dont get confuse...)

... and #715 hasn't been merged yet ... i'm going to catch up eventually i promise...

vdemeester · 2019-04-05T07:51:50Z

/test pull-tekton-pipeline-integration-tests

dlorenc · 2019-04-15T14:48:32Z

This LGTM except for the question about the prefix check on WorkingDir

bobcatfish

Thanks for this @dicarlo2 !! I have some feedback about creating some slightly more focused unit tests (and as a result, potentially changing the function interface a bit)

I also want to know, have you checked how much overhead this adds to TaskRuns that execute the extra container?

bobcatfish · 2019-04-15T20:39:04Z

config/controller.yaml

@@ -35,6 +35,7 @@ spec:
          "-git-image", "github.com/tektoncd/pipeline/cmd/git-init",
          "-nop-image", "github.com/tektoncd/pipeline/cmd/nop",
          "-bash-noop-image", "github.com/tektoncd/pipeline/cmd/bash",
+          "-bash-working-dir-image", "github.com/tektoncd/pipeline/cmd/bash",


(besides @vdemeester I think we can actually get rid of the noop-image now that we're not using init containers right? i'm having deja vu like we already discussed this... 🤔🤔🤔)

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

dicarlo2 · 2019-04-23T07:32:01Z

This LGTM except for the question about the prefix check on WorkingDir

I might just be blind, but I'm not seeing the question. With that said, I did change the check to look at the relpath rather than the prefix, so maybe I fixed it :)

dicarlo2 · 2019-04-23T07:32:43Z

pkg/reconciler/v1alpha1/taskrun/resources/pod_test.go

@@ -249,6 +251,52 @@ func TestMakePod(t *testing.T) {
 			},
 			Volumes: implicitVolumes,
 		},
+	}, {
+		desc: "working-dir-in-workspace-dir",


I left this test in so that there's at least one test sanity checking that we add the container as expected.

chmouel

LGTM

chmouel · 2019-04-23T08:44:24Z

pkg/reconciler/v1alpha1/taskrun/resources/pod_test.go

+		workingDirs: []string{"/workspace/foo", "/workspace/bar", "/baz"},
+		want:        "mkdir -p /workspace/foo /workspace/bar",
+	}, {
+		desc:        "empty",


nit: maybe add a test that checks for workingDirs and returns empty

Sorry, I'm not following (maybe because I'm tired and it's pretty late here). Can you restate/give an example?

something like this ? https://gist.github.com/chmouel/b1b085d96ffd4b7a620c26ed3708462c

Done, thanks for clarifying 😄

tekton-robot · 2019-04-23T08:51:40Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: chmouel, dicarlo2
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: imjasonh

If they are not already assigned, you can assign the PR to them by writing /assign @imjasonh in a comment when ready.

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:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

dicarlo2 · 2019-04-23T09:28:19Z

I also want to know, have you checked how much overhead this adds to TaskRuns that execute the extra container?

The container executes in < 1 second (GKE n1-standard-8, fwiw). I'd expect the overhead would be dominated by the time taken to download the container, which will vary across user setups. On that note, it might make sense to combine some of the binaries into a single image by default, in particular for the init container images (place tools, and this one).

chmouel · 2019-04-23T10:18:33Z

On that note, it might make sense to combine some of the binaries into a single image by default, in particular for the init container images (place tools, and this one).

👍 that would be a great optimisation indeed!

dicarlo2 · 2019-04-23T10:24:45Z

Oh, also, I expect the integration tests will fail since the override-with-bash-working-dir:latest image does not exist - if someone could give me a hand with fixing it that'd be awesome :)

vdemeester

@dicarlo2 I think we don't need to add a new image, just re-use a existing one 👼
Also, small comments on the code 😉

pkg/reconciler/v1alpha1/taskrun/resources/pod.go

dlorenc · 2019-04-23T19:10:59Z

I might just be blind, but I'm not seeing the question. With that said, I did change the check to look at the relpath rather than the prefix, so maybe I fixed it :)

D'oh, yeah I think I didn't hit submit or something. Looks good now!

dlorenc · 2019-04-30T14:19:44Z

Looks like this needs a rebase and has a few comments left to address. @dicarlo2 are you still planning to get this fixed up?

dicarlo2 · 2019-05-03T00:01:53Z

Yup, sorry about the delay, had another one of those weeks where I had too many things on my plate.

dlorenc · 2019-05-03T14:40:08Z

/test pull-tekton-pipeline-integration-tests

In order to allow steps to define workingDirs that are subdirectories of the workspace directory we need to make sure they have been created first since they will not exist on startup. Fixes tektoncd#725

dicarlo2 · 2019-05-03T23:13:18Z

Looks like the override-with-bash-noop image also does not exist.

abayer · 2019-05-08T10:03:58Z

/retest

tekton-robot · 2019-05-08T11:47:41Z

@dicarlo2: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
tekton-pipeline-unit-tests	`2f3691e`	link	`/test tekton-pipeline-unit-tests`
pull-tekton-pipeline-integration-tests	`2f3691e`	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.

Subsumes tektoncd#726, with a unit test fix and a tweak to make sure that artifact_bucket_test doesn't try to run in parallel, since that messes up various PipelineRun tests. Signed-off-by: Andrew Bayer <[email protected]>

Subsumes #726, with a unit test fix and a tweak to make sure that artifact_bucket_test doesn't try to run in parallel, since that messes up various PipelineRun tests. Signed-off-by: Andrew Bayer <[email protected]>

vdemeester · 2019-05-08T19:03:54Z

#843 is merged, we can close this one 😁
/close

tekton-robot · 2019-05-08T19:03:56Z

@vdemeester: Closing this PR.

In response to this:

#843 is merged, we can close this one 😁
/close

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.

Subsumes tektoncd#726, with a unit test fix and a tweak to make sure that artifact_bucket_test doesn't try to run in parallel, since that messes up various PipelineRun tests. Signed-off-by: Andrew Bayer <[email protected]>

Add KO_DATA_PATH env to Dockerfiles

tekton-robot requested review from bobcatfish and vdemeester April 4, 2019 19:25

googlebot added the cla: yes Trying to make the CLA bot happy with ppl from different companies work on one commit label Apr 4, 2019

tekton-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Apr 4, 2019

tekton-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Apr 4, 2019

vdemeester reviewed Apr 5, 2019

View reviewed changes

bobcatfish reviewed Apr 15, 2019

View reviewed changes

dicarlo2 force-pushed the workdir branch from f1dfc43 to 42b6b32 Compare April 23, 2019 07:29

dicarlo2 commented Apr 23, 2019

View reviewed changes

chmouel approved these changes Apr 23, 2019

View reviewed changes

dicarlo2 force-pushed the workdir branch from 42b6b32 to 34db1a7 Compare April 23, 2019 10:14

vdemeester reviewed Apr 23, 2019

View reviewed changes

pkg/reconciler/v1alpha1/taskrun/resources/pod.go Outdated Show resolved Hide resolved

pkg/reconciler/v1alpha1/taskrun/resources/pod.go Outdated Show resolved Hide resolved

dicarlo2 force-pushed the workdir branch from 34db1a7 to 871c87f Compare May 3, 2019 00:00

Initialize working dirs inside workspace dir

2f3691e

In order to allow steps to define workingDirs that are subdirectories of the workspace directory we need to make sure they have been created first since they will not exist on startup. Fixes tektoncd#725

dicarlo2 force-pushed the workdir branch from 871c87f to 2f3691e Compare May 3, 2019 23:13

tekton-robot closed this May 8, 2019

abayer mentioned this pull request May 8, 2019

Cleanup of initializing working dirs #843

Merged

3 tasks

nikhil-thomas pushed a commit to nikhil-thomas/pipeline that referenced this pull request Aug 18, 2021

Merge pull request tektoncd#726 from nikhil-thomas/fix/ci-kodta

29ceca1

Add KO_DATA_PATH env to Dockerfiles

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initialize working dirs inside workspace dir #726

Initialize working dirs inside workspace dir #726

dicarlo2 commented Apr 4, 2019 •

edited

Loading

dwnusbaum commented Apr 4, 2019

dicarlo2 commented Apr 4, 2019

vdemeester Apr 5, 2019

dicarlo2 Apr 10, 2019

dlorenc Apr 15, 2019

bobcatfish Apr 15, 2019

bobcatfish Apr 15, 2019

bobcatfish Apr 15, 2019

vdemeester commented Apr 5, 2019

dlorenc commented Apr 15, 2019

bobcatfish left a comment

bobcatfish Apr 15, 2019

dicarlo2 commented Apr 23, 2019

dicarlo2 Apr 23, 2019

chmouel left a comment

chmouel Apr 23, 2019

dicarlo2 Apr 23, 2019

chmouel Apr 23, 2019

dicarlo2 Apr 23, 2019

tekton-robot commented Apr 23, 2019

dicarlo2 commented Apr 23, 2019

chmouel commented Apr 23, 2019

dicarlo2 commented Apr 23, 2019

vdemeester left a comment

dlorenc commented Apr 23, 2019

dlorenc commented Apr 30, 2019

dicarlo2 commented May 3, 2019

dlorenc commented May 3, 2019

dicarlo2 commented May 3, 2019

abayer commented May 8, 2019

tekton-robot commented May 8, 2019

vdemeester commented May 8, 2019

tekton-robot commented May 8, 2019

Initialize working dirs inside workspace dir #726

Initialize working dirs inside workspace dir #726

Conversation

dicarlo2 commented Apr 4, 2019 • edited Loading

Changes

Submitter Checklist

Release Notes

dwnusbaum commented Apr 4, 2019

dicarlo2 commented Apr 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdemeester commented Apr 5, 2019

dlorenc commented Apr 15, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dicarlo2 commented Apr 23, 2019

Choose a reason for hiding this comment

chmouel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tekton-robot commented Apr 23, 2019

dicarlo2 commented Apr 23, 2019

chmouel commented Apr 23, 2019

dicarlo2 commented Apr 23, 2019

vdemeester left a comment

Choose a reason for hiding this comment

dlorenc commented Apr 23, 2019

dlorenc commented Apr 30, 2019

dicarlo2 commented May 3, 2019

dlorenc commented May 3, 2019

dicarlo2 commented May 3, 2019

abayer commented May 8, 2019

tekton-robot commented May 8, 2019

vdemeester commented May 8, 2019

tekton-robot commented May 8, 2019

dicarlo2 commented Apr 4, 2019 •

edited

Loading