Refactor SetStatusConditions logic

[catouc]

Signed-off-by: Philipp Böschen catouc@philipp.boeschen.me

A first try at implementing #2906 - moving the conditions setting logic into the pkg/util package.

Checklist

• Commits are signed with Developer Certificate of Origin (DCO - learn more)
• Tests have been added
• A PR is opened to update our Helm chart (repo) (if applicable, ie. when deployment manifests are modified)
• A PR is opened to update the documentation on (repo) (if applicable)
• Changelog has been updated and is aligned with our changelog requirements

Fixes #2906

[zroubalik]

Thanks for looking into this @catouc! Sorry for the delay in the review, I was off.

I think that the problem is that you are replacing existing conditions on the Status.

[zroubalik]

I think this call will replace all existing conditions of another type that are already defined on the ScaledObject's Status. See how Conditions are being set:

keda/apis/keda/v1alpha1/condition_types.go

Lines 126 to 191 in 98509a8

	// SetReadyCondition modifies Ready Condition according to input parameters
	func (c *Conditions) SetReadyCondition(status metav1.ConditionStatus, reason string, message string) {
	if *c == nil {
	c = GetInitializedConditions()
	}
	c.setCondition(ConditionReady, status, reason, message)
	}
	// SetActiveCondition modifies Active Condition according to input parameters
	func (c *Conditions) SetActiveCondition(status metav1.ConditionStatus, reason string, message string) {
	if *c == nil {
	c = GetInitializedConditions()
	}
	c.setCondition(ConditionActive, status, reason, message)
	}
	// SetFallbackCondition modifies Fallback Condition according to input parameters
	func (c *Conditions) SetFallbackCondition(status metav1.ConditionStatus, reason string, message string) {
	if *c == nil {
	c = GetInitializedConditions()
	}
	c.setCondition(ConditionFallback, status, reason, message)
	}
	// GetActiveCondition returns Condition of type Active
	func (c *Conditions) GetActiveCondition() Condition {
	if *c == nil {
	c = GetInitializedConditions()
	}
	return c.getCondition(ConditionActive)
	}
	// GetReadyCondition returns Condition of type Ready
	func (c *Conditions) GetReadyCondition() Condition {
	if *c == nil {
	c = GetInitializedConditions()
	}
	return c.getCondition(ConditionReady)
	}
	// GetFallbackCondition returns Condition of type Ready
	func (c *Conditions) GetFallbackCondition() Condition {
	if *c == nil {
	c = GetInitializedConditions()
	}
	return c.getCondition(ConditionFallback)
	}
	func (c Conditions) getCondition(conditionType ConditionType) Condition {
	for i := range c {
	if c[i].Type == conditionType {
	return c[i]
	}
	}
	return Condition{}
	}
	func (c Conditions) setCondition(conditionType ConditionType, status metav1.ConditionStatus, reason string, message string) {
	for i := range c {
	if c[i].Type == conditionType {
	c[i].Status = status
	c[i].Reason = reason
	c[i].Message = message
	break
	}
	}

[catouc]

Ah that's a good catch - I put in a first idea that seems to pass that one failing test, I'm currently only on my chromebook that can't run the full test suit without blowing up.
I haven't really thought about the implementation too much on that one and it's copying essentially the same switch statement for the two types there are.

One question for this I suppose is how much does performance matter on this code path? Do we need to worry about allocs or speed necessarily here?

[zroubalik]

Sorry for the late reply, I have been really busy, Kubecon and other stuff.

We should really care about performance on this. Status might be updated quite often and there might be tons of ScaledObjects to update.

[catouc]

I think this will definitely be a bit more involved with that in mind 🤔 the last implementation I wrote broke something else, I'm feeling the ways the two functions are originally designed are subtly different so I'll have to spend more time understanding - pretty busy myself currently so have not made any significant progress at this point.

[zroubalik]

Sure, let's keep this open :)

[JorTurFer]

hey @catouc ,
Could you solve the conflict please? I think that CI hasn't been triggered due to that

[catouc]

hey @catouc , Could you solve the conflict please? I think that CI hasn't been triggered due to that

Should be fine now - but I am having issues with functional tests now, I don't think my implementation actually works - which is odd since I'm just passing through to the original setter methods on the Conditions slice 🤔

[Fail] ScaledObjectController functional tests [It] deploys ScaledObject and creates HPA, when IdleReplicaCount, MinReplicaCount and MaxReplicaCount is defined 
/home/pb/Code/oss/keda/controllers/keda/scaledobject_controller_test.go:422

[Fail] ScaledObjectController functional tests [It] doesn't allow MinReplicaCount > MaxReplicaCount 
/home/pb/Code/oss/keda/controllers/keda/scaledobject_controller_test.go:512

[Fail] ScaledObjectController functional tests [It] doesn't allow IdleReplicaCount > MinReplicaCount 
/home/pb/Code/oss/keda/controllers/keda/scaledobject_controller_test.go:555

[Fail] ScaledObjectController functional tests [It] doesn't allow IdleReplicaCount > MaxReplicaCount, when MinReplicaCount is not explicitly defined 
/home/pb/Code/oss/keda/controllers/keda/scaledobject_controller_test.go:598

[tomkerkhove]

Any update on this?

[catouc]

Any update on this?

I'll likely not get around to this so if someone wants to actually pick this up instead you're very welcome.

[zroubalik]

Closing this in favor of: #4487