Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: revise some annnotations and fix fallback check bug #6346

Closed
wants to merge 6 commits into from
Closed

fix: revise some annnotations and fix fallback check bug #6346

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6ae9874
revise some object name and fix fallback check
ctccxxd Nov 19, 2024
6986e93
add changelog
ctccxxd Nov 19, 2024
55c962b
fix CAHNGELOG.MD
ctccxxd Nov 19, 2024
dfeed82
put new log in CAHNGELOG.MD fix section
ctccxxd Nov 19, 2024
f2ad730
change fallback check
ctccxxd Nov 28, 2024
b115dd4
Merge branch 'main' into checkfallbackvalid
ctccxxd Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ Here is an overview of all new **experimental** features:

- **General**: Centralize and improve automaxprocs configuration with proper structured logging ([#5970](https://github.com/kedacore/keda/issues/5970))
- **General**: Paused ScaledObject count is reported correctly after operator restart ([#6321](https://github.com/kedacore/keda/issues/6321))
- **General**: Revise some annotations above function and fix fallback check bug ([#6346](https://github.com/kedacore/keda/pull/6346))
- **General**: ScaledJobs ready status set to true when recoverred problem ([#6329](https://github.com/kedacore/keda/pull/6329))

### Deprecations
Expand Down
22 changes: 17 additions & 5 deletions apis/keda/v1alpha1/scaledobject_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,21 @@ import (
"reflect"
"strconv"

eventingv1alpha1 "github.com/kedacore/keda/v2/apis/eventing/v1alpha1"
"github.com/kedacore/keda/v2/pkg/common/message"
"github.com/kedacore/keda/v2/pkg/eventemitter"
"github.com/kedacore/keda/v2/pkg/eventreason"

autoscalingv2 "k8s.io/api/autoscaling/v2"
corev1 "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
logf "sigs.k8s.io/controller-runtime/pkg/log"
)

var scaledobjecttypeslog = logf.Log.WithName("scaledobject-types")

var EventEmitter eventemitter.EventHandler

// +genclient
// +kubebuilder:object:root=true
// +kubebuilder:subresource:status
Expand Down Expand Up @@ -237,7 +248,7 @@ func (so *ScaledObject) IsUsingModifiers() bool {
return so.Spec.Advanced != nil && !reflect.DeepEqual(so.Spec.Advanced.ScalingModifiers, ScalingModifiers{})
}

// getHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
// GetHPAMinReplicas returns MinReplicas based on definition in ScaledObject or default value if not defined
func (so *ScaledObject) GetHPAMinReplicas() *int32 {
if so.Spec.MinReplicaCount != nil && *so.Spec.MinReplicaCount > 0 {
return so.Spec.MinReplicaCount
Expand All @@ -246,15 +257,15 @@ func (so *ScaledObject) GetHPAMinReplicas() *int32 {
return &tmp
}

// getHPAMaxReplicas returns MaxReplicas based on definition in ScaledObject or default value if not defined
// GetHPAMaxReplicas returns MaxReplicas based on definition in ScaledObject or default value if not defined
func (so *ScaledObject) GetHPAMaxReplicas() int32 {
if so.Spec.MaxReplicaCount != nil {
return *so.Spec.MaxReplicaCount
}
return defaultHPAMaxReplicas
}

// checkReplicaCountBoundsAreValid checks that Idle/Min/Max ReplicaCount defined in ScaledObject are correctly specified
// CheckReplicaCountBoundsAreValid checks that Idle/Min/Max ReplicaCount defined in ScaledObject are correctly specified
// i.e. that Min is not greater than Max or Idle greater or equal to Min
func CheckReplicaCountBoundsAreValid(scaledObject *ScaledObject) error {
min := int32(0)
Expand Down Expand Up @@ -288,10 +299,11 @@ func CheckFallbackValid(scaledObject *ScaledObject) error {

for _, trigger := range scaledObject.Spec.Triggers {
if trigger.Type == cpuString || trigger.Type == memoryString {
return fmt.Errorf("type is %s , but fallback it is not supported by the CPU & memory scalers", trigger.Type)
scaledobjecttypeslog.Error(nil, fmt.Sprintf("type is %s , but fallback it is not supported by the CPU & memory scalers", trigger.Type))
EventEmitter.Emit(scaledObject, scaledObject.Namespace, corev1.EventTypeWarning, eventingv1alpha1.ScaledObjectFailedType, eventreason.ScaledObjectCheckFailed, message.ScaledObjectMayNotBehaveAsExpectationMsg)
}
if trigger.MetricType != autoscalingv2.AverageValueMetricType {
return fmt.Errorf("MetricType=%s, but Fallback can only be enabled for triggers with metric of type AverageValue", trigger.MetricType)
return fmt.Errorf("MetricType=%s, but fallback can only be enabled for triggers with metric of type AverageValue", trigger.MetricType)
}
}
return nil
Expand Down
2 changes: 1 addition & 1 deletion apis/keda/v1alpha1/scaledobject_webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@wozniakjan wozniakjan Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@ctccxxd ctccxxd Nov 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. code(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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}

func verifyTriggers(incomingObject interface{}, action string, _ bool) error {
Expand Down
4 changes: 2 additions & 2 deletions apis/keda/v1alpha1/scaledobject_webhook_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ var _ = It("shouldn't validate the so creation when the fallback is wrong", func
}).Should(HaveOccurred())
})

var _ = It("shouldn't validate the so creation When the fallback are configured and the scaler is either CPU or memory.", func() {
var _ = It("should validate the so creation When the fallback are configured and the scaler is either CPU or memory.", func() {
namespaceName := "wrong-fallback-cpu-memory"
namespace := createNamespace(namespaceName)
workload := createDeployment(namespaceName, true, true)
Expand All @@ -193,7 +193,7 @@ var _ = It("shouldn't validate the so creation When the fallback are configured

Eventually(func() error {
return k8sClient.Create(context.Background(), so)
}).Should(HaveOccurred())
}).ShouldNot(HaveOccurred())
})

var _ = It("shouldn't validate the so creation when there is another unmanaged hpa and so has transfer-hpa-ownership activated", func() {
Expand Down
2 changes: 2 additions & 0 deletions pkg/common/message/message.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,4 +42,6 @@ const (
ClusterTriggerAuthenticationCreatedMsg = "New ClusterTriggerAuthentication configured"

ClusterTriggerAuthenticationUpdatedMsg = "ClusterTriggerAuthentication %s is updated"

ScaledObjectMayNotBehaveAsExpectationMsg = "ScaledObject may not behave as expectation"
)
Loading