Simplify and split apart MakePod

[imjasonh]

Related to #1519

Today, the conversion between TaskRun to a Pod jumbled together (mostly) inside MakePod, which is responsible for lots of different things. This makes it hard to understand, hard to test in isolation, and hard to make fixes and improvements.

What MakePod does

(roughly in the ideal order)

Applies stepTemplate

• FIX: This should ignore stepTemplate.command and .args if the step specifies .script
• FIX: stepTemplate shouldn't apply to sidecars, only steps.

Convert Steps with script to Containers with command

• This adds an init container, place-scripts, which writes the user's scripts to /builder/scripts
• This adds a new volume, which needs to be mounted into all steps that use a script.
• Done in Split out entrypoint resolution and injection, and script conversion #1616

Resolve image ENTRYPOINT from remote registry if the step doesnt' specify a command

• This relies on k8schain
• FIX: Also resolve the image's digest, to avoid a race between entrypoint resolving and container running
• FIX: Cache tag -> digest lookup for each resolve, for speed
• Done in Split out entrypoint resolution and injection, and script conversion #1616

Rewrite step command + args to inject our entrypoint binary

• Adds an init container, place-tools, always.
• Adds a volume, /builder/tools
• Adds a downward API volume at /builder/downward
• Entrypoint rewriting should happen after script conversion (today it happens before)
• Done in Split out entrypoint resolution and injection, and script conversion #1616

Initialize credentials

• Adds an init container, creds-init, if annotated creds are found in the service account's secrets
• Adds /var/secrets/... for each secret
• Requires /builder/home volume and (I think) HOME env var
• Split out in Move creds-init to pkg/pod #1599

mkdir working dirs under /workspace

• Adds an init container, working-dir-init, if any steps specify a relative workingDir
• Split out in Move working dir initialization out to pkg/pod/ #1596

Apply resource requests/limits

• Find the maximum request per resource per step
• Apply that request to step 0
• Set all other steps' requests to 0, and limits to the max request.

Other minor operations

• Set workingDir to /workspace if the user didn't specify
• Set the container name to step-%s (step name), or to step-unnamed-%d (step index) if not specified

After these changes

• We can more easily make entrypoint changes described in Handle sidecar execution signalling with entrypoint injection #1569
• Generate any volume names to avoid conflict
• Rename /builder/ volumeMount paths to /tekton/

[vdemeester]

@imjasonh I wasn't sure if what is listed is what MakePod does currently (and that should change somehow) or what it should do ?

/kind feature

[imjasonh]

This lists what MakePod does, in the ~ideal order. Before these changes things happened in jumbled order because it mostly happened in one huge wall of text. Afterwards I'd like to have one MakePod method that just calls N smaller isolated methods with clear goals and tests.

[afrittoli]

Today the stepTemplate is also applied to injected steps like those from PipelineResources.
I think that may be ok, but we should probably document it.

[imjasonh]

Yeah I think that's expected behavior -- at least, I think that was the behavior before I started reorganizing MakePod -- but I agree it should be better documented.

[imjasonh]

After #1655 I think I'm comfortable declaring victory on this.

We might want to move PipelineResource interactions into MakePod as well (prepending/appending steps for resources, applying parameter replacements), but at least those are already fairly well isolated into their own methods with clear tests.

[lbernick]

/reopen

There are still a few TODOs associated with this issue-- this came up during review of #4598.

It's not clear where mergeSteps should live; the TODO here would result in a circular import as this functionality is used in API validation which is a bit weird. @wlynch suggested moving into an internal package.

[tekton-robot]

@lbernick: Reopened this issue.

In response to this:

/reopen

There are still a few TODOs associated with this issue-- this came up during review of #4598.

It's not clear where mergeSteps should live; the TODO here would result in a circular import as this functionality is used in API validation which is a bit weird. @wlynch suggested moving into an internal package.

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.

[tekton-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale with a justification.
Stale issues rot after an additional 30d of inactivity and eventually close.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle stale

Send feedback to tektoncd/plumbing.

[tekton-robot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten with a justification.
Rotten issues close after an additional 30d of inactivity.
If this issue is safe to close now please do so with /close with a justification.
If this issue should be exempted, mark the issue as frozen with /lifecycle frozen with a justification.

/lifecycle rotten

Send feedback to tektoncd/plumbing.