From 811341423bd9d3d65640cb4aa04d2840817182ca Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Mon, 6 Jan 2014 16:25:48 -0800 Subject: [PATCH 1/3] Fix ADD caching issue with . prefixed path Docker-DCO-1.0-Signed-off-by: Guillaume J. Charmes (github: creack) --- buildfile.go | 7 ++++ integration/buildfile_test.go | 71 ++++++++++++++++++++++++++++++----- 2 files changed, 68 insertions(+), 10 deletions(-) diff --git a/buildfile.go b/buildfile.go index 09b1f80739..8e92702ae0 100644 --- a/buildfile.go +++ b/buildfile.go @@ -412,6 +412,13 @@ func (b *buildFile) CmdAdd(args string) error { } else if fi.IsDir() { var subfiles []string for file, sum := range sums { + // Has tarsum stips the '.' and './', we put it back for comparaison. + if len(file) == 0 { + file = "./" + } + if file[0] != '.' && file[0] != '/' { + file = "./" + file + } if strings.HasPrefix(file, origPath) { subfiles = append(subfiles, sum) } diff --git a/integration/buildfile_test.go b/integration/buildfile_test.go index 89878926ed..5f3991636d 100644 --- a/integration/buildfile_test.go +++ b/integration/buildfile_test.go @@ -427,7 +427,7 @@ func TestBuildEntrypointRunCleanup(t *testing.T) { } } -func checkCacheBehavior(t *testing.T, template testContextTemplate, expectHit bool) { +func checkCacheBehavior(t *testing.T, template testContextTemplate, expectHit bool) (imageId string) { eng := NewTestEngine(t) defer nuke(mkRuntimeFromEngine(eng, t)) @@ -436,20 +436,36 @@ func checkCacheBehavior(t *testing.T, template testContextTemplate, expectHit bo t.Fatal(err) } - imageId := img.ID + imageId = img.ID - img = nil img, err = buildImage(template, t, eng, expectHit) if err != nil { t.Fatal(err) } - hit := imageId == img.ID - if hit != expectHit { - t.Logf("Cache misbehavior, got hit=%t, expected hit=%t: (first: %s, second %s)", - hit, expectHit, imageId, img.ID) - t.Fail() + if hit := imageId == img.ID; hit != expectHit { + t.Fatalf("Cache misbehavior, got hit=%t, expected hit=%t: (first: %s, second %s)", hit, expectHit, imageId, img.ID) } + return +} + +func checkCacheBehaviorFromEngime(t *testing.T, template testContextTemplate, expectHit bool, eng *engine.Engine) (imageId string) { + img, err := buildImage(template, t, eng, true) + if err != nil { + t.Fatal(err) + } + + imageId = img.ID + + img, err = buildImage(template, t, eng, expectHit) + if err != nil { + t.Fatal(err) + } + + if hit := imageId == img.ID; hit != expectHit { + t.Fatalf("Cache misbehavior, got hit=%t, expected hit=%t: (first: %s, second %s)", hit, expectHit, imageId, img.ID) + } + return } func TestBuildImageWithCache(t *testing.T) { @@ -476,11 +492,46 @@ func TestBuildADDLocalFileWithCache(t *testing.T) { maintainer dockerio run echo "first" add foo /usr/lib/bla/bar + run [ "$(cat /usr/lib/bla/bar)" = "hello" ] run echo "second" + add . /src/ + run [ "$(cat /src/foo)" = "hello" ] `, - [][2]string{{"foo", "hello"}}, + [][2]string{ + {"foo", "hello"}, + }, nil} - checkCacheBehavior(t, template, true) + eng := NewTestEngine(t) + defer nuke(mkRuntimeFromEngine(eng, t)) + + id1 := checkCacheBehaviorFromEngime(t, template, true, eng) + template.files = append(template.files, [2]string{"bar", "hello2"}) + id2 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id1 == id2 { + t.Fatal("The cache should have been invalided but hasn't.") + } + id3 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id2 != id3 { + t.Fatal("The cache should have been used but hasn't.") + } + template.files[1][1] = "hello3" + id4 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id3 == id4 { + t.Fatal("The cache should have been invalided but hasn't.") + } + template.dockerfile += ` + add ./bar /src2/ + run ls /src2/bar + ` + id5 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id4 == id5 { + t.Fatal("The cache should have been invalided but hasn't.") + } + template.files[1][1] = "hello4" + id6 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id5 == id6 { + t.Fatal("The cache should have been invalided but hasn't.") + } } func TestBuildADDLocalFileWithoutCache(t *testing.T) { From ef7e000a13eed5ebf761ab4ca83fa8f727170aa5 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 7 Jan 2014 16:02:41 -0800 Subject: [PATCH 2/3] Make vfs error more explicit Docker-DCO-1.0-Signed-off-by: Guillaume J. Charmes (github: creack) --- graphdriver/vfs/driver.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/graphdriver/vfs/driver.go b/graphdriver/vfs/driver.go index fab9d06f83..12230f463a 100644 --- a/graphdriver/vfs/driver.go +++ b/graphdriver/vfs/driver.go @@ -36,9 +36,8 @@ func (d *Driver) Cleanup() error { } func copyDir(src, dst string) error { - cmd := exec.Command("cp", "-aT", "--reflink=auto", src, dst) - if err := cmd.Run(); err != nil { - return err + if output, err := exec.Command("cp", "-aT", "--reflink=auto", src, dst).CombinedOutput(); err != nil { + return fmt.Errorf("Error VFS copying directory: %s (%s)", err, output) } return nil } From f3103e5c9157aed2a5ab0f4c70e328cd0aa69b59 Mon Sep 17 00:00:00 2001 From: "Guillaume J. Charmes" Date: Tue, 7 Jan 2014 16:53:55 -0800 Subject: [PATCH 3/3] Fix issue with file caching + prevent wrong cache hit Docker-DCO-1.0-Signed-off-by: Guillaume J. Charmes (github: creack) --- buildfile.go | 19 +++++++++++-------- integration/buildfile_test.go | 15 +++++++++++++++ 2 files changed, 26 insertions(+), 8 deletions(-) diff --git a/buildfile.go b/buildfile.go index 8e92702ae0..ef4b95c064 100644 --- a/buildfile.go +++ b/buildfile.go @@ -407,18 +407,20 @@ func (b *buildFile) CmdAdd(args string) error { hash string sums = b.context.GetSums() ) + + // Has tarsum strips the '.' and './', we put it back for comparaison. + for file, sum := range sums { + if len(file) == 0 || file[0] != '.' && file[0] != '/' { + delete(sums, file) + sums["./"+file] = sum + } + } + if fi, err := os.Stat(path.Join(b.contextPath, origPath)); err != nil { return err } else if fi.IsDir() { var subfiles []string for file, sum := range sums { - // Has tarsum stips the '.' and './', we put it back for comparaison. - if len(file) == 0 { - file = "./" - } - if file[0] != '.' && file[0] != '/' { - file = "./" + file - } if strings.HasPrefix(file, origPath) { subfiles = append(subfiles, sum) } @@ -435,7 +437,8 @@ func (b *buildFile) CmdAdd(args string) error { if err != nil { return err } - if hit { + // If we do not have a hash, never use the cache + if hit && hash != "" { return nil } } diff --git a/integration/buildfile_test.go b/integration/buildfile_test.go index 5f3991636d..efbdc54951 100644 --- a/integration/buildfile_test.go +++ b/integration/buildfile_test.go @@ -532,6 +532,21 @@ func TestBuildADDLocalFileWithCache(t *testing.T) { if id5 == id6 { t.Fatal("The cache should have been invalided but hasn't.") } + + template.dockerfile += ` + add bar /src2/bar2 + add /bar /src2/bar3 + run ls /src2/bar2 /src2/bar3 + ` + id7 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id6 == id7 { + t.Fatal("The cache should have been invalided but hasn't.") + } + template.files[1][1] = "hello5" + id8 := checkCacheBehaviorFromEngime(t, template, true, eng) + if id7 == id8 { + t.Fatal("The cache should have been invalided but hasn't.") + } } func TestBuildADDLocalFileWithoutCache(t *testing.T) {