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

Cache images slightly differently #165

Merged
merged 18 commits into from
Apr 11, 2023
Merged

Cache images slightly differently #165

merged 18 commits into from
Apr 11, 2023

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented Apr 9, 2023

closes #161

In an effort to speed things up, this changes the caching strategy from relying on Docker's somewhat opaque rules to simply caching the generated image. Here's how it works:

  1. We define the cache key to be the hash of the files in the implementation folder.
  2. We check s3 directly to see if we have a file named $IMAGE_NAME-$CACHE_KEY-$ARCH.tar.gz, if we do import that image.
  3. Yes? we create the image.json, and call make -o image.json which allows the implementation to do some extra work from cache (e.g. js-libp2p needs to build both the node and browser images, so it can cache the base and do quick work to build the final node and browser images).
  4. No? Build as normal.
  5. If we have the PUSH_CACHE env we push the built image to s3.

Q: Why not a GH registry?
A: Then we have to deal with GC. This lets us rely on the s3 behavior of a lifetime for objs.

Q: Why not an S3 registry?
A: That requires a bit more work to set up. You need a registry container running in the background. This is simpler. It also supports public read only access to the cache.

Benefits

  1. Faster CI runs for everyone. Less time spent building, even if only cached (I noticed one js-libp2p PR took 8 minutes before starting the test).
  2. More predictable caching. Caching strategy is dead simple here so it's easy to see why a cache missed happened.
  3. Anonymous S3 Reads. Everyone can use the cache
  4. Faster downloads of images. If the final image of the container-mage is small (it should be), then this is significantly less data than getting every single layer and rebuilding.

Why didn't we do this from the start?

One step at a time. The build layer caching strategy was convenient and it was correct. I was hoping it would be better, but here we are.

Note that this solution is different (and imo better) than each implementation managing its own images and pushing to a registry. I say better since only this repo needs to worry about pushing images and GC'ing them.

@MarcoPolo MarcoPolo marked this pull request as draft April 9, 2023 04:18
@thomaseizinger
Copy link
Contributor

Let me know when this is ready for review, it looks exciting!

@MarcoPolo MarcoPolo marked this pull request as ready for review April 10, 2023 17:34
for (const implFamily of fs.readdirSync(path.join(multidimInteropDir, 'impl'))) {
const ig = ignore()

addGitignoreIfPresent(ig, path.join(multidimInteropDir, ".gitignore"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to extract path.join(multidimInteropDir, ".gitignore") and others constants as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's the same, but unless you feel strongly I'd rather just leave it.

multidim-interop/helpers/cache.ts Outdated Show resolved Hide resolved
@thomaseizinger
Copy link
Contributor

Nice work @MarcoPolo!

@MarcoPolo MarcoPolo merged commit 23fdcef into master Apr 11, 2023
MarcoPolo added a commit to mxinden/test-plans that referenced this pull request Apr 13, 2023
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.

2 participants