Consider replacing reflect.DeepEqual

[sbueringer]

I think we should consider replacing reflect.DeepEqual. Whilte it works well to diff two structs, it's very very had to debug what exactly is not equal. This makes it very hard to produce good logs to tell users why something was considered not equal.

I think go-cmp might be a good alternative. For more details, see: https://github.com/google/go-cmp

We already use go-cmp today to produce a diff in test code after reflect.DeepEqual only told us that something is not equal. The comparision algorithm of go-cmp seems to be better and additionally we get the beneift of getting a result which tells us where the difference is. This would be super valuable for our prod code to tell folks why we are doing things.

Using cmp.Diff for logs while we use reflect.DeepEqual for the actual comparison is not great as there are small differences, e.g. unexported fields.

One example where I think this is super valuable is in the KCP controller where we could tell folks why exactly we are triggering a MachineRollout and not just: well reflect.DeepEqual told us that there is a difference.

There are a few more places like e.g. the ClusterClass controller where we compare variable definitions where we could have a log message (with high v) that could log exactly why the definitions are different.

(xref: #8107 (comment))

/kind feature

[sbueringer]

cc @fabriziopandini @killianmuldoon @ykakarap

[ykakarap]

Along the same lines, In our tests I would recommend switching to BeComparableTo from Equal when comparing maps, objects and other relevant types. In case of failure Equal just prints the 2 objects and we have to manually identify the differences. BeComparableTo produces a much more human readable output with a clearer diff.

BeComparableTo internally uses go-cmp.

[sbueringer]

Good point, we have a separate issue for that one: #8095

[vincepri]

/triage accepted

[DiptoChakrabarty]

/assign

[sbueringer]

Took a closer look at go-cmp. For me it seems like a good option:

• README sounds good to me: https://github.com/google/go-cmp
• godoc sounds good to me: https://github.com/google/go-cmp/blob/master/cmp/compare.go#L93
• Gomega is using it in the BeComparableTo matcher
• We already have a dependency to go-cmp through Gomega and because we use it directly in tests

If someone wants to consider other alternatives, now would be the time to bring them up :)

[killianmuldoon]

I'm +1 for replacing it in tests, but I'm not sure about doing so in non-test code.

From the go-doc:

// It is intended to only be used in tests, as performance is not a goal and
// it may panic if it cannot compare the values. Its propensity towards
// panicking means that its unsuitable for production environments where a
// spurious panic may be fatal.

That means that possibly we should wrap our equality checks in a util package and maybe use go-cmp underneath. If we do this we can catch panics and return them as errors. WDYT?

[sbueringer]

Sounds reasonable. gomega also catches panics from go-cmp in BeComparableTo.

I don't think performance will be an issue with the things we compare (but I don't have data)

[killianmuldoon]

Yeah - I'm not very worried about performance. I also think we shouldn't have things that can't be compared, given we're almost always going to be comparing the same types, but catching the panics and turning them into errors is still a useful backstop.

[killianmuldoon]

/reopen

This issue covers replacing the reflect DeepEqual in more places across the codebase than #8266

[k8s-ci-robot]

@killianmuldoon: Reopened this issue.

In response to this:

/reopen

This issue covers replacing the reflect DeepEqual in more places across the codebase than #8266

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[ykakarap]

Unlike reflect.DeepEqual, unexported fields are not compared by default; they result in panics unless suppressed by using an Ignore option (see cmpopts.IgnoreUnexported) or explicitly compared using the Exporter option.

This is probably something that we keep an eye one when moving to cmp.Diff and BeComparableTo. There might be some places in our tests were we inherently compare the unexported fields (I don't imagine there will be a lot as we mostly just compare API types and all those fields are exported) and we should be careful when switching over.

[sbueringer]

Definitely more an issue for tests, in prod code we should keep it in mind but I would expect it not be an issue as you said.

[k8s-triage-robot]

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

• Confirm that this issue is still relevant with /triage accepted (org members only)
• Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

[fabriziopandini]

/priority important-longterm

[fabriziopandini]

/assign @sbueringer
To re-assess