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 the option to exclude certain layers from the tarball. #209

Closed
wants to merge 1 commit into from

Conversation

mattmoor
Copy link
Collaborator

This should be considered a relatively advanced option, but for folks that know
what they are doing you can reduce the amount of data that you need to encode in
the tarball for the daemon to load it.

The ultimate use case of this option will be from daemon.Write, which
currently uses the docker load interface to pull image into the daemon,
however, this currently reuploads (and redownloads) the base image on each write
in context like ko. If we can determine the set of layers that already exist
in the daemon we can elide these from the tarball to dramatically improve
performance.

Related: #205

This should be considered a relatively advanced option, but for folks that know
what they are doing you can reduce the amount of data that you need to encode in
the tarball for the daemon to load it.

The ultimate use case of this option will be from `daemon.Write`, which
currently uses the `docker load` interface to pull image into the daemon,
however, this currently reuploads (and redownloads) the base image on each write
in context like `ko`. If we can determine the set of layers that already exist
in the daemon we can elide these from the tarball to dramatically improve
performance.

Related: google#205
@mattmoor mattmoor requested a review from jonjohnsonjr June 10, 2018 21:56
Copy link
Collaborator

@jonjohnsonjr jonjohnsonjr left a comment

Choose a reason for hiding this comment

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

I'm weakly -1 on this because I find it confusing. I would think we could fix the daemon.Write problem closer to the daemon?

@@ -89,6 +105,15 @@ func Write(tag name.Tag, img v1.Image, wo *WriteOptions, w io.Writer) error {
// https://www.gnu.org/software/gzip/manual/html_node/Overview.html
layerFiles[i] = fmt.Sprintf("%s.tar.gz", hex)

// We filter late because the lenght of layerFiles must match the diff_ids
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: length

@@ -66,6 +72,16 @@ func Write(tag name.Tag, img v1.Image, wo *WriteOptions, w io.Writer) error {
return err
}

// Default filter function
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe hoist this out into its own method?

return true, nil
}
if wo != nil {
if wo.LayerFilter != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Collapse this into if wo != nil && wo.LayerFilter != nil { ?

Though I guess that makes it harder to add more options... up to you.

@mattmoor
Copy link
Collaborator Author

@jonjohnsonjr I hear you, but at the same time, I'm not sure of a good way to do this closer to daemon.Write that doesn't also involve cloning a significant portion of tarball.Write.

@dlorenc
Copy link
Contributor

dlorenc commented Nov 2, 2018

can you rebase this?

@mattmoor
Copy link
Collaborator Author

mattmoor commented Nov 9, 2018

At this point, it's more work to rebase than redo. @jonjohnsonjr are you up for it?

@mattmoor mattmoor closed this Nov 9, 2018
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this pull request Oct 3, 2019
Continuation of this PR:
google#209

This should be considered a relatively advanced option, but for folks that know
what they are doing you can reduce the amount of data that you need to encode in
the tarball for the daemon to load it.

The ultimate use case of this option will be from daemon.Write, which
currently uses the docker load interface to pull image into the daemon,
however, this currently reuploads (and redownloads) the base image on each write
in context like ko. If we can determine the set of layers that already exist
in the daemon we can elide these from the tarball to dramatically improve
performance.

Related: google#205
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this pull request Oct 3, 2019
Continuation of this PR:
google#209

This should be considered a relatively advanced option, but for folks that know
what they are doing you can reduce the amount of data that you need to encode in
the tarball for the daemon to load it.

The ultimate use case of this option will be from daemon.Write, which
currently uses the docker load interface to pull image into the daemon,
however, this currently reuploads (and redownloads) the base image on each write
in context like ko. If we can determine the set of layers that already exist
in the daemon we can elide these from the tarball to dramatically improve
performance.

Related: google#205
jonjohnsonjr added a commit to jonjohnsonjr/go-containerregistry that referenced this pull request Nov 13, 2019
Continuation of this PR:
google#209

This should be considered a relatively advanced option, but for folks that know
what they are doing you can reduce the amount of data that you need to encode in
the tarball for the daemon to load it.

The ultimate use case of this option will be from daemon.Write, which
currently uses the docker load interface to pull image into the daemon,
however, this currently reuploads (and redownloads) the base image on each write
in context like ko. If we can determine the set of layers that already exist
in the daemon we can elide these from the tarball to dramatically improve
performance.

Related: google#205
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants