feat: Retain only metadata for member cluster resources not mapped to an existing Work - #7817
Open
zach593 wants to merge 2 commits into
Open
feat: Retain only metadata for member cluster resources not mapped to an existing Work#7817zach593 wants to merge 2 commits into
zach593 wants to merge 2 commits into
Conversation
Signed-off-by: zach593 <zach_li@outlook.com>
Signed-off-by: zach593 <zach_li@outlook.com>
karmada-bot
added
the
kind/feature
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
karmada-bot
requested review from
Garrybest,
XiShanYongYe-Chang,
lonelyCZ,
pigletfly,
whitewindmills,
yanfeng1992 and
zhzhuang-zju
August 10, 2026 05:01
karmada-bot
added
the
size/L
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces additional informer-cache memory optimization by transforming member-cluster objects so that resources not mapped to an existing Work retain only apiVersion, kind, and metadata, while resources that still map to a Work remain intact. It builds on prior cache transform support by wiring transform functions through the dynamic informer manager constructors and updating call sites accordingly.
Changes:
- Add
RetainMetadataFieldsand a work-status-specific transform (NewWorkStatusTransformFunc) to strip non-metadata fields for objects not associated with an existingWork. - Extend dynamic informer managers to accept a
cache.TransformFuncand ensure transforms are set once per informer. - Update controller and test instantiations of informer managers to pass
StripUnusedFields(and for work-status, the new conditional transform).
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/util/testing/mock_manager.go | Update fake single-cluster informer manager construction to pass StripUnusedFields. |
| pkg/util/helper/cache_test.go | Update multi/single-cluster informer manager test setup to pass StripUnusedFields. |
| pkg/util/helper/binding_test.go | Update informer manager creation in binding helper tests to pass StripUnusedFields. |
| pkg/util/fedinformer/transform.go | Add RetainMetadataFields helper to keep only type+metadata for unstructured objects. |
| pkg/util/fedinformer/transform_test.go | Add unit test coverage for RetainMetadataFields. |
| pkg/util/fedinformer/genericmanager/single-cluster-manager.go | Add transform plumbing and set-transform-once behavior to dynamic single-cluster manager. |
| pkg/util/fedinformer/genericmanager/multi-cluster-manager.go | Add transform plumbing and default singleton initialization using StripUnusedFields. |
| pkg/search/controllers_test.go | Update controller test wiring to pass StripUnusedFields into informer manager. |
| pkg/resourceinterpreter/customized/declarative/configmanager/manager_test.go | Update test to construct single-cluster manager with StripUnusedFields. |
| pkg/karmadactl/promote/promote.go | Ensure promote path uses single-cluster manager with StripUnusedFields. |
| pkg/estimator/server/server.go | Ensure estimator uses single-cluster manager with StripUnusedFields. |
| pkg/detector/preemption_test.go | Update detector tests to pass StripUnusedFields into manager. |
| pkg/detector/policy_test.go | Update detector tests to pass StripUnusedFields into manager. |
| pkg/dependenciesdistributor/dependencies_distributor_test.go | Update distributor tests to pass StripUnusedFields into manager. |
| pkg/controllers/status/work_status_controller.go | Add conditional transform that retains full objects only when mapped to an existing Work. |
| pkg/controllers/status/work_status_controller_test.go | Add unit tests for the new work-status transform; update manager construction to pass StripUnusedFields. |
| pkg/controllers/status/rb_status_controller_test.go | Update RB status controller test manager construction to pass StripUnusedFields. |
| pkg/controllers/status/crb_status_controller_test.go | Update CRB status controller test manager construction to pass StripUnusedFields. |
| pkg/controllers/binding/cluster_resource_binding_controller_test.go | Update binding controller test manager construction to pass StripUnusedFields. |
| pkg/controllers/binding/binding_controller_test.go | Update binding controller test manager construction to pass StripUnusedFields. |
| cmd/controller-manager/app/controllermanager.go | Use a dedicated multi-cluster manager for work-status with the new conditional transform; update other single-cluster manager creation to pass StripUnusedFields. |
| cmd/agent/app/agent.go | Use a dedicated multi-cluster manager for work-status with the new conditional transform; update control-plane single-cluster manager creation to pass StripUnusedFields. |
Suppressed comments (3)
pkg/controllers/status/work_status_controller.go:129
mapsToWorkusescontext.Background()for cache reads. This prevents cancellation during shutdown and can delay informer stopping under load. Prefer threading a context from the controller (e.g., passctxintoNewWorkStatusTransformFuncand close over it) so the transform can honor cancellation/timeouts.
work := &workv1alpha1.Work{}
if err := controlPlaneClient.Get(context.Background(), client.ObjectKey{Namespace: workNamespace, Name: workName}, work); err != nil {
if !apierrors.IsNotFound(err) {
return false, err
}
return false, nil
}
return true, nil
cmd/agent/app/agent.go:370
work-status-controlleris switched from the sharedgenericmanager.GetInstance()to a newMultiClusterInformerManager. In the same process, other controllers (notably the execution controller) still use the singleton, so this change can duplicate dynamic informers/caches and member-cluster watches, potentially offsetting the intended memory savings and increasing load on member clusters.
workStatusController := &status.WorkStatusController{
Client: ctx.Mgr.GetClient(),
EventRecorder: ctx.Mgr.GetEventRecorderFor(status.WorkStatusControllerName), //nolint:staticcheck // Note: GetEventRecorderFor is deprecated in controller-runtime v0.23.0 in favor of GetEventRecorder. This changes event API from v1 events to events.k8s.io. We need to migrate carefully, especially considering the impact on users and RBAC permission changes in installation/deployment tools.
RESTMapper: ctx.Mgr.GetRESTMapper(),
InformerManager: genericmanager.NewMultiClusterInformerManager(ctx.Context, status.NewWorkStatusTransformFunc(ctx.Mgr.GetClient())),
Context: ctx.Context,
ObjectWatcher: ctx.ObjectWatcher,
ClusterDynamicClientSetFunc: util.NewClusterDynamicClientSetForAgent,
ClusterCacheSyncTimeout: ctx.Opts.ClusterCacheSyncTimeout,
cmd/controller-manager/app/controllermanager.go:485
work-status-controlleris switched from the sharedgenericmanager.GetInstance()to a newMultiClusterInformerManager. Since the execution controller in this binary still uses the singleton informer manager, this can result in duplicated dynamic informers/caches for the same GVRs/clusters and increase both memory usage and API watch/list traffic.
func startWorkStatusController(ctx controllerscontext.Context) (enabled bool, err error) {
opts := ctx.Opts
workStatusController := &status.WorkStatusController{
Client: ctx.Mgr.GetClient(),
EventRecorder: ctx.Mgr.GetEventRecorderFor(status.WorkStatusControllerName), //nolint:staticcheck // Note: GetEventRecorderFor is deprecated in controller-runtime v0.23.0 in favor of GetEventRecorder. This changes event API from v1 events to events.k8s.io. We need to migrate carefully, especially considering the impact on users and RBAC permission changes in installation/deployment tools.
RESTMapper: ctx.Mgr.GetRESTMapper(),
InformerManager: genericmanager.NewMultiClusterInformerManager(ctx.Context, status.NewWorkStatusTransformFunc(ctx.Mgr.GetClient())),
Context: ctx.Context,
ObjectWatcher: ctx.ObjectWatcher,
WorkPredicateFunc: helper.WorkWithinPushClusterPredicate(ctx.Mgr),
ClusterDynamicClientSetFunc: util.NewClusterDynamicClientSet,
ClusterClientOption: ctx.ClusterClientOption,
ClusterCacheSyncTimeout: opts.ClusterCacheSyncTimeout,
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+167
to
+175
| func (s *singleClusterInformerManagerImpl) setTransformOnce(resource schema.GroupVersionResource, informer cache.SharedIndexInformer) { | ||
| if s.transformFunc == nil { | ||
| return | ||
| } | ||
| if _, loaded := s.transformedInformers.LoadOrStore(resource, struct{}{}); loaded { | ||
| return | ||
| } | ||
| _ = informer.SetTransform(s.transformFunc) | ||
| } |
Comment on lines
+47
to
+58
| // RetainMetadataFields keeps only type and object metadata on informer objects. | ||
| func RetainMetadataFields(obj any) (any, error) { | ||
| if tombstone, ok := obj.(cache.DeletedFinalStateUnknown); ok { | ||
| obj = tombstone.Obj | ||
| } | ||
|
|
||
| unstructuredObj, ok := obj.(*unstructured.Unstructured) | ||
| if !ok { | ||
| return obj, nil | ||
| } | ||
| return metadataOnlyUnstructured(unstructuredObj), nil | ||
| } |
Comment on lines
+99
to
+107
| mappedToWork, err := mapsToWork(controlPlaneClient, obj) | ||
| if err != nil { | ||
| return obj, err | ||
| } | ||
| if mappedToWork { | ||
| return obj, nil | ||
| } | ||
| return fedinformer.RetainMetadataFields(obj) | ||
| } |
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7817 +/- ##
==========================================
- Coverage 42.09% 42.09% -0.01%
==========================================
Files 879 879
Lines 54852 54906 +54
==========================================
+ Hits 23089 23111 +22
- Misses 30020 30043 +23
- Partials 1743 1752 +9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zach593
changed the title
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.
What type of PR is this?
/kind feature
What this PR does / why we need it:
The member-cluster resource informer cache is directly used by both the work-status controller and the execution controller.
Currently, complete objects are stored in the cache, including resources that are not mapped to an existing
Work. Retaining thespec,status, and other non-metadata fields of these resources is unnecessary:Work.Workmanifest. The absence of non-metadata fields in the cached object does not prevent the resource from being created or updated.This PR adds an informer transform that:
Work.apiVersion,kind, and object metadata for resources that are not mapped to an existingWork.This avoids retaining unused object content and reduces the memory consumed by the member-cluster resource cache.
Memory usage test:
In a fresh Karmada environment, only one Deployment was initially propagated to two member clusters to ensure that a Deployment informer was established for each cluster. Then, 40,000 Deployments were created in each member cluster, resulting in 80,000 test objects in total.
Under the same test conditions, the memory usage was:
Compared with the current master, this PR reduces the overall memory usage by approximately 39.2%. It also provides an additional reduction of approximately 18.7% on top of #7807.
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
This PR is based on #7807 which adds transform function support to the dynamic informer manager. That prerequisite PR is expected to be merged first.
Test report:
go test ./pkg/controllers/status ./pkg/util/fedinformer -count=1Does this PR introduce a user-facing change?: