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

Entrypoint cache should cache by digest #333

Closed
imjasonh opened this issue Dec 14, 2018 · 7 comments
Closed

Entrypoint cache should cache by digest #333

imjasonh opened this issue Dec 14, 2018 · 7 comments
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given

Comments

@imjasonh
Copy link
Member

When we lookup entrypoints for remote images, we cache it for later lookups. But, we cache by the requested image name, and not the resolved image by digest.

So if a user changes the entrypoint to an image without changing its tag, the new entrypoint might not get picked up since it will already have been cached. Instead, we should always resolve the image tag to its digest, then lookup in the cache whether that digest's entrypoint is already known.

Since we'd be resolving the image tag -> digest before running the build, we can also request the exact image by digest when we run the pod. This removes a race condition where the entrypoint changes between the time the build is requested and the time the step starts running. We should include builder image digests in TaskRunStatus for visibility.

To avoid looking up the digest for a tag each time, we could keep a separate cache for that lookup that expires after some (configurable) amount of time.

@imjasonh imjasonh added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given labels Dec 14, 2018
@Zyqsempai
Copy link
Contributor

@imjasonh Should we also use LRU cache for "tag->digest" thingy?

@bobcatfish
Copy link
Collaborator

Yep I think so @Zyqsempai ! Note that @mathspanda is converting our existing cache to lru in #350 so we can probably build on that :D

@Zyqsempai
Copy link
Contributor

@bobcatfish Then, it will be good to merge his PR asap;)

@zhyuri
Copy link
Contributor

zhyuri commented Jan 15, 2019

Hi @Zyqsempai , are you working on this issue?

Just want to confirm because I'm also looking for some issues to work on.

@Zyqsempai
Copy link
Contributor

@zhyuri Yep, i do. Sorry.

@zhyuri
Copy link
Contributor

zhyuri commented Jan 15, 2019

Cool! No problem. 🙌

@bobcatfish
Copy link
Collaborator

Thanks @Zyqsempai !!! 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. meaty-juicy-coding-work This task is mostly about implementation!!! And docs and tests of course but that's a given
Projects
None yet
Development

No branches or pull requests

4 participants