Skip to content
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

Add buffering for large layers. #428

Merged
merged 1 commit into from
Nov 6, 2018

Conversation

dlorenc
Copy link
Collaborator

@dlorenc dlorenc commented Nov 2, 2018

This writes layer tarballs to disk rather than keeping them all in memory.

#358

pkg/executor/build.go Outdated Show resolved Hide resolved
@@ -55,32 +54,16 @@ func (s *Snapshotter) Key() (string, error) {

// TakeSnapshot takes a snapshot of the specified files, avoiding directories in the whitelist, and creates
// a tarball of the changed files. Return contents of the tarball, and whether or not any files were changed
func (s *Snapshotter) TakeSnapshot(files []string) ([]byte, error) {
buf := bytes.NewBuffer([]byte{})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks like you decided to move this buffering up a layer - personally i think it's nicer for the caller not to have to worry about it but either way, maybe the commit message could go into some detail about why that choice was made?


if !s.shouldTakeSnapshot(index, files) {
continue
}

f, err := ioutil.TempFile("/kaniko", "")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there any case where we would want this to be in-memory instead? (i.e. configurable)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think so - I think if anything the path itself here would be configurable. Using a tmp or memfs directory should be basically the same thing as in memory vs. on disk.

@dlorenc dlorenc force-pushed the buffer branch 3 times, most recently from d1c2caa to 9f75451 Compare November 4, 2018 21:13
Copy link
Collaborator Author

@dlorenc dlorenc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, cleaned this up quite a bit. Ready for another look.

Copy link
Collaborator

@priyawadhwa priyawadhwa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, but otherwise LGTM!

pkg/snapshot/snapshot.go Outdated Show resolved Hide resolved
When building Docker images, layers were previously stored in memory.
This caused obvious issues when manipulating large layers, which could
cause Kaniko to crash.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants