[WIP] New .dockerignore implementation by pwaller · Pull Request #20128 · moby/moby

pwaller · 2016-02-08T23:22:44Z

This is a followup to #20088, and a project I wrote which shows the context of the docker context.

Headline figure: This reduces the docker build runtime for a largish project of mine by 10x:

real 0m17.472s
user 0m45.672s
sys  0m1.588s

To:

real 0m1.474s
user 0m0.440s
sys  0m0.220s

(That's a 100x CPU speedup!).

Detail: I have 14,000 files across 2GiB, with 18 .dockerignore instructions. It turns out that the old implementation was compiling a regular expression for every 14,000 * 18, an profiling showed it spent the majority of time there.

For a no-op docker build itself, it reduces from:

real 0m2.439s
user 0m3.176s
sys  0m0.320s

To:

real 0m1.339s
user 0m0.412s
sys  0m0.172s

Which is more modest, but only the case because docker's .dockerignore has only three lines in it.

I'm sending this PR for early review now. There is still a bit more work to be done, and a few broader implications to check as well. I could use some help there. Also I note that the logic is implemented by a few other projects (docker-compose interests me in particular) and I guess it would be good to get the logic to be the same there too at some point.

There is still a bit more work to do, including:

Make the tests pass if they don't work (I couldn't get them working last time I tried locally, sorry)
Rip out old code for performing ignores
Seek out regressions and write tests

calavera · 2016-02-08T23:37:22Z

@pwaller it would be awesome if we could have unit tests and benchmarks for this.

duglin · 2016-02-09T00:09:42Z

nice timings!

pwaller · 2016-02-14T14:54:20Z

I'm still working on this, but I've been a busy with other things. I hope to complete it within the next week. Please ping me if it is quiet longer than that. Feedback on the approach welcomed.

Right now it still needs a few more tests writing (best based from the documentation), and one of the tests doesn't pass, so the implementation isn't quite complete yet.

(dco/no is intentional until it is complete).

pwaller · 2016-02-14T15:00:09Z

OK. leaving it alone for now. Last failing test:

**/test.binary

It should ignore anywhere, but right now it only only ignores in directories which aren't the root. So a bit more work required to fix that. But almost there I think!

thaJeztah · 2016-02-15T13:52:03Z

I'll change the PR's title to "WIP" please ping if it's ready for review ❤️

Signed-off-by: Peter Waller <p@pwaller.net>

pwaller · 2016-02-16T20:39:48Z

In light of #20165, I have to move builder/dockerignore to pkg/dockerignore. Does that sound OK?

calavera · 2016-02-16T23:50:02Z

@pwaller I think a better option is to not have the Excluder in the dockerignore package. It looks like a generic enough interface to have in pkg/archive.

I do not want to move the whole dockerignore package out of builder.

pwaller · 2016-02-21T11:54:04Z

I'm having difficulty proceeding development because when I run the unit tests locally, they hang indefinitely immediately after the btrfs test. I've waited more than 30 minutes with no apparent progress. Is that expected? I'm currently trying to run just the tests on v1.10.1 on my Ubuntu Trusty (14.04) machine, without even my changes present and not having any success.

---> Making bundle: test-unit (in bundles/1.11.0-dev/test-unit)
Sun Feb 21 11:41:53 UTC 2016
ok      github.com/docker/docker/api    0.028s  coverage: 92.5% of statements
ok      github.com/docker/docker/api/client 0.025s  coverage: 1.4% of statements
ok      github.com/docker/docker/api/client/formatter   0.035s  coverage: 99.4% of statements
ok      github.com/docker/docker/api/client/inspect 0.034s  coverage: 92.7% of statements
ok      github.com/docker/docker/api/server 0.005s  coverage: 15.6% of statements
ok      github.com/docker/docker/api/server/httputils   0.019s  coverage: 19.5% of statements
?       github.com/docker/docker/api/server/router  [no test files]
?       github.com/docker/docker/api/server/router/build    [no test files]
?       github.com/docker/docker/api/server/router/container    [no test files]
?       github.com/docker/docker/api/server/router/image    [no test files]
?       github.com/docker/docker/api/server/router/network  [no test files]
?       github.com/docker/docker/api/server/router/system   [no test files]
?       github.com/docker/docker/api/server/router/volume   [no test files]
?       github.com/docker/docker/api/types/backend  [no test files]
?       github.com/docker/docker/autogen/winresources   [no test files]
ok      github.com/docker/docker/builder    0.034s  coverage: 8.2% of statements
ok      github.com/docker/docker/builder/dockerfile 0.012s  coverage: 19.5% of statements
?       github.com/docker/docker/builder/dockerfile/command [no test files]
ok      github.com/docker/docker/builder/dockerfile/parser  0.009s  coverage: 80.9% of statements
?       github.com/docker/docker/builder/dockerfile/parser/dumper   [no test files]
ok      github.com/docker/docker/builder/dockerignore   0.032s  coverage: 93.3% of statements
?       github.com/docker/docker/cli    [no test files]
ok      github.com/docker/docker/cliconfig  0.020s  coverage: 91.8% of statements
ok      github.com/docker/docker/container  0.164s  coverage: 11.0% of statements
?       github.com/docker/docker/contrib/apparmor   [no test files]
?       github.com/docker/docker/contrib/docker-device-tool [no test files]
?       github.com/docker/docker/contrib/httpserver [no test files]
ok      github.com/docker/docker/daemon 0.027s  coverage: 7.6% of statements
ok      github.com/docker/docker/daemon/events  0.071s  coverage: 39.1% of statements
?       github.com/docker/docker/daemon/exec    [no test files]
?       github.com/docker/docker/daemon/execdriver  [no test files]
?       github.com/docker/docker/daemon/execdriver/execdrivers  [no test files]
?       github.com/docker/docker/daemon/execdriver/native   [no test files]
?       github.com/docker/docker/daemon/execdriver/native/template  [no test files]
?       github.com/docker/docker/daemon/execdriver/windows  [no test files]
?       github.com/docker/docker/daemon/graphdriver [no test files]
ok      github.com/docker/docker/daemon/graphdriver/aufs    14.393s coverage: 71.1% of statements
ok      github.com/docker/docker/daemon/graphdriver/btrfs   0.010s  coverage: 3.7% of statements

pwaller · 2016-02-21T13:53:08Z

