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

[JorTurFer]

PTAL @zroubalik @wozniakjan

[rickbrouwer]

I see that the changelog is missing anyway. Can you add it?

[y-rabie]

@rickbrouwer There was already a fix for this very part that added a changelog line. I though I might append my PR to the same line but apparently it fails the CI check. Should I replace the line, or append a new line?

[rickbrouwer]

@rickbrouwer There was already a fix for this very part that added a changelog line. I though I might append my PR to the same line but apparently it fails the CI check. Should I replace the line, or append a new line?

I think a new line to the other one with a description of what makes this PR different from the previous one so that both pull requests are mentioned. This also for some traceability.

[zroubalik]

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