MCO-2468: Dump compact cache to CM for persistence - #6379
Conversation
This change dumps the cache (a reduced version of it with the bare minimal for OS Image Streams) to a new CM to allow new MCC pods that use a different name and thus, a new cache file, to read already existing cache info. It's specially useful in disconnected environments. Signed-off-by: Pablo Rodriguez Nava <git@amail.pablintino.eu>
|
@pablintino: This pull request references MCO-2468 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
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
WalkthroughThe change adds ConfigMap-backed inspection-cache synchronization, cache-entry filtering and transformation, mutation notifications, and coordinated controller startup. OSImageStream entries receive specialized filtering, and cache-related tests cover persistence, eviction, loading, and transformation behavior. ChangesInspection cache persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant machine-config-controller
participant FileInspectionCache
participant ConfigMapCacheSyncer
participant Kubernetes ConfigMap
machine-config-controller->>ConfigMapCacheSyncer: Create syncer
machine-config-controller->>FileInspectionCache: Start cache
FileInspectionCache->>ConfigMapCacheSyncer: Load persisted entries
ConfigMapCacheSyncer->>Kubernetes ConfigMap: Read cache data
FileInspectionCache->>ConfigMapCacheSyncer: Notify after mutations
ConfigMapCacheSyncer->>Kubernetes ConfigMap: Debounced snapshot update
🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pablintino 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 |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
pkg/imageutils/configmap_cache_syncer_test.go (1)
147-156: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
TestConfigMapCacheSyncer_SaveSkipsDuplicatedoes not verify deduplication.The test calls
savetwice and asserts only that both calls returnnil. It passes even if the second call issues a full update. Assert that the secondsaveperforms no write. Check theResourceVersionof the ConfigMap, or inspect the actions recorded by the fake client.🧪 Proposed fix
func TestConfigMapCacheSyncer_SaveSkipsDuplicate(t *testing.T) { - syncer, _ := newFakeSyncer(t) + syncer, client := newFakeSyncer(t) entries := map[string]*InspectionCacheEntry{ "sha256:aaa": {Labels: map[string]string{"k": "v"}}, } require.NoError(t, syncer.save(context.Background(), entries)) + + first, err := client.CoreV1().ConfigMaps(testNamespace).Get(context.Background(), testCMName, metav1.GetOptions{}) + require.NoError(t, err) + require.NoError(t, syncer.save(context.Background(), entries)) + + second, err := client.CoreV1().ConfigMaps(testNamespace).Get(context.Background(), testCMName, metav1.GetOptions{}) + require.NoError(t, err) + assert.Equal(t, first.ResourceVersion, second.ResourceVersion, "second save must not write the ConfigMap") }🤖 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/configmap_cache_syncer_test.go` around lines 147 - 156, Update TestConfigMapCacheSyncer_SaveSkipsDuplicate to verify the second save performs no write, not merely that it succeeds. After the first save, inspect the fake client’s recorded actions or the ConfigMap ResourceVersion, then assert it is unchanged after the second save.pkg/imageutils/inspect_cache.go (1)
208-216: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Startcannot detect a failed syncer start.
CacheSyncer.Startreturns no value. InConfigMapCacheSyncer.Start, aWaitForCacheSynctimeout logs a warning and returns without launching the sync loop.FileInspectionCache.Startthen callsloadFromSyncer, which reads an unsynced lister and receives no entries. The cache silently runs without external persistence for the whole process lifetime.Consider returning an error from
CacheSyncer.Startand propagating it, so the caller can log or retry.🤖 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.go` around lines 208 - 216, Update the CacheSyncer.Start contract to return an error, including the ConfigMapCacheSyncer.Start timeout path, and propagate that result through FileInspectionCache.Start so a failed syncer initialization is surfaced instead of continuing silently. Update all implementations and call sites to handle the returned error while preserving normal startup and eviction behavior.pkg/imageutils/inspect_cache_test.go (1)
223-235: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
TestFileInspectionCache_StartSyncNoFlushWithoutChangesuses a fixed sleep.
time.Sleep(200 * time.Millisecond)adds fixed runtime to every test run and stays sensitive to scheduling on loaded CI machines. Preferassert.Neverwith the same condition, which fails fast and states the intent.🤖 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 223 - 235, Replace the fixed time.Sleep in TestFileInspectionCache_StartSyncNoFlushWithoutChanges with assert.Never, polling syncer.saveCount over the equivalent observation window to verify it remains zero without changes. Keep the existing cache startup and cancellation setup unchanged.
🤖 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 106-108: Defer persisted-entry loading in NewFileInspectionCache
until the namespaced informer has synchronized, or update
ConfigMapCacheSyncer.Load to use a direct client read during initial
construction; ensure Start does not leave the cache empty when the ConfigMap
already exists. Add a startup test covering a pre-existing cache ConfigMap with
an initially unstarted informer.
In `@pkg/imageutils/cache_entry_transformer.go`:
- Around line 12-34: Update NewCacheFileTransformer to deep-copy the cache entry
before passing its file data to transform, ensuring the callback receives an
isolated byte slice and cannot mutate the live entry. Preserve the existing
behavior for missing files and transformation errors, and return the copied
entry with the transformed data on success.
In `@pkg/imageutils/inspect_cache_test.go`:
- Around line 159-183: Protect mockSyncer.saved and saveCount with a mutex,
locking writes in Start’s goroutine and reads through a state() accessor that
returns a consistent snapshot. Update the affected tests’ direct field
assertions and Eventually callbacks to use state() instead.
In `@pkg/imageutils/inspect_cache.go`:
- Around line 225-245: Update FileInspectionCache.loadFromSyncer to call
saveLocked after merging the loaded entries while c.mu remains held, ensuring
restored entries are persisted to the local file before returning.
---
Nitpick comments:
In `@pkg/imageutils/configmap_cache_syncer_test.go`:
- Around line 147-156: Update TestConfigMapCacheSyncer_SaveSkipsDuplicate to
verify the second save performs no write, not merely that it succeeds. After the
first save, inspect the fake client’s recorded actions or the ConfigMap
ResourceVersion, then assert it is unchanged after the second save.
In `@pkg/imageutils/inspect_cache_test.go`:
- Around line 223-235: Replace the fixed time.Sleep in
TestFileInspectionCache_StartSyncNoFlushWithoutChanges with assert.Never,
polling syncer.saveCount over the equivalent observation window to verify it
remains zero without changes. Keep the existing cache startup and cancellation
setup unchanged.
In `@pkg/imageutils/inspect_cache.go`:
- Around line 208-216: Update the CacheSyncer.Start contract to return an error,
including the ConfigMapCacheSyncer.Start timeout path, and propagate that result
through FileInspectionCache.Start so a failed syncer initialization is surfaced
instead of continuing silently. Update all implementations and call sites to
handle the returned error while preserving normal startup and eviction behavior.
🪄 Autofix
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: 6ee4b366-5900-4bed-bf1d-853b9df1406d
📒 Files selected for processing (14)
cmd/machine-config-controller/start.gopkg/controller/common/constants.gopkg/controller/pinnedimageset/cache_warmer_test.gopkg/imageutils/cache_entry_transformer.gopkg/imageutils/cache_entry_transformer_test.gopkg/imageutils/configmap_cache_syncer.gopkg/imageutils/configmap_cache_syncer_test.gopkg/imageutils/inspect_cache.gopkg/imageutils/inspect_cache_test.gopkg/osimagestream/entry_transformer.gopkg/osimagestream/entry_transformer_test.gopkg/osimagestream/image_data.gopkg/osimagestream/imagestream_source.gotest/e2e-2of2/osimagestream_test.go
| inspectionCache := imageutils.NewFileInspectionCache( | ||
| path.Join(startOpts.streamsCache, "image-inspection.json"), 48*time.Hour, syncer, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Load the ConfigMap only after its informer is ready.
NewFileInspectionCache loads persisted entries during construction. At this point, the namespaced informer factory has not started yet. The ConfigMap lister in ConfigMapCacheSyncer.Load therefore returns NotFound, and the cache starts empty. inspectionCache.Start waits for synchronization later, but it does not load the persisted entries again.
Defer the external load until after ConfigMap informer synchronization, or make the initial load use a direct Kubernetes client read. Add a startup test with a pre-existing cache ConfigMap and an initially unstarted informer.
🤖 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 `@cmd/machine-config-controller/start.go` around lines 106 - 108, Defer
persisted-entry loading in NewFileInspectionCache until the namespaced informer
has synchronized, or update ConfigMapCacheSyncer.Load to use a direct client
read during initial construction; ensure Start does not leave the cache empty
when the ConfigMap already exists. Add a startup test covering a pre-existing
cache ConfigMap with an initially unstarted informer.
| // NewCacheFileTransformer returns a CacheEntryTransformer that applies a | ||
| // transformation function to a cached file matching the given path. Other | ||
| // files and labels are preserved. | ||
| func NewCacheFileTransformer(path string, transform func([]byte) ([]byte, error)) CacheEntryTransformer { | ||
| return func(_ string, entry *InspectionCacheEntry) *InspectionCacheEntry { | ||
| if entry.Files == nil { | ||
| return entry | ||
| } | ||
| data, ok := entry.Files[path] | ||
| if !ok { | ||
| return entry | ||
| } | ||
|
|
||
| transformed, err := transform(data) | ||
| if err != nil { | ||
| return entry | ||
| } | ||
|
|
||
| cp := entry.DeepCopy() | ||
| cp.Files[path] = transformed | ||
| return cp | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Copy the entry before calling transform.
Line 25 passes the live entry.Files[path] slice to the callback. A callback can modify that slice before Line 30 creates the copy. This violates the no-mutation contract and can corrupt the live cache entry.
Proposed fix
- transformed, err := transform(data)
+ cp := entry.DeepCopy()
+ transformed, err := transform(cp.Files[path])
if err != nil {
return entry
}
- cp := entry.DeepCopy()
cp.Files[path] = transformed
return cp📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // NewCacheFileTransformer returns a CacheEntryTransformer that applies a | |
| // transformation function to a cached file matching the given path. Other | |
| // files and labels are preserved. | |
| func NewCacheFileTransformer(path string, transform func([]byte) ([]byte, error)) CacheEntryTransformer { | |
| return func(_ string, entry *InspectionCacheEntry) *InspectionCacheEntry { | |
| if entry.Files == nil { | |
| return entry | |
| } | |
| data, ok := entry.Files[path] | |
| if !ok { | |
| return entry | |
| } | |
| transformed, err := transform(data) | |
| if err != nil { | |
| return entry | |
| } | |
| cp := entry.DeepCopy() | |
| cp.Files[path] = transformed | |
| return cp | |
| } | |
| } | |
| // NewCacheFileTransformer returns a CacheEntryTransformer that applies a | |
| // transformation function to a cached file matching the given path. Other | |
| // files and labels are preserved. | |
| func NewCacheFileTransformer(path string, transform func([]byte) ([]byte, error)) CacheEntryTransformer { | |
| return func(_ string, entry *InspectionCacheEntry) *InspectionCacheEntry { | |
| if entry.Files == nil { | |
| return entry | |
| } | |
| data, ok := entry.Files[path] | |
| if !ok { | |
| return entry | |
| } | |
| cp := entry.DeepCopy() | |
| transformed, err := transform(cp.Files[path]) | |
| if err != nil { | |
| return entry | |
| } | |
| cp.Files[path] = transformed | |
| return cp | |
| } | |
| } |
🤖 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/cache_entry_transformer.go` around lines 12 - 34, Update
NewCacheFileTransformer to deep-copy the cache entry before passing its file
data to transform, ensuring the callback receives an isolated byte slice and
cannot mutate the live entry. Preserve the existing behavior for missing files
and transformation errors, and return the copied entry with the transformed data
on success.
| type mockSyncer struct { | ||
| loadEntries map[string]*InspectionCacheEntry | ||
| loadErr error | ||
| saved map[string]*InspectionCacheEntry | ||
| saveCount int | ||
| } | ||
|
|
||
| func (m *mockSyncer) Load(_ context.Context) (map[string]*InspectionCacheEntry, error) { | ||
| return m.loadEntries, m.loadErr | ||
| } | ||
|
|
||
| func (m *mockSyncer) Start(ctx context.Context, src SyncableCache, debounce time.Duration) { | ||
| ch := src.SyncNotify() | ||
| go func() { | ||
| for waitForNotify(ctx, ch) { | ||
| if !debounceDrain(ctx, ch, debounce) { | ||
| break | ||
| } | ||
| m.saved = src.Snapshot() | ||
| m.saveCount++ | ||
| } | ||
| m.saved = src.Snapshot() | ||
| m.saveCount++ | ||
| }() | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
mockSyncer has a data race on saved and saveCount.
The goroutine started in Start writes m.saved and m.saveCount. The tests read both fields from the test goroutine, inside require.Eventually and in the final assertions. No synchronization protects these fields. go test -race will report a race in TestFileInspectionCache_StartSyncFlushesAfterPut, TestFileInspectionCache_StartSyncNoFlushWithoutChanges, TestFileInspectionCache_StartSyncFlushOnShutdown, and TestFileInspectionCache_StartSyncEvictionNotifies.
Guard the fields with a mutex and add accessor methods.
🔒 Proposed fix
type mockSyncer struct {
+ mu sync.Mutex
loadEntries map[string]*InspectionCacheEntry
loadErr error
saved map[string]*InspectionCacheEntry
saveCount int
}
+func (m *mockSyncer) record(snapshot map[string]*InspectionCacheEntry) {
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ m.saved = snapshot
+ m.saveCount++
+}
+
+func (m *mockSyncer) state() (map[string]*InspectionCacheEntry, int) {
+ m.mu.Lock()
+ defer m.mu.Unlock()
+ return m.saved, m.saveCount
+}
+
func (m *mockSyncer) Load(_ context.Context) (map[string]*InspectionCacheEntry, error) {
return m.loadEntries, m.loadErr
}
func (m *mockSyncer) Start(ctx context.Context, src SyncableCache, debounce time.Duration) {
ch := src.SyncNotify()
go func() {
for waitForNotify(ctx, ch) {
if !debounceDrain(ctx, ch, debounce) {
break
}
- m.saved = src.Snapshot()
- m.saveCount++
+ m.record(src.Snapshot())
}
- m.saved = src.Snapshot()
- m.saveCount++
+ m.record(src.Snapshot())
}()
}Then update the assertions to use state(), for example:
require.Eventually(t, func() bool {
_, count := syncer.state()
return count > 0
}, 5*time.Second, 50*time.Millisecond)
saved, _ := syncer.state()
assert.Contains(t, saved, "sha256:new")🤖 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 159 - 183, Protect
mockSyncer.saved and saveCount with a mutex, locking writes in Start’s goroutine
and reads through a state() accessor that returns a consistent snapshot. Update
the affected tests’ direct field assertions and Eventually callbacks to use
state() instead.
| func (c *FileInspectionCache) loadFromSyncer(ctx context.Context) { | ||
| entries, err := c.syncer.Load(ctx) | ||
| if err != nil { | ||
| klog.Warningf("Failed to load inspection cache from external store: %v", err) | ||
| return | ||
| } | ||
| if len(entries) == 0 { | ||
| return | ||
| } | ||
|
|
||
| c.mu.Lock() | ||
| defer c.mu.Unlock() | ||
| for digest, entry := range entries { | ||
| existing, ok := c.entries[digest] | ||
| // Keep the local entry if it is the same age or newer. | ||
| if ok && !existing.CreatedAt.Before(entry.CreatedAt) { | ||
| continue | ||
| } | ||
| c.entries[digest] = entry.DeepCopy() | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Entries loaded from the external store are not written to the local file.
loadFromSyncer mutates c.entries but does not call saveLocked or notifySync. If the pod restarts before the next Put or eviction, the restored entries are lost from the on-disk cache. Add a saveLocked call after the merge loop.
💾 Proposed fix
c.mu.Lock()
defer c.mu.Unlock()
+ loaded := false
for digest, entry := range entries {
existing, ok := c.entries[digest]
// Keep the local entry if it is the same age or newer.
if ok && !existing.CreatedAt.Before(entry.CreatedAt) {
continue
}
c.entries[digest] = entry.DeepCopy()
+ loaded = true
+ }
+ if loaded {
+ if err := c.saveLocked(); err != nil {
+ klog.Warningf("Failed to persist inspection cache after external load: %v", err)
+ }
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (c *FileInspectionCache) loadFromSyncer(ctx context.Context) { | |
| entries, err := c.syncer.Load(ctx) | |
| if err != nil { | |
| klog.Warningf("Failed to load inspection cache from external store: %v", err) | |
| return | |
| } | |
| if len(entries) == 0 { | |
| return | |
| } | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| for digest, entry := range entries { | |
| existing, ok := c.entries[digest] | |
| // Keep the local entry if it is the same age or newer. | |
| if ok && !existing.CreatedAt.Before(entry.CreatedAt) { | |
| continue | |
| } | |
| c.entries[digest] = entry.DeepCopy() | |
| } | |
| } | |
| func (c *FileInspectionCache) loadFromSyncer(ctx context.Context) { | |
| entries, err := c.syncer.Load(ctx) | |
| if err != nil { | |
| klog.Warningf("Failed to load inspection cache from external store: %v", err) | |
| return | |
| } | |
| if len(entries) == 0 { | |
| return | |
| } | |
| c.mu.Lock() | |
| defer c.mu.Unlock() | |
| loaded := false | |
| for digest, entry := range entries { | |
| existing, ok := c.entries[digest] | |
| // Keep the local entry if it is the same age or newer. | |
| if ok && !existing.CreatedAt.Before(entry.CreatedAt) { | |
| continue | |
| } | |
| c.entries[digest] = entry.DeepCopy() | |
| loaded = true | |
| } | |
| if loaded { | |
| if err := c.saveLocked(); err != nil { | |
| klog.Warningf("Failed to persist inspection cache after external load: %v", err) | |
| } | |
| } | |
| } |
🤖 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.go` around lines 225 - 245, Update
FileInspectionCache.loadFromSyncer to call saveLocked after merging the loaded
entries while c.mu remains held, ensuring restored entries are persisted to the
local file before returning.
|
/hold Still needs some work to get it working |
openshift-ci
Bot
added
the
do-not-merge/hold
|
@pablintino: The following tests failed, say
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. |
2 participants
- What I did
This change dumps the cache (a reduced version of it with the bare minimal for OS Image Streams) to a new CM to allow new MCC pods that use a different name and thus, a new cache file, to read already existing cache info. It's specially useful in disconnected environments.
- How to verify it
TBD
- Description for the changelog
Dump a reduced version of the image cache to a CM to allow it to survive MCC Pod recreations.
Summary by CodeRabbit
New Features
Bug Fixes