-
Notifications
You must be signed in to change notification settings - Fork 21
feat: decouple informer cache population and event handling #380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: decouple informer cache population and event handling #380
Conversation
Signed-off-by: Wei Weng <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR decouples informer cache population from event handling to enable webhook validation on follower hub agents. Previously, the ChangeDetector created informers and added event handlers in one step, which only ran on the leader. Now, a new InformerPopulator controller creates informers on all pods (leader and followers) while the ChangeDetector (leader-only) adds event handlers to those informers.
Key changes:
- Introduced InformerPopulator that runs on all pods to populate informer caches
- Refactored ChangeDetector to only add event handlers to existing informers
- Extracted shared resource discovery logic into reusable helper functions
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/resourcewatcher/informer_populator.go | New controller that discovers resources and creates informers without handlers on all pods |
| pkg/resourcewatcher/informer_populator_test.go | Comprehensive test coverage for the new InformerPopulator |
| pkg/resourcewatcher/change_dector.go | Refactored to use shared discovery logic and only add event handlers |
| pkg/resourcewatcher/change_detector_test.go | Tests for the refactored ChangeDetector behavior |
| pkg/resourcewatcher/resource_collector.go | Extracted shared helper functions for resource discovery and filtering |
| pkg/resourcewatcher/resource_collector_test.go | Tests for the shared resource discovery helpers |
| pkg/utils/informer/informermanager.go | Added new interface methods: CreateInformerForResource and AddEventHandlerToInformer |
| pkg/utils/informer/informermanager_test.go | Tests for the new informer manager methods |
| pkg/utils/informer/readiness/readiness.go | Updated comment to reference InformerPopulator instead of ChangeDetector |
| cmd/hubagent/workload/setup.go | Wired up InformerPopulator and ChangeDetector in the hub agent |
| test/utils/handler/handler.go | New test utility for testing event handlers |
| test/utils/informer/manager.go | Added stub implementations for new interface methods |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Signed-off-by: Wei Weng <[email protected]>
dc9f555 to
088ea47
Compare
Signed-off-by: Wei Weng <[email protected]>
Description of your changes
decouple informer cache population and event handling
This is a pre-requisite for #366 to make validating webhook on follower hub agents work.
If I want to run a leader hub agent and multiple followers, the followers need to run webhook validation, which requires informer cache. However, the informer cache is populated by change detector, which only runs on leader replica.
Previously, change detector calls AddDynamicResources to add informer and attach event handler
Now, I am changing it so that a new controller informer populator starts the informer caches while change detector attaches event handler to the informer caches.
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
Special notes for your reviewer