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

refactor(cache): make RefOption types public #4184

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

goller
Copy link
Contributor

@goller goller commented Aug 29, 2023

Howdy! I'm trying to extend buildkit by adding a specialized cache.Manager.
I'm unable to implement cache.Accessor as the RefOption types are a mix
of public and private types behind an empty interface.

This updates the options to now be public so Accessors can be implemented.

I've also updated the RefOption type itself to have a private function. This helps to keep the list of options types fixed. I can totally remove this if you would like.

Previously, RefOption usage was a mix of public and
private types behind an empty interface.

It was not possible to implement the `Accessor` interface as
many RefOptions types were private.

Now, using a private interface the list of options types is fixed.
The options are now public so `Accessors` can be implemented.

Signed-off-by: Chris Goller <[email protected]>

func CachePolicyDefault(m *cacheMetadata) error {
return m.SetCachePolicyDefault()
func CachePolicyDefault() RefOption {
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 noticed CachePolicyDefault was unused. I could totally remove it if you would like

@@ -1449,7 +1449,20 @@ func IsNotFound(err error) bool {
return errors.Is(err, errNotFound)
}

type RefOption interface{}
type RefOption interface {
opt()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned above, I can for sure remove this opt() function.

@tonistiigi
Copy link
Member

I'm trying to extend buildkit by adding a specialized cache.Manager.

Why do you need to do that? At first thought, that doesn't seem like a good point for custom implementations as cache package leaks to ops/executor and is already abstraction over snapshot/blob implementations.

@goller
Copy link
Contributor Author

goller commented Aug 29, 2023

I'm trying to extend buildkit by adding a specialized cache.Manager.

Why do you need to do that? At first thought, that doesn't seem like a good point for custom implementations as cache package leaks to ops/executor and is already abstraction over snapshot/blob implementations.

Howdy howdy!

I'm looking to use the rest of BuildKit with a custom cache implementation that doesn't use bolt or local storage.
This PR happens to decouple the option parameters from the bolt implementation as I moved the "action" of the option into the private initializeMetadata and setImageRefMetadata functions.

@tonistiigi
Copy link
Member

For that one, I think you should look more into solver.CacheManager, solver.CacheKeyStorage, solver.CacheResultStorage. The naming is confusing but the cache.Manager is the holder for the local storage references (snapshot-blob pairs).

@goller
Copy link
Contributor Author

goller commented Sep 5, 2023

For that one, I think you should look more into solver.CacheManager, solver.CacheKeyStorage, solver.CacheResultStorage. The naming is confusing but the cache.Manager is the holder for the local storage references (snapshot-blob pairs).

Hi there thanks for the pointers. I looked at implementing those interfaces but there are other important code paths that use the cache.Manager that are not wrapped by the interfaces you mentioned. One example that uses cache.Manager is the Worker.ResolveOp. The ops interact with the cache.Manager.

This patch would allow one to create a different cache.Manager without need of private types.

Also, it checks the RefOption type at compile time rather than runtime now.

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