fix!: fix CheckFallbackValid logic

[y-rabie]

This fixes the CheckFallbackValid logic used in the admission webhook. The way it should go is:

• If we are using ScalingModifiers and ScaledObject.Spec.Advanced.ScalingModifiers.MetricType is not AverageValue, then this is invalid. So, this becomes inline with the change in fix: fix isFallbackEnabled when using scaling modifiers #6521

• If we are not using ScalingModifiers

  1. If only cpu and memory are used, then fallback is invalid.
  2. If some other triggers are used (solely or in addition to cpu/memory), and not one of them has AverageValue type, then fallback is invalid.
  3. If some other triggers are used (solely or in addition to cpu/memory), and at least one of them has AverageValue type, then fallback is valid. This is actually inline with the code logic with the function doFallback, where it is called for every metricSpec that has the type AverageValue.

The PR #6407 seems related to this. I am not sure what it is trying to do per se with regards to the cpu and memory. Because it seems to be logging instead of returning the error when encountering cpu/memory, then falling to the next if condition which will return the error either way

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)

[y-rabie]

@zroubalik @JorTurFer If any of you guys has time to look into this 😅 , I think this needs to be released in v2.17.0 along with the rest of changes.

[JorTurFer]

The PR #6407 seems related to this. I am not sure what it is trying to do per se with regards to the cpu and memory. Because it seems to be logging instead of returning the error when encountering cpu/memory, then falling to the next if condition which will return the error either way

Actually, CPU and memory support AverageValue but it's true that this doesn't make sense, the check should be:

• If CPU/Memory scaler, just continue with the iteration without failing
• At the end of the iterations, if there are only CPU/Memory scalers and fallback enabled, raise an error (because fallback with only CPU/Memory is a failure)

[JorTurFer]

lol, my point was literally your message 🤦
I must read more slowly

[zroubalik]

/run-e2e fallback
Update: You can check the progress here

[heshamelsherif97]

I feel like this needs to get merged before the 2.17 release @JorTurFer