Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

deepequal check in helper.CreateOrUpdateWork() always return false, because resource.MarshalJSON() will add \n at the end of JSON #5938

Open
zach593 opened this issue Dec 11, 2024 · 3 comments · May be fixed by #5939 or #5940
Assignees
Labels
kind/bug Categorizes issue or PR as related to a bug.

Comments

@zach593
Copy link
Contributor

zach593 commented Dec 11, 2024

What happened:

deepequal check in helper.CreateOrUpdateWork() always return false, because resource.MarshalJSON() will add \n at the end of JSON, but the workloads read from work.Spec.Workload.Manifests never have this \n. So everytime this function is called, an update operation will occur.

Fortunately, the same thing doesn't happen to work-status-controller, rb/crb-status-controller. Although they are both using DeepEqual() on runtime.RawExtension, but the byte slices are obtained from resource interpret webhook.

What you expected to happen:

remove unecessary update operations.

How to reproduce it (as minimally and precisely as possible):

func Test(t *testing.T) {
	mgr, err := ctrl.NewManager(restConf, ctrl.Options{
		Scheme: gclient.NewSchema(),
	})
	if err != nil {
		panic(err)
	}
	go mgr.GetCache().Start(context.TODO())
	time.Sleep(time.Second)

	work := &workv1alpha1.Work{}
	err = mgr.GetClient().Get(context.TODO(), client.ObjectKey{Namespace: "example-ns", Name: "example-name"}, work)
	if err != nil {
		panic(err)
	}
	fmt.Println(work.Spec.Workload.Manifests[0].Raw)

	workload := &unstructured.Unstructured{}
	_ = json.Unmarshal(work.Spec.Workload.Manifests[0].Raw, workload)

	j0, _ := json.Marshal(workload)
	fmt.Println(j0)

	j1, _ := workload.MarshalJSON()
	fmt.Println(j1)
}

image

Anything else we need to know?:

@snowplayfire found this, I'm just providing some possible fixes here. Many thanks to her, I sincerely hope she has a happy holiday.

This is the root cause why binding controller can’t use DeepEqual() to skip the update operation when calling the ensureWork(). But for deepequal to work, we need another PR to copy part of the karmada webhook logic to the update operation, which I will complete as soon as possible.

For fixing this issue, one way is just need one line: replace resource.MarshalJSON() with json.Marshal(resource), it could work. #5939

But deepequal checks should be used in semantic scenarios, not to judge byte streams. Therefore, if we want to use semantic deepequal checking to fix it, we need to directly judge the unmarshaled workload read from work. #5940

Environment:

  • Karmada version: any
  • kubectl-karmada or karmadactl version (the result of kubectl-karmada version or karmadactl version):
  • Others:
@zhzhuang-zju
Copy link
Contributor

thanks @zach593 @snowplayfire
Follow the test file you provided, I have also reproduced this issue. workload.MarshalJSON() compared to json.Marshal does add \n to the end. And the evidence of this can be found in the code:

https://github.com/golang/go/blob/80a2982a801eaedc416d59801ac8fefcf1e4acf5/src/encoding/json/stream.go#L214-L221

@XiShanYongYe-Chang
Copy link
Member

Thank you very much, it was a great find.

I still have a doubt. Before the work is updated, the manifest file ends with 10. However, why does the manifest in the work geted by using the client not end with 10? Where is the 10 digested?

@XiShanYongYe-Chang
Copy link
Member

/assign @zach593

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug.
Projects
Status: No status
3 participants