Merge pull request #9122 from dqminh/debug-huge-expose

Expose a large number of ports should not slow down builder

builder/dispatchers.go

@@ -12,6 +12,7 @@ import (
 	"io/ioutil"
 	"path/filepath"
 	"regexp"
+	"sort"
 	"strings"
 
 	log "github.com/Sirupsen/logrus"
@@ -326,14 +327,21 @@ func expose(b *Builder, args []string, attributes map[string]bool, original stri
 		return err
 	}
 
+	// instead of using ports directly, we build a list of ports and sort it so
+	// the order is consistent. This prevents cache burst where map ordering
+	// changes between builds
+	portList := make([]string, len(ports))
+	var i int
 	for port := range ports {
 		if _, exists := b.Config.ExposedPorts[port]; !exists {
 			b.Config.ExposedPorts[port] = struct{}{}
 		}
+		portList[i] = string(port)
+		i++
 	}
+	sort.Strings(portList)
 	b.Config.PortSpecs = nil
-
-	return b.commit("", b.Config.Cmd, fmt.Sprintf("EXPOSE %v", ports))
+	return b.commit("", b.Config.Cmd, fmt.Sprintf("EXPOSE %s", strings.Join(portList, " ")))
 }
 
 // USER foo

builder/evaluator.go

@@ -212,6 +212,21 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error {
 		msg += " " + ast.Value
 	}
 
+	// count the number of nodes that we are going to traverse first
+	// so we can pre-create the argument and message array. This speeds up the
+	// allocation of those list a lot when they have a lot of arguments
+	cursor := ast
+	var n int
+	for cursor.Next != nil {
+		cursor = cursor.Next
+		n++
+	}
+	l := len(strs)
+	strList := make([]string, n+l)
+	copy(strList, strs)
+	msgList := make([]string, n)
+
+	var i int
 	for ast.Next != nil {
 		ast = ast.Next
 		var str string
@@ -219,16 +234,18 @@ func (b *Builder) dispatch(stepN int, ast *parser.Node) error {
 		if _, ok := replaceEnvAllowed[cmd]; ok {
 			str = b.replaceEnv(ast.Value)
 		}
-		strs = append(strs, str)
-		msg += " " + ast.Value
+		strList[i+l] = str
+		msgList[i] = ast.Value
+		i++
 	}
 
+	msg += " " + strings.Join(msgList, " ")
 	fmt.Fprintln(b.OutStream, msg)
 
 	// 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(b, strs, attrs, original)
+		return f(b, strList, attrs, original)
 	}
 
 	fmt.Fprintf(b.ErrStream, "# Skipping unknown instruction %s\n", strings.ToUpper(cmd))

daemon/daemon.go

@@ -1099,9 +1099,9 @@ func (daemon *Daemon) ImageGetCached(imgID string, config *runconfig.Config) (*i
 	// Loop on the children of the given image and check the config
 	var match *image.Image
 	for elem := range imageMap[imgID] {
-		img, err := daemon.Graph().Get(elem)
-		if err != nil {
-			return nil, err
+		img, ok := images[elem]
+		if !ok {
+			return nil, fmt.Errorf("unable to find image %q", elem)
 		}
 		if runconfig.Compare(&img.ContainerConfig, config) {
 			if match == nil || match.Created.Before(img.Created) {

integration-cli/docker_cli_build_test.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"archive/tar"
+	"bytes"
 	"encoding/json"
 	"fmt"
 	"io/ioutil"
@@ -11,9 +12,11 @@ import (
 	"path/filepath"
 	"reflect"
 	"regexp"
+	"strconv"
 	"strings"
 	"syscall"
 	"testing"
+	"text/template"
 	"time"
 
 	"github.com/docker/docker/pkg/archive"
@@ -1942,6 +1945,85 @@ func TestBuildExpose(t *testing.T) {
 	logDone("build - expose")
 }
 
+func TestBuildExposeMorePorts(t *testing.T) {
+	// start building docker file with a large number of ports
+	portList := make([]string, 50)
+	line := make([]string, 100)
+	expectedPorts := make([]int, len(portList)*len(line))
+	for i := 0; i < len(portList); i++ {
+		for j := 0; j < len(line); j++ {
+			p := i*len(line) + j + 1
+			line[j] = strconv.Itoa(p)
+			expectedPorts[p-1] = p
+		}
+		if i == len(portList)-1 {
+			portList[i] = strings.Join(line, " ")
+		} else {
+			portList[i] = strings.Join(line, " ") + ` \`
+		}
+	}
+
+	dockerfile := `FROM scratch
+	EXPOSE {{range .}} {{.}}
+	{{end}}`
+	tmpl := template.Must(template.New("dockerfile").Parse(dockerfile))
+	buf := bytes.NewBuffer(nil)
+	tmpl.Execute(buf, portList)
+
+	name := "testbuildexpose"
+	defer deleteImages(name)
+	_, err := buildImage(name, buf.String(), true)
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	// check if all the ports are saved inside Config.ExposedPorts
+	res, err := inspectFieldJSON(name, "Config.ExposedPorts")
+	if err != nil {
+		t.Fatal(err)
+	}
+	var exposedPorts map[string]interface{}
+	if err := json.Unmarshal([]byte(res), &exposedPorts); err != nil {
+		t.Fatal(err)
+	}
+
+	for _, p := range expectedPorts {
+		ep := fmt.Sprintf("%d/tcp", p)
+		if _, ok := exposedPorts[ep]; !ok {
+			t.Errorf("Port(%s) is not exposed", ep)
+		} else {
+			delete(exposedPorts, ep)
+		}
+	}
+	if len(exposedPorts) != 0 {
+		t.Errorf("Unexpected extra exposed ports %v", exposedPorts)
+	}
+	logDone("build - expose large number of ports")
+}
+
+func TestBuildExposeOrder(t *testing.T) {
+	buildID := func(name, exposed string) string {
+		_, err := buildImage(name, fmt.Sprintf(`FROM scratch
+		EXPOSE %s`, exposed), true)
+		if err != nil {
+			t.Fatal(err)
+		}
+		id, err := inspectField(name, "Id")
+		if err != nil {
+			t.Fatal(err)
+		}
+		return id
+	}
+
+	id1 := buildID("testbuildexpose1", "80 2375")
+	id2 := buildID("testbuildexpose2", "2375 80")
+	defer deleteImages("testbuildexpose1", "testbuildexpose2")
+	if id1 != id2 {
+		t.Errorf("EXPOSE should invalidate the cache only when ports actually changed")
+	}
+	logDone("build - expose order")
+}
+
 func TestBuildEmptyEntrypointInheritance(t *testing.T) {
 	name := "testbuildentrypointinheritance"
 	name2 := "testbuildentrypointinheritance2"