Changing build steps from initContainers to Containers by nader-ziada · Pull Request #564 · tektoncd/pipeline

nader-ziada · 2019-02-28T19:04:48Z

Changes

Replace log-writer entrypoint with waiting entrypoint
Remove configmap for entrypoint image
Update docs to describe entrypoint function
Add support for getting remote image …
- use service account in task
- fallback to default service account
add vendor dependencies for k8schain

Fixes: #224

Co-authored by @bobcatfish

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

The containers executing the Task steps are changed from using InitContainers to Containers in the TaskRun pod. This is achieved by wrapping each container by a custom binary entrypoint to manage the order of the steps. As such, it is always recommended to explicitly specify a command for a Task step. If a step's image is in a private registry, a ServiceAccount with ImagePullSecrets should be provided in the Task .

imjasonh

This PR is my favorite PR that I've ever seen, and I'm excited to see it's this close to finally happening! 🎉 Thanks @pivotal-nader-ziada!

Can you add a YAML test that ensures that a build with two steps has those steps execute sequentially? We used to just be able to trust init containers to do that for us, but now we need something to ensure it's working. Even something as simple as:

steps:
- image: busybox
  entrypoint: bash
  args: ['-c', 'sleep 3 && touch foo']
- image: busybox
  args: ['ls', 'foo']

This would fail if steps are swapped or if they're run concurrently due to a future bug.

imjasonh · 2019-02-28T19:49:56Z

  github.com/knative/build-pipeline/cmd/creds-init: gcr.io/knative-nightly/github.com/knative/build/build-base:latest
  github.com/knative/build-pipeline/cmd/git-init: gcr.io/knative-nightly/github.com/knative/build/build-base:latest
  github.com/knative/build-pipeline/cmd/bash: busybox # image should have shell in $PATH
+  github.com/knative/build-pipeline/cmd/entrypoint: busybox # image should have shell in $PATH


What does this image need a shell for?

thinking about it now again I guess it doesn't really need a shell. Will try it again without this

I wanted to have a shell because we are using this same image for the step to copy the binary to the tools/entrypoint location

imjasonh · 2019-02-28T19:58:52Z

+		args:     []string{"hello", "world"},
+		expectedArgs: []string{
+			"-wait_file", "",
+			"-post_file", "/tools/0",


We should put this in /builder/tools/, since it's not unreasonable to expect a user's process already uses a /tools directory. Namespacing things in /builder helps since we can just sort of "reserve" the whole directory.

used /tools because it was already there from the log writing entrypoint that got removed. Is builder created anyway, or just as part of a git resource?

moved tools to /builder/tools

nader-ziada · 2019-02-28T20:18:42Z

@imjasonh sure, will create the yaml test. Thanks

nader-ziada · 2019-02-28T20:52:46Z

the ko app recently with its latest version changed the path of where it puts the binary, which I something we need to know as part of running the entrypoint image
if you are trying this locally, please update your ko

go get -u github.com/google/go-containerregistry/cmd/ko

nader-ziada · 2019-03-01T15:07:04Z

/test pull-knative-build-pipeline-integration-tests

bobcatfish

very exciting!! just a few nitpicks from me :D

bobcatfish · 2019-03-01T19:37:01Z

-overwritten with a custom binary that redirects the logs to a separate location
-for aggregating the log output (this currently does nothing but is to support
-future work, see [#107](https://github.com/knative/build-pipeline/issues/107)).
+overwritten with a custom binary that ensures the containers within the `Task`pod


space after Task

nice catch :)

bobcatfish · 2019-03-01T19:39:07Z

+      args: ['-c', 'sleep 3 && touch foo']
+    - image: busybox
+      cmd: bash
+      args: ['-c', 'ls', 'foo']


this is a total nitpick and feel free to ignore me, but id personally rather see this as an end to end test than an example. to me the distinction is:

examples: things ppl look at when they want to know how to do something

end to end tests: things ppl dont need to look at which test our actual machinery

In knative/build everything was a yaml test since we didnt have the distinction 🤷‍♀️

yes, makes sense, will do that

bobcatfish · 2019-03-01T19:40:00Z

+		Name:    InitContainerName,
+		Image:   *entrypointImage,
+		Command: []string{"/bin/sh"},
+		// based on the ko version, the binary could be in one of two different locations


bobcatfish · 2019-03-01T19:41:07Z

+			var err error
+			step.Command, err = GetRemoteEntrypoint(cache, step.Image, kubeclient, build)
+			if err != nil {
+				logger.Errorf("**ALERT**: Error getting entry point image", err.Error())


does this error mean:

the controller has a problem <-- this is what the message currently implies

the user may have made an error (problem with image, problem with credentials)

if it's more likely the second, i dont think we should log something at all - if we do, i think it should be a bit less urgent looking

I'll remove the alert part of the message, but I think its useful to log something when we couldn't get the remote image, wdyt?

bobcatfish · 2019-03-01T19:42:07Z

-	img, err := remote.Image(ref, remote.WithAuthFromKeychain(authn.DefaultKeychain))
+	// this will first try to authenticate using the k8schain,
+	// then fall back to the google keychain,
+	// then fall back to anonymous


i think the docs are a bit light on how this works, could we add a bit more? (i think right now they just say you should provide a serviceaccount that has the right credentials, but this logic looks like it'll do a bit more)

will add more

bobcatfish · 2019-03-01T19:42:34Z

+with the binary and file(s) is mounted.
+
+If the image is a private registry, the service account should include an
+[ImagePullSecret](https://kubernetes.io/docs/tasks/configure-pod-container/configure-service-account/#add-imagepullsecrets-to-a-service-account)


khrisrichardson · 2019-03-04T14:52:51Z

+  taskSpec:
+    steps:
+    - image: busybox
+      cmd: bash


s/cmd:/command:/

thanks, will fix that

khrisrichardson · 2019-03-04T14:53:21Z

+      cmd: bash
+      args: ['-c', 'sleep 3 && touch foo']
+    - image: busybox
+      cmd: bash


s/cmd:/command:/

- Replace log-writer entrypoint with waiting entrypoint - Remove configmap for entrypoint image - Update docs to describe entrypoint function

- use service account in task - fallback to default service account - add vendor dependencies for k8schain Signed-off-by: Nader Ziada <nziada@pivotal.io>

…ute sequentially

- based on the version of ko used, the binary could be in /ko-app or in /ko-app/entrypoint

now with entrypoint, steps should specify the command in the command field and not in the args

- add e2e test to ensure entrypoint runs in the specified order - move entrypoint mount folder to /builder/tools since the /builder folder is already used by other parts of the controller - fix the taskrun order yaml - add more docs about pulling images

nader-ziada · 2019-03-04T16:41:22Z

addressed comments and rebased, ready for review

- add e2e test to ensure entrypoint runs in the specified order - move entrypoint mount folder to /builder/tools since the /builder folder is already used by other parts of the controller - fix the taskrun order yaml - add more docs about pulling images Signed-off-by: Nader Ziada <nziada@pivotal.io>

shashwathi · 2019-03-04T18:17:07Z

@pivotal-nader-ziada : In e2e yaml test logs one of the test is failing.

pull-private-image-run=SucceededFalse but should be succeededtrue

Primary problem is the e2e yaml script not catching the error 🤦‍♀️

- add e2e test to ensure entrypoint runs in the specified order - move entrypoint mount folder to /builder/tools since the /builder folder is already used by other parts of the controller - fix the taskrun order yaml - add more docs about pulling images Signed-off-by: Nader Ziada <nziada@pivotal.io>

knative-metrics-robot · 2019-03-04T19:27:10Z

The following is the coverage report on pkg/.
Say /test pull-knative-build-pipeline-go-coverage to re-run this coverage report

File	Old Coverage	New Coverage	Delta
pkg/reconciler/v1alpha1/taskrun/entrypoint/entrypoint.go	80.8%	63.4%	-17.4
pkg/reconciler/v1alpha1/taskrun/resources/pod.go	83.7%	83.5%	-0.3
pkg/reconciler/v1alpha1/taskrun/taskrun.go	74.3%	76.5%	2.2

shashwathi

Awesome PR 👍 🎆 Great job @pivotal-nader-ziada

My only feedback is about relying more k8s chain auth lib to pull image secrets from configured SA name instead of entry point doing that. Rest looks good.

knative-prow-robot · 2019-03-04T19:44:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pivotal-nader-ziada, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [pivotal-nader-ziada,shashwathi]

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

nader-ziada · 2019-03-05T01:58:44Z

The yaml tests are fixed now, the service account was missing ImagePullSecrets

I0304 19:56:21.762] >> Waiting for tests to finish
I0304 19:56:25.535] >> Checking test results
I0304 19:56:27.456] >> All YAML tests passed

shashwathi · 2019-03-05T15:34:45Z

/lgtm

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

knative-prow-robot requested review from imjasonh and shashwathi February 28, 2019 19:04

knative-prow-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 28, 2019

nader-ziada force-pushed the realcontainers branch 3 times, most recently from 7d9546e to d3a0b6e Compare February 28, 2019 19:50

imjasonh reviewed Feb 28, 2019

View reviewed changes

This was referenced Mar 1, 2019

Can't parse image references for gcr.io/cloud-builders/go:debian #573

Closed

Entrypoint rewriting logic is ignoring cmd #574

Closed

bobcatfish reviewed Mar 1, 2019

View reviewed changes

khrisrichardson reviewed Mar 4, 2019

View reviewed changes

nader-ziada added 5 commits March 4, 2019 11:39

Changing build steps from initContainers to Containers

535f5cd

- Replace log-writer entrypoint with waiting entrypoint - Remove configmap for entrypoint image - Update docs to describe entrypoint function

add support for getting remote image

8ebcf1c

- use service account in task - fallback to default service account - add vendor dependencies for k8schain Signed-off-by: Nader Ziada <nziada@pivotal.io>

YAML test that ensures that a build with 2 steps has those steps exec…

e6274a8

…ute sequentially

update the entrypoint copy step to be more generic

57cd24d

- based on the version of ko used, the binary could be in /ko-app or in /ko-app/entrypoint

update yaml examples (tests) to use command

9b72a85

now with entrypoint, steps should specify the command in the command field and not in the args

nader-ziada force-pushed the realcontainers branch from 981a767 to d22f5eb Compare March 4, 2019 16:40

nader-ziada force-pushed the realcontainers branch from d22f5eb to 71f6659 Compare March 4, 2019 17:34

nader-ziada force-pushed the realcontainers branch from 71f6659 to c8a4751 Compare March 4, 2019 18:28

nader-ziada force-pushed the realcontainers branch from c8a4751 to 8a506cc Compare March 4, 2019 19:25

shashwathi approved these changes Mar 4, 2019

View reviewed changes

knative-prow-robot assigned shashwathi Mar 5, 2019

knative-prow-robot added the lgtm Indicates that a PR is ready to be merged. label Mar 5, 2019

knative-prow-robot merged commit 163477f into tektoncd:master Mar 5, 2019

mchmarny unassigned shashwathi Mar 7, 2019

tejal29 mentioned this pull request Apr 30, 2019

Implement reference Unstructured store API to upload TaskRun logs GCS store #107

Closed

Uh oh!

Conversation

nader-ziada commented Feb 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Submitter Checklist

Release Notes

Uh oh!

imjasonh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Feb 28, 2019

Uh oh!

nader-ziada commented Feb 28, 2019

Uh oh!

nader-ziada commented Mar 1, 2019

Uh oh!

bobcatfish left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nader-ziada commented Mar 4, 2019

Uh oh!

shashwathi commented Mar 4, 2019

Uh oh!

knative-metrics-robot commented Mar 4, 2019

Uh oh!

shashwathi left a comment

Choose a reason for hiding this comment

Uh oh!

knative-prow-robot commented Mar 4, 2019

Uh oh!

nader-ziada commented Mar 5, 2019

Uh oh!

shashwathi commented Mar 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

nader-ziada commented Feb 28, 2019 •

edited

Loading