fix: revise some annnotations and fix fallback check bug

[ctccxxd]

Revise some annotations and fix fallback check, originally,
1, Annotations above function is not well
2, Fallback check does not take effect

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
#6355

[wozniakjan]

this would likely introduce undesired regression on denying SOs with fallback and multiple triggers with at least one being cpu or memory - #5962 (review)

[ctccxxd]

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?

[wozniakjan]

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.

[ctccxxd]

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.

[wozniakjan]

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.

[ctccxxd]

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

[wozniakjan]

hope you to review and discuss, much thanks.

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 SO. To improve feedback to the user, keda-operator should throw a WARNING event that fallback with CPU/memory scalers is not supported.

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.