From 4886c7824694dc08e809f0e355fbc2548562ac21 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Sat, 22 Apr 2017 18:34:04 -0400 Subject: [PATCH 1/3] Fix setting b.runConfig.Image at arbitrary places. Previously this value was set at some point attrbitrarily between when it was updated and when it was going to be used next. Instead always set it as the last step of dispatch. Signed-off-by: Daniel Nephin Upstream-commit: 3dcab289821ddd4575b7e48d463ba8ef2af492ea Component: engine --- components/engine/builder/dockerfile/dispatchers.go | 7 ------- components/engine/builder/dockerfile/evaluator.go | 9 ++++++++- components/engine/builder/dockerfile/internals.go | 10 ++-------- 3 files changed, 10 insertions(+), 16 deletions(-) diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index 715161270d..f83a17ce33 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -315,10 +315,6 @@ func workdir(req dispatchRequest) error { return nil } - // TODO: why is this done here. This seems to be done at random places all over - // the builder - req.runConfig.Image = req.builder.image - comment := "WORKDIR " + req.runConfig.WorkingDir runConfigWithCommentCmd := copyRunConfig(req.runConfig, withCmdCommentString(comment)) if hit, err := req.builder.probeCache(req.builder.image, runConfigWithCommentCmd); err != nil || hit { @@ -372,9 +368,6 @@ func run(req dispatchRequest) error { saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs) } - // TODO: this was previously in b.create(), why is it necessary? - req.runConfig.Image = req.builder.image - runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd)) hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe) if err != nil || hit { diff --git a/components/engine/builder/dockerfile/evaluator.go b/components/engine/builder/dockerfile/evaluator.go index abad15e096..e98a4e2f0d 100644 --- a/components/engine/builder/dockerfile/evaluator.go +++ b/components/engine/builder/dockerfile/evaluator.go @@ -173,7 +173,14 @@ func (b *Builder) dispatch(stepN int, stepTotal int, node *parser.Node, shlex *S // XXX yes, we skip any cmds that are not valid; the parser should have // picked these out already. if f, ok := evaluateTable[cmd]; ok { - return f(newDispatchRequestFromNode(node, b, strList, shlex)) + if err := f(newDispatchRequestFromNode(node, b, strList, shlex)); err != nil { + return err + } + // TODO: return an object instead of setting things on builder + // If the step created a new image set it as the imageID for the + // current runConfig + b.runConfig.Image = b.image + return nil } return fmt.Errorf("Unknown instruction: %s", upperCasedCmd) diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index a772c615a8..ec411c4638 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -42,8 +42,6 @@ func (b *Builder) commit(comment string) error { if !b.hasFromImage() { return errors.New("Please provide a source image with `from` prior to commit") } - // TODO: why is this set here? - b.runConfig.Image = b.image runConfigWithCommentCmd := copyRunConfig(b.runConfig, withCmdComment(comment)) hit, err := b.probeCache(b.image, runConfigWithCommentCmd) @@ -100,10 +98,6 @@ func (b *Builder) runContextCommand(args []string, allowRemote bool, allowLocalD // Work in daemon-specific filepath semantics dest := filepath.FromSlash(args[len(args)-1]) // last one is always the dest - // TODO: why is this done here. This seems to be done at random places all over - // the builder - b.runConfig.Image = b.image - var infos []copyInfo // Loop through each src file and calculate the info we need to @@ -542,12 +536,12 @@ func (b *Builder) processImageFrom(img builder.Image) error { // If an image is found, probeCache returns `(true, nil)`. // If no image is found, it returns `(false, nil)`. // If there is any error, it returns `(false, err)`. -func (b *Builder) probeCache(imageID string, runConfig *container.Config) (bool, error) { +func (b *Builder) probeCache(parentID string, runConfig *container.Config) (bool, error) { c := b.imageCache if c == nil || b.options.NoCache || b.cacheBusted { return false, nil } - cache, err := c.GetCache(imageID, runConfig) + cache, err := c.GetCache(parentID, runConfig) if err != nil { return false, err } From 8e846811d4e00def40db9ef112619fd5b2ef6388 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Thu, 20 Apr 2017 15:46:12 -0400 Subject: [PATCH 2/3] Remove ContainerUpdateCmdOnBuild, it does nothing. Set a blank entrypoint to preserve the old behaviour. Signed-off-by: Daniel Nephin Upstream-commit: 97f860716497f6f75236b72e4af90e01afad832c Component: engine --- components/engine/builder/builder.go | 2 -- .../engine/builder/dockerfile/dispatchers.go | 5 +++++ .../engine/builder/dockerfile/dispatchers_test.go | 3 +++ components/engine/builder/dockerfile/internals.go | 6 ------ .../engine/builder/dockerfile/mockbackend_test.go | 4 ---- components/engine/daemon/update.go | 14 -------------- .../integration-cli/docker_cli_build_test.go | 4 ++-- 7 files changed, 10 insertions(+), 28 deletions(-) diff --git a/components/engine/builder/builder.go b/components/engine/builder/builder.go index 785f0eb3c3..ccc43f6cc2 100644 --- a/components/engine/builder/builder.go +++ b/components/engine/builder/builder.go @@ -54,8 +54,6 @@ type Backend interface { ContainerStart(containerID string, hostConfig *container.HostConfig, checkpoint string, checkpointDir string) error // ContainerWait stops processing until the given container is stopped. ContainerWait(containerID string, timeout time.Duration) (int, error) - // ContainerUpdateCmdOnBuild updates container.Path and container.Args - ContainerUpdateCmdOnBuild(containerID string, cmd []string) error // ContainerCreateWorkdir creates the workdir ContainerCreateWorkdir(containerID string) error diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index f83a17ce33..85f82806e9 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -383,6 +383,11 @@ func run(req dispatchRequest) error { logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) + // Set blank entrypoint to cancel the entrypoint from the parent image + if len(runConfig.Cmd) > 0 { + runConfig.Entrypoint = strslice.StrSlice{""} + } + cID, err := req.builder.create(runConfig) if err != nil { return err diff --git a/components/engine/builder/dockerfile/dispatchers_test.go b/components/engine/builder/dockerfile/dispatchers_test.go index 72a82dbd91..0dd3894348 100644 --- a/components/engine/builder/dockerfile/dispatchers_test.go +++ b/components/engine/builder/dockerfile/dispatchers_test.go @@ -448,6 +448,7 @@ func TestRunWithBuildArgs(t *testing.T) { getCacheFunc: func(parentID string, cfg *container.Config) (string, error) { // Check the runConfig.Cmd sent to probeCache() assert.Equal(t, cachedCmd, cfg.Cmd) + assert.Equal(t, strslice.StrSlice(nil), cfg.Entrypoint) return "", nil }, } @@ -462,12 +463,14 @@ func TestRunWithBuildArgs(t *testing.T) { // Check the runConfig.Cmd sent to create() assert.Equal(t, cmdWithShell, config.Config.Cmd) assert.Contains(t, config.Config.Env, "one=two") + assert.Equal(t, strslice.StrSlice{""}, config.Config.Entrypoint) return container.ContainerCreateCreatedBody{ID: "12345"}, nil } mockBackend.commitFunc = func(cID string, cfg *backend.ContainerCommitConfig) (string, error) { // Check the runConfig.Cmd sent to commit() assert.Equal(t, origCmd, cfg.Config.Cmd) assert.Equal(t, cachedCmd, cfg.ContainerConfig.Cmd) + assert.Equal(t, strslice.StrSlice(nil), cfg.Config.Entrypoint) return "", nil } diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index ec411c4638..094331f1ff 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -601,12 +601,6 @@ func (b *Builder) create(runConfig *container.Config) (string, error) { b.tmpContainers[c.ID] = struct{}{} fmt.Fprintf(b.Stdout, " ---> Running in %s\n", stringid.TruncateID(c.ID)) - - // override the entry point that may have been picked up from the base image - if err := b.docker.ContainerUpdateCmdOnBuild(c.ID, runConfig.Cmd); err != nil { - return "", err - } - return c.ID, nil } diff --git a/components/engine/builder/dockerfile/mockbackend_test.go b/components/engine/builder/dockerfile/mockbackend_test.go index e8647f426c..bdd198ccfe 100644 --- a/components/engine/builder/dockerfile/mockbackend_test.go +++ b/components/engine/builder/dockerfile/mockbackend_test.go @@ -73,10 +73,6 @@ func (m *MockBackend) ContainerWait(containerID string, timeout time.Duration) ( return 0, nil } -func (m *MockBackend) ContainerUpdateCmdOnBuild(containerID string, cmd []string) error { - return nil -} - func (m *MockBackend) ContainerCreateWorkdir(containerID string) error { return nil } diff --git a/components/engine/daemon/update.go b/components/engine/daemon/update.go index 6e26eeb96a..76e4a3f93f 100644 --- a/components/engine/daemon/update.go +++ b/components/engine/daemon/update.go @@ -22,20 +22,6 @@ func (daemon *Daemon) ContainerUpdate(name string, hostConfig *container.HostCon return container.ContainerUpdateOKBody{Warnings: warnings}, nil } -// ContainerUpdateCmdOnBuild updates Path and Args for the container with ID cID. -func (daemon *Daemon) ContainerUpdateCmdOnBuild(cID string, cmd []string) error { - if len(cmd) == 0 { - return nil - } - c, err := daemon.GetContainer(cID) - if err != nil { - return err - } - c.Path = cmd[0] - c.Args = cmd[1:] - return nil -} - func (daemon *Daemon) update(name string, hostConfig *container.HostConfig) error { if hostConfig == nil { return nil diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 5ee64cbe4b..2e9b897d0f 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -337,13 +337,13 @@ func (s *DockerSuite) TestBuildOnBuildCmdEntrypointJSON(c *check.C) { name1 := "onbuildcmd" name2 := "onbuildgenerated" - buildImageSuccessfully(c, name1, build.WithDockerfile(` + cli.BuildCmd(c, name1, build.WithDockerfile(` FROM busybox ONBUILD CMD ["hello world"] ONBUILD ENTRYPOINT ["echo"] ONBUILD RUN ["true"]`)) - buildImageSuccessfully(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1))) + cli.BuildCmd(c, name2, build.WithDockerfile(fmt.Sprintf(`FROM %s`, name1))) result := cli.DockerCmd(c, "run", name2) result.Assert(c, icmd.Expected{Out: "hello world"}) From d3da06632a1d6538f43a623bc93ace72b0c5f6b7 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Tue, 25 Apr 2017 12:21:43 -0400 Subject: [PATCH 3/3] Fix run with entrypoint in base image Update a test to use a base image with entrypoint to that the linux build has at least one test that behaves like all the windows tests. Signed-off-by: Daniel Nephin Upstream-commit: d9371ee80764d0eecf7b8a562121f0a6234167a3 Component: engine --- .../engine/builder/dockerfile/dispatchers.go | 13 +++++-------- .../engine/builder/dockerfile/internals.go | 18 +++++++++++++++++- .../integration-cli/docker_cli_build_test.go | 18 ++++++++++++------ 3 files changed, 34 insertions(+), 15 deletions(-) diff --git a/components/engine/builder/dockerfile/dispatchers.go b/components/engine/builder/dockerfile/dispatchers.go index 85f82806e9..0b76f2ff8e 100644 --- a/components/engine/builder/dockerfile/dispatchers.go +++ b/components/engine/builder/dockerfile/dispatchers.go @@ -368,7 +368,9 @@ func run(req dispatchRequest) error { saveCmd = prependEnvOnCmd(req.builder.buildArgs, buildArgs, cmdFromArgs) } - runConfigForCacheProbe := copyRunConfig(req.runConfig, withCmd(saveCmd)) + runConfigForCacheProbe := copyRunConfig(req.runConfig, + withCmd(saveCmd), + withEntrypointOverride(saveCmd, nil)) hit, err := req.builder.probeCache(req.builder.image, runConfigForCacheProbe) if err != nil || hit { return err @@ -376,18 +378,13 @@ func run(req dispatchRequest) error { runConfig := copyRunConfig(req.runConfig, withCmd(cmdFromArgs), - withEnv(append(req.runConfig.Env, buildArgs...))) + withEnv(append(req.runConfig.Env, buildArgs...)), + withEntrypointOverride(saveCmd, strslice.StrSlice{""})) // set config as already being escaped, this prevents double escaping on windows runConfig.ArgsEscaped = true logrus.Debugf("[BUILDER] Command to be executed: %v", runConfig.Cmd) - - // Set blank entrypoint to cancel the entrypoint from the parent image - if len(runConfig.Cmd) > 0 { - runConfig.Entrypoint = strslice.StrSlice{""} - } - cID, err := req.builder.create(runConfig) if err != nil { return err diff --git a/components/engine/builder/dockerfile/internals.go b/components/engine/builder/dockerfile/internals.go index 094331f1ff..55f3c15490 100644 --- a/components/engine/builder/dockerfile/internals.go +++ b/components/engine/builder/dockerfile/internals.go @@ -65,7 +65,8 @@ func (b *Builder) commitContainer(id string, containerConfig *container.Config) ContainerCommitConfig: types.ContainerCommitConfig{ Author: b.maintainer, Pause: true, - Config: b.runConfig, + // TODO: this should be done by Commit() + Config: copyRunConfig(b.runConfig), }, ContainerConfig: containerConfig, } @@ -233,6 +234,21 @@ func withEnv(env []string) runConfigModifier { } } +// withEntrypointOverride sets an entrypoint on runConfig if the command is +// not empty. The entrypoint is left unmodified if command is empty. +// +// The dockerfile RUN instruction expect to run without an entrypoint +// so the runConfig entrypoint needs to be modified accordingly. ContainerCreate +// will change a []string{""} entrypoint to nil, so we probe the cache with the +// nil entrypoint. +func withEntrypointOverride(cmd []string, entrypoint []string) runConfigModifier { + return func(runConfig *container.Config) { + if len(cmd) > 0 { + runConfig.Entrypoint = entrypoint + } + } +} + // getShell is a helper function which gets the right shell for prefixing the // shell-form of RUN, ENTRYPOINT and CMD instructions func getShell(c *container.Config) []string { diff --git a/components/engine/integration-cli/docker_cli_build_test.go b/components/engine/integration-cli/docker_cli_build_test.go index 2e9b897d0f..701572908b 100644 --- a/components/engine/integration-cli/docker_cli_build_test.go +++ b/components/engine/integration-cli/docker_cli_build_test.go @@ -1785,11 +1785,17 @@ func (s *DockerSuite) TestBuildConditionalCache(c *check.C) { } } -// FIXME(vdemeester) this really seems to test the same thing as before func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check.C) { name := "testbuildaddmultiplelocalfilewithcache" - dockerfile := ` + baseName := name + "-base" + + cli.BuildCmd(c, baseName, build.WithDockerfile(` FROM busybox + ENTRYPOINT ["/bin/sh"] + `)) + + dockerfile := ` + FROM testbuildaddmultiplelocalfilewithcache-base MAINTAINER dockerio ADD foo Dockerfile /usr/lib/bla/ RUN sh -c "[ $(cat /usr/lib/bla/foo) = "hello" ]"` @@ -1799,15 +1805,15 @@ func (s *DockerSuite) TestBuildAddMultipleLocalFileWithAndWithoutCache(c *check. defer ctx.Close() cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) id1 := getIDByName(c, name) - cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) + result2 := cli.BuildCmd(c, name, build.WithExternalBuildContext(ctx)) id2 := getIDByName(c, name) - cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx)) + result3 := cli.BuildCmd(c, name, build.WithoutCache, build.WithExternalBuildContext(ctx)) id3 := getIDByName(c, name) if id1 != id2 { - c.Fatal("The cache should have been used but hasn't.") + c.Fatalf("The cache should have been used but hasn't: %s", result2.Stdout()) } if id1 == id3 { - c.Fatal("The cache should have been invalided but hasn't.") + c.Fatalf("The cache should have been invalided but hasn't: %s", result3.Stdout()) } }