EC-1706: Remove filesystem image layer cache#3217
EC-1706: Remove filesystem image layer cache#3217robnester-rh wants to merge 1 commit intoconforma:mainfrom
Conversation
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. CodeRabbit does not review PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughRemoved the filesystem-backed image cache and all explicit Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Review Summary by QodoRemove filesystem image layer cache for thread-safety
WalkthroughsDescription• Remove non-thread-safe filesystem image layer cache entirely • Eliminate EC_CACHE environment variable from Tekton task definitions • Simplify OCI client by removing cache initialization and wrapping logic • Clean up related tests and benchmark code that relied on caching Diagramflowchart LR
A["OCI Client with Cache"] -->|Remove cache logic| B["Simplified OCI Client"]
C["Tekton Tasks with EC_CACHE"] -->|Remove env var| D["Tekton Tasks without EC_CACHE"]
E["Cache-dependent Tests"] -->|Simplify/Remove| F["Streamlined Tests"]
B --> G["Direct remote.Image calls"]
D --> H["No workaround needed"]
File Changes1. internal/utils/oci/client.go
|
Code Review by Qodo
1. Unclosed layer readers
|
robnester-rh
force-pushed
the
EC-1706
branch
2 times, most recently
from
7122de9 to
022d176
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/utils/oci/client.go (1)
193-193: Consider request-scoped image memoization after cache removal.Line 193 now guarantees a fresh
remote.Imagefetch per call. Ininternal/fetchers/oci/config/config.go(Lines 30-75),FetchConfig()andFetchParentImage()each calloci.NewClient(ctx).Image(ref)separately, which can duplicate registry round-trips for the same reference. Consider sharing one fetched image (or a short-lived request-scoped map keyed by digest) to reduce latency and 429 pressure without reintroducing global cache.As per coding guidelines "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/utils/oci/client_test.go`:
- Around line 119-121: Tests construct a zero-value defaultClient which leaves
ctx nil, causing trace.StartRegion in defaultClient.Image and
defaultClient.Layer to receive a nil context; initialize the test clients with a
non-nil context (for example defaultClient{ctx: context.Background()} or
context.TODO()) wherever defaultClient{} is used in the tests so Image and Layer
call trace.StartRegion with a valid context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2fa98bfa-8ae2-4a56-bbe0-bc90856e97e5
📒 Files selected for processing (6)
benchmark/simple/simple.gointernal/utils/oci/client.gointernal/utils/oci/client_test.gotasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yamltasks/verify-conforma-konflux-vsa-ta/0.1/verify-conforma-konflux-vsa-ta.yamltasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
💤 Files with no reviewable changes (4)
- benchmark/simple/simple.go
- tasks/verify-conforma-konflux-vsa-ta/0.1/verify-conforma-konflux-vsa-ta.yaml
- tasks/verify-conforma-konflux-ta/0.1/verify-conforma-konflux-ta.yaml
- tasks/verify-enterprise-contract/0.1/verify-enterprise-contract.yaml
The image layer cache was not thread-safe and caused random redhat_manifests.redhat_manifests_missing violations when validating multiple images sharing layers (conforma#1109). All Tekton tasks already disabled it via EC_CACHE=false, and benchmarking showed no measurable performance gain from caching. Remove the cache entirely rather than fixing it: - Remove imgCache/initCache and cache.Image wrapping from OCI client - Remove EC_CACHE env var from Tekton task definitions - Remove EC_CACHE=false from benchmark setup - Remove related test (TestCacheInit) and simplify TestImage - Remove stale caching TODO in Layer() Ref: EC-1706 Ref: EC-1669 Signed-off-by: Rob Nester <rnester@redhat.com> Assisted-by: Cursor Made-with: Cursor
Codecov Report✅ All modified and coverable lines are covered by tests.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
EC_CACHEenv var from Tekton task definitions where it was set to"false"to work around the thread-safety issueThe cache was originally intended to avoid re-fetching shared image layers from the registry, but caused random
redhat_manifests.redhat_manifests_missingviolations when validating multiple images with shared layers. Rather than merging the thread-safe cache fix (PR #3150), we opted to remove the cache entirely since benchmarks showed no meaningful gain.Test plan
make testpassescr --plainreturns no findingsRef: EC-1706
Related: EC-1669, #1109
Made with Cursor