Entrypoint img #522

aaron-prindle · 2019-01-07T21:21:38Z

What is the problem being solved?
This PR adds an entrypoint image which is capable of running another command with some pre-run/post-run hooks. The goal of this is to allow for the the entrypoint of a container in a Build Step to have it's entrypoint rewritten by mounting this entrypoint image into the container provided in a Step, rewriting the entrypoint, then running the user container w/ the additional pre-run/post-run hooks. The addition of this image enables the removal of init containers as per #521. This was originally part of #470 but is now split out.

What future work remains to be done?
This PR adds the entrypoint image and release step. The future work involved is modifying Build to stop using init container and use this entrypoint image as a way of making that possible.

TODO:

Add entrypoint image to release process

knative-prow-robot · 2019-01-07T21:21:43Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

If they are not already assigned, you can assign the PR to them by writing /assign @mattmoor 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

knative-metrics-robot · 2019-01-14T21:12:33Z

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

File	Old Coverage	New Coverage
pkg/entrypoint/options.go	Do not exist	22.2%
pkg/entrypoint/options/load.go	Do not exist	66.7%
pkg/entrypoint/run.go	Do not exist	83.7%

knative-prow-robot · 2019-01-14T21:12:40Z

@aaron-prindle: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-knative-build-go-coverage	`6046ed1`	link	`/test pull-knative-build-go-coverage`

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.

bobcatfish

Looks like it's getting there! I think it's a lot easier to review this on its own vs in the context of #470 :)

My initial feedback at this point is that it seems like we could significantly simplify the options logic, looks like there's a lot of sophistication in there that we probably don't need at this point.

Also @imjasonh had mentioned in #470 (review) adding a README that describes in more detail how the logic works, which would be very handy (esp. for reviewing!)

bobcatfish · 2019-01-23T00:02:39Z

cmd/entrypoint/main.go

+To use it, run
+```
+image: github.com/knative/build/cmd/entrypoint
+```


im not clear on what it means to run this, is this configuration part of a config file somewhere?

bobcatfish · 2019-01-23T00:03:45Z

cmd/entrypoint/main.go

+```
+
+It used in knative/build as a method of running containers in
+order that are in the same pod this is done by:


maybe this could be two sentences, like:

"It is used as a method of running container in a specified order on the same pod. This is done by:"

bobcatfish · 2019-01-23T00:04:34Z

cmd/entrypoint/main.go

+image: github.com/build-pipeline/cmd/entrypoint
+args: ['-args', 'ARGUMENTS_FOR_SHELL_COMMAND']
+"args":["/kaniko/executor"],shouldWaitForPrevStep":false,"preRunFile":"/tools/0","shouldRunPostRun":true,"postRunFile":"/tools/1"
+```


im kinda confused about what im looking at here, is this part of a build config?

I think array brackets are misplaced. Is it like this?

["/kaniko/executor", "shouldWaitForPrevStep": false, "preRunFile":"/tools/0", "shouldRunPostRun":true, "postRunFile":"/tools/1"]

That list corresponds to list of accepted args for entrypoint image I think.

The example shows how a user can just create step with entrypoint image and pass those args. I think this is the pattern in other cmd files(I think)

bobcatfish · 2019-01-23T00:06:53Z

cmd/entrypoint/main.go

+args: ['-args', 'ARGUMENTS_FOR_SHELL_COMMAND']
+"args":["/kaniko/executor"],shouldWaitForPrevStep":false,"preRunFile":"/tools/0","shouldRunPostRun":true,"postRunFile":"/tools/1"
+```
+*/


can we move this to a README in the same dir? ( @shashwathi do you think that makes sense? i think that's not been the norm for other cmds)

Its not the norm but if we are taking the pain to add README file then I would rather have documentation under docs folder instead of scattered README across the repo(exception like test package).

Would it make sense to add this information to a README file under docs/ then? Or would it make more sense to leave it as a comment (as it is currently) like it is done in the other cmds?

if we are taking the pain to add README file then I would rather have documentation under docs folder instead of scattered README across the repo(exception like test package)

hm i think this might make it hard to find the README tho @shashwathi ? i find that if docs are close to what they document they're often easier to find - unless we link to that README from a README here 🤔

@aaron-prindle it doesn't matter too much cuz we can always move the doc later, so I think create a README in either location for now - the important thing is to have it documented

bobcatfish · 2019-01-23T00:10:37Z

pkg/entrypoint/run_test.go

+
+ // reset paths to new temp dir
+ options.PreRunFile = filepath.Join(tmpDir, options.PreRunFile)
+ options.PostRunFile = filepath.Join(tmpDir, options.PostRunFile)


what do you think about making it so that the Run function takes the options values as a parameter? Then we can pass in whatever values we want in the test without having to change global state

the options.Load function could return an options object instead of setting global state

bobcatfish · 2019-01-23T00:12:28Z

pkg/entrypoint/options/load.go

+ LoadConfig(config string) error
+ AddFlags(flags *flag.FlagSet)
+ Complete(args []string)
+}


whats the use case for this OptionLoader interface? seems like in our case we only have one kind of option loader (maybe im wrong tho!). if that's the case, we could simplify this by removing the option loader interface and just using the type we're using

would it work to simplify the option loading to something like what we do with other cmd executables in build:

build/cmd/git-init/main.go

Lines 29 to 33 in addfaf4

var (

url = flag.String("url", "", "The url of the Git repository to initialize.")

revision = flag.String("revision", "", "The Git revision to make the repository HEAD")

path = flag.String("path", "", "Path of directory under which git repository will be copied")

)

e.g. declaring a set of flags?

I like the idea of providing all image relevant flags in entrypoint/main.go. We can avoid layer of abstraction if its not needed

pkg/entrypoint/run.go

+ var arguments []string
+ if len(o.Args) > 1 {
+ arguments = o.Args[1:]
+ }


pkg/entrypoint/options_test.go

+ var testCases = []struct {
+ name string
+ input Options
+ expectedErr bool


pkg/entrypoint/run.go

+
+ var returnCode int
+ if status, ok := command.ProcessState.Sys().(syscall.WaitStatus); ok {
+ returnCode = status.ExitStatus()


pkg/entrypoint/run.go

+ return returnCode, commandErr
+}
+
+func (o *Options) waitForPrevStep() error {


pkg/entrypoint/options_test.go

+ },
+ }}
+
+ for _, testCase := range testCases {


pkg/entrypoint/run.go

+ // execute the user specified command
+ done := make(chan error)
+ go func() {
+ done <- command.Wait()


shashwathi

Great work. 🎆

imjasonh · 2019-02-02T02:54:19Z

Closing in favor of tektoncd/pipeline#448

knative-prow-robot added the size/XXL label Jan 7, 2019

knative-prow-robot requested review from imjasonh and shashwathi January 7, 2019 21:21

aaron-prindle force-pushed the entrypoint-img branch 3 times, most recently from b2027b1 to 12925bd Compare January 7, 2019 23:40

added entrypoint img

1877a7e

aaron-prindle force-pushed the entrypoint-img branch from 12925bd to 1877a7e Compare January 14, 2019 01:55

knative-prow-robot added size/XL and removed size/XXL labels Jan 14, 2019

remove entrypoint/wrapper

6046ed1

aaron-prindle force-pushed the entrypoint-img branch from f5527aa to 6046ed1 Compare January 14, 2019 21:11

bobcatfish mentioned this pull request Jan 23, 2019

No init containers #470

Closed

bobcatfish reviewed Jan 23, 2019

View reviewed changes

shashwathi reviewed Jan 23, 2019

View reviewed changes

pkg/entrypoint/options_test.go

},

}}

for _, testCase := range testCases {

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

shashwathi reviewed Jan 23, 2019

View reviewed changes

pkg/entrypoint/run.go

// execute the user specified command

done := make(chan error)

go func() {

done <- command.Wait()

This comment was marked as resolved.

Sign in to view

This comment was marked as resolved.

Sign in to view

shashwathi reviewed Jan 23, 2019

View reviewed changes

imjasonh mentioned this pull request Jan 29, 2019

Add entrypoint overrider image tektoncd/pipeline#448

Merged

imjasonh closed this Feb 2, 2019

bobcatfish mentioned this pull request Feb 12, 2019

Explore moving away from init containers tektoncd/pipeline#224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entrypoint img #522

Entrypoint img #522

aaron-prindle commented Jan 7, 2019 •

edited

Loading

knative-prow-robot commented Jan 7, 2019

knative-metrics-robot commented Jan 14, 2019

knative-prow-robot commented Jan 14, 2019

bobcatfish left a comment

bobcatfish Jan 23, 2019

bobcatfish Jan 23, 2019

bobcatfish Jan 23, 2019

shashwathi Jan 23, 2019

shashwathi Jan 23, 2019

bobcatfish Jan 23, 2019

shashwathi Jan 23, 2019

aaron-prindle Jan 23, 2019

bobcatfish Jan 24, 2019

bobcatfish Jan 23, 2019

bobcatfish Jan 23, 2019

bobcatfish Jan 23, 2019

shashwathi Jan 23, 2019

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

shashwathi left a comment

imjasonh commented Feb 2, 2019

	var (
	url = flag.String("url", "", "The url of the Git repository to initialize.")
	revision = flag.String("revision", "", "The Git revision to make the repository HEAD")
	path = flag.String("path", "", "Path of directory under which git repository will be copied")
	)

Entrypoint img #522

Entrypoint img #522

Conversation

aaron-prindle commented Jan 7, 2019 • edited Loading

knative-prow-robot commented Jan 7, 2019

knative-metrics-robot commented Jan 14, 2019

knative-prow-robot commented Jan 14, 2019

bobcatfish 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

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

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

shashwathi left a comment

Choose a reason for hiding this comment

imjasonh commented Feb 2, 2019

aaron-prindle commented Jan 7, 2019 •

edited

Loading