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.
There was a problem hiding this comment.
there is still value in printing the error in this case and maybe we should consider enhancing the user experience by throwing an event too. CPU / memory metrics are not owned by external.metrics.k8s.io API but instead by metrics.k8s.io API. That means KEDA can't fabricate the metric in a way that HPA would scale to fallback numbers.
|
Friendly ping @zroubalik @wozniakjan @SpiritZhou @psi @wonko @fivesheep @JorTurFer hope you to review and discuss, much thanks. |
ctccxxd
changed the title
ctccxxd
force-pushed
the
checkfallbackvalid
branch
from
cf5860b
to
dfeed82
Compare
I won't block anything if the consensus is reached and it goes the other way than I'm suggesting, but I would personally keep the webhook as is, that means print error but allow the The discussion what would be the ideal desired behaviour is likely going to take time. As a KEDA user, I would like some form of fallback when using some external trigger together with CPU/memory too. Fallback is a great strategy to make autoscaling robust against partial system outages and CPU/memory is far too common of a trigger. If using CPU/memory trigger prevents users from setting fallback on other scalers, that would be imho bad UX. Now how it's going to be implemented and working, that might be challenging to figure out. |
ctccxxd
force-pushed
the
checkfallbackvalid
branch
from
65fc9e8
to
cd3aaf6
Compare
![semgrep-app[bot]](https://avatars.githubusercontent.com/in/60555?s=60&v=4){kind=small}
token.txt
Outdated
| @@ -0,0 +1 @@ | |||
| eyJhbGciOiJSUzI1NiIsImtpZCI6IlpvX3F2SlFaZ1F1anVNUlNPYzQyU1dlOFdNNGlwSHlUXy1yUUh0Z0h5TGMifQ.eyJpc3MiOiJrdWJlcm5ldGVzL3NlcnZpY2VhY2NvdW50Iiwia3ViZXJuZXRlcy5pby9zZXJ2aWNlYWNjb3VudC9uYW1lc3BhY2UiOiJrdWJlLXN5c3RlbSIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VjcmV0Lm5hbWUiOiJrdWJlcm5ldGVzLWRhc2hib2FyZC10b2tlbiIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50Lm5hbWUiOiJrdWJlcm5ldGVzLWRhc2hib2FyZCIsImt1YmVybmV0ZXMuaW8vc2VydmljZWFjY291bnQvc2VydmljZS1hY2NvdW50LnVpZCI6ImJkNGUwNDNhLWQ0MzYtNDZiNi1iZDBjLTJjNTVkM2I5OTdjMCIsInN1YiI6InN5c3RlbTpzZXJ2aWNlYWNjb3VudDprdWJlLXN5c3RlbTprdWJlcm5ldGVzLWRhc2hib2FyZCJ9.W9f2IbIpZmcXaILw6rhXTFa0Sy2doUjy8FI72eVCDrHEi_3EVgUp8BoIsMxs56wVDIAPPBf0N-zJTVim97p5xwZ2KS0bCY-758xolE9vRx80Qoyfvm_Yv-qXGmoQRPLAlBRCAH5zQ8E-po9XqtTZwHuI8WRN_vd6Sz9lvDC6Iscmq--VG1Xo3UhVdhH4P15w56VDkCzHyXIZXrX1_iYYGS15g8x0qVov6l_uFiwOcVG1Jpytq8uCPs2cmvmQZqnOQaQmlA0Er7ZOrNqqDCfQ5mA62Cz6yjn42AifnMSYBGK75l3WytK1TTfjBwOVCVuX1gMYVowM5qrb1s37bMxitg | |||
There was a problem hiding this comment.
Signed-off-by: Shane <[email protected]>
ctccxxd
force-pushed
the
checkfallbackvalid
branch
from
cd3aaf6
to
f2ad730
Compare
Signed-off-by: Shane <[email protected]>
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