Merge place-tools and step-init together #4826

vdemeester · 2022-05-03T16:11:15Z

Changes

This is part of an effort to reduce the number of init container to
the minimum. Both place-tools and step-init are using the same image
and can be easily and safely merged together.

This has few benefits, but the main one is that it reduces the number
of container to run, and thus doesn't reduce the max size of a Result.

Signed-off-by: Andrew Bayer andrew.bayer@gmail.com
Signed-off-by: Vincent Demeester vdemeest@redhat.com

/kind bug

/cc @imjasonh @dibyom @lbernick @abayer

This is the first part of a bigger refactoring aiming to tackle #4808

Submitter Checklist

As the author of this PR, please check off the items in this checklist:

Docs included if any changes are user facing
Tests included if any functionality added or changed
Follows the commit message standard
Meets the Tekton contributor standards (including
functionality, content, code)
Release notes block below has been filled in
(if there are no user facing changes, use release note "NONE")

Release Notes

place-tools and step-init init containers are merged together to reduce the number of container in each `Task`'s Pod.

vdemeester · 2022-05-03T16:12:47Z

Note: This is not removing the cp subcommand, nor the step-init subcommands, as they might be used independently of the init containers by users. If we feel we should remove then, we will need to go through a deprecation notice, …

tekton-robot · 2022-05-03T16:17:14Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

imjasonh

This looks good so far, thanks for doing this!

imjasonh · 2022-05-03T17:14:25Z

cmd/entrypoint/subcommands/subcommands.go

+ case InitCommand:
+ // If invoked in "init mode" (`entrypoint init <src> <dst> [<step-name>]`),
+ // it will copy the src path to the dst path (like CopyCommand), and initialize
+ // the /tekton/steps forder (like StepInitCommand)


Suggested change

// the /tekton/steps forder (like StepInitCommand)

// the /tekton/steps folder (like StepInitCommand)

abayer · 2022-05-04T00:11:15Z

/retest

tekton-robot · 2022-05-04T09:11:42Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

tekton-robot · 2022-05-04T13:14:30Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

tekton-robot · 2022-05-04T13:17:58Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

abayer · 2022-05-04T13:31:55Z

/retest

tekton-robot · 2022-05-04T14:34:42Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

abayer · 2022-05-04T15:02:24Z

pkg/pod/pod.go

 // Initialize any workingDirs under /workspace.
 if workingDirInit := workingDirInit(b.Images.WorkingDirInitImage, stepContainers); workingDirInit != nil {
 initContainers = append(initContainers, *workingDirInit)
 }

+ // place the entrypoint first in case other init containers rely on its


Given that we always want the entrypoint first, why not just do this before we add anything else to initContainers?

abayer · 2022-05-04T15:03:13Z

pkg/pod/pod.go

-func tektonDirInit(image string, steps []v1beta1.Step) corev1.Container {
- cmd := make([]string, 0, len(steps)+2)
- cmd = append(cmd, "/ko-app/entrypoint", "step-init")
+// prepareInitContainers generate a few init containers based of a set of command (in images) and volumes to run


Should be prepareInitContainer generates an init container... rather than the plural containers there now. =)

abayer · 2022-05-04T15:04:23Z

pkg/pod/pod.go

