OCPBUGS-95238: Dump compact cache to CM for persistence - #6379
Conversation
|
@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 |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe change adds ConfigMap-backed inspection-cache synchronization, cache-entry filtering and transformation, ImageStream-specific persistence rules, and controller startup wiring. It updates cache construction callers and tests. ChangesInspection cache persistence
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: ⚪ Minimal · up to This change is merge-ready after normal checks and review; no actionable merge-blocking risk remains. Sequence Diagram(s)sequenceDiagram
participant FileInspectionCache
participant ConfigMapCacheSyncer
participant ConfigMapInformer
participant KubernetesClient
FileInspectionCache->>ConfigMapCacheSyncer: mutation notification
ConfigMapCacheSyncer->>FileInspectionCache: Snapshot()
ConfigMapCacheSyncer->>ConfigMapInformer: read cached ConfigMap state
ConfigMapCacheSyncer->>KubernetesClient: create or update ConfigMap
ConfigMapCacheSyncer->>FileInspectionCache: load persisted entries
Suggested reviewers: 🚥 Pre-merge checks | ✅ 14 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
![coderabbitai[bot]](https://avatars.github.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
|
/hold Still needs some work to get it working |
openshift-ci
Bot
added
the
do-not-merge/hold
004033b to
60109d6
Compare
|
/unhold |
|
Scheduling tests matching the |
openshift-ci
Bot
removed
the
do-not-merge/hold
60109d6 to
e9384f4
Compare
|
/retest-required |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
Generally seems fine to me - added a couple of questions inline. Also asked for claude to review this - but no nits seems that relevant.
|
|
||
| close(ctrlctx.InformersStarted) | ||
|
|
||
| // Start the cache before any controller that consumes it has a chance to run. |
There was a problem hiding this comment.
The PinnedImageSet controller seems to be the one controller that starts before this. Just to check, this PR seems targetted for that use case, should this start before the PIS controller does?
There was a problem hiding this comment.
Good catch, fixed.
|
|
||
| existing, err := s.kubeclient.CoreV1().ConfigMaps(s.namespace).Get(ctx, s.cmName, metav1.GetOptions{}) | ||
| if apierrors.IsNotFound(err) { | ||
| cm := &corev1.ConfigMap{ |
There was a problem hiding this comment.
I know we're not super consistent on object labelling, etc. but I wonder if it would be worth adding something like k8s-app label and/or the openshift.io/owning-component annotation, so there's a bit more metadata around it. It's in our namespace already so not a blocker by any means.
There was a problem hiding this comment.
Fixed by added the annotation. It seems we are already using the same annotation for other CMs. Thanks for the point.
|
|
||
| // releaseImageStreamLocation is the path inside a release payload image | ||
| // where the image-references ImageStream manifest is stored. | ||
| releaseImageStreamLocation = "/release-manifests/image-references" |
There was a problem hiding this comment.
the minor-est of nits: I noticed that we technically have a duplicate definition of this path with a slightly different name: https://github.com/openshift/machine-config-operator/blob/main/pkg/controller/pinnedimageset/cache_warmer.go#L16
Might be worth at least using the same name in case we change this for some reason (although looks like you just moved this definition, so we had this in the past, so it's not that relevant to this PR)
There was a problem hiding this comment.
To avoid future circular dependencies I've fixed this by using the same variable name. Not the perfect fix but it's a bit better than using different names for the same thing.
e9384f4 to
272375f
Compare
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>
272375f to
a560514
Compare
pablintino
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@pablintino: This pull request references Jira Issue OCPBUGS-95238, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/jira refresh |
|
@pablintino: This pull request references Jira Issue OCPBUGS-95238, which is invalid:
Comment 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. |
yuqi-zhang
left a comment
yuqi-zhang
left a comment
There was a problem hiding this comment.
/lgtm
Assuming we didn't break any ordering that the original setup was dependent on
|
Scheduling tests matching the |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
|
@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. |
3 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
Tests