Skip to content

Commit

Permalink
Stop caching COPY layers
Browse files Browse the repository at this point in the history 
Cached COPY layers are expensive in that they both need to beretrieved
over the network and occupy space in the layer cache.

They are unnecessary in that we already have all resources needed to
execute the COPY locally, and doing so is a trivial file-system
operation.  This is in contrast to RUN layers, which can do
arbitrary and unbounded work.

The end result is that cached COPY commands were more expensive when
cached, not less.  Remove them.

Resolves GoogleContainerTools#1357
  • Loading branch information
Ian Kerins committed Aug 31, 2020
1 parent f20f495 commit 8cf4065
Show file tree
Hide file tree
Showing 6 changed files with 4 additions and 240 deletions.
2 changes: 1 addition & 1 deletion README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ as a remote image destination:
### Caching

#### Caching Layers
kaniko can cache layers created by `RUN` and `COPY` commands in a remote repository.
kaniko can cache layers created by `RUN` commands in a remote repository.
Before executing a command, kaniko checks the cache for the layer.
If it exists, kaniko will pull and extract the cached layer instead of executing the command.
If not, kaniko will execute the command and then push the newly created layer to the cache.
Expand Down
2 changes: 2 additions & 0 deletions pkg/commands/commands.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ type DockerCommand interface {

RequiresUnpackedFS() bool

// Whether the output layer of this command should be cached in and
// retrieved from the layer cache.
ShouldCacheOutput() bool

// ShouldDetectDeletedFiles returns true if the command could delete files.
Expand Down
81 changes: 0 additions & 81 deletions pkg/commands/copy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@ limitations under the License.
package commands

import (
"fmt"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -142,90 +141,10 @@ func (c *CopyCommand) RequiresUnpackedFS() bool {
return true
}

func (c *CopyCommand) ShouldCacheOutput() bool {
return true
}

// CacheCommand returns true since this command should be cached
func (c *CopyCommand) CacheCommand(img v1.Image) DockerCommand {

return &CachingCopyCommand{
img: img,
cmd: c.cmd,
buildcontext: c.buildcontext,
extractFn: util.ExtractFile,
}
}

func (c *CopyCommand) From() string {
return c.cmd.From
}

type CachingCopyCommand struct {
BaseCommand
caching
img v1.Image
extractedFiles []string
cmd *instructions.CopyCommand
buildcontext string
extractFn util.ExtractFunction
}

func (cr *CachingCopyCommand) ExecuteCommand(config *v1.Config, buildArgs *dockerfile.BuildArgs) error {
logrus.Infof("Found cached layer, extracting to filesystem")
var err error

if cr.img == nil {
return errors.New(fmt.Sprintf("cached command image is nil %v", cr.String()))
}

layers, err := cr.img.Layers()
if err != nil {
return errors.Wrapf(err, "retrieve image layers")
}

if len(layers) != 1 {
return errors.New(fmt.Sprintf("expected %d layers but got %d", 1, len(layers)))
}

cr.layer = layers[0]
cr.extractedFiles, err = util.GetFSFromLayers(kConfig.RootDir, layers, util.ExtractFunc(cr.extractFn), util.IncludeWhiteout())

logrus.Debugf("extractedFiles: %s", cr.extractedFiles)
if err != nil {
return errors.Wrap(err, "extracting fs from image")
}

return nil
}

func (cr *CachingCopyCommand) FilesUsedFromContext(config *v1.Config, buildArgs *dockerfile.BuildArgs) ([]string, error) {
return copyCmdFilesUsedFromContext(config, buildArgs, cr.cmd, cr.buildcontext)
}

func (cr *CachingCopyCommand) FilesToSnapshot() []string {
f := cr.extractedFiles
logrus.Debugf("%d files extracted by caching copy command", len(f))
logrus.Tracef("Extracted files: %s", f)

return f
}

func (cr *CachingCopyCommand) MetadataOnly() bool {
return false
}

func (cr *CachingCopyCommand) String() string {
if cr.cmd == nil {
return "nil command"
}
return cr.cmd.String()
}

func (cr *CachingCopyCommand) From() string {
return cr.cmd.From
}

func resolveIfSymlink(destPath string) (string, error) {
if !filepath.IsAbs(destPath) {
return "", errors.New("dest path must be abs")
Expand Down
154 changes: 0 additions & 154 deletions pkg/commands/copy_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@ limitations under the License.
package commands

import (
"archive/tar"
"fmt"
"io"
"io/ioutil"
Expand Down Expand Up @@ -236,159 +235,6 @@ func Test_resolveIfSymlink(t *testing.T) {
}
}

func Test_CachingCopyCommand_ExecuteCommand(t *testing.T) {
tempDir := setupTestTemp()

tarContent, err := prepareTarFixture([]string{"foo.txt"})
if err != nil {
t.Errorf("couldn't prepare tar fixture %v", err)
}

config := &v1.Config{}
buildArgs := &dockerfile.BuildArgs{}

type testCase struct {
desctiption string
expectLayer bool
expectErr bool
count *int
expectedCount int
command *CachingCopyCommand
extractedFiles []string
contextFiles []string
}
testCases := []testCase{
func() testCase {
err = ioutil.WriteFile(filepath.Join(tempDir, "foo.txt"), []byte("meow"), 0644)
if err != nil {
t.Errorf("couldn't write tempfile %v", err)
t.FailNow()
}

c := &CachingCopyCommand{
img: fakeImage{
ImageLayers: []v1.Layer{
fakeLayer{TarContent: tarContent},
},
},
buildcontext: tempDir,
cmd: &instructions.CopyCommand{
SourcesAndDest: []string{
"foo.txt", "foo.txt",
},
},
}
count := 0
tc := testCase{
desctiption: "with valid image and valid layer",
count: &count,
expectedCount: 1,
expectLayer: true,
extractedFiles: []string{"/foo.txt"},
contextFiles: []string{"foo.txt"},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
*tc.count++
return nil
}
tc.command = c
return tc
}(),
func() testCase {
c := &CachingCopyCommand{}
tc := testCase{
desctiption: "with no image",
expectErr: true,
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
return nil
}
tc.command = c
return tc
}(),
func() testCase {
c := &CachingCopyCommand{
img: fakeImage{},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
return nil
}
return testCase{
desctiption: "with image containing no layers",
expectErr: true,
command: c,
}
}(),
func() testCase {
c := &CachingCopyCommand{
img: fakeImage{
ImageLayers: []v1.Layer{
fakeLayer{},
},
},
}
c.extractFn = func(_ string, _ *tar.Header, _ io.Reader) error {
return nil
}
tc := testCase{
desctiption: "with image one layer which has no tar content",
expectErr: false, // this one probably should fail but doesn't because of how ExecuteCommand and util.GetFSFromLayers are implemented - cvgw- 2019-11-25
expectLayer: true,
}
tc.command = c
return tc
}(),
}

for _, tc := range testCases {
t.Run(tc.desctiption, func(t *testing.T) {
c := tc.command
err := c.ExecuteCommand(config, buildArgs)
if !tc.expectErr && err != nil {
t.Errorf("Expected err to be nil but was %v", err)
} else if tc.expectErr && err == nil {
t.Error("Expected err but was nil")
}

if tc.count != nil {
if *tc.count != tc.expectedCount {
t.Errorf("Expected extractFn to be called %v times but was called %v times", tc.expectedCount, *tc.count)
}
for _, file := range tc.extractedFiles {
match := false
cFiles := c.FilesToSnapshot()
for _, cFile := range cFiles {
if file == cFile {
match = true
break
}
}
if !match {
t.Errorf("Expected extracted files to include %v but did not %v", file, cFiles)
}
}

cmdFiles, err := c.FilesUsedFromContext(
config, buildArgs,
)
if err != nil {
t.Errorf("failed to get files used from context from command %v", err)
}

if len(cmdFiles) != len(tc.contextFiles) {
t.Errorf("expected files used from context to equal %v but was %v", tc.contextFiles, cmdFiles)
}
}

if c.layer == nil && tc.expectLayer {
t.Error("expected the command to have a layer set but instead was nil")
} else if c.layer != nil && !tc.expectLayer {
t.Error("expected the command to have no layer set but instead found a layer")
}
})
}
}

func TestCopyCommand_ExecuteCommand_Extended(t *testing.T) {
setupDirs := func(t *testing.T) (string, string) {
testDir, err := ioutil.TempDir("", "")
Expand Down
2 changes: 0 additions & 2 deletions pkg/executor/build.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,8 +185,6 @@ func (s *stageBuilder) populateCompositeKey(command fmt.Stringer, files []string
switch v := command.(type) {
case *commands.CopyCommand:
compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey)
case *commands.CachingCopyCommand:
compositeKey = s.populateCopyCmdCompositeKey(command, v.From(), compositeKey)
}

srcCtx := s.opts.SrcContext
Expand Down
3 changes: 1 addition & 2 deletions pkg/executor/build_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -824,8 +824,7 @@ func Test_stageBuilder_build(t *testing.T) {
},
rootDir: dir,
expectedCacheKeys: []string{copyCommandCacheKey},
// CachingCopyCommand is not pushed to the cache
pushedCacheKeys: []string{},
pushedCacheKeys: []string{},
commands: getCommands(dir, []instructions.Command{
&instructions.CopyCommand{
SourcesAndDest: []string{
Expand Down

0 comments on commit 8cf4065

Please sign in to comment.