+ // Rewrite steps with entrypoint binary. Append the entrypoint init
+ // container to place the entrypoint binary. Also add timeout flags
+ // to entrypoint binary.
+ prepareInitContainer := corev1.Container{


I'd rename this to just initContainer.

Renamed to entrypointInitContainer 🙃

This is part of an effort to reduce the number of init container to the minimum. Both place-tools and step-init are using the same image and can be easily and safely merged together. This has few benefits, but the main one is that it reduces the number of container to run, and thus doesn't reduce the max size of a Result. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com> Signed-off-by: Vincent Demeester <vdemeest@redhat.com>

tekton-robot · 2022-05-04T15:42:54Z

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

File	Old Coverage	New Coverage	Delta
cmd/entrypoint/subcommands/init.go	Do not exist	66.7%
cmd/entrypoint/subcommands/subcommands.go	73.7%	56.0%	-17.7

vdemeester · 2022-05-04T16:05:58Z

/test pull-tekton-pipeline-build-tests

abayer · 2022-05-04T16:30:46Z

/retest

abayer · 2022-05-04T16:31:39Z

/lgtm

Cutting down on the number of init containers is great - looking forward to seeing more of them get folded in!

afrittoli

Really nice! Ideally we will have a single init container eventually?
/approve

tekton-robot · 2022-05-04T17:05:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afrittoli

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

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

abayer · 2022-05-04T17:06:40Z

@afrittoli That's the goal. =) @vdemeester and I chatted about this and merging them together one by one seemed the best approach!

lbernick · 2022-05-04T17:10:51Z

#4519 is relevant

vdemeester · 2022-05-04T19:46:01Z

Really nice! Ideally we will have a single init container eventually?
/approve

Yes the goal is to reduce it to one 😇

vdemeester added this to the Pipelines v0.36 milestone May 3, 2022

tekton-robot requested a review from abayer May 3, 2022 16:11

tekton-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label May 3, 2022

tekton-robot requested review from dibyom, imjasonh and lbernick May 3, 2022 16:11

tekton-robot added kind/bug Categorizes issue or PR as related to a bug. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 3, 2022

imjasonh reviewed May 3, 2022

View reviewed changes

vdemeester force-pushed the reduce-initcontainer-numbers branch from 13f0400 to 6c34216 Compare May 4, 2022 09:09

vdemeester force-pushed the reduce-initcontainer-numbers branch from 6c34216 to 6db845e Compare May 4, 2022 13:12

vdemeester force-pushed the reduce-initcontainer-numbers branch from 6db845e to 2b67426 Compare May 4, 2022 13:15

vdemeester force-pushed the reduce-initcontainer-numbers branch from 2b67426 to 62f1338 Compare May 4, 2022 14:30

abayer reviewed May 4, 2022

View reviewed changes

vdemeester force-pushed the reduce-initcontainer-numbers branch from 62f1338 to f5a7fdf Compare May 4, 2022 15:38

vdemeester mentioned this pull request May 4, 2022

Intermittent "InvalidTaskResultReference" when running tasks with big results #4529

Closed

tekton-robot assigned abayer May 4, 2022

tekton-robot added the lgtm Indicates that a PR is ready to be merged. label May 4, 2022

afrittoli reviewed May 4, 2022

View reviewed changes

tekton-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 4, 2022

tekton-robot merged commit 7059cfd into tektoncd:main May 4, 2022

vdemeester deleted the reduce-initcontainer-numbers branch May 4, 2022 19:46

vdemeester mentioned this pull request Jul 5, 2022

Tasks are failing when "init" is the first argument followed by two or more other arguments. #5080

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Merge place-tools and step-init together #4826

Merge place-tools and step-init together #4826

vdemeester commented May 3, 2022

vdemeester commented May 3, 2022

tekton-robot commented May 3, 2022

imjasonh left a comment

imjasonh May 3, 2022

abayer commented May 4, 2022

tekton-robot commented May 4, 2022

tekton-robot commented May 4, 2022

tekton-robot commented May 4, 2022

abayer commented May 4, 2022

tekton-robot commented May 4, 2022

abayer May 4, 2022

abayer May 4, 2022

abayer May 4, 2022

vdemeester May 4, 2022

tekton-robot commented May 4, 2022

vdemeester commented May 4, 2022

abayer commented May 4, 2022

abayer commented May 4, 2022

afrittoli left a comment

tekton-robot commented May 4, 2022

abayer commented May 4, 2022

lbernick commented May 4, 2022

vdemeester commented May 4, 2022

	// the /tekton/steps forder (like StepInitCommand)
	// the /tekton/steps folder (like StepInitCommand)

Merge place-tools and step-init together #4826

Merge place-tools and step-init together #4826

Conversation

vdemeester commented May 3, 2022

Changes

Submitter Checklist

Release Notes

vdemeester commented May 3, 2022

tekton-robot commented May 3, 2022

imjasonh left a comment

Choose a reason for hiding this comment

imjasonh May 3, 2022

Choose a reason for hiding this comment

abayer commented May 4, 2022

tekton-robot commented May 4, 2022

tekton-robot commented May 4, 2022

tekton-robot commented May 4, 2022

abayer commented May 4, 2022

tekton-robot commented May 4, 2022

abayer May 4, 2022

Choose a reason for hiding this comment

abayer May 4, 2022

Choose a reason for hiding this comment

abayer May 4, 2022

Choose a reason for hiding this comment

vdemeester May 4, 2022

Choose a reason for hiding this comment

tekton-robot commented May 4, 2022

vdemeester commented May 4, 2022

abayer commented May 4, 2022

abayer commented May 4, 2022

afrittoli left a comment

Choose a reason for hiding this comment

tekton-robot commented May 4, 2022

abayer commented May 4, 2022

lbernick commented May 4, 2022

vdemeester commented May 4, 2022