From 707e4422c2eab615a2f323043eeb1cec6692719b Mon Sep 17 00:00:00 2001 From: zach593 Date: Mon, 24 Feb 2025 13:53:25 +0800 Subject: [PATCH] add work mutating in ctrlutil.CreateOrUpdateWork() Signed-off-by: zach593 --- pkg/controllers/ctrlutil/work.go | 48 ++++++++++++------- .../endpointslice_dispatch_controller_test.go | 4 +- pkg/util/helper/work.go | 11 +++++ pkg/webhook/work/mutating.go | 15 ++---- 4 files changed, 49 insertions(+), 29 deletions(-) diff --git a/pkg/controllers/ctrlutil/work.go b/pkg/controllers/ctrlutil/work.go index b35d69598..2ae90367d 100644 --- a/pkg/controllers/ctrlutil/work.go +++ b/pkg/controllers/ctrlutil/work.go @@ -31,13 +31,15 @@ import ( workv1alpha1 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha1" workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" + "github.com/karmada-io/karmada/pkg/resourceinterpreter/default/native/prune" "github.com/karmada-io/karmada/pkg/util" + "github.com/karmada-io/karmada/pkg/util/helper" ) // CreateOrUpdateWork creates a Work object if not exist, or updates if it already exists. -func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta metav1.ObjectMeta, resource *unstructured.Unstructured, options ...WorkOption) error { +func CreateOrUpdateWork(ctx context.Context, c client.Client, workMeta metav1.ObjectMeta, resource *unstructured.Unstructured, options ...WorkOption) error { + resource = resource.DeepCopy() if workMeta.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed { - resource = resource.DeepCopy() // set labels util.MergeLabel(resource, util.ManagedByKarmadaLabel, util.ManagedByKarmadaLabelValue) // set annotations @@ -48,26 +50,16 @@ func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta meta util.MergeAnnotation(resource, workv1alpha2.ResourceConflictResolutionAnnotation, conflictResolution) } } - - workloadJSON, err := json.Marshal(resource) + // Do the same thing as the mutating webhook does, remove the irrelevant fields for the resource. + // This is to avoid unnecessary updates to the Work object, especially when controller starts. + err := prune.RemoveIrrelevantFields(resource, prune.RemoveJobTTLSeconds) if err != nil { - klog.Errorf("Failed to marshal workload(%s/%s), error: %v", resource.GetNamespace(), resource.GetName(), err) + klog.Errorf("Failed to prune irrelevant fields for resource %s/%s. Error: %v", resource.GetNamespace(), resource.GetName(), err) return err } work := &workv1alpha1.Work{ ObjectMeta: workMeta, - Spec: workv1alpha1.WorkSpec{ - Workload: workv1alpha1.WorkloadTemplate{ - Manifests: []workv1alpha1.Manifest{ - { - RawExtension: runtime.RawExtension{ - Raw: workloadJSON, - }, - }, - }, - }, - }, } applyWorkOptions(work, options) @@ -75,15 +67,35 @@ func CreateOrUpdateWork(ctx context.Context, client client.Client, workMeta meta runtimeObject := work.DeepCopy() var operationResult controllerutil.OperationResult err = retry.RetryOnConflict(retry.DefaultRetry, func() (err error) { - operationResult, err = controllerutil.CreateOrUpdate(ctx, client, runtimeObject, func() error { + operationResult, err = controllerutil.CreateOrUpdate(ctx, c, runtimeObject, func() error { if !runtimeObject.DeletionTimestamp.IsZero() { return fmt.Errorf("work %s/%s is being deleted", runtimeObject.GetNamespace(), runtimeObject.GetName()) } - runtimeObject.Spec = work.Spec runtimeObject.Labels = util.DedupeAndMergeLabels(runtimeObject.Labels, work.Labels) runtimeObject.Annotations = util.DedupeAndMergeAnnotations(runtimeObject.Annotations, work.Annotations) runtimeObject.Finalizers = work.Finalizers + runtimeObject.Spec = work.Spec + + // Do the same thing as the mutating webhook does, add the permanent ID to workload if not exist, + // This is to avoid unnecessary updates to the Work object, especially when controller starts. + if runtimeObject.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed { + helper.SetLabelsAndAnnotationsForWorkload(resource, runtimeObject) + } + workloadJSON, err := json.Marshal(resource) + if err != nil { + klog.Errorf("Failed to marshal workload(%s/%s), error: %v", resource.GetNamespace(), resource.GetName(), err) + return err + } + runtimeObject.Spec.Workload = workv1alpha1.WorkloadTemplate{ + Manifests: []workv1alpha1.Manifest{ + { + RawExtension: runtime.RawExtension{ + Raw: workloadJSON, + }, + }, + }, + } return nil }) return err diff --git a/pkg/controllers/multiclusterservice/endpointslice_dispatch_controller_test.go b/pkg/controllers/multiclusterservice/endpointslice_dispatch_controller_test.go index ec12fff79..6a97113fe 100644 --- a/pkg/controllers/multiclusterservice/endpointslice_dispatch_controller_test.go +++ b/pkg/controllers/multiclusterservice/endpointslice_dispatch_controller_test.go @@ -604,7 +604,9 @@ func TestEnsureEndpointSliceWork(t *testing.T) { "endpointslice.karmada.io/provision-cluster": "provider", "work.karmada.io/name": "test-work", "work.karmada.io/namespace": "karmada-es-consumer", - "resourcetemplate.karmada.io/uid": "" + "resourcetemplate.karmada.io/uid": "", + "resourcetemplate.karmada.io/managed-annotations": "endpointslice.karmada.io/provision-cluster,resourcetemplate.karmada.io/managed-annotations,resourcetemplate.karmada.io/managed-labels,resourcetemplate.karmada.io/uid,work.karmada.io/name,work.karmada.io/namespace", + "resourcetemplate.karmada.io/managed-labels":"endpointslice.kubernetes.io/managed-by,karmada.io/managed,kubernetes.io/service-name" } }, "endpoints": [ diff --git a/pkg/util/helper/work.go b/pkg/util/helper/work.go index 2d67630bd..5e54a86f9 100644 --- a/pkg/util/helper/work.go +++ b/pkg/util/helper/work.go @@ -111,3 +111,14 @@ func IsWorkContains(manifests []workv1alpha1.Manifest, targetResource schema.Gro func IsWorkSuspendDispatching(work *workv1alpha1.Work) bool { return ptr.Deref(work.Spec.SuspendDispatching, false) } + +// SetLabelsAndAnnotationsForWorkload sets the associated work object labels and annotations for workload. +func SetLabelsAndAnnotationsForWorkload(workload *unstructured.Unstructured, work *workv1alpha1.Work) { + util.RecordManagedAnnotations(workload) + if work.Labels[workv1alpha2.WorkPermanentIDLabel] != "" { + workload.SetLabels(util.DedupeAndMergeLabels(workload.GetLabels(), map[string]string{ + workv1alpha2.WorkPermanentIDLabel: work.Labels[workv1alpha2.WorkPermanentIDLabel], + })) + } + util.RecordManagedLabels(workload) +} diff --git a/pkg/webhook/work/mutating.go b/pkg/webhook/work/mutating.go index bc0b72df4..5ef079a29 100644 --- a/pkg/webhook/work/mutating.go +++ b/pkg/webhook/work/mutating.go @@ -31,6 +31,7 @@ import ( workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" "github.com/karmada-io/karmada/pkg/resourceinterpreter/default/native/prune" "github.com/karmada-io/karmada/pkg/util" + "github.com/karmada-io/karmada/pkg/util/helper" ) // MutatingAdmission mutates API request if necessary. @@ -42,6 +43,9 @@ type MutatingAdmission struct { var _ admission.Handler = &MutatingAdmission{} // Handle yields a response to an AdmissionRequest. +// This function could affect the informer cache of controller-manager. +// In controller-manager, redundant updates to Work are avoided as much as possible, and changes to informer cache will affect this behavior. +// Therefore, if someone modify this function, please ensure that the relevant logic in controller-manager also be updated. func (a *MutatingAdmission) Handle(_ context.Context, req admission.Request) admission.Response { work := &workv1alpha1.Work{} @@ -73,7 +77,7 @@ func (a *MutatingAdmission) Handle(_ context.Context, req admission.Request) adm // Skip label/annotate the workload of Work that is not intended to be propagated. if work.Labels[util.PropagationInstruction] != util.PropagationInstructionSuppressed { - setLabelsAndAnnotationsForWorkload(workloadObj, work) + helper.SetLabelsAndAnnotationsForWorkload(workloadObj, work) } workloadJSON, err := workloadObj.MarshalJSON() @@ -92,12 +96,3 @@ func (a *MutatingAdmission) Handle(_ context.Context, req admission.Request) adm return admission.PatchResponseFromRaw(req.Object.Raw, marshaledBytes) } - -// setLabelsAndAnnotationsForWorkload sets the associated work object labels and annotations for workload. -func setLabelsAndAnnotationsForWorkload(workload *unstructured.Unstructured, work *workv1alpha1.Work) { - util.RecordManagedAnnotations(workload) - workload.SetLabels(util.DedupeAndMergeLabels(workload.GetLabels(), map[string]string{ - workv1alpha2.WorkPermanentIDLabel: work.Labels[workv1alpha2.WorkPermanentIDLabel], - })) - util.RecordManagedLabels(workload) -}