make DeepEqual judgments more efficient

[zhzhuang-zju]

What type of PR is this?
/kind cleanup

What this PR does / why we need it:
Function CreateOrUpdateClusterObject is used to create cluster object in karmada control plane, or update it if it has been existed.

karmada/pkg/util/cluster.go

Lines 145 to 151 in da7689f

	if exist {
	if reflect.DeepEqual(cluster.Spec, clusterObj.Spec) {
	klog.Warningf("Cluster(%s) already exist and newest", clusterObj.Name)
	return cluster, nil
	}
	mutate(cluster)
	cluster, err = updateCluster(controlPlaneClient, cluster)

reflect.DeepEqual(cluster.Spec, clusterObj.Spec) It doesn't make sense to compare the cluster to the clusterobj, it should be the cluster to the mutate(cluster) in order to correctly determine if an update is needed.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

[chaosi-zju]

I got it, so clusterObj.Spec is a default value, it will always not deep equal to cluster.Spec, it is a redundant comparison.

what we should compare is cluster.Spec with cluster after mutated, right?

[zhzhuang-zju]

yes~you're right

[chaosi-zju]

generally looks good, but I may prefer improving the code style. I think this function although named CreateOrUpdateClusterObject actually does two things: BuildCluster and CreateOrUpdateCluster, may be we can split into two functions.

Besides, I searched the key words CreateOrUpdate in k8s source code, his code is written like this:

• before k8s release-1.30:

https://github.com/kubernetes/kubernetes/blob/3df26e386335110e13b32c2f679a3cd2ee4bd8cb/cmd/kubeadm/app/util/apiclient/idempotency.go#L147-L159

• after k8s release-1.30:

https://github.com/kubernetes/kubernetes/blob/698ad6630c56ffc27946f766b5c77e1f16482a77/cmd/kubeadm/app/util/apiclient/idempotency.go#L190-L213

The way k8s code is written and its code style evolution from v1.29 to v1.30 version could be used as a more mainstream reference.

[chaosi-zju]

thanks for your important commit again, above is just for reference only.

[zhzhuang-zju]

thanks for your import suggestion!

	err := wait.PollUntilContextTimeout(context.Background(),
		apiCallRetryInterval, kubeadmapi.GetActiveTimeouts().KubernetesAPICall.Duration,
		true, func(_ context.Context) (bool, error) {
			ctx := context.Background()
			if _, err := client.AppsV1().Deployments(deploy.ObjectMeta.Namespace).Create(ctx, deploy, metav1.CreateOptions{}); err != nil {
				if !apierrors.IsAlreadyExists(err) {
					lastError = errors.Wrap(err, "unable to create Deployment")
					return false, nil
				}
				if _, err := client.AppsV1().Deployments(deploy.ObjectMeta.Namespace).Update(ctx, deploy, metav1.UpdateOptions{}); err != nil {
					lastError = errors.Wrap(err, "unable to update Deployment")
					return false, nil
				}
			}
			return true, nil
		})

I don't understand where the resourceversion of the deploy is initialized, I checked the call chain of this function, but I didn't find the corresponding place. @chaosi-zju

[zhzhuang-zju]

karmada/vendor/sigs.k8s.io/controller-runtime/pkg/controller/controllerutil/controllerutil.go

Lines 196 to 224 in da7689f

	func CreateOrUpdate(ctx context.Context, c client.Client, obj client.Object, f MutateFn) (OperationResult, error) {
	key := client.ObjectKeyFromObject(obj)
	if err := c.Get(ctx, key, obj); err != nil {
	if !apierrors.IsNotFound(err) {
	return OperationResultNone, err
	}
	if err := mutate(f, key, obj); err != nil {
	return OperationResultNone, err
	}
	if err := c.Create(ctx, obj); err != nil {
	return OperationResultNone, err
	}
	return OperationResultCreated, nil
	}
	existing := obj.DeepCopyObject()
	if err := mutate(f, key, obj); err != nil {
	return OperationResultNone, err
	}
	if equality.Semantic.DeepEqual(existing, obj) {
	return OperationResultNone, nil
	}
	if err := c.Update(ctx, obj); err != nil {
	return OperationResultNone, err
	}
	return OperationResultUpdated, nil
	}

How about this? @chaosi-zju

[XiShanYongYe-Chang]

/assign

[zhzhuang-zju]

cc @XiShanYongYe-Chang @chaosi-zju

[XiShanYongYe-Chang]

Thanks
/lgtm

[chaunceyjiang]

Nice finding.
/LGTM

[karmada-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chaunceyjiang

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• pkg/util/OWNERS [chaunceyjiang]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment