refactor: switch InformerManager to context and cache sync lookups#674
Open
Yetkin Timocin (ytimocin) wants to merge 1 commit into
Open
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Yetkin Timocin (ytimocin)
force-pushed
the
refactor/informermanager-context-and-sync-cache
branch
from
April 29, 2026 17:22
6484d7c to
883a270
Compare
Yetkin Timocin (ytimocin)
force-pushed
the
refactor/informermanager-context-and-sync-cache
branch
from
May 1, 2026 17:43
883a270 to
14f59bf
Compare
Yetkin Timocin (ytimocin)
force-pushed
the
refactor/informermanager-context-and-sync-cache
branch
3 times, most recently
from
May 13, 2026 03:04
e74da99 to
6c38148
Compare
…lookups Replace the custom stop channel parameter on `NewInformerManager` with a plain `context.Context`. The manager derives a child context via `context.WithCancel`, so `Stop()` keeps working while parent-context cancellation also drives shutdown. The `ContextForChannel` helper had no remaining callers and is removed. Add a `sync.Map` cache (`syncedInformers`) that records GVRs whose informers have already reported `HasSynced() == true`. `HasSynced` is monotonic for the life of an informer instance, so a presence-only set with no invalidation is sufficient. Hot callers (for example the webhook scope check on every readiness probe) skip the factory mutex on subsequent lookups. Update the five call sites in `cmd/hubagent`, the rollout / updaterun / placement integration suites, and the unit tests; tests adopt `t.Context()` since the module is on Go 1.25. Boy Scout fixes on touched files: type-name-prefix the `Manager` interface doc, fix `include` -> `including` and `Stop` doc grammar, drop a redundant outer `RLock` in `TestAddEventHandlerToInformer`, and modernize one `for i := 0; i < n; i++` loop. Add new tests `TestIsInformerSynced_CachesSyncedResult` and `TestIsInformerSynced_DoesNotCacheUnsyncedResult` covering the cache write-on-true and never-cache-false paths. Fixes kubefleet-dev#649 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Yetkin Timocin <ytimocin@microsoft.com>
Yetkin Timocin (ytimocin)
force-pushed
the
refactor/informermanager-context-and-sync-cache
branch
from
May 18, 2026 18:59
6c38148 to
0a5b658
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description of your changes
Three small changes in
pkg/utils/informer/informermanager.go:Channel → context. Replace
parentCh <-chan struct{}onNewInformerManagerwith a plaincontext.Context. The manager derives a child context viacontext.WithCancel, soStop()still works while parent-context cancellation also drives shutdown. TheContextForChannelhelper (which spawned a goroutine to bridge channel-close into context-cancel) had no remaining callers and is removed.Lazy sync cache. Add a
sync.Map(syncedInformers) that records GVRs whose informers have already reportedHasSynced() == true.HasSyncedis monotonic for the life of an informer instance, so a presence-only set with no invalidation is sufficient. The cache lets hot reconcile-path callers —pkg/utils/controller/resource_selector_resolver.go(called per CRP reconcile) andpkg/controllers/resourcechange/resourcechange_controller.go(called per resource-change event) — skip the factory mutex on subsequent lookups. The readiness probe (pkg/utils/informer/readiness) also benefits.Phantom-informer guard. Add a
registeredInformers sync.Mappopulated byAddStaticResource,CreateInformerForResource, andAddEventHandlerToInformer.IsInformerSyncedconsults it before callinginformerFactory.ForResource(), which otherwise silently registers an unstarted, handler-less informer for any unknown GVR. Observable behaviour is unchanged (callers already treatfalseas "not ready, requeue"); the factory just no longer accumulates phantom entries.Five call sites updated:
cmd/hubagent/workload/setup.go,pkg/controllers/{rollout,updaterun,placement}/suite_test.go, plus the package's own test file (which now adoptst.Context()since the module is on Go 1.25).Boy-Scout: type-name-prefix the
Managerinterface doc, fixinclude→including, fixStopdoc grammar, drop a redundant outerRLockinTestAddEventHandlerToInformer, and modernize onefor i := 0; i < n; i++loop.Fixes #649
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
make reviewableclean (fmt, vet, golangci-lint, staticcheck, go mod tidy).go test -race ./pkg/utils/informer/...— all tests pass under the new signature.TestIsInformerSynced_CachesSyncedResult— first call populates the cache afterWaitForCacheSync; subsequent calls hit the cache and behaviour is unchanged.TestIsInformerSynced_DoesNotCacheUnsyncedResult— a registered-but-unstarted informer reportsfalseand is not cached, so a later sync still flips the result.TestIsInformerSynced_UnregisteredGVR_ReturnsFalseWithoutCreatingInformer— unknown GVR short-circuits beforefactory.ForResource()and never enters either map.NewInformerManager(rollout, updaterun, placement) compile and run.Special notes for your reviewer
Stop()semantics are preserved: it still cancels the manager's context exactly once, andcancel()of an already-cancelled context is a documented no-op.informerManagerImpl(not global), matching the per-factory scope of the underlying informer instances.LoadOrStorewas deliberately not used forsyncedInformers— it would force-store even onHasSynced() == false, poisoning the cache. TheLoadthen conditionalStorepattern is the correct write-once-on-true idiom.