status-manager, Fix bad compare of status

[RamLavi]

When comparing statuses to see if the status has changed, there is a bug
since oldStatus is a pointer and config.Status is not, hence the
DeepEqual() will always return true

Note that this fix will not reduce status issues, since the heartbeat of statuses Degraded and Available are updated every reconcile.

What this PR does / why we need it:

Special notes for your reviewer:

Release note:

NONE

[RamLavi]

/cherry-pick release-0.58

@phoracek do we want to cherry-pick to other stable branches?

[kubevirt-bot]

@RamLavi: once the present PR merges, I will cherry-pick it on top of release-0.58 in a new PR and assign it to you.

In response to this:

/cherry-pick release-0.58

@phoracek do we want to cherry-pick to other stable branches?

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.

[RamLavi]

/retest

[oshoval]

DeepEqual() will always return true

you mean false maybe ?

[qinqon]

Looks like there are better ways to do this.

[qinqon]

What do you think about using what is proposed here ? kubernetes-sigs/kubebuilder#592 (comment)

[RamLavi]

interesting! will try

[RamLavi]

ok, so I compared the two, and although for most cases it will act the same, in some it will not, in a way that is not suitable for this case.
the reason is that DeepDerivative ignores unset parameters (see example), which is good for comparing configuration specs, but not good for comparing statuses.

Although we don't really "remove" conditions, only change them, I don't think it's better than DeepEqual for this particular case, so let's keep it :)

This is an example of how it works (this unit tests passes)

Context("LALALA", func() {
		status1 := shared.NetworkAddonsConfigStatus{}
		status2 := shared.NetworkAddonsConfigStatus{}
		It("RI!!", func() {
			Expect(equality.Semantic.DeepDerivative(status1, status2)).To(BeTrue(), "1.1")
			Expect(equality.Semantic.DeepDerivative(status2, status1)).To(BeTrue(), "1.2")

			status1.Conditions = []conditionsv1.Condition{}
			Expect(equality.Semantic.DeepDerivative(status1, status2)).To(BeTrue(), "2.1")
			Expect(equality.Semantic.DeepDerivative(status2, status1)).To(BeTrue(), "2.2")

			status1.Conditions = []conditionsv1.Condition{
				conditionsv1.Condition{Reason: "simple diff"},
			}
			Expect(equality.Semantic.DeepDerivative(status1, status2)).To(BeFalse(), "3.1")
			Expect(equality.Semantic.DeepDerivative(status2, status1)).To(BeTrue(), "3.2")
		})
	})

[qinqon]

Can we encapsulate this into a function that expect proper types? Also check if we are doing the same at other parts of the code and use that function, or similar. This is the kind of things that are super difficult to find if something is failing, this kind of unsafe code has to be encapsulated.

[RamLavi]

Can we encapsulate this into a function that expect proper types?

can you explain? I'm not sure I understand your concern.

btw we don't use reflect.DeepEqual() anywhere else on CNAO.

[RamLavi]

ohhh now I get what you mean and why you asked this.. I was on another plane hehe, sure will do

[RamLavi]

since shared.NetworkAddonsConfigStatus is defined on pkg/apis/shared/networkaddonsconfig_types.go, do you think it's ok to add the function there? I worry that this file might break operator-sdk scripts there

Is it ok if I add a function on statusManager? (status *StatusManager) statusDeepEqual(oldStatus, newStatus cnao.NetworkAddonsConfigStatus) bool

[qinqon]

We can add at "shared" package whatever we want, we don't have to add the functions to _types.go we can add a _operators.go, also the "shared" stuff was right at the moment but looks like it's not the actual patter at operator-sdk/kubebuilder.

[RamLavi]

I'll open a networkaddonsconfig_operators.go on cluster-network-addons-operator/pkg/apis/networkaddonsoperator/shared/

pff.. well let deal with it one dish at a time :) I'll open an issue to align with operator-sdk/kubebuilder

[RamLavi]

fixed here

[RamLavi]

/retest

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[qinqon]

/lgtm
/approve

[kubevirt-bot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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:

• OWNERS [qinqon]

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

[kubevirt-bot]

@RamLavi: new pull request created: #1034

In response to this:

/cherry-pick release-0.58

@phoracek do we want to cherry-pick to other stable branches?

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.