New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows: Case-insensitive filename matching against builder cache #35793

Merged
merged 1 commit into from Dec 15, 2017

Conversation

Projects
None yet
6 participants
@jhowardmsft
Contributor

jhowardmsft commented Dec 14, 2017

Signed-off-by: John Howard jhoward@microsoft.com

This fixes #33107 where if a local directory is in uppercase and a docker build is run, then a file in that directory is altered and another build is run, it uses the cache rather than taking the update. This is because the comparison was being done exactly, where-as on Windows, c:\app and c:\APP are the same on NTFS. See #33107 (comment)

Given this build context (note the App directory has an upper-case letter in it):

PS E:\docker\build\33107> dir -r


    Directory: E:\docker\build\33107


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       12/13/2017   3:46 PM                App
-a----       11/28/2017   3:16 PM            339 dockerfile


    Directory: E:\docker\build\33107\App


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       12/13/2017  12:30 PM                UPPER
-a----       11/28/2017   2:48 PM              6 addedfile.aspx.cs
-a----       11/28/2017   3:13 PM              6 new1
-a----       11/28/2017   2:47 PM              6 somefile.aspx.cs
-a----       11/28/2017   3:28 PM            284 tEsT.txt

And this dockerfile content:

# escape=`
FROM microsoft/windowsservercore
RUN powershell md c:\app
COPY app c:\app

Here's the output from docker master 1cea9d3 12/13/2017 showing the bug:

First build, forcing no-cache.

PS E:\docker\build\33107> docker build --no-cache .
Sending build context to Docker daemon  7.168kB
Step 1/3 : FROM microsoft/windowsservercore
 ---> 511077485562
Step 2/3 : RUN powershell md c:\app
 ---> Running in ed9b684e05e0


    Directory: C:\


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       12/13/2017   4:35 PM                app


Removing intermediate container ed9b684e05e0
 ---> 3644daaaa31f
Step 3/3 : COPY app c:\app
 ---> 9250b3aad8cd
Successfully built 9250b3aad8cd
PS E:\docker\build\33107>

Update the contents of app\test.txt

PS E:\docker\build\33107> type .\App\test.txt
 FROM microsoft/windowsservercore
RUN dism /online /enable-feature /all /featurename:iis-webserver /NoRestart
RUNEXPOSE 8081
RUN powershell Remove-Website -Name 'Default Web Site'
RUN powershell md c:\
RUN powershell New-Website -Name 'app' -Port 8081 -PhysicalPath 'c:\app'

changed asjdfklasjdf
PS E:\docker\build\33107> echo "foobar" > .\App\test.txt
PS E:\docker\build\33107>

And build again, you see the bug - step 3 is using the cache when it shouldn't.

PS E:\docker\build\33107> docker build .
Sending build context to Docker daemon  7.168kB
Step 1/3 : FROM microsoft/windowsservercore
 ---> 511077485562
Step 2/3 : RUN powershell md c:\app
 ---> Using cache
 ---> 3644daaaa31f
Step 3/3 : COPY app c:\app
 ---> Using cache
 ---> 9250b3aad8cd
Successfully built 9250b3aad8cd
PS E:\docker\build\33107>

With this fix, you get the following instead. First, build forcing a cache-miss:

PS E:\docker\build\33107> docker build --no-cache .
Sending build context to Docker daemon  7.168kB
Step 1/3 : FROM microsoft/windowsservercore
 ---> 511077485562
Step 2/3 : RUN powershell md c:\app
 ---> Running in d504a5d35157


    Directory: C:\


Mode                LastWriteTime         Length Name
----                -------------         ------ ----
d-----       12/13/2017   4:39 PM                app


Removing intermediate container d504a5d35157
 ---> 2d0ba8e32f64
Step 3/3 : COPY app c:\app
 ---> e7287a3c2d5f
Successfully built e7287a3c2d5f
PS E:\docker\build\33107>

Next, update app\test.txt again

PS E:\docker\build\33107> type .\App\test.txt
foobar
PS E:\docker\build\33107> echo "fixed" > .\App\test.txt
PS E:\docker\build\33107>

And then a build showing that that cache is correctly broken through the changed file in the build context:

PS E:\docker\build\33107> docker build .
Sending build context to Docker daemon  7.168kB
Step 1/3 : FROM microsoft/windowsservercore
 ---> 511077485562
Step 2/3 : RUN powershell md c:\app
 ---> Using cache
 ---> 2d0ba8e32f64
Step 3/3 : COPY app c:\app
 ---> 1996084f86f8
Successfully built 1996084f86f8
PS E:\docker\build\33107>
@jhowardmsft

This comment has been minimized.

Show comment
Hide comment
@jhowardmsft
Contributor

jhowardmsft commented Dec 14, 2017

@johnstep FYI

@jhowardmsft jhowardmsft changed the title from [WORK IN PROGRESS] Windows: Case-insensitive filename matching against builder cache to Windows: Case-insensitive filename matching against builder cache Dec 14, 2017

Show outdated Hide outdated builder/builder.go

@thaJeztah thaJeztah added this to backlog in maintainers-session Dec 14, 2017

@thaJeztah thaJeztah removed this from backlog in maintainers-session Dec 14, 2017

Windows: Case-insensitive filename matching against builder cache
Signed-off-by: John Howard <jhoward@microsoft.com>
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Dec 15, 2017

Member

Ugh, this one is flaky again

22:27:52 ----------------------------------------------------------------------
22:27:52 PANIC: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
22:27:52 
22:27:52 ... Panic: runtime error: index out of range (PC=0x45AC01)
22:27:52 
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:509
22:27:52   in call32
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/panic.go:491
22:27:52   in gopanic
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/panic.go:28
22:27:52   in panicindex
22:27:52 docker_api_logs_test.go:175
22:27:52   in DockerSuite.TestLogsAPIUntil
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:509
22:27:52   in call32
22:27:52 d:/CI/CI-7caa30e89/go/src/reflect/value.go:434
22:27:52   in Value.call
22:27:52 d:/CI/CI-7caa30e89/go/src/reflect/value.go:302
22:27:52   in Value.Call
22:27:52 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:816
22:27:52   in suiteRunner.forkTest.func1
22:27:52 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:672
22:27:52   in suiteRunner.forkCall.func1
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:2337
22:27:52   in goexit
22:27:54 
22:27:54 ----------------------------------------------------------------------
Member

thaJeztah commented Dec 15, 2017

Ugh, this one is flaky again

22:27:52 ----------------------------------------------------------------------
22:27:52 PANIC: docker_api_logs_test.go:152: DockerSuite.TestLogsAPIUntil
22:27:52 
22:27:52 ... Panic: runtime error: index out of range (PC=0x45AC01)
22:27:52 
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:509
22:27:52   in call32
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/panic.go:491
22:27:52   in gopanic
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/panic.go:28
22:27:52   in panicindex
22:27:52 docker_api_logs_test.go:175
22:27:52   in DockerSuite.TestLogsAPIUntil
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:509
22:27:52   in call32
22:27:52 d:/CI/CI-7caa30e89/go/src/reflect/value.go:434
22:27:52   in Value.call
22:27:52 d:/CI/CI-7caa30e89/go/src/reflect/value.go:302
22:27:52   in Value.Call
22:27:52 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:816
22:27:52   in suiteRunner.forkTest.func1
22:27:52 c:/gopath/src/github.com/docker/docker/vendor/github.com/go-check/check/check.go:672
22:27:52   in suiteRunner.forkCall.func1
22:27:52 d:/CI/CI-7caa30e89/go/src/runtime/asm_amd64.s:2337
22:27:52   in goexit
22:27:54 
22:27:54 ----------------------------------------------------------------------
@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Dec 15, 2017

Member

LGTM

Member

tonistiigi commented Dec 15, 2017

LGTM

@johnstep

LGTM

@thaJeztah thaJeztah merged commit a59061d into moby:master Dec 15, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 38412 has succeeded
Details
janky Jenkins build Docker-PRs 47139 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 7522 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 18665 has succeeded
Details
z Jenkins build Docker-PRs-s390x 7346 has succeeded
Details

@jhowardmsft jhowardmsft deleted the Microsoft:33107 branch Jan 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment