ROX-33333: Sensor targeted cache invalidation changes#19708
Conversation
|
This change is part of the following stack: Change managed by git-spice. |
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at d82be84. To use with deploy scripts, first |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #19708 +/- ##
==========================================
- Coverage 49.60% 49.60% -0.01%
==========================================
Files 2763 2763
Lines 208271 208339 +68
==========================================
+ Hits 103317 103346 +29
- Misses 97288 97329 +41
+ Partials 7666 7664 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
9a957fc to
eb11813
Compare
1ad1e5f to
782897c
Compare
eb11813 to
cbd7a3e
Compare
cbd7a3e to
d2e46f3
Compare
|
/test all |
9c3d8c2 to
8625ac2
Compare
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis pull request introduces targeted image cache invalidation capability to the admission control system. A new sensor capability constant is added, and the settings manager, manager interface, and management service are extended to support cache invalidation via streams. The reprocessor handler is updated to use targeted invalidation instead of flushing the entire cache. Changes
Sequence DiagramsequenceDiagram
participant Reprocessor as Reprocessor Handler
participant SettingsMgr as Settings Manager
participant Stream as Invalidation Stream
participant MgmtService as Management Service
participant Manager as Admission Control Manager
participant Sensor as Sensor/Client
Reprocessor->>SettingsMgr: InvalidateImageCache(imageKeys)
SettingsMgr->>Stream: Push AdmCtrlImageCacheInvalidation
par Management Service Processing
MgmtService->>Stream: Iterator.Next()
Stream-->>MgmtService: Return invalidation object
MgmtService->>Sensor: Send MsgToAdmissionControl_ImageCacheInvalidation
end
par Manager Processing
Manager->>Manager: ImageCacheInvalidationC receives message
Manager->>Manager: Iterate image keys
Manager->>Manager: Compute cache keys (considering flatten setting)
Manager->>Manager: Remove entries from imageCache
Manager->>Manager: Clear name-to-cache-key mappings
Manager->>Manager: Call imageFetchGroup.Forget()
Manager->>Manager: Log invalidation count
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
sensor/common/reprocessor/handler_test.go (1)
37-37: Assert invalidation payload content, not only invocation.Using
gomock.Any()can hide regressions where wrong or empty key sets are forwarded. Please assert thatInvalidateImageCachereceives expected keys (IDs/full names) for each test case.Also applies to: 121-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sensor/common/reprocessor/handler_test.go` at line 37, The test currently uses gomock.Any() for mockAdmCtrlSettingsMgr.EXPECT().InvalidateImageCache which only asserts invocation; change the expectation to assert the actual payload content by matching the expected key set for each test case (use gomock.Eq(expectedKeys) or gomock.Match(func(v interface{}) bool { /* compare against expectedKeys */ }) as appropriate) and update both occurrences (the call at mockAdmCtrlSettingsMgr.EXPECT().InvalidateImageCache and the other occurrence around lines referenced) so the mock validates the exact IDs/full names forwarded to InvalidateImageCache rather than accepting any value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sensor/admission-control/manager/manager_impl.go`:
- Around line 533-541: The loop in the invalidation path currently chooses a
single cache key variant based on m.currentState() flatten mode (using cacheKey
= key.GetImageId() or key.GetImageIdV2()), which can leave the other variant
stale; instead, in the loop over inv.GetImageKeys() always attempt eviction for
both non-empty forms: call the evict/remove logic for key.GetImageId() if
non-empty and separately for key.GetImageIdV2() if non-empty (do not gate on
flatten), and increment invalidated appropriately for each successful eviction;
update the code around the cacheKey/flatten logic in the loop that references
m.currentState(), key.GetImageId(), key.GetImageIdV2(), and the eviction call so
both variants are evicted unconditionally when present.
In `@sensor/common/admissioncontroller/management_service.go`:
- Line 114: Create the invalidation iterator before calling sync so it won't
miss events: move the call to s.imageCacheInvalidationStream.Iterator(true) (the
imageCacheInvIt variable) to immediately before the sync(stream) invocation (or
at the start of the method where the stream is set up) so the iterator exists
prior to s.sync(stream) and will observe any invalidation events emitted during
sync.
In `@sensor/common/reprocessor/handler.go`:
- Around line 65-69: The sender (central) must check sensor capabilities before
broadcasting InvalidateImageCache messages: update the broadcast sites in
service_impl.go (the InvalidateImageCache calls around lines 1277 and 1293) to
gate sending with
AllSensorsHaveCapability(centralsensor.TargetedImageCacheInvalidationCap) or
implement a fallback broadcast path for sensors lacking the capability; ensure
the capability symbol centralsensor.TargetedImageCacheInvalidationCap and the
capability-advertising handler (the function returning
[]centralsensor.SensorCapability in sensor/common/reprocessor/handler.go) are
used to decide routing so old sensors don’t receive unsupported targeted
invalidation messages.
---
Nitpick comments:
In `@sensor/common/reprocessor/handler_test.go`:
- Line 37: The test currently uses gomock.Any() for
mockAdmCtrlSettingsMgr.EXPECT().InvalidateImageCache which only asserts
invocation; change the expectation to assert the actual payload content by
matching the expected key set for each test case (use gomock.Eq(expectedKeys) or
gomock.Match(func(v interface{}) bool { /* compare against expectedKeys */ }) as
appropriate) and update both occurrences (the call at
mockAdmCtrlSettingsMgr.EXPECT().InvalidateImageCache and the other occurrence
around lines referenced) so the mock validates the exact IDs/full names
forwarded to InvalidateImageCache rather than accepting any value.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 7c015b2e-67ba-46bf-94aa-5a2301df47a7
📒 Files selected for processing (11)
pkg/centralsensor/caps_list.gosensor/admission-control/manager/manager.gosensor/admission-control/manager/manager_impl.gosensor/admission-control/settingswatch/sensor_push.gosensor/common/admissioncontroller/management_service.gosensor/common/admissioncontroller/management_service_test.gosensor/common/admissioncontroller/mocks/settings_manager.gosensor/common/admissioncontroller/settings_manager.gosensor/common/admissioncontroller/settings_manager_impl.gosensor/common/reprocessor/handler.gosensor/common/reprocessor/handler_test.go
e354972 to
d82be84
Compare
0d7675a to
6fed273
Compare
|
/test all |
🚀 Build Images ReadyImages are ready for commit 2e6eff0. To use with deploy scripts: export MAIN_IMAGE_TAG=4.11.x-559-g2e6eff05a1 |
Description
This PR introduces:
FlushCacheon admission controller.Please see merged foundational proto PR here.
Note:
Central already sends targeted cache invalidation messages to sensor in two cases:
We are now forwarding those to admission controller instead of aggressively flushing admission controller cache in these cases.
These are secured cluster side changes only, and hence no version compatibility concerns.
Follow on Central PR to send a targeted cache invalidation message when
ScanImageis complete is hereUser-facing documentation
Testing and quality
Automated testing
How I validated my change
Manual testing on an openshift infra cluster using delete image workflow, for images deployed after the deployments pass the admission review requests.