Split out entrypoint resolution and injection, and script conversion #1616

imjasonh · 2019-11-25T20:28:48Z

Split out entrypoint resolution and injection, and script conversion

This is the next and largest step of the effort to simplify and separate
MakePod out into digestible chunks (#1605)

Behavioral changes

Script->Command conversion now happens before entrypoint rewriting,
rather than converting the rewritten entrypoint args.
Image name->digest lookups are cached locally while resolving a single
TaskRun's steps (with test!)
Entrypoint lookups also update the step's digest. This was a race
before: if an image was pushed between resolution and pod start, the
resolved command might be out-of-date.

Some redundant test cases have been removed from taskrun_test.go -- this
file should test only behavior of taskrun.go, which is now smaller.

Unit tests for individual transformation behavior has been moved into
individual test files in pkg/pod, with some integration tests in
pod_test.go -- some of these could likely be removed as well, if we feel
like it.

Submitter Checklist

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

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

See the contribution guide for more details.

/assign @sbwsg

tekton-robot · 2019-11-25T20:32:41Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	93.8%	6.5
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

tekton-robot · 2019-11-25T20:42:36Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	93.8%	6.5
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

pkg/pod/entrypoint.go

pkg/reconciler/taskrun/controller.go

ghost · 2019-11-25T20:48:25Z

/lgtm

tekton-robot · 2019-11-25T21:04:35Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.7%	5.4
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

bobcatfish

In general I very much ❤️ this!!! +10000

I'm a bit confused by all these "defensive copies" but mostly b/c I don't know what they are 😅

Might I also add: beautiful commit message!!

Lastly: to tell you the truth I looked in a lot of detail in the first 1/2 of the code and a lot less at the 2nd half so that is what it is 😅

/approve

pkg/pod/entrypoint.go

pkg/pod/entrypoint_lookup.go

pkg/pod/entrypoint.go

pkg/pod/entrypoint_test.go

pkg/pod/entrypoint.go

pkg/pod/entrypoint_lookup.go

tekton-robot · 2019-11-25T21:05:44Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bobcatfish

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~~ [bobcatfish]

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

tekton-robot · 2019-11-25T21:27:10Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.7%	5.4
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

tekton-robot · 2019-11-25T21:28:38Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.7%	5.4
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

tekton-robot · 2019-11-25T21:43:58Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.8%	5.5
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

tekton-robot · 2019-11-25T22:06:53Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.8%	5.5
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

This is the next and largest step of the effort to simplify and separate MakePod out into digestible chunks (tektoncd#1605) Behavioral changes: - Script->Command conversion now happens before entrypoint rewriting, rather than converting the rewritten entrypoint args. - Image name->digest lookups are cached locally while resolving a single TaskRun's steps (with test!) - Entrypoint lookups also update the step's digest. This was a race before: if an image was pushed between resolution and pod start, the resolved command might be out-of-date. Some redundant test cases have been removed from taskrun_test.go -- this file should test only behavior of taskrun.go, which is now smaller. Unit tests for individual transformation behavior has been moved into individual test files in pkg/pod, with some integration tests in pod_test.go -- some of these could likely be removed as well, if we feel like it.

tekton-robot · 2019-11-26T01:58:27Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.9%	5.6
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

tekton-robot · 2019-11-26T02:00:35Z

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
pkg/reconciler/taskrun/controller.go	100.0%	96.2%	-3.8
pkg/reconciler/taskrun/resources/pod.go	87.3%	92.9%	5.6
pkg/reconciler/taskrun/taskrun.go	73.1%	74.9%	1.8
test/builder/pod.go	79.2%	77.1%	-2.1

vdemeester

This looks gooooood 😻
/lgtm

vdemeester · 2019-11-26T07:49:38Z

pkg/pod/entrypoint.go

+	return nil
+}
+
+func IsContainerStep(name string) bool    { return strings.HasPrefix(name, StepPrefix) }


nit: can we have docstring here too (just for godoc.org 😛 🤗 )

Yeah good point. I'll make sure to have doc strings everywhere before calling this done.

vdemeester · 2019-11-26T07:51:14Z

pkg/pod/entrypoint_lookup.go

+	}
+	e.lru.Add(digest.String(), ep) // Populate the cache.
+
+	d, err = name.NewDigest(imageName+"@"+digest.String(), name.WeakValidation)


nit: I wonder if we could use name:tag@digest notation here (thinking outloud, ignore me 👼)

I'm not really sure what it would buy us, this string is never surfaced directly to users.

Isn't it surfaced in the final pod spec ? (anyway it's really not that big of a deal)

It is, but that pod spec is intended to be an implementation detail, and we shouldn't expect users to see it. The user's specified tag will be shown in the TaskRun's .spec, and the resolved digest will be shown in the TaskRun's .status, that's all that really matters.

vdemeester · 2019-11-26T07:58:08Z

pkg/reconciler/taskrun/resources/pod.go

-		Args:         []string{"-c", ""},
-		VolumeMounts: []corev1.VolumeMount{scriptsVolumeMount},
+	// Zero out non-max resource requests.
+	// TODO(jasonhall): Split this out so it's more easily testable.


nit: TODO(#1605) 😝

vdemeester · 2019-11-26T07:59:48Z

pkg/status/taskrunpod.go

@@ -23,7 +23,7 @@ import (

 	"github.com/tektoncd/pipeline/pkg/apis/pipeline/v1alpha1"
 	listers "github.com/tektoncd/pipeline/pkg/client/listers/pipeline/v1alpha1"
-	"github.com/tektoncd/pipeline/pkg/reconciler/taskrun/resources"
+	podconvert "github.com/tektoncd/pipeline/pkg/pod"


Any reason for naming it podconvert instead of pod ?

There's a variable called pod below that causes problems. When this is all done I hope to avoid this named import.

tekton-robot assigned ghost Nov 25, 2019

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

tekton-robot requested review from dibyom and vdemeester November 25, 2019 20:28

tekton-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Nov 25, 2019

imjasonh force-pushed the entrypoint-big-one branch from 6157f91 to 6338b66 Compare November 25, 2019 20:37

ghost reviewed Nov 25, 2019

View reviewed changes

pkg/pod/entrypoint.go Outdated Show resolved Hide resolved

pkg/pod/entrypoint.go Outdated Show resolved Hide resolved

pkg/reconciler/taskrun/controller.go Outdated Show resolved Hide resolved

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2019

imjasonh force-pushed the entrypoint-big-one branch from 6338b66 to 54631fd Compare November 25, 2019 21:01

tekton-robot removed the lgtm Indicates that a PR is ready to be merged. label Nov 25, 2019

bobcatfish reviewed Nov 25, 2019

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 25, 2019

imjasonh force-pushed the entrypoint-big-one branch 2 times, most recently from d9522af to c0545fe Compare November 25, 2019 21:24

imjasonh force-pushed the entrypoint-big-one branch from c0545fe to 796948c Compare November 25, 2019 21:41

imjasonh force-pushed the entrypoint-big-one branch from 796948c to c37efe2 Compare November 25, 2019 22:03

imjasonh force-pushed the entrypoint-big-one branch from c37efe2 to 83b5374 Compare November 26, 2019 01:54

imjasonh force-pushed the entrypoint-big-one branch from 83b5374 to b83dd20 Compare November 26, 2019 01:56

vdemeester reviewed Nov 26, 2019

View reviewed changes

tekton-robot assigned vdemeester Nov 26, 2019

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label Nov 26, 2019

tekton-robot merged commit e228f11 into tektoncd:master Nov 26, 2019

imjasonh mentioned this pull request Nov 26, 2019

Simplify and split apart MakePod #1605

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split out entrypoint resolution and injection, and script conversion #1616

Split out entrypoint resolution and injection, and script conversion #1616

imjasonh commented Nov 25, 2019 •

edited

Loading

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

ghost commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

bobcatfish left a comment

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

vdemeester left a comment

vdemeester Nov 26, 2019

imjasonh Nov 26, 2019

vdemeester Nov 26, 2019

imjasonh Nov 26, 2019

vdemeester Nov 26, 2019

imjasonh Nov 26, 2019

vdemeester Nov 26, 2019

vdemeester Nov 26, 2019

imjasonh Nov 26, 2019

Split out entrypoint resolution and injection, and script conversion #1616

Split out entrypoint resolution and injection, and script conversion #1616

Conversation

imjasonh commented Nov 25, 2019 • edited Loading

Behavioral changes

Submitter Checklist

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

ghost commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

bobcatfish left a comment

Choose a reason for hiding this comment

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 25, 2019

tekton-robot commented Nov 26, 2019

tekton-robot commented Nov 26, 2019

vdemeester 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

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

imjasonh commented Nov 25, 2019 •

edited

Loading