MCO-2413: Image inspection cache - #6306
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@pablintino: This pull request references MCO-2413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
approved
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds optional file-backed image inspection caching, lazy system-context creation, digest-based eviction retention, and controller/deployment wiring for cache operation. ChangesImage inspection caching and controller integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ControllerRuntime
participant OSImageStreamController
participant CachedImagesInspector
participant FileInspectionCache
participant RenderController
ControllerRuntime->>OSImageStreamController: Build OSImageStream
OSImageStreamController->>CachedImagesInspector: Inspect image pullspecs
CachedImagesInspector->>FileInspectionCache: Read or write inspection data
ControllerRuntime->>FileInspectionCache: Start periodic eviction
FileInspectionCache->>OSImageStreamController: Request retained digests
FileInspectionCache->>RenderController: Request retained digests
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
647fd77 to
fc94819
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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@manifests/machineconfigcontroller/deployment.yaml`:
- Around line 30-36: Update the controller container resources to define
measured CPU and memory limits alongside the existing requests, and add an
ephemeral-storage limit as appropriate. Set a sizeLimit on the emptyDir volume
named image-inspection-cache to bound cache growth, ensuring every container in
the deployment has CPU and memory limits.
In `@pkg/imageutils/inspect_cache_test.go`:
- Around line 79-88: Update the eviction tests around cache.StartEviction and
the related cases to remove time.Sleep-based synchronization; invoke
cache.evict() directly for deterministic execution. For min-age scenarios, set
each entry’s CreatedAt explicitly so the tests validate age behavior without
relying on elapsed wall-clock time, preserving the existing eviction assertions.
In `@pkg/imageutils/inspect_cache.go`:
- Around line 66-89: The FileInspectionCache boundary must deep-copy entries to
prevent caller mutations from altering cached state. Update Put to copy the
incoming InspectionCacheEntry, including Labels, Files, and nested byte slices,
before storing or merging it; update Get to return an independent deep copy,
including delegated labels, while preserving locking and persistence behavior.
In `@pkg/osimagestream/cached_inspector.go`:
- Line 43: Update the cache-hit and cache-miss logging in the cached inspector
to log the already-derived digest instead of the raw img pullspec, including the
related log statement around the second referenced location. Preserve the
existing log levels and message context while ensuring private-registry
hostnames are not emitted.
In `@pkg/osimagestream/inspector.go`:
- Around line 81-100: Update both cleanup defers in ImagesInspectorImpl.Inspect
and FetchImageFile to capture and propagate SysContext.Cleanup errors instead of
discarding them. Preserve any existing inspection or file-fetch error while
returning cleanup failures when no earlier error exists, using named return
values or equivalent error-aware deferred cleanup.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f43885d3-e248-4288-8b45-66ab207bf66e
📒 Files selected for processing (18)
cmd/machine-config-controller/start.gocmd/machine-config-osimagestream/helpers.gomanifests/machineconfigcontroller/deployment.yamlpkg/controller/bootstrap/bootstrap.gopkg/controller/bootstrap/bootstrap_test.gopkg/controller/osimagestream/osimagestream_controller.gopkg/controller/render/render_controller.gopkg/imageutils/inspect_cache.gopkg/imageutils/inspect_cache_test.gopkg/imageutils/reference.gopkg/imageutils/sys_context.gopkg/osimagestream/cached_inspector.gopkg/osimagestream/cached_inspector_test.gopkg/osimagestream/inspector.gopkg/osimagestream/mocks_test.gopkg/osimagestream/osimagestream.gopkg/osimagestream/osimagestream_test.gotest/e2e-2of2/osimagestream_test.go
| resources: | ||
| requests: | ||
| cpu: 20m | ||
| memory: 50Mi | ||
| volumeMounts: | ||
| - mountPath: /var/cache/mcc | ||
| name: image-inspection-cache |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Bound the controller and cache resource usage.
The changed container still has no CPU/memory limits, and the new emptyDir has no sizeLimit. Cache growth can consume node ephemeral storage. Add measured container limits and bound the cache volume, preferably with an accompanying ephemeral-storage limit.
As per path instructions, Kubernetes manifests require “Resource limits (cpu, memory) on every container.”
Also applies to: 83-84
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@manifests/machineconfigcontroller/deployment.yaml` around lines 30 - 36,
Update the controller container resources to define measured CPU and memory
limits alongside the existing requests, and add an ephemeral-storage limit as
appropriate. Set a sizeLimit on the emptyDir volume named image-inspection-cache
to bound cache growth, ensuring every container in the deployment has CPU and
memory limits.
Source: Path instructions
There was a problem hiding this comment.
In practice this isn't relevant since the cache would be very light, but let's say that something goes horribly wrong and we do balloon the directory, I think we'd end up getting evicted silently?
Might be worth bounding the emptydir like coderabbit is suggesting, just for additional safety, WDYT
There was a problem hiding this comment.
@yuqi-zhang, agreed. An unbounded emptyDir contributes to node ephemeral-storage pressure; the kubelet can evict the pod when pressure thresholds are crossed. That should be visible in Pod status/events, rather than truly silent, but it is still an avoidable controller disruption and the cache is lost on restart.
Adding a conservative sizeLimit is sensible defense in depth. It bounds the failure to cache writes failing once full instead of allowing the cache to contribute to node-wide pressure. I’d keep this change focused on the new volume and choose the limit from expected cache-entry sizing/observed usage, rather than adding arbitrary CPU or memory limits.
There was a problem hiding this comment.
I've added a 128Mi limit, far far over what I'd expect? 10Mi?, but still doable and small, just in case for some reason this gets crazy.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| cache.StartEviction(ctx, time.Second, 0) | ||
| time.Sleep(2 * time.Second) | ||
|
|
||
| assert.Nil(t, cache.Get("sha256:aaa")) | ||
| assert.Nil(t, cache.Get("sha256:bbb")) | ||
|
|
||
| reloaded := NewFileInspectionCache(path, 0) | ||
| assert.Nil(t, reloaded.Get("sha256:aaa")) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove timing-dependent sleeps from eviction tests.
These assertions can become flaky under scheduler load, especially the min-age check at exactly one second. Invoke cache.evict() synchronously and set CreatedAt explicitly for min-age scenarios.
Also applies to: 105-117, 125-133
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/imageutils/inspect_cache_test.go` around lines 79 - 88, Update the
eviction tests around cache.StartEviction and the related cases to remove
time.Sleep-based synchronization; invoke cache.evict() directly for
deterministic execution. For min-age scenarios, set each entry’s CreatedAt
explicitly so the tests validate age behavior without relying on elapsed
wall-clock time, preserving the existing eviction assertions.
There was a problem hiding this comment.
Similar to what coderabbit is saying here, is there a way you see to make the tests more deterministic? I agree that using sleep() seems like it could be flaky (although I guess in practice you've tested this and it should be significantly faster than 2 seconds?
Applies to other tests where you use sleep(). Maybe a poll loop with a timeout is better, if there's no deterministic way to test this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/imageutils/inspect_cache.go --items all
printf '\n--- sleep usages in cache tests ---\n'
rg -n -C 8 '\btime\.Sleep\s*\(' pkg/imageutils/inspect_cache_test.go
printf '\n--- eviction implementation ---\n'
rg -n -C 16 '\b(StartEviction|evict)\s*\(' pkg/imageutils/inspect_cache.go
printf '\n--- relevant test range ---\n'
sed -n '70,190p' pkg/imageutils/inspect_cache_test.goLength of output: 9253
@yuqi-zhang, yes—there is a fully deterministic option here, so a poll loop is not needed for these tests.
Because pkg/imageutils/inspect_cache_test.go is in the imageutils package, it can invoke the unexported cache.evict() method directly. That method performs both the eviction and cache-file persistence synchronously. The three tests are testing eviction semantics, not the goroutine/ticker wiring in StartEviction, so they can remove context, StartEviction, and every time.Sleep call.
For the min-age case, explicitly backdate one entry under cache.mu, then invoke cache.evict():
func TestFileInspectionCache_EvictRespectsMinAge(t *testing.T) {
path := filepath.Join(t.TempDir(), "cache.json")
cache := NewFileInspectionCache(path, 2*time.Second)
require.NoError(t, cache.Put("sha256:fresh", &InspectionCacheEntry{
Labels: map[string]string{"fresh": "1"},
}))
require.NoError(t, cache.Put("sha256:old", &InspectionCacheEntry{
Labels: map[string]string{"old": "1"},
}))
cache.mu.Lock()
cache.entries["sha256:old"].CreatedAt = time.Now().Add(-3 * time.Second)
cache.mu.Unlock()
cache.evict()
assert.NotNil(t, cache.Get("sha256:fresh"))
assert.Nil(t, cache.Get("sha256:old"))
}Likewise, TestFileInspectionCache_EvictNoEvicters and TestFileInspectionCache_EvictRetainsUnion can replace their StartEviction/Sleep setup with one cache.evict() call. This eliminates scheduling variance and cuts several seconds from the test suite.
There was a problem hiding this comment.
I've fixed this one with a polling check, as I don't like at all testing/calling non-exported functions like evict.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/machine-config-controller/start.go`:
- Around line 287-301: Update createControllers to include renderCtrl in the
slice of controllers it returns, ensuring the controller constructed by
render.New and registered with inspectionCache is passed to runStartCmd and
started via Run.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 04d473bd-677d-44d5-821a-4e7550333fa8
📒 Files selected for processing (9)
cmd/machine-config-controller/start.gomanifests/machineconfigcontroller/deployment.yamlpkg/controller/osimagestream/osimagestream_controller.gopkg/controller/render/render_controller.gopkg/imageutils/inspect_cache.gopkg/imageutils/inspect_cache_test.gopkg/imageutils/reference.gopkg/osimagestream/cached_inspector.gopkg/osimagestream/cached_inspector_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- pkg/imageutils/reference.go
- pkg/imageutils/inspect_cache_test.go
- manifests/machineconfigcontroller/deployment.yaml
- pkg/controller/render/render_controller.go
- pkg/osimagestream/cached_inspector.go
- pkg/controller/osimagestream/osimagestream_controller.go
- pkg/imageutils/inspect_cache.go
- pkg/osimagestream/cached_inspector_test.go
This changes modifies the OSImageStream APIs to replace the requested SysContext with an interface that allows lazily creating it. This is necessary for the future cache addition to avoid creating a SysContext when there's a cache hit. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
fc94819 to
8f64c8e
Compare
|
/pipeline required |
|
Scheduling tests matching the |
8f64c8e to
278cd00
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/osimagestream/cached_inspector_test.go`:
- Around line 76-94: Update CachedImagesInspector.Inspect to track all result
indexes for each pullspec instead of overwriting a single resultIndex entry, so
duplicate uncached inputs populate every corresponding result slot. Extend the
tests around TestCachedImagesInspector_MixedHitsAndMisses with a duplicate-input
regression case that verifies all duplicate results are populated correctly.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9e8e8e83-ee7b-42f7-8cff-7037015ffa9f
📒 Files selected for processing (6)
cmd/machine-config-controller/start.gomanifests/machineconfigcontroller/deployment.yamlpkg/controller/osimagestream/osimagestream_controller.gopkg/controller/render/render_controller.gopkg/osimagestream/cached_inspector.gopkg/osimagestream/cached_inspector_test.go
🚧 Files skipped from review as they are similar to previous changes (5)
- manifests/machineconfigcontroller/deployment.yaml
- pkg/osimagestream/cached_inspector.go
- pkg/controller/osimagestream/osimagestream_controller.go
- cmd/machine-config-controller/start.go
- pkg/controller/render/render_controller.go
278cd00 to
23202b5
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/test e2e-gcp-op-part1 e2e-gcp-op-ocl-part1 |
23202b5 to
8862d18
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
Overall, generally makes sense to me - thanks for breaking down the commits, it was very helpful during the review. My only nit would be to add a few comments for the Retain() functions added to the main controllers. The simple e2e could perhaps use some explanation since it is not a traditional e2e, but one that is testing just the cache stack we're adding(perhaps that is similar to the other e2es in this package tho?) /approve |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Mostly lgtm as well (can merge as is if you want). Had a couple of very minor questions/comments
| func (ctrl *Controller) Retain(digests []string) []string { | ||
| osis, err := ctrl.getExistingOSImageStream() | ||
| if err != nil || osis == nil { | ||
| return digests |
There was a problem hiding this comment.
Just to understand this a bit better, we're saying that if we can't get the OSImageSteam object we stop eviction altogether?
Probably fine since if your lister is persistently failing, you probably have a bigger problem than a (relatively small) cache retaining.
There was a problem hiding this comment.
Correct, an error shouldn't be permanent, and if so, what you said, there will be other more important issues.
| ctx, cancel := context.WithCancel(context.Background()) | ||
| defer cancel() | ||
| cache.StartEviction(ctx, time.Second, 0) | ||
| time.Sleep(2 * time.Second) | ||
|
|
||
| assert.Nil(t, cache.Get("sha256:aaa")) | ||
| assert.Nil(t, cache.Get("sha256:bbb")) | ||
|
|
||
| reloaded := NewFileInspectionCache(path, 0) | ||
| assert.Nil(t, reloaded.Get("sha256:aaa")) |
There was a problem hiding this comment.
Similar to what coderabbit is saying here, is there a way you see to make the tests more deterministic? I agree that using sleep() seems like it could be flaky (although I guess in practice you've tested this and it should be significantly faster than 2 seconds?
Applies to other tests where you use sleep(). Maybe a poll loop with a timeout is better, if there's no deterministic way to test this
| resources: | ||
| requests: | ||
| cpu: 20m | ||
| memory: 50Mi | ||
| volumeMounts: | ||
| - mountPath: /var/cache/mcc | ||
| name: image-inspection-cache |
There was a problem hiding this comment.
In practice this isn't relevant since the cache would be very light, but let's say that something goes horribly wrong and we do balloon the directory, I think we'd end up getting evicted silently?
Might be worth bounding the emptydir like coderabbit is suggesting, just for additional safety, WDYT
| } | ||
|
|
||
| dir := filepath.Dir(c.path) | ||
| tmp, err := os.CreateTemp(dir, ".inspection-cache-*.tmp") |
There was a problem hiding this comment.
(suggestion from claude): this can cause puts to silently fail if the directory we're creating this on doesn't exist, can maybe have a defensive os.MkdirAll in the contructor
That said I think you only call this on the /var/cache/mcc which should exist from the mount, so not a big problem
There was a problem hiding this comment.
Added a protection that creates the directory. BTW, Claude has suggested a good change: Move my by-hand atomic write to the library the daemon uses. I've done it.
Create a new type in imageutils to cache the labels from inspected images in disk. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
Use the new images cache to create a cache enabled inspector Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
8684a46 to
a819ff5
Compare
|
/pipeline required |
|
Scheduling tests matching the |
|
/retest-required |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
/lgtm
Changes lgtm, thanks!
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: djoshy, pablintino, yuqi-zhang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test e2e-aws-ovn-upgrade |
|
Ran all 10 longduration jobs (5 for 4.23, 5 for 5.0) against the fix.
Checked nightly history (last 10 runs per job): These jobs have 0-10% pass rate in nightly. Every failure in this run matches existing nightly failures. No new regressions from this PR. All failures are pre-existing or infra-related. /verified by @ptalgulk01 |
|
@ptalgulk01: The label(s) DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
openshift-ci-robot
added
the
verified
|
@ptalgulk01: This PR has been marked as verified by DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/label qe-approved |
openshift-ci
Bot
added
the
qe-approved
|
@pablintino: This pull request references MCO-2413 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
@pablintino: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
5 participants
- What I did
Added a cache for the image inspection operations in the MCC that use an emptyDir in the pod to store the data.
- How to verify it
TBD
- Description for the changelog
Added a cache for the image inspection operations in the MCC that use an emptyDir in the pod to store the data.
Summary by CodeRabbit
--streams-cache, default/var/cache/mcc), including periodic eviction that keeps active digests warm./var/cache/mcc) so inspection data can persist across restarts.