[Builder] Introduce a typed command system and 2 phase parse/dispatch build #33492

simonferquel · 2017-06-02T12:33:47Z

This is a work base to introduce more features like build time
dockerfile optimisations, dependency analysis and parallel build, as
well as a first step to go from a dispatch-inline process to a
frontend+backend process.

- What I did
I moved the whole parsing of Dockerfiles into a separate package generating a typed command graph.
Then I changed the current dispatch logic so that it processes typed commands instead of AST subtrees.

This allows to have a global view of the build process before actually running it, allowing command tree optimisations, parallel execution of build stages, and even distributed execution in the future.

It has also the benefit to make the Builder code much more maintainable with a clear separation of concerns between, parsing, env variables / build args expansion and command dispatching.

- How I did it

I refactored the code to introduce a collection of typed commands corresponding to each operation in the dispatchers.go file.

- How to verify it

Builder keeps the same public interface. The current test suite is OK.
~~I still have to re-implement existing in-package unit tests though~~ <- unit tests are back

- Description for the changelog

Refactoring of dockerfile builder to enable future improvements

cc @tonistiigi and @dnephin

(Note: I squashed all my commits due to rebase complexity. Unsquashed - and outdated - branch is here: https://github.com/simonferquel/docker/tree/typed-builder-commands)

dnephin · 2017-06-06T17:57:13Z

builder/dockerfile/builder.go

 	if err != nil {
+		if strings.Index(err.Error(), "unknown instruction:") == 0 {


Could we handle this with an error type instead of string comparison? This is likely to break

dnephin · 2017-06-06T18:01:10Z

builder/dockerfile/builder.go

+		totalCommands += len(stage.Commands)
+	}
+	for _, meta := range metaArgs {
+		shlex := NewShellLex(escapeToken)


This can be done before the for loop

dnephin · 2017-06-06T18:03:39Z

builder/dockerfile/builder.go

+		fmt.Fprintf(b.Stdout, "%v / %v %v", currentCommandIndex, totalCommands, &meta)
+		currentCommandIndex++
+		fmt.Fprintln(b.Stdout)
+	}


Splitting out a function for "dispatching" metaArgs would be great

tonistiigi · 2017-06-07T21:30:08Z

builder/dockerfile/instructions/commands.go

+		return err
+	}
+	c.Dest = dst
+	from, err := expander(c.From)


I don't think this is supported atm. We have to be careful about this as it may prevent us from determining dependencies between stages before the image config has been pulled. We may want to do this in the context of global args like the strings in FROM command but that's a discussion for later.

hmm, flags where not expanded then ? i'll change that!

tonistiigi · 2017-06-07T21:33:42Z

builder/dockerfile/instructions/parse.go

+		switch c := cmd.(type) {
+		case *FromCommand:
+			if c.StageName == "" {
+				c.StageName = strconv.Itoa(len(stages))


This does not look correct. --from=n syntax is for accessing all stages, not the ones that didn't have a name.

tonistiigi · 2017-06-07T21:40:25Z

builder/dockerfile/instructions/parse.go

+			if c.StageName == "" {
+				c.StageName = strconv.Itoa(len(stages))
+			}
+			stage := BuildableStage{Name: c.StageName, Commands: []interface{}{c}}


I'd really like to remove a from command inside a stage and just make FROM==buildstage. If complicates things atm it could be a follow-up with a comment.

It complicates things a bit for the BuildFromConfig stuff: in one case we have a from command that will initialize the dispatchState with a named stage or image lookup, in the other case, the resume build command just copy an existing runConfig. Also, I like the idea of having every command dispatch logic mapped to a given typed command and for consistency sake, I like the idea to treat "FROM" command as any other dispatchable command (even if on parsing it has a slightly different behavior as it is a marker for a stage beginning)

FROM is not allowed in BuildFromConfig. Only commands that change image config are allowed there. Basically, it is not building anything, it is just converting parameters in dockerfile syntax to a json config syntax.

moby/builder/dockerfile/builder.go

Lines 25 to 36 in 274cc09

var validCommitCommands = map[string]bool{

"cmd": true,

"entrypoint": true,

"healthcheck": true,

"env": true,

"expose": true,

"label": true,

"onbuild": true,

"user": true,

"volume": true,

"workdir": true,

}

Yes, I understand, my point is that a stageBuilder can be initialized by either a FROM command or a ResumeBuild command created when doing a BuildFromConfig. I like to leverage the typed-command style dispatching to change this alternative behavior.
Some other things to consider:

From is not only about initializing the stage: At dispatch time, it might trigger onbuild expression dispatching

From supports buildargs expansion and cannot be entirely resolved at parse time

From can reference another stage, in this case it is not resolvable at parse time either, as it needs to get the runconfig produced by the referenced stage dispatching

I'd like to keep the degree of consistency I have now: 1 dockerfile instruction == 1 typed command (even meta args are typed command, they just don't belong to a specific stage)

In term of consistency, I really like the way it is, as the builder don't need to know anything about stage initialization, it only execute one list of commands per stage. The 1st command accidently initialize the stage runConfig, but the builder itself does not even care.

tonistiigi · 2017-06-07T21:41:56Z

cc @AkihiroSuda as well as this is related to #32550 (comment)

AkihiroSuda · 2017-06-08T06:58:02Z

builder/dockerfile/builder.go

+			select {
+			case <-b.clientCtx.Done():
+				logrus.Debug("Builder: build cancelled!")
+				fmt.Fprint(b.Stdout, "Build cancelled")


\n missing?

AkihiroSuda · 2017-06-08T07:02:56Z

builder/dockerfile/instructions/commands.go

+
+type BuildableStage struct {
+	Name     string
+	Commands []interface{}


interface{} -> BaseCommand?

There is one specific command that does not embed CommandBase as it is not parsed from source code (ResumeBuild, used in BuildFromConfig). This also opens up some cool features like in the future, being able to split typed commands into more fine grained subcommands to increase parallelism opportunities. (for exemple, having the AddCommand split into many fetch subcommands -1 per source- and join them in a mergeInto command)

AkihiroSuda · 2017-06-08T07:08:16Z

builder/dockerfile/instructions/parse.go

+
+	labels := KeyValuePairs{}
+
+	for j := 0; j < len(req.args); j++ {


Can we fold two j++ into a single j+=2? (as in parseEnv)

AkihiroSuda · 2017-06-08T07:22:10Z

builder/dockerfile/builder.go

-		if n.Value == command.From && state.isCurrentStage(b.options.Target) {
-			break
-		}
+			fmt.Fprintf(b.Stdout, "%v / %v %v", currentCommandIndex, totalCommands, cmd)


"Step %d/%d : %v" for compatibility?

I think there are user applications that depend on the current logging format.

(should be converted to machine-readable format in the new RPC)

There is actually a failing test for that that I am going to fix

simonferquel · 2017-06-08T11:44:30Z

Fixed in last commit:

Unknown instruction is a typed error
MetaArgs processing in a function
Shlex created once out of the meta-args loop
\n in Build canceled message
--from in COPY command is not expanded
reverted to the old logic of build stage indexing
parseLabel has a closer logic to parseEnv

simonferquel · 2017-06-08T14:14:29Z

Last push:
Various integration tests fixes + golint

simonferquel · 2017-06-09T12:33:17Z

All unit tests are now back (and due to typedCommands, it is easier to isolate parsing and dispatching tests).
Note, we might want to write more instructions parsing tests, or implement more end to end parse/dispatch tests like the ones in evaluator_tests

dnephin

This is looking good.

I think most of my comments are just about removing some duplication in the new instruction types.

If we really want to keep unique types for each. I think we could still do that with type foo bar aliases, instead of repeating the structs and the methods.

dnephin · 2017-06-09T18:49:30Z

builder/dockerfile/instructions/parse_test.go

+
+	for _, command := range commands {
+		ast, err := parser.Parse(strings.NewReader(command))
+		if err != nil {


require.NoError(t, err)

dnephin · 2017-06-09T18:51:26Z

builder/dockerfile/builder.go

+		return err
+	}
+	b.buildArgs.AddArg(meta.Name, meta.Value)
+	b.buildArgs.AddMetaArg(meta.Name, meta.Value)


Can we remove buildArgs from Builder and make it a field on the stage?

I think that would allow us to move this method to the build stage

Hmm I like the idea (and it is required for build parallelisation). We still need a common buildArgs instance for metaArgs construction, but once it is done, we should simply clone it once per stage, and have individual stage have their own instance.

dnephin · 2017-06-09T18:53:21Z

builder/dockerfile/builder.go

-}
+	// dispatchState := newDispatchState()
+	// dispatchState.runConfig = config
+	// return dispatchFromDockerfile(b, dockerfile, dispatchState)


commented out code (please remove)

dnephin · 2017-06-09T18:55:37Z

builder/dockerfile/builder.go

+		if err != nil {
+			if instructions.IsUnknownInstruction(err) {
+				buildsFailed.WithValues(metricsUnknownInstructionError).Inc()
+			}


It seems like it would be easier to export buildsFailed. That way we could update the metrics from the source instead of having to check a bunch of error conditions. I think this is the forth check so far (3 for unknown instruction, another for target not reachable)

Hmm I do not agree (and that is actually the opposite of @tonistiigi feedback on the same subject before). I'd like to keep the node -> typed command parsing logic the least dependent on anything else as possible.

It might have been a mistake to log metrics for BuildFromConfig as it is just wrongly named function that doesn't have anything to do with docker build. There should be only one place then. Also, maybe there is no need to check for the error type at all as the point should be to log metrics for all errors. The selection of errors that are logged separately and what are not is completely arbitrary anyway.

the metrics name is explitily "unknown_instruction_error" should we add a "general_parsing_error" metric then for other parsing errors?

dnephin · 2017-06-09T18:57:15Z

builder/dockerfile/dispatchers.go

-	}
-
-	runConfig := req.state.runConfig
+func (d *stageBuilder) dispatchEnv(c *instructions.EnvCommand) error {


I don't think these should be methods on a struct. They don't really make sense grouped together, they are unrelated functions that happen to take similar dependencies.

I think this worked better as functions which accept some request or options type struct.

We recently made a similar change to the Cli (moving away from a massive struct with methods, to individual functions which accepted a common type).

I come from a very object oriented background where we tend to group state and methods together in a coherent stuff and avoid free functions, so I am not sure I 100% agree, but for consistency sake, I'll do that.

dnephin · 2017-06-09T19:34:33Z

builder/dockerfile/instructions/commands.go

+	BaseCommand
+	Cmd          strslice.StrSlice
+	PrependShell bool
+}


Cmd/RunEntrypoint are all the same struct , same idea here.

dnephin · 2017-06-09T19:39:14Z

builder/dockerfile/instructions/parse.go

+	}, nil
+
+	//TODO
+	//req.buildArgs.ResetAllowed()


commented out code to remove

dnephin · 2017-06-09T19:40:14Z

builder/dockerfile/instructions/parse.go

+
+}
+
+func parseCmd(req parseRequest) (*CmdCommand, error) {


Duplicate of parseRun() which would go away with the suggested struct changes for instructions.

I want to keep multiple functions (for maintainability sake), but I will try to share some internal code here

How does more code make it easier to maintain?

My point is if we add flags to cmd in the future that are not in run, it would be easier if the code is already split (even if in its implementation it calls a single shared function with run and entrypoint). It is also more explicit: if I'm looking to change run parsing behavior, I know that I can modify parseRun without any side effect on other command parsing logic

dnephin · 2017-06-09T19:41:54Z

builder/dockerfile/instructions/parse.go

+	var value *string
+	if hasDefault {
+		value = &newValue
+	}


I think this if is unnecessary. Why not just set it in the branch above?

dnephin · 2017-06-09T19:43:26Z

builder/dockerfile/instructions/parse.go

+}
+
+// IsUnknownInstruction checks if the error is an UnknownInstruction or a parseError containing an UnknownInstruction
+func IsUnknownInstruction(err error) bool {


Sorry, I know it was my suggestion to make this a type. I should have looked at the code some more. I think exporting the metrics function and calling it directly where the error is raised would be cleaner.

dnephin · 2017-06-12T19:42:48Z

builder/dockerfile/dispatchers.go

-		return nil
+	state := d.state
+	state.beginStage(cmd.StageName, image)
+	if state.runConfig.OnBuild != nil && len(state.runConfig.OnBuild) > 0 {


This first nil check is unnecessary, you can just check length (https://play.golang.org/p/E0QPccyYAU)

dnephin · 2017-06-12T19:43:32Z

builder/dockerfile/dispatchers.go

+		err = dispatchTriggeredOnBuild(d, triggers)
+		if err != nil {
+			return err
+		}


This err condition is unnecessary. You can just

return dispatchTriggeredOnBuild(d, triggers)

dnephin · 2017-06-12T19:48:31Z

builder/dockerfile/evaluator_test.go

-		Stdout:    ioutil.Discard,
-		buildArgs: newBuildArgs(options.BuildArgs),
+	if err != nil {
+		testutil.ErrorContains(t, err, testCase.expectedError)


I think, instead of the comments in the test cases above, it would be good to split this into two test cases. One for parse errors, and another for dispatch errors.

Aggreed. I should move parsing error related tests to the instructions package as well, and change evaluator_tests to not rely on parsing, but on already constructed typed errors.

dnephin · 2017-06-12T19:49:22Z

builder/dockerfile/instructions/commands.go

+type KeyValuePairs []KeyValuePair
+
+// WithNameAndCode is the base class for Dockerfile (String() returns its source code)
+type WithNameAndCode struct {


It seems this doesn't need to be exported.

dnephin · 2017-06-12T19:53:27Z

builder/dockerfile/instructions/commands.go

+type LabelCommand struct {
+	BaseCommand
+	Labels KeyValuePairs // kvp slice instead of map to preserve ordering
+}


I don't agree: the whole point of these PR is to have typed commands.

We can still have strict types without the code duplication. As discussed in slack I think we can do this with embedded structs. The field name can just be Value and it doesn't weaken the type guarantees at all.

what if we want to add a flag on ENV for exemple that has no meaning at all on Label at some point in the future ?

Then at that time we can create a new struct. There's no reason to maintain extra code in the mean time for a case that may never occur.

dnephin · 2017-06-12T21:38:27Z

builder/dockerfile/instructions/parse.go

+
+}
+
+func parseCmd(req parseRequest) (*CmdCommand, error) {


How does more code make it easier to maintain?

dnephin · 2017-06-12T21:42:29Z

builder/dockerfile/evaluator.go

+	shlex   *ShellLex
+	builder *Builder
+	source  builder.Source
+}


Could this be renamed back to dispatchRequest (and with the variable name in the dispatch functions back to req). I think that will help focus the diff on the changes related to the new typed instructions.

Ok, I found the "stageBuild" name more explicit (it represents the whole state and dependencies required to build a stage), but I agree it would simplify the diff. Might be worth adding a Naming PR after that though

dnephin · 2017-06-12T21:43:08Z

builder/dockerfile/evaluator.go

 }

+func newStageBuild(builder *Builder, escapeToken rune, source builder.Source, buildArgs *buildArgs) *stageBuild {


Minor nit. The definition for dispatchState and StageBuild are a bit tangled here. Could you move this above newDispatchState(), so their "constructors" are together with the struct.

simonferquel · 2017-07-18T07:13:51Z

@tonistiigi, @dnephin have you any more feedback or should we make this PR advance one more step toward merging ?

shouze · 2017-08-04T08:52:31Z

Yup, would be nice to see this one merged

AkihiroSuda · 2017-08-08T06:57:55Z

@tonistiigi Is this ready to merge?

simonferquel · 2017-08-29T07:29:54Z

Yet another rebase for the chown flag on add/copy. Let us see how it behaves in CI

tonistiigi

LGTM

Follow ups could be in areas of moving more argument expansion to the instructions pkg and generalizing the build args. These seem areas where a different dispatcher still needs to duplicate some logic.

ping @dnephin

tonistiigi · 2017-08-30T23:23:08Z

builder/dockerfile/evaluator.go

 	"runtime"
 	"strings"

+	"reflect"
+


nit: extra whitespace

tonistiigi · 2017-08-30T23:26:59Z

builder/dockerfile/instructions/commands.go

+
+// Sources list the source paths
+func (s SourcesAndDest) Sources() []string {
+	return s[:len(s)-1]


This should make a copy to avoid caller doing an append leaking to destination variable.

yongtang · 2017-09-06T14:27:27Z

@simonferquel Can you squash the PR commits? The lint/rebase fixes might be too verbose to show up in the git history.

This is a work base to introduce more features like build time dockerfile optimisations, dependency analysis and parallel build, as well as a first step to go from a dispatch-inline process to a frontend+backend process. Signed-off-by: Simon Ferquel <simon.ferquel@docker.com>

AkihiroSuda

LGTM

GordonTheTurtle added the status/0-triage label Jun 2, 2017

tonistiigi added the status/failing-ci Indicates that the PR in its current state fails the test suite label Jun 2, 2017

vdemeester requested review from dnephin and tonistiigi June 2, 2017 20:08

vdemeester added status/1-design-review and removed status/0-triage labels Jun 2, 2017

dnephin reviewed Jun 7, 2017

View reviewed changes

tonistiigi reviewed Jun 7, 2017

View reviewed changes

AkihiroSuda reviewed Jun 8, 2017

View reviewed changes

simonferquel force-pushed the typed-builder-commands-squashed branch 2 times, most recently from 2f7c5d5 to 7349187 Compare June 8, 2017 12:03

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

GordonTheTurtle removed the rebuild/windowsRS1 label Jun 9, 2017

simonferquel force-pushed the typed-builder-commands-squashed branch 3 times, most recently from cca3a45 to 3077710 Compare June 9, 2017 13:43

dnephin reviewed Jun 9, 2017

View reviewed changes

simonferquel force-pushed the typed-builder-commands-squashed branch from c9300ca to aee5129 Compare June 12, 2017 12:24

dnephin reviewed Jun 12, 2017

View reviewed changes

GordonTheTurtle assigned runcom Jun 17, 2017

tonistiigi mentioned this pull request Jun 23, 2017

Dockerfile integration moby/buildkit#29

Closed

simonferquel force-pushed the typed-builder-commands-squashed branch from 3ded678 to 3689774 Compare June 30, 2017 09:45

shouze mentioned this pull request Jul 17, 2017

builder: Proposal: Add support for RUN --mount #32507

Closed

tonistiigi assigned tonistiigi and dnephin and unassigned runcom Jul 18, 2017

simonferquel force-pushed the typed-builder-commands-squashed branch from 3689774 to 9dbf14f Compare August 4, 2017 08:47

simonferquel force-pushed the typed-builder-commands-squashed branch from 9dbf14f to 3490d7f Compare August 28, 2017 12:11

simonferquel requested a review from vdemeester as a code owner August 28, 2017 12:11

simonferquel force-pushed the typed-builder-commands-squashed branch from 7c7cdde to f777173 Compare August 28, 2017 14:18

tonistiigi mentioned this pull request Aug 28, 2017

base of dockerfile frontend moby/buildkit#106

Merged

simonferquel force-pushed the typed-builder-commands-squashed branch from f777173 to 5cb972d Compare August 29, 2017 07:29

tonistiigi approved these changes Aug 30, 2017

View reviewed changes

simonferquel force-pushed the typed-builder-commands-squashed branch from 6f723db to e260cfc Compare September 5, 2017 07:52

simonferquel force-pushed the typed-builder-commands-squashed branch 2 times, most recently from 52588c9 to c2bea33 Compare September 7, 2017 14:28

yongtang added rebuild/powerpc labels Sep 7, 2017

GordonTheTurtle removed rebuild/powerpc labels Sep 7, 2017

simonferquel force-pushed the typed-builder-commands-squashed branch 2 times, most recently from 06b7f3d to 861c696 Compare September 14, 2017 13:08

simonferquel force-pushed the typed-builder-commands-squashed branch from 861c696 to 669c067 Compare September 18, 2017 07:49

AkihiroSuda approved these changes Sep 19, 2017

View reviewed changes

AkihiroSuda merged commit 22e1572 into moby:master Sep 19, 2017

		if err != nil {
		if strings.Index(err.Error(), "unknown instruction:") == 0 {

	var validCommitCommands = map[string]bool{
	"cmd": true,
	"entrypoint": true,
	"healthcheck": true,
	"env": true,
	"expose": true,
	"label": true,
	"onbuild": true,
	"user": true,
	"volume": true,
	"workdir": true,
	}


		labels := KeyValuePairs{}

		for j := 0; j < len(req.args); j++ {

		}

		func newStageBuild(builder Builder, escapeToken rune, source builder.Source, buildArgs buildArgs) *stageBuild {

[Builder] Introduce a typed command system and 2 phase parse/dispatch build #33492

[Builder] Introduce a typed command system and 2 phase parse/dispatch build #33492

Conversation

simonferquel commented Jun 2, 2017 • edited Loading

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

tonistiigi commented Jun 7, 2017

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

AkihiroSuda Jun 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonferquel commented Jun 8, 2017

simonferquel commented Jun 8, 2017

simonferquel commented Jun 9, 2017

dnephin 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

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

Choose a reason for hiding this comment

simonferquel commented Jul 18, 2017

shouze commented Aug 4, 2017

AkihiroSuda commented Aug 8, 2017

simonferquel commented Aug 29, 2017

tonistiigi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yongtang commented Sep 6, 2017

AkihiroSuda left a comment

Choose a reason for hiding this comment

simonferquel commented Jun 2, 2017 •

edited

Loading

AkihiroSuda Jun 8, 2017 •

edited

Loading