fix: optimize all checkRepliacas code and fix the problem does not take effect #6344

ctccxxd · 2024-11-19T03:13:05Z

fix and optimize all checkReplicas code, originally
1, Webhook for replicas check does not take effect.
2, No check for negative value.
3, Variable max or min is same with build-in function name, not graceful
4, Have no defaultMaxReplicas for hpa.
5, Code has redundency.

Checklist

When introducing a new scaler, I agree with the scaling governance policy
I have verified that my change is according to the deprecations & breaking changes policy
Tests have been added
Changelog has been updated and is aligned with our changelog requirements
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)
Commits are signed with Developer Certificate of Origin (DCO - learn more)

related issue
#6356

Signed-off-by: Shane <[email protected]>

ctccxxd · 2024-11-21T03:43:02Z

Friendly ping @zroubalik @wozniakjan @SpiritZhou @psi @wonko @fivesheep @JorTurFer hope you to review and discuss, much thanks.

JorTurFer

I think that I'm missing something. Can't we just include the negative check message in the current GetHPAMin/MaxReplicas funcs and it'd do almost the same, wouldn't?

ctccxxd · 2024-11-24T05:10:50Z

I think that I'm missing something. Can't we just include the negative check message in the current GetHPAMin/MaxReplicas funcs and it'd do almost the same, wouldn't?

Much thanks for your comments. Originally. it has much logic in GetHPAMin/MaxReplicas funcs including default replicas logic. I
hink it is not very explicit for user to read. I want to make the logic more clean. Besides, I have more purposes,

1, Webhook for replicas check does not take effect.
2, No check for negative value.
3, Variable max or min is same with build-in function name, not graceful
4, Have no defaultMaxReplicas for hpa.
5, Code has redundency. Hope for your reply.

@JorTurFer PTAL much thanks.

change changelog Signed-off-by: Shane <[email protected]>

ctccxxd · 2024-11-28T07:53:34Z

I think that I'm missing something. Can't we just include the negative check message in the current GetHPAMin/MaxReplicas funcs and it'd do almost the same, wouldn't?

Much thanks for your comments. Originally. it has much logic in GetHPAMin/MaxReplicas funcs including default replicas logic. I hink it is not very explicit for user to read. I want to make the logic more clean. Besides, I have more purposes,

1, Webhook for replicas check does not take effect. 2, No check for negative value. 3, Variable max or min is same with build-in function name, not graceful 4, Have no defaultMaxReplicas for hpa. 5, Code has redundency. Hope for your reply.

@JorTurFer @wozniakjan PTAL much thanks.

Signed-off-by: Shane <[email protected]>

wozniakjan

I'm wondering why the webhook only logged the wrong min/max and never prevented the SO from creation. So the bug is imho real but we should ensure the coding style is matching conventions.

wozniakjan · 2024-12-02T14:11:59Z

apis/keda/v1alpha1/scaledobject_types.go

+// GetDefaultHPAMinReplicas returns defaultHPAMinReplicas
+func (so *ScaledObject) GetDefaultHPAMinReplicas() *int32 {
+	tmp := defaultHPAMinReplicas
+	return &tmp


I don't think this needs a receiver, the so is ignored in the body

Quote reply

make sense

wozniakjan · 2024-12-02T14:12:53Z

apis/keda/v1alpha1/scaledobject_types.go

@@ -254,21 +260,49 @@ func (so *ScaledObject) GetHPAMaxReplicas() int32 {
 	return defaultHPAMaxReplicas
 }

+// GetDefaultHPAMaxReplicas returns defaultHPAMaxReplicas
+func (so *ScaledObject) GetDefaultHPAMaxReplicas() int32 {
+	return defaultHPAMaxReplicas


same here, having this as a method on ScaledObject structure feels unnecessary

wozniakjan · 2024-12-02T14:14:36Z

apis/keda/v1alpha1/scaledobject_types.go

+			return false
+		}
+	}
+	return true


and here too, the pointer receiver is ignored. But I think the code can be made even cleaner without this function entirely

and here too, the pointer receiver is ignored. But I think the code can be made even cleaner without this function entirely

ok

wozniakjan · 2024-12-02T14:17:37Z

apis/keda/v1alpha1/scaledobject_types.go

+
+// GetIdleReplicasIfDefined returns bool based on whether idleRelicas is defined
+func (so *ScaledObject) GetIdleReplicasIfDefined() bool {
+	return so.Spec.IdleReplicaCount != nil


imho, the check is more readable than GetIdleReplicasIfDefined name, I'd ditch this helper too and keep so.Spec.IdleReplicaCount != nil which is very common convention in kubernetes codebase.

imho, the check is more readable than GetIdleReplicasIfDefined name, I'd ditch this helper too and keep so.Spec.IdleReplicaCount != nil which is very common convention in kubernetes codebase.

ok, I will follow the code style you suggest.

wozniakjan · 2024-12-02T14:21:27Z

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -184,7 +184,7 @@ func verifyReplicaCount(incomingSo *ScaledObject, action string, _ bool) error {
 		scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
 		metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-replicas")
 	}
-	return nil
+	return err


hmm, @JorTurFer, @zroubalik, any thoughts why the webhook used to allow wrong min/max? I just tried it with 2.16.0 and I can easily create SO with min=25 and max=10.

2024-12-02T14:09:36Z ERROR scaledobject-validation-webhook validation error {"name": "http-server", "error": "MinReplicaCount=25 must be less than MaxReplicaCount=10"}

Probably it was a mistake, the admission webhook should prevent that clear misconfiguration

wozniakjan · 2024-12-02T14:23:58Z

apis/keda/v1alpha1/scaledobject_types.go

@@ -239,13 +239,19 @@ func (so *ScaledObject) IsUsingModifiers() bool {

 // getHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
 func (so *ScaledObject) GetHPAMinReplicas() *int32 {
-	if so.Spec.MinReplicaCount != nil && *so.Spec.MinReplicaCount > 0 {
+	if so.Spec.MinReplicaCount != nil {


this was imho better before, this should be used by the HPA controller which then needs to reimplement the logic of figuring out the 0.

this was imho better before, this should be used by the HPA controller which then needs to reimplement the logic of figuring out the 0.

Originally, there is no defaultMaxReplicas in HPA and I will add defaultMaxReplicas and defaultMinReplicas together this time. I think it is more clear in this way. Although, I give up the original logic here.

I think that this is just to prevent something like minReplicas -10, but it's true that it's already covered in explicitly outside this function

wozniakjan · 2024-12-02T14:25:16Z

controllers/keda/hpa.go

+		maxReplicas = scaledObject.GetDefaultHPAMaxReplicas()
+	}
+
+	if *minReplicas < 0 || maxReplicas < 0 {


nit: no need to check max for less than 0, it has to be bigger than min and min is checked for larger than 0 so transitively max is already larger

nit: no need to check max for less than 0, it has to be bigger than min and min is checked for larger than 0 so transitively max is already larger

make sense

Signed-off-by: Shane <[email protected]>

ctccxxd · 2024-12-12T12:56:11Z

Friendly ping @wozniakjan @JorTurFer I have almostly followed your opinion, hope you to review and discuss, much thanks.

ctccxxd · 2024-12-17T00:13:08Z

@JorTurFer @wozniakjan I have follow your opinions. Hope for your review, and e2e test, much thanks.

Signed-off-by: Shane <[email protected]>

JorTurFer · 2024-12-22T15:36:02Z

apis/keda/v1alpha1/scaledobject_webhook.go

@@ -184,7 +184,7 @@ func verifyReplicaCount(incomingSo *ScaledObject, action string, _ bool) error {
 		scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error")
 		metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-replicas")
 	}
-	return nil
+	return err


Probably it was a mistake, the admission webhook should prevent that clear misconfiguration

JorTurFer · 2024-12-22T15:53:20Z

apis/keda/v1alpha1/scaledobject_types.go

@@ -239,13 +239,19 @@ func (so *ScaledObject) IsUsingModifiers() bool {

 // getHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
 func (so *ScaledObject) GetHPAMinReplicas() *int32 {
-	if so.Spec.MinReplicaCount != nil && *so.Spec.MinReplicaCount > 0 {
+	if so.Spec.MinReplicaCount != nil {


I think that this is just to prevent something like minReplicas -10, but it's true that it's already covered in explicitly outside this function

JorTurFer · 2024-12-22T15:54:07Z

apis/keda/v1alpha1/scaledobject_types.go

+	if scaledObject.Spec.IdleReplicaCount != nil && *idleReplicas >= minReplicas {
+		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, minReplicas)


Actually idleReplicas only can be 0 nowadays, other value will be wrong

fix and optimize all checkRepliacas code

d951cec

Signed-off-by: Shane <[email protected]>

ctccxxd requested a review from a team as a code owner November 19, 2024 03:13

add changelog

e3cb479

Signed-off-by: Shane <[email protected]>

ctccxxd marked this pull request as draft November 19, 2024 04:35

change some logic and variable name

59079d8

Signed-off-by: Shane <[email protected]>

ctccxxd force-pushed the webhook branch from 0551651 to 59079d8 Compare November 19, 2024 05:07

ctccxxd marked this pull request as ready for review November 19, 2024 05:31

ctccxxd changed the title ~~fix and optimize all checkRepliacas code~~ optimize all checkRepliacas code and fix some Nov 19, 2024

ctccxxd changed the title ~~optimize all checkRepliacas code and fix some~~ optimize all checkRepliacas code and fix the problem does not take effect Nov 19, 2024

This was referenced Nov 21, 2024

fix: revise some annnotations and fix fallback check bug #6346

Closed

minReplicas, maxReplicas and idleReplicas check bug for scaleobject #6356

Open

ctccxxd changed the title ~~optimize all checkRepliacas code and fix the problem does not take effect~~ fix: optimize all checkRepliacas code and fix the problem does not take effect Nov 21, 2024

JorTurFer reviewed Nov 23, 2024

View reviewed changes

Update CHANGELOG.md

4299fd6

change changelog Signed-off-by: Shane <[email protected]>

Merge branch 'main' into webhook

e8a4052

Signed-off-by: Shane <[email protected]>

wozniakjan reviewed Dec 2, 2024

View reviewed changes

ctccxxd added 5 commits December 10, 2024 09:09

revise code

5d80f1c

Signed-off-by: Shane <[email protected]>

add test case

55120ac

Signed-off-by: Shane <[email protected]>

revise test namespace

fae8688

Signed-off-by: Shane <[email protected]>

Update scaledobject_webhook_test.go

55d48fa

Signed-off-by: Shane <[email protected]>

Update scaledobject_webhook_test.go

f6f5c51

Signed-off-by: Shane <[email protected]>

ctccxxd added 2 commits December 17, 2024 08:13

Merge branch 'main' into webhook

05b6dc4

Signed-off-by: Shane <[email protected]>

Update CHANGELOG.md

542b9df

Signed-off-by: Shane <[email protected]>

ctccxxd requested review from wozniakjan and JorTurFer December 18, 2024 04:03

JorTurFer reviewed Dec 22, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: optimize all checkRepliacas code and fix the problem does not take effect #6344

fix: optimize all checkRepliacas code and fix the problem does not take effect #6344

ctccxxd commented Nov 19, 2024 •

edited

Loading

ctccxxd commented Nov 21, 2024 •

edited

Loading

JorTurFer left a comment

ctccxxd commented Nov 24, 2024 •

edited

Loading

ctccxxd commented Nov 28, 2024

wozniakjan left a comment

wozniakjan Dec 2, 2024

ctccxxd Dec 9, 2024

wozniakjan Dec 2, 2024

wozniakjan Dec 2, 2024

ctccxxd Dec 9, 2024

wozniakjan Dec 2, 2024

ctccxxd Dec 9, 2024

wozniakjan Dec 2, 2024

JorTurFer Dec 22, 2024

wozniakjan Dec 2, 2024

ctccxxd Dec 9, 2024

JorTurFer Dec 22, 2024

wozniakjan Dec 2, 2024

ctccxxd Dec 9, 2024

ctccxxd commented Dec 12, 2024 •

edited

Loading

ctccxxd commented Dec 17, 2024

JorTurFer Dec 22, 2024

JorTurFer Dec 22, 2024

JorTurFer Dec 22, 2024

		if scaledObject.Spec.IdleReplicaCount != nil && *idleReplicas >= minReplicas {
		return fmt.Errorf("IdleReplicaCount=%d must be less than MinReplicaCount=%d", *scaledObject.Spec.IdleReplicaCount, minReplicas)

fix: optimize all checkRepliacas code and fix the problem does not take effect #6344

Are you sure you want to change the base?

fix: optimize all checkRepliacas code and fix the problem does not take effect #6344

Conversation

ctccxxd commented Nov 19, 2024 • edited Loading

ctccxxd commented Nov 21, 2024 • edited Loading

JorTurFer left a comment

Choose a reason for hiding this comment

ctccxxd commented Nov 24, 2024 • edited Loading

ctccxxd commented Nov 28, 2024

wozniakjan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctccxxd commented Dec 12, 2024 • edited Loading

ctccxxd commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctccxxd commented Nov 19, 2024 •

edited

Loading

ctccxxd commented Nov 21, 2024 •

edited

Loading

ctccxxd commented Nov 24, 2024 •

edited

Loading

ctccxxd commented Dec 12, 2024 •

edited

Loading