Add filesystem caching#232
Conversation
| return "", err | ||
| } | ||
| rootDir := filepath.Join(dir, ".container-diff", "cache") | ||
| imageName = strings.Replace(imageName, string(os.PathSeparator), "", -1) |
There was a problem hiding this comment.
We should probably only cache on digests, not tags. If there's a tag, let's resolve it to a digest then cache.
There was a problem hiding this comment.
In general I agree, but what about images that were just built locally and don't have digests? I could just do a docker build -t "foo:bar" . and that image won't have a digest.
There was a problem hiding this comment.
Hmm, we need some way to cache at a content-addressable level, otherwise you risk stale images for things like "latest". What happens if you call Digest() on an image from the daemon? It should work.
There was a problem hiding this comment.
The latest changes from go-containerregistry fix this (it used to take on the order of 7 minutes to call Digest()), so we're good here.
nkubala
force-pushed
the
cache
branch
2 times, most recently
from
cc3dca6 to
158af30
Compare
| for _, layer := range imgLayers { | ||
| path, err := ioutil.TempDir("", strings.Replace(imageName, "/", "", -1)) | ||
| layerStart := time.Now() | ||
| diffID, err := layer.DiffID() |
There was a problem hiding this comment.
I would think we would want to use the Digest, not the DiffID here. Could you explain in a comment why we're using one vs. the other?
My thinking is that caching is most important from a registry, where the layers are compressed. The digest will be faster in that case.
There was a problem hiding this comment.
Sorry, I was experimenting with some things across machines and absentmindedly pushed to this branch. google/go-containerregistry#150 is why I was originally apprehensive to using Digest, I was just trying to get something unique to create the cache directory, but I'll be changing this back to Digest
There was a problem hiding this comment.
Cool, I think this is good to go once you switch it back.
| elapsed := time.Now().Sub(start) | ||
| logrus.Infof("retrieving image from daemon took %f seconds", elapsed.Seconds()) | ||
| // TODO(nkubala): remove this when we can set compression level in containerregistry | ||
| noCache = true // force noCache if image is in daemon |
There was a problem hiding this comment.
How does this work if one image is in the registry and one isn't?
There was a problem hiding this comment.
ah yeah, good point. changed to use a different variable
| } | ||
|
|
||
| func getExtractPathForImage(imageName string, image v1.Image) (string, error) { | ||
| if isDaemon { |
There was a problem hiding this comment.
Can we check if something is from the daemon here, instead of at the package level? I'm still confused about how this would work with two images, one from a daemon and one from a registry.
There was a problem hiding this comment.
Picking up the latest changes to go-containerregistry eliminates the need for this, removed.
nkubala
force-pushed
the
cache
branch
2 times, most recently
from
3b353f8 to
4a14a48
Compare
2 participants
This adds naive caching of the extracted filesystem tarball. On successive runs, the tarball on disk will be used for all runs of container-diff. The
--no-cacheflag can be passed to force a reextraction of the filesystem, and the--saveflag can be used to persist that tarball on disk.Fixes #230