[Builder] Cleanup unnecessary mutate then revert of b.runConfig #32773

dnephin · 2017-04-21T20:45:17Z

This PR changes how runConfig is modified by the builder dispatchers. Previously a single runConfig reference was passed around everywhere. This made it possible for the builder to modify (either accidentally or intentionally) the config of an already created container without using the established interface.

Instead of passing around a single reference, copies of runConfig are created when the dispatcher needs to make a temporary modification to runConfig. Creating a copy removes the need to setup a defer to revert the temporary modification. It also avoids any accidental modification of a container configuration.

ContainerConfig was added to the commit options to support the existing behaviour of dispatchers.run(). Cmd was modified after the container was created, and this change was picked up by commit() because of the shared reference. With this API change the behaviour is a lot more obvious.

I think this changes makes the dispatchers and builder internals much easier to understand. It also removes some dead code from run() which wasn't obvious because of the clutter.

dnephin · 2017-04-21T20:49:55Z

builder/dockerfile/internals.go

-	if id == "" {
-		cmd := b.runConfig.Cmd
-		b.runConfig.Cmd = strslice.StrSlice(append(getShell(b.runConfig), "#(nop) ", comment))
-		defer func(cmd strslice.StrSlice) { b.runConfig.Cmd = cmd }(cmd)


There was an inconsistency here. The other two places that added a #(nop) comment did it with string concat, where as this adds it as another item in the slice.

I'm not sure if this will cause a cache miss, but at least it's consistent now.

dnephin · 2017-04-23T01:57:23Z

builder/dockerfile/dispatchers.go

+	// daemon.Commit() does a daemon.getContainer() to build the history, and
+	// this modifies the config after the container has been run, changing it
+	// to the updated Cmd
+	// saveCmd


@tonistiigi this is a big surprise to me. I wonder if it is to you as well.

I guess this has been the case for a while, but nothing relied on it until #31584. It looks like there was lots of discussion about design and docs in that issue, but not much about the implementation.

This was relying of the fact ContainerCreate used the reference directly, so it was possible for the builder to modify runConfig and it would change the config of the already created container!

I'm trying to find a way to implement this without this hack. I think Commit() will need to change to accept more args? Maybe it should accept the history?

Ok, I fixed this by adding a CreatedBy field to ContainerCommitConfig.

dnephin · 2017-04-28T16:13:49Z

builder/dockerfile/builder.go

@@ -316,6 +316,10 @@ func (b *Builder) hasFromImage() bool {
 //
 // TODO: Remove?
 func BuildFromConfig(config *container.Config, changes []string) (*container.Config, error) {
+	if len(changes) == 0 {
+		return config, nil
+	}


This change wasn't strictly necessary, but I think it helps clarify the behaviour of this function when changes is empty.

BuildFromConfig() needs a much larger cleanup, but this PR isn't the place.

dnephin · 2017-04-28T16:14:41Z

builder/dockerfile/dispatchers.go

-	defer func(cmd strslice.StrSlice) { req.runConfig.Cmd = cmd }(cmd)
+	// TODO: why is this done here. This seems to be done at random places all over
+	// the builder
+	req.runConfig.Image = req.builder.image


These TODO comments about setting runConfig.Image from b.Image are addressed in a follow up PR #32749

tonistiigi

LGTM

tonistiigi · 2017-05-01T21:26:59Z

api/types/backend/backend.go

@@ -98,6 +99,9 @@ type ExecProcessConfig struct {
 type ContainerCommitConfig struct {
 	types.ContainerCommitConfig
 	Changes []string
+	// TODO: ContainerConfig is only used by the dockerfile Builder, so remove it
+	// once the Builder has been updated to use a different interface
+	ContainerConfig *container.Config


Should we mark in the name that this should not be used by the API so we don't need to deprecate it? Or maybe it could be useful for docker commit as well.

This is not actually part of the HTTP API (even though it's under api/types). It's only in the interface between the router and the daemon. I don't think we'll have to deprecate it, we can just remove it at some point.

Instead of mutating and reverting, just create a copy and pass the copy around. Add a unit test for builder dispatcher.run Fix two test failures Fix image history by adding a CreatedBy to commit options. Previously the createdBy field was being created by modifying a reference to the runConfig that was held from when the container was created. Fix a test that expected a trailing slash. Previously the runConfig was being modified by container create. Now that we're creating a copy of runConfig instead of sharing a reference the runConfig retains the trailing slash. Signed-off-by: Daniel Nephin <dnephin@docker.com>

vdemeester

LGTM 🐯

GordonTheTurtle added the status/0-triage label Apr 21, 2017

dnephin force-pushed the refactor-builder-split-commit-2 branch from c47106d to d6ff7b2 Compare April 21, 2017 20:47

dnephin commented Apr 21, 2017

View reviewed changes

dnephin added status/failing-ci Indicates that the PR in its current state fails the test suite status/2-code-review and removed status/0-triage labels Apr 21, 2017

dnephin force-pushed the refactor-builder-split-commit-2 branch 2 times, most recently from d5a2cc4 to 510b888 Compare April 22, 2017 22:10

dnephin commented Apr 23, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-split-commit-2 branch 3 times, most recently from 27cdc24 to 13b64e1 Compare April 24, 2017 20:16

dnephin added the rebuild/windowsRS1 label Apr 24, 2017

dnephin mentioned this pull request Apr 24, 2017

[Builder] Remove ContainerUpdateCmdOnBuild #32749

Merged

dnephin added rebuild/windowsRS1 and removed rebuild/windowsRS1 status/failing-ci Indicates that the PR in its current state fails the test suite labels Apr 24, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Apr 24, 2017

dnephin added the rebuild/powerpc label Apr 24, 2017

GordonTheTurtle removed the rebuild/powerpc label Apr 24, 2017

dnephin force-pushed the refactor-builder-split-commit-2 branch from 13b64e1 to e1eebec Compare April 25, 2017 00:09

dnephin added the rebuild/windowsRS1 label Apr 25, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label Apr 25, 2017

dnephin force-pushed the refactor-builder-split-commit-2 branch from e1eebec to f87e629 Compare April 25, 2017 15:04

dnephin force-pushed the refactor-builder-split-commit-2 branch 2 times, most recently from 4614f99 to 5f00665 Compare April 27, 2017 16:12

dnephin added the rebuild/z label Apr 27, 2017

GordonTheTurtle removed the rebuild/z label Apr 27, 2017

dnephin force-pushed the refactor-builder-split-commit-2 branch 4 times, most recently from caa55c2 to ec27c6f Compare April 28, 2017 16:22

dnephin commented Apr 28, 2017

View reviewed changes

dnephin mentioned this pull request Apr 28, 2017

Fix cache miss when builtin build args are used #32921

Merged

dnephin force-pushed the refactor-builder-split-commit-2 branch 4 times, most recently from 0f7bb0f to f452a2e Compare May 1, 2017 16:52

dnephin requested review from tonistiigi and cpuguy83 May 1, 2017 20:05

dnephin added the rebuild/windowsRS1 label May 1, 2017

GordonTheTurtle removed the rebuild/windowsRS1 label May 1, 2017

dnephin requested a review from vdemeester May 1, 2017 21:06

tonistiigi approved these changes May 1, 2017

View reviewed changes

dnephin force-pushed the refactor-builder-split-commit-2 branch from f452a2e to 9f738cc Compare May 1, 2017 22:36

dnephin added rebuild/powerpc labels May 2, 2017

GordonTheTurtle removed rebuild/powerpc labels May 2, 2017

vdemeester approved these changes May 3, 2017

View reviewed changes

vdemeester merged commit d35fc14 into moby:master May 3, 2017

GordonTheTurtle added this to the 17.06.0 milestone May 3, 2017

dnephin deleted the refactor-builder-split-commit-2 branch May 3, 2017 14:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Builder] Cleanup unnecessary mutate then revert of b.runConfig #32773

[Builder] Cleanup unnecessary mutate then revert of b.runConfig #32773

dnephin commented Apr 21, 2017 •

edited

Loading

dnephin Apr 21, 2017 •

edited

Loading

dnephin Apr 23, 2017

dnephin Apr 24, 2017

dnephin Apr 28, 2017

dnephin Apr 28, 2017

tonistiigi left a comment

tonistiigi May 1, 2017

dnephin May 1, 2017

vdemeester left a comment

[Builder] Cleanup unnecessary mutate then revert of b.runConfig #32773

[Builder] Cleanup unnecessary mutate then revert of b.runConfig #32773

Conversation

dnephin commented Apr 21, 2017 • edited Loading

dnephin Apr 21, 2017 • edited Loading

Choose a reason for hiding this comment

dnephin Apr 23, 2017

Choose a reason for hiding this comment

dnephin Apr 24, 2017

Choose a reason for hiding this comment

dnephin Apr 28, 2017

Choose a reason for hiding this comment

dnephin Apr 28, 2017

Choose a reason for hiding this comment

tonistiigi left a comment

Choose a reason for hiding this comment

tonistiigi May 1, 2017

Choose a reason for hiding this comment

dnephin May 1, 2017

Choose a reason for hiding this comment

vdemeester left a comment

Choose a reason for hiding this comment

dnephin commented Apr 21, 2017 •

edited

Loading

dnephin Apr 21, 2017 •

edited

Loading