fix: revise some annnotations and fix fallback check bug #6346
Conversation
Signed-off-by: Shane <[email protected]>
Signed-off-by: Shane <[email protected]>
ctccxxd
changed the title
ctccxxd
changed the title
Signed-off-by: Shane <[email protected]>
Signed-off-by: Shane <[email protected]>
| @@ -193,7 +193,7 @@ func verifyFallback(incomingSo *ScaledObject, action string, _ bool) error { | |||
| scaledobjectlog.WithValues("name", incomingSo.Name).Error(err, "validation error") | |||
| metricscollector.RecordScaledObjectValidatingErrors(incomingSo.Namespace, action, "incorrect-fallback") | |||
| } | |||
| return nil | |||
| return err | |||
There was a problem hiding this comment.
this would likely introduce undesired regression on denying SOs with fallback and multiple triggers with at least one being cpu or memory - #5962 (review)
There was a problem hiding this comment.
this would likely introduce undesired regression on denying SOs with fallback and multiple triggers with at least one being cpu or memory - #5962 (review)
so, the code deny trigger which has cpu or memory in function CheckFallbackValid is wrong? so here, it's better to return nil and not take effect?
There was a problem hiding this comment.
I think the discussion on this has still not been concluded. Imho it makes sense to allow ScaledObjects that have fallback defined and at least one trigger that is not CPU / memory.
I would go as far as allowing ScaledObjects that have only CPU / memory fallback but throw an Event with type Warning periodically informing the end user that this ScaledObject might not work the way they intended. So this code swallowing the error and just logging it follows that idea.
There was a problem hiding this comment.
I see the official website(https://keda.sh/docs/2.16/reference/scaledobject-spec/#fallback) and the code (scaledobject_types.go function CheckFallbackValid), they all forbidden CPU / memory fallback. I think the initial purpose is to forbidden the metric type which is not AverageValue, and I don't know why forbidden CPU / memory fallback? I see that CPU / memory also support type AverageValue. So, if I think there should be a conclusion whether forbidden it. If not forbidden, I think just change code to allow the CPU / memory, but webhook in this part have to take effect to support the value validation. 
(maybe just delete this part). If forbidden, so just make it take effect.
Besides, I also make other code contribution, (#6344)
(#6350), and hope you to review, thank you much.
|
Friendly ping @zroubalik @wozniakjan @SpiritZhou @psi @wonko @fivesheep @JorTurFer hope you to review and discuss, much thanks. |
ctccxxd
changed the title
Revise some annotations and fix fallback check, originally,
1, Annotations above function is not well
2, Fallback check does not take effect
Checklist
related issue
#6355