OK, it finally failed after two hours. Any ideas how to fix? (I'm searching, too).

time="2016-02-21T11:51:55Z" level=warning msg="devmapper: Udev sync is not supported. This will lead to unexpected behavior, data loss and errors. For more information, see https://docs.docker.com/reference/commandline/daemon/#daemon-storage-driver-option" 
panic: test timed out after 2h0m0s

goroutine 7 [running]:
testing.startAlarm.func1()
    /usr/local/go/src/testing/testing.go:703 +0x132
created by time.goFunc
    /usr/local/go/src/time/sleep.go:129 +0x3a

goroutine 1 [chan receive]:
testing.RunTests(0x90c310, 0xc491a0, 0x8, 0x8, 0xc820090a01)
    /usr/local/go/src/testing/testing.go:562 +0x8ad
testing.(*M).Run(0xc820053ef8, 0xc64c80)
    /usr/local/go/src/testing/testing.go:494 +0x70
main.main()
    github.com/docker/docker/daemon/graphdriver/devmapper/_test/_testmain.go:120 +0x252

goroutine 17 [syscall, locked to thread]:
runtime.goexit()
    /usr/local/go/src/runtime/asm_amd64.s:1721 +0x1

goroutine 6 [syscall, locked to thread]:
github.com/docker/docker/pkg/devicemapper._C2func_dm_task_run(0xfe45c0, 0x0, 0x0, 0x0)
    ??:0 +0x47
github.com/docker/docker/pkg/devicemapper.dmTaskRunFct(0xfe45c0, 0xc8202cd428)
    /go/src/github.com/docker/docker/pkg/devicemapper/devmapper_wrapper.go:96 +0x21
github.com/docker/docker/pkg/devicemapper.(*Task).run(0xc8200320b0, 0x0, 0x0)
    /go/src/github.com/docker/docker/pkg/devicemapper/devmapper.go:155 +0x37
github.com/docker/docker/pkg/devicemapper.CreatePool(0xc82000b220, 0x15, 0xc820032088, 0xc8200320a8, 0x80, 0x0, 0x0)
    /go/src/github.com/docker/docker/pkg/devicemapper/devmapper.go:461 +0x786
github.com/docker/docker/daemon/graphdriver/devmapper.(*DeviceSet).initDevmapper(0xc820075520, 0xc820075501, 0x0, 0x0)
    github.com/docker/docker/daemon/graphdriver/devmapper/_test/_obj_test/deviceset.go:2177 +0x17b1
github.com/docker/docker/daemon/graphdriver/devmapper.NewDeviceSet(0xc820019650, 0x30, 0x1, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    github.com/docker/docker/daemon/graphdriver/devmapper/_test/_obj_test/deviceset.go:3133 +0x60f
github.com/docker/docker/daemon/graphdriver/devmapper.Init(0xc820019650, 0x30, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    github.com/docker/docker/daemon/graphdriver/devmapper/_test/_obj_test/driver.go:37 +0xe8
github.com/docker/docker/daemon/graphdriver.GetDriver(0x8645a0, 0xc, 0xc820019590, 0x23, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, ...)
    /go/src/github.com/docker/docker/daemon/graphdriver/driver.go:112 +0x210
github.com/docker/docker/daemon/graphdriver/graphtest.newDriver(0xc8200a7830, 0x8645a0, 0xc, 0x8)
    /go/src/github.com/docker/docker/daemon/graphdriver/graphtest/graphtest_unix.go:79 +0x2e8
github.com/docker/docker/daemon/graphdriver/graphtest.GetDriver(0xc8200a7830, 0x8645a0, 0xc, 0x0, 0x0)
    /go/src/github.com/docker/docker/daemon/graphdriver/graphtest/graphtest_unix.go:100 +0x57
github.com/docker/docker/daemon/graphdriver/devmapper.TestDevmapperSetup(0xc8200a7830)
    /go/src/github.com/docker/docker/daemon/graphdriver/devmapper/devmapper_test.go:28 +0x36
testing.tRunner(0xc8200a7830, 0xc491a0)
    /usr/local/go/src/testing/testing.go:456 +0x98
created by testing.RunTests
    /usr/local/go/src/testing/testing.go:561 +0x86d

pwaller · 2016-02-23T18:46:37Z

Ping docker team. What is the best way getting help here? I'm a bit stuck and I don't have many spare cycles left for this at the moment.

thaJeztah · 2016-02-23T21:10:56Z

ping @duglin would you be able to help out?

duglin · 2016-02-23T21:29:33Z

let's see what #20470 does w.r.t. timings first.

duglin · 2016-02-23T22:06:23Z

@pwaller what's in your test .dockerignore file? I'm trying to reproduce it.

pwaller · 2016-02-24T08:44:52Z

Trivial reproducer, no directories, no negations. ~~Easy to see the quadratic behaviour in number of dockerignore directives:~~ (Edit: not quadratic, sorry, just slow performance of the startup).

mkdir -p manyfiles
cd manyfiles
echo $'FROM scratch\nADD . /' > Dockerfile
for f in {1..10000}; do echo $f > f-${f}.txt; done
for f in {1..20}; do echo f-${f}.txt; done > .dockerignore
time docker build -t tmp .

pwaller · 2016-02-24T08:50:44Z

For me with Docker 1.10.1, with nfiles=10k, nignore = 20:

real    0m14.064s
user    0m25.700s
sys     0m0.776s

with nfiles=10k, nignore = 40:

real    0m28.886s
user    0m50.852s
sys     0m1.656s

nfiles=5k, nignore=20

real    0m8.069s
user    0m12.788s
sys     0m0.444s

nfiles=5k, nignore=40

real    0m14.231s
user    0m25.260s
sys     0m0.868s

pwaller · 2016-02-24T08:53:12Z

Forgot to include the obvious nignore=0, nfiles=5k:

real    0m2.399s
user    0m0.340s
sys     0m0.120s

unclejack · 2016-02-24T17:35:57Z

pkg/archive/archive.go


 	"github.com/Sirupsen/logrus"
-	"github.com/docker/docker/pkg/fileutils"
+	"github.com/docker/docker/builder/dockerignore"


This isn't permitted. A package shouldn't depend on the Docker internals.

agreed. This archive package must not depend on docker internals. The Excluder struct should be moved to this package, without the whole dockerignore package.

duglin · 2016-02-26T18:10:19Z

@pwaller been trying to find time to look at this and help but I have a question for ya....
in your investigation did you find that the slowness wasn't so much due to the .dockerignore file processing as much as it was due to the ValidateContextDirectory() function? And you sped things up by having .dockerignore be taken into account during that validation step (so it could skip files) ?

icecrime · 2016-02-29T03:25:15Z

@pwaller Thanks! Have you seen the CI failures? it seems the code adds some unwanted dependency from pkg/ to docker internals:

20:37:34 ---> Making bundle: validate-pkg (in bundles/1.11.0-dev/validate-pkg)
20:37:34 These files import internal code: (either directly or indirectly)
20:37:34  - pkg/archive/archive.go imports github.com/docker/docker/builder/dockerignore

pwaller · 2016-02-29T08:24:54Z

Hi @icecrime, I'm still blocked on not being able to run the tests locally. I'm picking it up every few days, but then still more or less stuck. I'll make another attempt this evening but I'm more or less depleted on cycles for this.

pwaller · 2016-02-29T08:26:40Z

@duglin apologies, I replied by email but it seems my reply was lost.

Basically, ValidateContextDirectory was also a bottleneck, yes. It doubled the timings. However, my original timings (which motivated this work) were just taken using docker-show-context which doesn't include a call to ValidateContextDirectory.

calavera · 2016-03-17T16:44:10Z

Hey @pwaller, I'm going to temporarily close this PR. It still looks like a good idea, but we cannot take it like this. Please, feel free to open a new PR once you have a clear branch with these changes.

My piece of advice to make sure the tests run cleanly is to start a dedicated docker daemon in another location, that way you don't need to worry about what's going on with your main installation while working on this.

I think GitHub doesn't allow to re-open this as soon as I close it, but feel free to ping me here or IRC and I'll reopen it for you when you have the code in a better shape, if you don't want to open a new one.

Thanks a lot for your hard work.

pwaller · 2016-03-18T16:38:40Z

No worries @calavera. Unfortunately I ran myself out of time for this for now. I was able to run the tests but I was hitting a specific problem. Maybe enough time has elapsed / development has happened that now it's no longer a problem. I can see that other things have happened in the meantime, and #21020 is in flight. It looks like my concerns may become addressed without too much further input from me, which is great. I'll try and find the time to feed back some performance numbers there too.

All the best.

pwaller · 2016-09-22T13:21:02Z

Filed #26816

GordonTheTurtle added area/distribution Image Distribution status/0-triage labels Feb 8, 2016

pwaller mentioned this pull request Feb 8, 2016

Memoize compiled regexps in fileutils.regexpMatch #20088

Closed

calavera added status/1-design-review and removed status/0-triage labels Feb 8, 2016

GordonTheTurtle added the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 9, 2016

pwaller force-pushed the new-dockerignore-logic branch from 1667362 to 32cbd92 Compare February 9, 2016 09:15

pwaller force-pushed the new-dockerignore-logic branch from 95c4a1b to 2524e4f Compare February 14, 2016 14:59

thaJeztah changed the title ~~New .dockerignore implementation~~ [WIP] New .dockerignore implementation Feb 15, 2016

pwaller added 2 commits February 16, 2016 20:32

Vendor gobwas/glob

bfe08c4

Signed-off-by: Peter Waller <p@pwaller.net>

Implement new dockerignore logic

278e30f

Signed-off-by: Peter Waller <p@pwaller.net>

pwaller force-pushed the new-dockerignore-logic branch from 2524e4f to 278e30f Compare February 16, 2016 20:36

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Feb 16, 2016

duglin added area/builder Build and removed area/distribution Image Distribution labels Feb 23, 2016

unclejack reviewed Feb 24, 2016
View reviewed changes

icecrime added the status/failing-ci Indicates that the PR in its current state fails the test suite label Feb 29, 2016

calavera mentioned this pull request Mar 2, 2016

Optimize .dockerignore when there are exclusions #20872

Merged

duglin mentioned this pull request Mar 8, 2016

Added recursive .dockerignore capabilities #21020

Closed

calavera closed this Mar 17, 2016

thaJeztah removed the status/failing-ci Indicates that the PR in its current state fails the test suite label May 31, 2016

pwaller mentioned this pull request Sep 22, 2016

"Sending build context to daemon" performance with large projects #26816

Closed

pwaller mentioned this pull request Sep 22, 2016

.dockerignore makes sending context to daemon super slow #23470

Closed

Conversation

pwaller commented Feb 8, 2016

Uh oh!

calavera commented Feb 8, 2016

Uh oh!

duglin commented Feb 9, 2016

Uh oh!

pwaller commented Feb 14, 2016

Uh oh!

pwaller commented Feb 14, 2016

Uh oh!

thaJeztah commented Feb 15, 2016

Uh oh!

pwaller commented Feb 16, 2016

Uh oh!

calavera commented Feb 16, 2016

Uh oh!

pwaller commented Feb 21, 2016

Uh oh!

pwaller commented Feb 21, 2016

Uh oh!

pwaller commented Feb 23, 2016

Uh oh!

thaJeztah commented Feb 23, 2016

Uh oh!

duglin commented Feb 23, 2016

Uh oh!

duglin commented Feb 23, 2016

Uh oh!

pwaller commented Feb 24, 2016

Uh oh!

pwaller commented Feb 24, 2016

Uh oh!

pwaller commented Feb 24, 2016

Uh oh!

unclejack Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

calavera Feb 24, 2016

Choose a reason for hiding this comment

Uh oh!

duglin commented Feb 26, 2016

Uh oh!

icecrime commented Feb 29, 2016

Uh oh!

pwaller commented Feb 29, 2016

Uh oh!

pwaller commented Feb 29, 2016

Uh oh!

calavera commented Mar 17, 2016

Uh oh!

pwaller commented Mar 18, 2016

Uh oh!

pwaller commented Sep 22, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants