diff --git a/pkg/controllers/status/common.go b/pkg/controllers/status/common.go index 75afc6e3cf7a..9a78a688f2d1 100644 --- a/pkg/controllers/status/common.go +++ b/pkg/controllers/status/common.go @@ -20,12 +20,15 @@ import ( "context" "reflect" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/api/meta" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime/schema" - "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/dynamic" + "k8s.io/client-go/tools/record" + "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/event" @@ -34,8 +37,8 @@ import ( configv1alpha1 "github.com/karmada-io/karmada/pkg/apis/config/v1alpha1" 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/events" "github.com/karmada-io/karmada/pkg/resourceinterpreter" - "github.com/karmada-io/karmada/pkg/util/helper" "github.com/karmada-io/karmada/pkg/util/restmapper" ) @@ -105,43 +108,67 @@ func updateResourceStatus( dynamicClient dynamic.Interface, restMapper meta.RESTMapper, interpreter resourceinterpreter.ResourceInterpreter, - resource *unstructured.Unstructured, + eventRecorder record.EventRecorder, + objRef workv1alpha2.ObjectReference, bindingStatus workv1alpha2.ResourceBindingStatus, ) error { - gvr, err := restmapper.GetGroupVersionResource(restMapper, schema.FromAPIVersionAndKind(resource.GetAPIVersion(), resource.GetKind())) + gvr, err := restmapper.GetGroupVersionResource(restMapper, schema.FromAPIVersionAndKind(objRef.APIVersion, objRef.Kind)) if err != nil { - klog.Errorf("Failed to get GVR from GVK(%s/%s), Error: %v", resource.GetAPIVersion(), resource.GetKind(), err) + klog.Errorf("Failed to get GVR from GVK(%s/%s), Error: %v", objRef.APIVersion, objRef.Kind, err) return err } - if !interpreter.HookEnabled(resource.GroupVersionKind(), configv1alpha1.InterpreterOperationAggregateStatus) { + gvk := schema.GroupVersionKind{Group: gvr.Group, Version: gvr.Version, Kind: objRef.Kind} + if !interpreter.HookEnabled(gvk, configv1alpha1.InterpreterOperationAggregateStatus) { return nil } - newObj, err := interpreter.AggregateStatus(resource, bindingStatus.AggregatedStatus) - if err != nil { - klog.Errorf("Failed to aggregate status for resource(%s/%s/%s, Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err) - return err - } - oldStatus, _, _ := unstructured.NestedFieldNoCopy(resource.Object, "status") - newStatus, _, _ := unstructured.NestedFieldNoCopy(newObj.Object, "status") - if reflect.DeepEqual(oldStatus, newStatus) { - klog.V(3).Infof("Ignore update resource(%s/%s/%s) status as up to date.", gvr, resource.GetNamespace(), resource.GetName()) - return nil - } + var resource *unstructured.Unstructured + if err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + // Fetch resource template from karmada-apiserver instead of informer cache, to avoid retry due to + // resource conflict which often happens, especially with a huge amount of resource templates and + // the informer cache doesn't sync quickly enough. + // For more details refer to https://github.com/karmada-io/karmada/issues/5285. + resource, err = dynamicClient.Resource(gvr).Namespace(objRef.Namespace).Get(ctx, objRef.Name, metav1.GetOptions{}) + if err != nil { + if apierrors.IsNotFound(err) { + // It might happen when the resource template has been removed but the garbage collector hasn't removed + // the ResourceBinding which dependent on resource template. + // So, just return without retry(requeue) would save unnecessary loop. + return nil + } + klog.Errorf("Failed to fetch resource template(%s/%s/%s), Error: %v.", gvr, objRef.Namespace, objRef.Name, err) + return err + } - patchBytes, err := helper.GenReplaceFieldJSONPatch("/status", oldStatus, newStatus) - if err != nil { - klog.Errorf("Failed to gen patch bytes for resource(%s/%s/%s, Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err) - return err - } + newObj, err := interpreter.AggregateStatus(resource, bindingStatus.AggregatedStatus) + if err != nil { + klog.Errorf("Failed to aggregate status for resource template(%s/%s/%s), Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err) + return err + } - _, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()). - Patch(ctx, resource.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}, "status") - if err != nil { - klog.Errorf("Failed to update resource(%s/%s/%s), Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err) + oldStatus, _, _ := unstructured.NestedFieldNoCopy(resource.Object, "status") + newStatus, _, _ := unstructured.NestedFieldNoCopy(newObj.Object, "status") + if reflect.DeepEqual(oldStatus, newStatus) { + klog.V(3).Infof("Ignore update resource(%s/%s/%s) status as up to date.", gvr, resource.GetNamespace(), resource.GetName()) + return nil + } + + if _, err = dynamicClient.Resource(gvr).Namespace(resource.GetNamespace()).UpdateStatus(ctx, newObj, metav1.UpdateOptions{}); err != nil { + klog.Errorf("Failed to update resource(%s/%s/%s), Error: %v", gvr, resource.GetNamespace(), resource.GetName(), err) + return err + } + + eventRecorder.Event(resource, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, "Update Resource with AggregatedStatus successfully.") + klog.V(3).Infof("Update resource(%s/%s/%s) status successfully.", gvr, resource.GetNamespace(), resource.GetName()) + + return nil + }); err != nil { + if resource != nil { + eventRecorder.Event(resource, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error()) + } return err } - klog.V(3).Infof("Update resource(%s/%s/%s) status successfully.", gvr, resource.GetNamespace(), resource.GetName()) + return nil } diff --git a/pkg/controllers/status/crb_status_controller.go b/pkg/controllers/status/crb_status_controller.go index b41dbad9f323..79a11fb76cbc 100644 --- a/pkg/controllers/status/crb_status_controller.go +++ b/pkg/controllers/status/crb_status_controller.go @@ -109,27 +109,14 @@ func (c *CRBStatusController) SetupWithManager(mgr controllerruntime.Manager) er } func (c *CRBStatusController) syncBindingStatus(ctx context.Context, binding *workv1alpha2.ClusterResourceBinding) error { - resource, err := helper.FetchResourceTemplate(ctx, c.DynamicClient, c.InformerManager, c.RESTMapper, binding.Spec.Resource) - if err != nil { - if apierrors.IsNotFound(err) { - // It might happen when the resource template has been removed but the garbage collector hasn't removed - // the ResourceBinding which dependent on resource template. - // So, just return without retry(requeue) would save unnecessary loop. - return nil - } - klog.Errorf("Failed to fetch workload for clusterResourceBinding(%s). Error: %v", - binding.GetName(), err) - return err - } - - err = helper.AggregateClusterResourceBindingWorkStatus(ctx, c.Client, binding, resource, c.EventRecorder) + err := helper.AggregateClusterResourceBindingWorkStatus(ctx, c.Client, binding, c.EventRecorder) if err != nil { klog.Errorf("Failed to aggregate workStatues to clusterResourceBinding(%s), Error: %v", binding.Name, err) return err } - err = updateResourceStatus(ctx, c.DynamicClient, c.RESTMapper, c.ResourceInterpreter, resource, binding.Status) + err = updateResourceStatus(ctx, c.DynamicClient, c.RESTMapper, c.ResourceInterpreter, c.EventRecorder, binding.Spec.Resource, binding.Status) if err != nil { return err } diff --git a/pkg/controllers/status/rb_status_controller.go b/pkg/controllers/status/rb_status_controller.go index 219fa05e3ad1..d5f1b8989732 100644 --- a/pkg/controllers/status/rb_status_controller.go +++ b/pkg/controllers/status/rb_status_controller.go @@ -111,29 +111,17 @@ func (c *RBStatusController) SetupWithManager(mgr controllerruntime.Manager) err } func (c *RBStatusController) syncBindingStatus(ctx context.Context, binding *workv1alpha2.ResourceBinding) error { - resourceTemplate, err := helper.FetchResourceTemplate(ctx, c.DynamicClient, c.InformerManager, c.RESTMapper, binding.Spec.Resource) + err := helper.AggregateResourceBindingWorkStatus(ctx, c.Client, binding, c.EventRecorder) if err != nil { - if apierrors.IsNotFound(err) { - // It might happen when the resource template has been removed but the garbage collector hasn't removed - // the ResourceBinding which dependent on resource template. - // So, just return without retry(requeue) would save unnecessary loop. - return nil - } - klog.Errorf("Failed to fetch workload for resourceBinding(%s/%s). Error: %v.", - binding.GetNamespace(), binding.GetName(), err) - return err - } - - err = helper.AggregateResourceBindingWorkStatus(ctx, c.Client, binding, resourceTemplate, c.EventRecorder) - if err != nil { - klog.Errorf("Failed to aggregate workStatues to resourceBinding(%s/%s), Error: %v", + klog.Errorf("Failed to aggregate workStatus to resourceBinding(%s/%s), Error: %v", binding.Namespace, binding.Name, err) return err } - err = updateResourceStatus(ctx, c.DynamicClient, c.RESTMapper, c.ResourceInterpreter, resourceTemplate, binding.Status) + err = updateResourceStatus(ctx, c.DynamicClient, c.RESTMapper, c.ResourceInterpreter, c.EventRecorder, binding.Spec.Resource, binding.Status) if err != nil { return err } + return nil } diff --git a/pkg/controllers/status/work_status_controller.go b/pkg/controllers/status/work_status_controller.go index 51cbdd6beec7..671128e856a5 100644 --- a/pkg/controllers/status/work_status_controller.go +++ b/pkg/controllers/status/work_status_controller.go @@ -390,7 +390,9 @@ func (c *WorkStatusController) reflectStatus(ctx context.Context, work *workv1al } func (c *WorkStatusController) buildStatusIdentifier(work *workv1alpha1.Work, clusterObj *unstructured.Unstructured) (*workv1alpha1.ResourceIdentifier, error) { - ordinal, err := helper.GetManifestIndex(work.Spec.Workload.Manifests, clusterObj) + manifestRef := helper.ManifestReference{APIVersion: clusterObj.GetAPIVersion(), Kind: clusterObj.GetKind(), + Namespace: clusterObj.GetNamespace(), Name: clusterObj.GetName()} + ordinal, err := helper.GetManifestIndex(work.Spec.Workload.Manifests, &manifestRef) if err != nil { return nil, err } diff --git a/pkg/util/helper/patch.go b/pkg/util/helper/patch.go index 6cd655e646ba..89f9f798376a 100644 --- a/pkg/util/helper/patch.go +++ b/pkg/util/helper/patch.go @@ -19,28 +19,10 @@ package helper import ( "encoding/json" "fmt" - "reflect" jsonpatch "github.com/evanphx/json-patch/v5" ) -// RFC6902 JSONPatch operations -const ( - JSONPatchOPAdd = "add" - JSONPatchOPReplace = "replace" - JSONPatchOPRemove = "remove" - JSONPatchOPMove = "move" - JSONPatchOPCopy = "copy" - JSONPatchOPTest = "test" -) - -type jsonPatch struct { - OP string `json:"op"` - From string `json:"from,omitempty"` - Path string `json:"path"` - Value interface{} `json:"value,omitempty"` -} - // GenMergePatch will return a merge patch document capable of converting the // original object to the modified object. // The merge patch format is primarily intended for use with the HTTP PATCH method @@ -84,40 +66,3 @@ func GenFieldMergePatch(fieldName string, originField interface{}, modifiedField patchBytes = append([]byte(`{"`+fieldName+`":`), patchBytes...) return patchBytes, nil } - -// GenReplaceFieldJSONPatch returns the RFC6902 JSONPatch array as []byte, which is used to simply -// add/replace/delete certain JSON **Object** field. -func GenReplaceFieldJSONPatch(path string, originalFieldValue, newFieldValue interface{}) ([]byte, error) { - if reflect.DeepEqual(originalFieldValue, newFieldValue) { - return nil, nil - } - if newFieldValue == nil { - return GenJSONPatch(JSONPatchOPRemove, "", path, nil) - } - // The implementation of "add" and "replace" for JSON objects is actually the same - // in "github.com/evanphx/json-patch/v5", which is used by Karmada and K8s. - // We implemented it here just to follow the RFC6902. - if originalFieldValue == nil { - return GenJSONPatch(JSONPatchOPAdd, "", path, newFieldValue) - } - return GenJSONPatch(JSONPatchOPReplace, "", path, newFieldValue) -} - -// GenJSONPatch return JSONPatch array as []byte according to RFC6902 -func GenJSONPatch(op, from, path string, value interface{}) ([]byte, error) { - jp := jsonPatch{ - OP: op, - Path: path, - } - switch op { - case JSONPatchOPAdd, JSONPatchOPReplace, JSONPatchOPTest: - jp.Value = value - case JSONPatchOPMove, JSONPatchOPCopy: - jp.From = from - case JSONPatchOPRemove: - default: - return nil, fmt.Errorf("unrecognized JSONPatch OP: %s", op) - } - - return json.Marshal([]jsonPatch{jp}) -} diff --git a/pkg/util/helper/patch_test.go b/pkg/util/helper/patch_test.go index 13ec3f6db4b9..9b4572b93f67 100644 --- a/pkg/util/helper/patch_test.go +++ b/pkg/util/helper/patch_test.go @@ -17,12 +17,8 @@ limitations under the License. package helper import ( - "encoding/json" - "fmt" - "math" "testing" - appsv1 "k8s.io/api/apps/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" workv1alpha2 "github.com/karmada-io/karmada/pkg/apis/work/v1alpha2" @@ -126,277 +122,3 @@ func TestGenMergePatch(t *testing.T) { }) } } - -func TestGenJSONPatch(t *testing.T) { - type args struct { - op string - from string - path string - value interface{} - } - tests := []struct { - name string - args args - want string - wantErr bool - }{ - { - name: "add object field", - args: args{ - op: "add", - path: "/abc", - value: 1, - }, - want: `[{"op":"add","path":"/abc","value":1}]`, - wantErr: false, - }, - { - name: "replace object field", - args: args{ - op: "replace", - path: "/abc", - value: 1, - }, - want: `[{"op":"replace","path":"/abc","value":1}]`, - wantErr: false, - }, - { - name: "remove object field, redundant args will be ignored", - args: args{ - op: "remove", - from: "123", - path: "/abc", - value: 1, - }, - want: `[{"op":"remove","path":"/abc"}]`, - wantErr: false, - }, - { - name: "move object field", - args: args{ - op: "move", - from: "/abc", - path: "/123", - }, - want: `[{"op":"move","from":"/abc","path":"/123"}]`, - wantErr: false, - }, - { - name: "copy object field, redundant array value will be ignored", - args: args{ - op: "copy", - from: "/123", - path: "/abc", - value: []interface{}{1, "a", false, 4.5}, - }, - want: `[{"op":"copy","from":"/123","path":"/abc"}]`, - wantErr: false, - }, - { - name: "replace object field, input string typed number", - args: args{ - op: "replace", - path: "/abc", - value: "1", - }, - want: `[{"op":"replace","path":"/abc","value":"1"}]`, - wantErr: false, - }, - { - name: "replace object field, input invalid type", - args: args{ - op: "replace", - path: "/abc", - value: make(chan int), - }, - want: "", - wantErr: true, - }, - { - name: "replace object field, input invalid value", - args: args{ - op: "replace", - path: "/abc", - value: math.Inf(1), - }, - want: "", - wantErr: true, - }, - { - name: "replace object field, input struct value", - args: args{ - op: "replace", - path: "/abc", - value: struct { - A string - B int - C float64 - D bool - }{"a", 1, 1.2, true}, - }, - want: `[{"op":"replace","path":"/abc","value":{"A":"a","B":1,"C":1.2,"D":true}}]`, - wantErr: false, - }, - { - name: "test object field, input array value", - args: args{ - op: "test", - path: "/abc", - value: []interface{}{1, "a", false, 4.5}, - }, - want: `[{"op":"test","path":"/abc","value":[1,"a",false,4.5]}]`, - wantErr: false, - }, - { - name: "move object field, input invalid path, but we won't verify it", - args: args{ - op: "move", - from: "123", - path: "abc", - }, - want: `[{"op":"move","from":"123","path":"abc"}]`, - wantErr: false, - }, - { - name: "input invalid op", - args: args{ - op: "whatever", - path: "/abc", - }, - want: "", - wantErr: true, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - patchBytes, err := GenJSONPatch(tt.args.op, tt.args.from, tt.args.path, tt.args.value) - if tt.wantErr != (err != nil) { - t.Errorf("wantErr: %v, but got err: %v", tt.wantErr, err) - } - if tt.want != string(patchBytes) { - t.Errorf("want: %s, but got: %s", tt.want, patchBytes) - } - }) - } -} - -func TestGenReplaceFieldJSONPatch(t *testing.T) { - originalObj := &appsv1.Deployment{Status: appsv1.DeploymentStatus{ - ObservedGeneration: 1, - Replicas: 1, - UpdatedReplicas: 1, - ReadyReplicas: 1, - AvailableReplicas: 1, - }} - newObj := originalObj.DeepCopy() - newObj.Status = appsv1.DeploymentStatus{ - ObservedGeneration: 2, - Replicas: 2, - UpdatedReplicas: 2, - ReadyReplicas: 2, - AvailableReplicas: 2, - } - newStatusJSON, _ := json.Marshal(newObj.Status) - pathStatus := "/status" - type args struct { - path string - originalFieldValue interface{} - newFieldValue interface{} - } - tests := []struct { - name string - args args - want func() ([]byte, error) - }{ - { - name: "return nil when no patch is needed", - args: args{ - path: pathStatus, - originalFieldValue: originalObj.Status, - newFieldValue: originalObj.Status, - }, - want: func() ([]byte, error) { - return nil, nil - }, - }, - { - name: "return add JSONPatch when field in original obj is nil", - args: args{ - path: pathStatus, - originalFieldValue: nil, - newFieldValue: newObj.Status, - }, - want: func() ([]byte, error) { - return GenJSONPatch(JSONPatchOPAdd, "", pathStatus, newObj.Status) - }, - }, - { - name: "e2e return add JSONPatch when field in original obj is nil", - args: args{ - path: pathStatus, - originalFieldValue: nil, - newFieldValue: newObj.Status, - }, - want: func() ([]byte, error) { - return []byte(fmt.Sprintf(`[{"op":"add","path":"%s","value":%s}]`, pathStatus, newStatusJSON)), nil - }, - }, - { - name: "return replace JSONPatch when field in original obj in non-nil, whatever what's in the original field", - args: args{ - path: pathStatus, - originalFieldValue: originalObj.Status, - newFieldValue: newObj.Status, - }, - want: func() ([]byte, error) { - return GenJSONPatch(JSONPatchOPReplace, "", pathStatus, newObj.Status) - }, - }, - { - name: "e2e return replace JSONPatch when field in original obj in non-nil, whatever what's in the original field", - args: args{ - path: pathStatus, - originalFieldValue: originalObj.Status, - newFieldValue: newObj.Status, - }, - want: func() ([]byte, error) { - return []byte(fmt.Sprintf(`[{"op":"replace","path":"%s","value":%s}]`, pathStatus, newStatusJSON)), nil - }, - }, - { - name: "return remove JSONPatch when field in new obj is nil", - args: args{ - path: pathStatus, - originalFieldValue: originalObj.Status, - newFieldValue: nil, - }, - want: func() ([]byte, error) { - return GenJSONPatch(JSONPatchOPRemove, "", pathStatus, nil) - }, - }, - { - name: "e2e return remove JSONPatch when field in new obj is nil", - args: args{ - path: pathStatus, - originalFieldValue: originalObj.Status, - newFieldValue: nil, - }, - want: func() ([]byte, error) { - return []byte(fmt.Sprintf(`[{"op":"remove","path":"%s"}]`, pathStatus)), nil - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got, err := GenReplaceFieldJSONPatch(tt.args.path, tt.args.originalFieldValue, tt.args.newFieldValue) - want, wantErr := tt.want() - - if fmt.Sprint(wantErr) != fmt.Sprint(err) { - t.Errorf("wantErr: %s, but got err: %s", fmt.Sprint(wantErr), fmt.Sprint(err)) - } - if string(want) != string(got) { - t.Errorf("\nwant: %s\nbut got: %s\n", want, got) - } - }) - } -} diff --git a/pkg/util/helper/workstatus.go b/pkg/util/helper/workstatus.go index 097313b469e8..1184a11e266f 100644 --- a/pkg/util/helper/workstatus.go +++ b/pkg/util/helper/workstatus.go @@ -59,7 +59,6 @@ func AggregateResourceBindingWorkStatus( ctx context.Context, c client.Client, binding *workv1alpha2.ResourceBinding, - resourceTemplate *unstructured.Unstructured, eventRecorder record.EventRecorder, ) error { workList, err := GetWorksByBindingID(ctx, c, binding.Labels[workv1alpha2.ResourceBindingPermanentIDLabel], true) @@ -67,7 +66,7 @@ func AggregateResourceBindingWorkStatus( return err } - aggregatedStatuses, err := assembleWorkStatus(workList.Items, resourceTemplate) + aggregatedStatuses, err := assembleWorkStatus(workList.Items, binding.Spec.Resource) if err != nil { return err } @@ -85,14 +84,12 @@ func AggregateResourceBindingWorkStatus( return err }); err != nil { eventRecorder.Event(binding, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error()) - eventRecorder.Event(resourceTemplate, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error()) return err } if operationResult == controllerutil.OperationResultUpdatedStatusOnly { msg := fmt.Sprintf("Update ResourceBinding(%s/%s) with AggregatedStatus successfully.", binding.Namespace, binding.Name) eventRecorder.Event(binding, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, msg) - eventRecorder.Event(resourceTemplate, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, msg) } else { klog.Infof("New aggregatedStatuses are equal with old ResourceBinding(%s/%s) AggregatedStatus, no update required.", binding.Namespace, binding.Name) } @@ -105,7 +102,6 @@ func AggregateClusterResourceBindingWorkStatus( ctx context.Context, c client.Client, binding *workv1alpha2.ClusterResourceBinding, - resourceTemplate *unstructured.Unstructured, eventRecorder record.EventRecorder, ) error { workList, err := GetWorksByBindingID(ctx, c, binding.Labels[workv1alpha2.ClusterResourceBindingPermanentIDLabel], false) @@ -113,7 +109,7 @@ func AggregateClusterResourceBindingWorkStatus( return err } - aggregatedStatuses, err := assembleWorkStatus(workList.Items, resourceTemplate) + aggregatedStatuses, err := assembleWorkStatus(workList.Items, binding.Spec.Resource) if err != nil { return err } @@ -131,14 +127,12 @@ func AggregateClusterResourceBindingWorkStatus( return err }); err != nil { eventRecorder.Event(binding, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error()) - eventRecorder.Event(resourceTemplate, corev1.EventTypeWarning, events.EventReasonAggregateStatusFailed, err.Error()) return err } if operationResult == controllerutil.OperationResultUpdatedStatusOnly { msg := fmt.Sprintf("Update ClusterResourceBinding(%s) with AggregatedStatus successfully.", binding.Name) eventRecorder.Event(binding, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, msg) - eventRecorder.Event(resourceTemplate, corev1.EventTypeNormal, events.EventReasonAggregateStatusSucceed, msg) } else { klog.Infof("New aggregatedStatuses are equal with old ClusterResourceBinding(%s) AggregatedStatus, no update required.", binding.Name) } @@ -155,14 +149,15 @@ func generateFullyAppliedCondition(spec workv1alpha2.ResourceBindingSpec, aggreg } // assemble workStatuses from workList which list by selector and match with workload. -func assembleWorkStatus(works []workv1alpha1.Work, workload *unstructured.Unstructured) ([]workv1alpha2.AggregatedStatusItem, error) { +func assembleWorkStatus(works []workv1alpha1.Work, objRef workv1alpha2.ObjectReference) ([]workv1alpha2.AggregatedStatusItem, error) { statuses := make([]workv1alpha2.AggregatedStatusItem, 0) for _, work := range works { if !work.DeletionTimestamp.IsZero() { continue } - identifierIndex, err := GetManifestIndex(work.Spec.Workload.Manifests, workload) + manifestRef := ManifestReference{APIVersion: objRef.APIVersion, Kind: objRef.Kind, Namespace: objRef.Namespace, Name: objRef.Name} + identifierIndex, err := GetManifestIndex(work.Spec.Workload.Manifests, &manifestRef) if err != nil { klog.Errorf("Failed to get manifestIndex of workload in work.Spec.Workload.Manifests. Error: %v.", err) return nil, err @@ -208,7 +203,7 @@ func assembleWorkStatus(works []workv1alpha1.Work, workload *unstructured.Unstru } for i := range work.Status.ManifestStatuses { - equal, err := equalIdentifier(&work.Status.ManifestStatuses[i].Identifier, identifierIndex, workload) + equal, err := equalIdentifier(&work.Status.ManifestStatuses[i].Identifier, identifierIndex, &manifestRef) if err != nil { return nil, err } @@ -227,18 +222,25 @@ func assembleWorkStatus(works []workv1alpha1.Work, workload *unstructured.Unstru return statuses, nil } +// ManifestReference identifies an object in manifest list +type ManifestReference struct { + APIVersion string + Kind string + Namespace string + Name string +} + // GetManifestIndex gets the index of clusterObj in manifest list, if not exist return -1. -func GetManifestIndex(manifests []workv1alpha1.Manifest, clusterObj *unstructured.Unstructured) (int, error) { +func GetManifestIndex(manifests []workv1alpha1.Manifest, manifestRef *ManifestReference) (int, error) { for index, rawManifest := range manifests { manifest := &unstructured.Unstructured{} if err := manifest.UnmarshalJSON(rawManifest.Raw); err != nil { return -1, err } - - if manifest.GetAPIVersion() == clusterObj.GetAPIVersion() && - manifest.GetKind() == clusterObj.GetKind() && - manifest.GetNamespace() == clusterObj.GetNamespace() && - manifest.GetName() == clusterObj.GetName() { + if manifest.GetAPIVersion() == manifestRef.APIVersion && + manifest.GetKind() == manifestRef.Kind && + manifest.GetNamespace() == manifestRef.Namespace && + manifest.GetName() == manifestRef.Name { return index, nil } } @@ -246,8 +248,8 @@ func GetManifestIndex(manifests []workv1alpha1.Manifest, clusterObj *unstructure return -1, fmt.Errorf("no such manifest exist") } -func equalIdentifier(targetIdentifier *workv1alpha1.ResourceIdentifier, ordinal int, workload *unstructured.Unstructured) (bool, error) { - groupVersion, err := schema.ParseGroupVersion(workload.GetAPIVersion()) +func equalIdentifier(targetIdentifier *workv1alpha1.ResourceIdentifier, ordinal int, manifestRef *ManifestReference) (bool, error) { + groupVersion, err := schema.ParseGroupVersion(manifestRef.APIVersion) if err != nil { return false, err } @@ -255,9 +257,9 @@ func equalIdentifier(targetIdentifier *workv1alpha1.ResourceIdentifier, ordinal if targetIdentifier.Ordinal == ordinal && targetIdentifier.Group == groupVersion.Group && targetIdentifier.Version == groupVersion.Version && - targetIdentifier.Kind == workload.GetKind() && - targetIdentifier.Namespace == workload.GetNamespace() && - targetIdentifier.Name == workload.GetName() { + targetIdentifier.Kind == manifestRef.Kind && + targetIdentifier.Namespace == manifestRef.Namespace && + targetIdentifier.Name == manifestRef.Name { return true, nil } diff --git a/pkg/util/helper/workstatus_test.go b/pkg/util/helper/workstatus_test.go index 3ff329960759..bd5119524dc7 100644 --- a/pkg/util/helper/workstatus_test.go +++ b/pkg/util/helper/workstatus_test.go @@ -192,7 +192,9 @@ func TestGetManifestIndex(t *testing.T) { } t.Run("Service", func(t *testing.T) { - index, err := GetManifestIndex(manifests, service) + manifestRef := ManifestReference{APIVersion: service.GetAPIVersion(), Kind: service.GetKind(), + Namespace: service.GetNamespace(), Name: service.GetName()} + index, err := GetManifestIndex(manifests, &manifestRef) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -202,7 +204,9 @@ func TestGetManifestIndex(t *testing.T) { }) t.Run("Deployment", func(t *testing.T) { - index, err := GetManifestIndex(manifests, deployment) + manifestRef := ManifestReference{APIVersion: deployment.GetAPIVersion(), Kind: deployment.GetKind(), + Namespace: deployment.GetNamespace(), Name: deployment.GetName()} + index, err := GetManifestIndex(manifests, &manifestRef) if err != nil { t.Errorf("unexpected error: %v", err) } @@ -212,7 +216,7 @@ func TestGetManifestIndex(t *testing.T) { }) t.Run("No match", func(t *testing.T) { - _, err := GetManifestIndex(manifests, &unstructured.Unstructured{}) + _, err := GetManifestIndex(manifests, &ManifestReference{}) if err == nil { t.Errorf("expected error, got nil") } @@ -224,7 +228,7 @@ func TestEqualIdentifier(t *testing.T) { name string target *workv1alpha1.ResourceIdentifier ordinal int - workload *unstructured.Unstructured + workload *ManifestReference expectedOutput bool }{ { @@ -238,15 +242,11 @@ func TestEqualIdentifier(t *testing.T) { Name: "test-deployment", }, ordinal: 0, - workload: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "apps/v1", - "kind": "Deployment", - "metadata": map[string]interface{}{ - "namespace": "default", - "name": "test-deployment", - }, - }, + workload: &ManifestReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "default", + Name: "test-deployment", }, expectedOutput: true, }, @@ -261,15 +261,11 @@ func TestEqualIdentifier(t *testing.T) { Name: "test-deployment", }, ordinal: 0, - workload: &unstructured.Unstructured{ - Object: map[string]interface{}{ - "apiVersion": "apps/v1", - "kind": "Deployment", - "metadata": map[string]interface{}{ - "namespace": "default", - "name": "test-deployment", - }, - }, + workload: &ManifestReference{ + APIVersion: "apps/v1", + Kind: "Deployment", + Namespace: "default", + Name: "test-deployment", }, expectedOutput: false, },