kedacore / keda

KEDA is a Kubernetes-based Event Driven Autoscaling component. It provides event driven scale for any container running in Kubernetes
https://keda.sh
Apache License 2.0
8.5k stars 1.07k forks source link

GCP PubSub activationValue not working correctly #5383

Closed DP19 closed 10 months ago

DP19 commented 10 months ago

Report

In the docs for the scaler the activationValue is activationValue - Target value for activating the scaler.

This reads as the value at which the scaler should become active. In practice it appears that the metric value has to be greater than this value and not equal to.

Where we use KEDA is mostly in our development environments where we don't have many tasks in pub sub and we've set this activationValue to 1 by default. For queues that only get a single task every now and again we notice this is not activating the scaler and instead will only do so when there are at least 2 messages

Expected Behavior

Setting the activationValue should activate the scaler when the metric hits this number

Actual Behavior

Setting the activationValue activates the scaler only when it exceeds this value

Steps to Reproduce the Problem

  1. Add new gcp pubsub scaler
  2. Set the activationValue to 1
  3. See ScaledObject show scaling as Inactive
  4. Add 1 message to the queue
  5. Notice the activation not trigger
  6. Add a second message to the queue
  7. Notice the scaler go active

Logs from KEDA operator

example

KEDA Version

2.13.0

Kubernetes Version

1.27

Platform

Google Cloud

Scaler Details

Google Cloud Platform‎ Pub/Sub

Anything else?

It looks like this is where the issue lies - https://github.com/kedacore/keda/blame/main/pkg/scalers/gcp_pubsub_scaler.go#L197

I believe the comparison here should be a >= instead of just a >

JorTurFer commented 10 months ago

Hello, We use > instead of >= because as default the value is '0', so the scaler is not active with 0 and active with > 0. We could change this behaviour to >= but it also requires to change default value to 1 because if we would keep default 0, 0>= 0 which means that the scaler will be always active. Taking this into account would need to set activationValue: 2 because default value would be activationValue: 1, which is basically the current solution because both options are equivalent.

DP19 commented 10 months ago

Taking this into account would need to set activationValue: 2 because default value would be activationValue: 1

Could you clarify this a bit @JorTurFer ? If the default was 1 and it now used the updated logic why would we need to set it to two and not just use the default?

Either way reading the docs and how I and my team understood this value is "active the scaler when this number is hit" but the logic backing this at the moment doesn't line up.

I think it either needs to be made more clear in the docs or changes need to be made here

JamesBalazs commented 10 months ago

+1 that activationValue: 0 sounds like the scaler will always be active. Intuitively, to a new user activationValue: 1 would imply active when 1 or more messages in a Pub/Sub queue for example.

The docs only state that the default is 0, I can't find anywhere that specifies this is a ">" rather than the ">=" you would expect based on the name. Eg:

Activation: Defines when the scaler is active or not and scales from/to 0 based on it.

is currently the most detailed explanation of the activation threshold in the docs, which is not very specific.

Updating the docs at least would make this significantly less likely to confuse people.

JorTurFer commented 10 months ago

Could you clarify this a bit @JorTurFer ? If the default was 1 and it now used the updated logic why would we need to set it to two and not just use the default?

Because if the logic is >=1, 1 will activate the scaler and you want to activate the scaler when there are at least 2 messages, don't? I mean, if that's not your goal, why would you want to change current behaviour which is the exactly behaviour that you expect indeed? I mean, if you want to scale when there is 1 message, just don't set the activation and that's all. It will works exactly as you expect as default. You are modifying the default behavior.

Either way reading the docs and how I and my team understood this value is "active the scaler when this number is hit" but the logic backing this at the moment doesn't line up.

I think it either needs to be made more clear in the docs or changes need to be made here

Let's update docs to be more clear. We are happy improving our docs as much as possible. Changing the current behaviour isn't possible as it would be a breaking change (a user with activationValue:1 would be affect), so let's clarify current behaviour in docs (maybe adding an extra point here)

DP19 commented 10 months ago

Because if the logic is >=1, 1 will activate the scaler and you want to activate the scaler when there are at least 2 messages, don't?

No we want to activate it when we get at least 1 message which is why we set the activationValue to 1; assuming it meant (as the name implies) active the scaler when the value hits this number.

Just without reading the docs the assumption is built into the name itself. I don't mind updating the docs and this likely doesn't matter in 99% of the cases but does for us since this is only active in our dev environments where having 0/1 message makes a giant difference.

I'd still advocate for updating the logic here though;

Having it use 1 as the default and using >=1 would have the same effect it does today if there are 0 messages in the queue but with the added benefit of not having to clarify a field which implies something it doesn't do

JorTurFer commented 10 months ago

No we want to activate it when we get at least 1 message which is why we set the activationValue to 1; assuming it meant (as the name implies) active the scaler when the value hits this number.

This is default behaviour, the scaler is active it there is at least 1 message and not active if there isn't any message

I'd still advocate for updating the logic here though; Having it use 1 as the default and using >=1 would have the same effect it does today if there are 0 messages in the queue but with the added benefit of not having to clarify a field which implies something it doesn't do

IMHO, this can't be done because it changes the current behaviour and we aren't allowed by our deprecation policy. Despite our deprecation policy, I don't think either that this change is required and docs seems the best place for explaining this.

The last but not least, there are other 60 scalers with the same logic which need to be updated as well if we agree with changing the logic from > to =>.

@zroubalik , @tomkerkhove , WDYT about this?

zroubalik commented 10 months ago

I understand the confusion on this, but I would like to avoid changing the behavior. It is practially a breaking change for 65+ different scalers.

The behavior you would like to achieve can be easily done by setting activationValue to 0 (or omitting this setting completely).

What we could definitely do is to update docs to be more clear what the activation is about.

DP19 commented 10 months ago

Thanks @zroubalik @JorTurFer . I've gone ahead and updated the docs to hopefully make this clearer https://github.com/kedacore/keda-docs/pull/1295