Add shared cache for resolvers #8825
Conversation
tekton-robot
added
release-note-none
tekton-robot
added
the
size/XXL
|
The following is the coverage report on the affected files.
|
brianwcook
force-pushed
the
resolver-cache
branch
from
1c42710 to
c606874
Compare
|
The following is the coverage report on the affected files.
|
brianwcook
force-pushed
the
resolver-cache
branch
from
c606874 to
a512086
Compare
|
The following is the coverage report on the affected files.
|
brianwcook
force-pushed
the
resolver-cache
branch
from
a512086 to
0d8d277
Compare
tekton-robot
added
size/XL
|
The following is the coverage report on the affected files.
|
tekton-robot
added
size/XXL
|
The following is the coverage report on the affected files.
|
|
The following is the coverage report on the affected files.
|
brianwcook
force-pushed
the
resolver-cache
branch
from
76731e4 to
d54830f
Compare
|
The following is the coverage report on the affected files.
|
|
/kind feature |
tekton-robot
added
the
kind/feature
|
The following is the coverage report on the affected files.
|
brianwcook
force-pushed
the
resolver-cache
branch
from
9a9a2e7 to
7df3f08
Compare
tekton-robot
removed
the
size/XL
brianwcook
force-pushed
the
resolver-cache
branch
from
aa85bd0 to
0423cc2
Compare
|
The following is the coverage report on the affected files.
|
|
/retest |
This commit adds caching for bundle and git resolvers to reduce chances of being rate limited by registries and git forges. - Add cache interface and in-memory implementation - Add cache configuration options to docs - Add a simple 'always on or off' option for cluster resolver - Add documentation for cache configuration Signed-off-by: Brian Cook <[email protected]>
- Remove dead GetGlobalCache function and globalCache variable - Update tests to use dependency injection cache instead of global cache - Add cache clearing in test setup to ensure test isolation - Fix all GetGlobalCache references in cache_test.go and resolver tests This resolves the shared cache state issue that was causing e2e test failures (TestPropagatedParams, TestLargerResultsSidecarLogs) by ensuring each test gets a clean cache state.
Co-authored-by: Stanislav Jakuschevskij <[email protected]>
- Add cache_test.go with 100% coverage of cache.go functions - Test ShouldUseCache with all priority levels and cache modes - Test GetSystemDefaultCacheMode for different resolver types - Test ValidateCacheMode with valid/invalid inputs - Test cache constants validation - Includes 20+ test cases covering edge cases and error conditions This resolves CI coverage issue for pkg/remoteresolution/resolver/framework/cache.go
- Replace nil, nil return with proper sentinel error - Add errors import to support error creation - Resolves nilnil linter warning in mockCacheAwareResolver.Resolve The mock method was never called in tests but needed proper error handling.
…ve cache API make GenerateCacheKey private and improve cache API Fix cache unit tests for private generateCacheKey function make generateCacheKey private cache API refactoring move unit tests Implement RunCacheOperations Add comprehensive package-scoped unit tests for generateCacheKey function Fix bundle resolver tests: restore original tests + add cache tests Fix bundle resolver compatibility with existing tests Implement Interface Simplification (ImmutabilityChecker) Remove duplicate IsOCIPullSpecByDigest function Move cache decision tests to framework Fix method signature consistency Complete Code Style & Cleanup
make GenerateCacheKey private and improve cache API Fix cache unit tests for private generateCacheKey function make generateCacheKey private cache API refactoring move unit tests Implement RunCacheOperations Add comprehensive package-scoped unit tests for generateCacheKey function Fix bundle resolver tests: restore original tests + add cache tests Fix bundle resolver compatibility with existing tests Implement Interface Simplification (ImmutabilityChecker) Remove duplicate IsOCIPullSpecByDigest function Move cache decision tests to framework Fix method signature consistency Complete Code Style & Cleanup
- Add missing pipelinerun-with-task-timeout-override.yaml example - Increase timeout from 20s to 45s to account for pod startup overhead - Provides 35s buffer for Kubernetes pod initialization + 10s task execution - Maintains test intent (TaskRunSpecs override Pipeline task timeouts) - Eliminates timing-related flakiness in CI environments Resolves intermittent test failures in examples test matrix where pod startup overhead (17s observed) exceeded the 20s timeout window.
- Add FeatureFlags.AnyResolverEnabled() helper function - Implement lazy cache initialization with sync.Once - Cache only loads when any resolver is enabled - RunCacheOperations handles nil cache gracefully
- Remove extra blank lines in resolver_integration_test.go - Fix whitespace in conversion_test.go - Fix trailing space in start_time_test.go comment
brianwcook
force-pushed
the
resolver-cache
branch
from
9783116 to
a6bd055
Compare
|
The following is the coverage report on the affected files.
|
|
/retest |
|
|
|
/cc @twoGiants it looks like this is ready for re-review. I'm going to try and make some time to work through it this week, but since you requested changes I think you'll need to review again as well |
@waveywaves @brianwcook yes please 🥺 😸 👍
@aThorp96 yesssss! Looking forward to it. @brianwcook can I re-review? Just let me know when its ready and thank you for the good work 🥇 ! |
There was a problem hiding this comment.
- If the admin enable cache after installation, do we have to restart the controller or is it dynamically switch on / off (like other features) ?
- No need to change copyright dates for existing files
This still needs a lot of work and review comment to take into acount. Given the amount of time we spend for reviewing this, I really think one of us should carry it on.
| } | ||
|
|
||
| // createContextWithResolverConfig creates a test context with resolver configuration | ||
| func createContextWithResolverConfig(t *testing.T, anyResolverEnabled bool) context.Context { |
There was a problem hiding this comment.
Shouldn't this param named differently ? it's only setting the git resolver on or off.
|
|
||
| // GetCacheConfigName returns the name of the cache configuration ConfigMap. | ||
| // This can be overridden via the CONFIG_RESOLVER_CACHE_NAME environment variable. | ||
| func GetCacheConfigName() string { |
There was a problem hiding this comment.
Interesting. I think there is, on other part where we look for a configmap, some env var like this as well. So maybe it should be there, but used also outside of the tests ?
| func (c *ResolverCache) Add(resolverType string, params []pipelinev1.Param, resource resolutionframework.ResolvedResource) resolutionframework.ResolvedResource { | ||
| key := generateCacheKey(resolverType, params) | ||
|
|
||
| if c.logger != nil { |
There was a problem hiding this comment.
In almost any places in the code, we don't check if the logger we use is nil, it is assumed it is always set.
| for i, val := range arrayVals { | ||
| if i > 0 { | ||
| paramStr += "," | ||
| } | ||
| paramStr += val | ||
| } |
There was a problem hiding this comment.
we could use strings.Join here I think.
| }) | ||
| } | ||
|
|
||
| func withCacheFromConfig(ctx context.Context, cfg *rest.Config) context.Context { |
There was a problem hiding this comment.
the cfg parameter is not used, we can remove it.
| @@ -1,5 +1,5 @@ | |||
| /* | |||
| Copyright 2024 The Tekton Authors | |||
| LabelValueBundleResolverType string = "bundles" | ||
|
|
||
| // BundleResolverName is the name that the bundle resolver should be associated with. | ||
| BundleResolverName = "bundleresolver" |
There was a problem hiding this comment.
I am not sure why it should be made deprecated ? The bundle resolver is here to stay (the bundle field is deprecated, but not the bundles nor the resolver).
I also don't see any reason to change this if it causes some issues (metrics, workqueue, ...). If we do change it, I'd rather do it on its own PR, etc..
| return nil, errors.New("the Resolve method has not been implemented.") | ||
|
|
||
| // Get the resolve function - default to bundleresolution.ResolveRequest if not set | ||
| resolveFunc := r.resolveRequestFunc |
There was a problem hiding this comment.
How can r.resolveRequestFunc be set ? Given that we have another return in the if r.useCache(.., I don't really understand this change. We could have a simpler implementation, isn't it ?
(and it seems to be what is done in other resolvers as well)
| // associated with. | ||
| func (r *FakeResolver) GetName(_ context.Context) string { | ||
| func (r *FakeResolver) GetName(ctx context.Context) string { | ||
| return framework.FakeResolverName | ||
| } | ||
|
|
||
| // GetSelector returns the labels that resource requests are required to have for | ||
| // the fake resolver to process them. | ||
| func (r *FakeResolver) GetSelector(_ context.Context) map[string]string { | ||
| func (r *FakeResolver) GetSelector(ctx context.Context) map[string]string { | ||
| return map[string]string{ |
There was a problem hiding this comment.
Any reason for those changes ? ctx not being used, it can be anynomized.
| func (r *Resolver) Validate(ctx context.Context, req *v1beta1.ResolutionRequestSpec) error { | ||
| if len(req.Params) > 0 { | ||
| return git.ValidateParams(ctx, req.Params) | ||
| return git.ValidateParams(ctx, req.Params) |
There was a problem hiding this comment.
Is git the package itself ? If yes, it is not required here, I am amazed that go doesn't complain about it even.
Same below and in the tests.
Changes
Resolvers are awesome but can be noisy. In high traffic environments we have seen rate limiting both from registries when using the bundle resolver and from git forges when using the git resolver. This PR adds caching to the resolvers (except the cluster resolver). It is only enabled by default when 'safe' - meaning that the object was fetched by some hash / digest. Otherwise, it can be turned on with a resolver parameter.
Note
This PR is a work in progress posted for initial thoughts and feedback. It works for some cases already (git resolver) - others, maybe not.
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes